[webkit-reviews] review denied: [Bug 53160] HTMLInputElement::setValue() should schedule change event when the element is focused : [Attachment 84402] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ilya Sherman <isherman at chromium.org>'s
request for review:
Bug 53160: HTMLInputElement::setValue() should schedule change event when the
element is focused
https://bugs.webkit.org/show_bug.cgi?id=53160

Attachment 84402: Patch
https://bugs.webkit.org/attachment.cgi?id=84402&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84402&action=review

Great approach. This looks good.

If it wasn’t for WML I would be absolutely sure we should put this bit into
HTMLFormControlElement and not add any new virtual functions to Element. I am
really unhappy we have to make such a mess just to handle WML. I still think we
should put this into HTMLFormControlElement. If we have to have virtual
functions that’s OK, but I would prefer not to spread this across so many
classes.

> Source/WebCore/dom/Document.cpp:3172
> +	   if (oldFocusedNode->isElementNode()) {

Since we have to do a type check here anyway, I suggest we instead call
isFormControlElement and then cast to HTMLFormControlElement instead of just
casting to Element. We then should put the flag bit into HTMLFormControlElement
instead of having two separate flag bits and virtual functions to access them.
Even though only text input and textareas would use the flags, I think we can
spare the bit in HTMLFormControlElement.

> Source/WebCore/dom/Document.cpp:3173
> +	       Element* element = static_cast<Element *>(oldFocusedNode.get());


Instead of Element * this should say Element*. But actually
HTMLFormControlElement* if you take my advice above.

> Source/WebCore/dom/Element.h:328
> +    virtual void setChangedSinceLastChangeEvent(bool) { }
>      virtual void dispatchFormControlChangeEvent() { }

I think the function names should consistently use the phrase “form control
change event” if we are adding them to Element.h. But we should not add new
functions to Element.h (see my comment above).

> Source/WebCore/html/HTMLFormControlElement.cpp:656
> +void HTMLTextFormControlElement::dispatchFormControlChangeEvent()
> +{
> +    HTMLFormControlElement::dispatchFormControlChangeEvent();
> +    setChangedSinceLastChangeEvent(false);
> +}

Since we’ll put the bit directly into HTMLFormControlElement, this code can go
there too.

> Source/WebCore/html/HTMLInputElement.cpp:899
> +	   // For text fields, don't dispatch the change event when focused.
> +	   // It will be dispatched when the control loses focus.
> +	   if (isTextField() && document()->focusedNode() == this)
> +	       setChangedSinceLastChangeEvent(true);
> +	   else
> +	       dispatchFormControlChangeEvent();

What about the other sites that call dispatchFormControlChangeEvent? Aren’t
there any others that have the same issue, and should defer the change event in
the case of a text input or textarea? A change specific to setValue may not
cover enough cases.

We should also investigate whether we can call focused() here instead of
document()->focusedNode() == this.

Do we need to clear this flag when the type of the input element changes?

> Source/WebCore/html/HTMLInputElement.cpp:-1044
>	   // Fire onChange for text fields.
> -	   RenderObject* r = renderer();
> -	   if (r && r->isTextField() &&
toRenderTextControl(r)->wasChangedSinceLastChangeEvent()) {
> +	   if (isTextField() && wasChangedSinceLastChangeEvent())
>	       dispatchFormControlChangeEvent();
> -	       // Refetch the renderer since arbitrary JS code run during
onchange can do anything, including destroying it.
> -	       r = renderer();
> -	       if (r && r->isTextField())
> -		  
toRenderTextControl(r)->setChangedSinceLastChangeEvent(false);
> -	   }

We don’t need the isTextField check here. It was needed before because we
needed to type check the renderer before casting and calling functions on it.
But here we would be OK unconditionally dispatching the change event if the
wasChangedSinceLastChangeEvent flag was set. It won’t be set for other types of
input elements that don’t need to send a change event.

Please remove the isTextField check and also consider removing or rewording the
existing comment. For one thing, the event is a "change event", not "onChange".
And a comment that just states what the code does is not valuable. It needs to
explain *why* the code does something.

> Source/WebCore/html/HTMLInputElement.cpp:1413
>	   if (renderer() && renderer()->isTextField())
> -	      
toRenderTextControl(renderer())->setChangedSinceLastChangeEvent(true);
> +	       setChangedSinceLastChangeEvent(true);

The if statement here should be removed. It was a type check for the renderer.
Instead we can unconditionally call setChangedSinceLastChangeEvent(true).

> Source/WebCore/html/HTMLInputElement.h:144
> +    virtual bool wasChangedSinceLastChangeEvent() const { return
m_wasChangedSinceLastChangeEvent; }
> +    virtual void setChangedSinceLastChangeEvent(bool changed) {
m_wasChangedSinceLastChangeEvent = changed; }

Virtual functions like these should not have function definitions in the
header; there is no chance of inlining them.

I know that others in this file do, but one effect of this is to make our
compile and link times slower with the many duplicate copies of such functions.
Another effect is to make it so we have to recompile more if we ever decide to
change the function bodies. The functions don’t actually get inlined when
called as virtual functions, and these are never called in a non-virtual
fashion.

Please move the function definitions into the .cpp file, except you probably
will remove them entirely if you take my other suggestion above.

> Source/WebCore/html/HTMLTextAreaElement.h:55
> +    virtual bool wasChangedSinceLastChangeEvent() const { return
m_wasChangedSinceLastChangeEvent; }
> +    virtual void setChangedSinceLastChangeEvent(bool changed) {
m_wasChangedSinceLastChangeEvent = changed; }

Same comment as above. Please put these in the .cpp file.

> Source/WebCore/html/shadow/TextControlInnerElements.cpp:214
> +		   input->setValue("", true);

Good change, but this points out why we prefer enums instead of booleans. That
“true” is completely mysterious!

Is there a test that covers this? To put it another way: What test fails if you
pass false here instead of true?

> Source/WebCore/rendering/RenderTextControlMultiLine.cpp:49
> +    textArea->setChangedSinceLastChangeEvent(true);

Which test fails if you don’t add this?