[webkit-reviews] review denied: [Bug 53160] HTMLInputElement::setValue() should schedule change event when the element is focused : [Attachment 84531] 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 84531: Patch
https://bugs.webkit.org/attachment.cgi?id=84531&action=review

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

> Source/WebCore/dom/Document.cpp:3182
> +	       if (element->isTextFormControl()) {
> +		   bool wasChanged = false;
> +		   if (element->isHTMLElement())
> +		       wasChanged =
static_cast<HTMLTextFormControlElement*>(element)->wasChangedSinceLastChangeEve
nt();
> +#if ENABLE(WML)
> +		   else if (element->isWMLElement())
> +		       wasChanged =
static_cast<WMLInputElement*>(element)->wasChangedSinceLastChangeEvent();
> +#endif

I think I steered you in the wrong direction moving this code out of Element
like this. It kills me that WML is the reason we have to do it with virtual
functions, but all these if statements and typecasts are terrible.

If we do stick with this design, then getting the “was changed” flag should be
encapsulated in a separate function. The code would read much more clearly if
it was.

Honestly, this code and the code below in
RenderTextControlSingleLine::subtreeHasChanged seems like an argument for
putting the virtual functions back into Element. All these specific cases are
ugly and hide the real logic in a forest of trivia.

> Source/WebCore/html/HTMLFormControlElement.h:209
> +    virtual bool wasChangedSinceLastChangeEvent() const;
> +    virtual void setChangedSinceLastChangeEvent(bool);

Why are these functions virtual? They should not be!

> Source/WebCore/html/HTMLFormControlElement.h:237
> +    bool m_wasChangedSinceLastChangeEvent : 1;

Adding the bit toHTMLTextFormControlElement  is not great. By adding a single
bit here we end up spending an entire machine word. I know the flag is only
needed for the text controls, but I think it’s OK to have the bit in the base
class. I suggest adding this to HTMLFormControlElement instead because that has
other bits that this could combine with in a single word.

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

This comment does not make it clear why it’s right to send an input event. I
think we could have a much better comment here, while still keeping it short.

> Source/WebCore/html/HTMLInputElement.cpp:1046
> +	   // Dispatch a change event for text fields that have been edited
prior
> +	   // to form submission. Normally this event is dispatched when the
field
> +	   // loses focus, but form submission is another special case where we

> +	   // consider editing to have finished.

Can you say the same thing in a shorter comment? I would say something more
like:

    // Form submission finishes editing as loss of focus does. If there was a
change, send the event now.

> Source/WebCore/html/HTMLInputElement.cpp:1418
> +	   if (isTextField())
> +	       setChangedSinceLastChangeEvent(true);

Might be clearer to put this inside the else below. It’s a little confusing
since the range control branch sends a change event and this involves change
events too.

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:205
> +    bool wasChanged = false;
> +    if (element->isHTMLElement()) {
> +	   HTMLTextFormControlElement* textFormControlElement =
static_cast<HTMLTextFormControlElement*>(element);
> +	   wasChanged =
textFormControlElement->wasChangedSinceLastChangeEvent();
> +	   textFormControlElement->setChangedSinceLastChangeEvent(true);
> +    }
> +#if ENABLE(WML)
> +    else if (element->isWMLElement()) {
> +	   WMLInputElement* wmlInput = static_cast<WMLInputElement*>(element);
> +	   wasChanged = wmlInput->wasChangedSinceLastChangeEvent();
> +	   wmlInput->setChangedSinceLastChangeEvent(true);
> +    }
> +#endif

If we do stick with this design, then we should find some way to encapsulate
this in a separate function. The code would read much more clearly if we could
concentrate on what we are getting and setting and not so much on the mechanics
of how.

> Source/WebCore/wml/WMLInputElement.h:70
> +    virtual bool wasChangedSinceLastChangeEvent() const { return
m_wasChangedSinceLastChangeEvent; }
> +    virtual void setChangedSinceLastChangeEvent(bool changed) {
m_wasChangedSinceLastChangeEvent = changed; }

Why are these functions virtual?

> Source/WebKit/mac/WebView/WebView.mm:6266
> +    JSElement* jsElement =
static_cast<JSElement*>(asObject(jsElementValue));
> +    Element* webcoreElement = jsElement->impl();
> +    InputElement* inputElement = toInputElement(webcoreElement);

There are too many local variables here. Makes the code confusing for no good
reason. It should just be like this:

    JSValue elementValue = toJS(exec, element);
    if (!elementValue.inherits(&JSElement::s_info))
	return;
    InputElement* inputElement =
toInputElement(static_cast<JSElement*>(asObject(elementValue))->impl());

> Source/WebKit/mac/WebView/WebView.mm:6271
> +    if (!JSValueIsString(context, value))
> +	   return;

This check is incorrect. Any JSValue can be converted to a string. There is no
need to reject the argument since it isn’t already a string.

> Source/WebKit/mac/WebView/WebView.mm:6273
> +    JSStringRef jsStringValue = JSValueToStringCopy(context, value, NULL);
> +    String stringValue(jsStringValue->characters(),
jsStringValue->length());

The jsStringValue here leaks. You should be using a JSRetainPtr or explicitly
releasing it.

I don’t know why this code is compiling for you. For a JSStringRef, you can’t
just call ->characters().

> Source/WebKit/mac/WebView/WebView.mm:6275
> +    inputElement->setValueForUser(stringValue);

The local variable is not needed. It doesn’t make things read better. It should
be like this:

    JSRetainPtr<JSStringRef> string(Adopt, JSValueToStringCopy(context, value,
0);
    inputElement->setValueForUser(String(JSStringGetCharacterPtr(string.get()),
JSStringGetLength(string.get())));

> Source/WebKit/mac/WebView/WebViewPrivate.h:681
> +- (void)_setValueForUser:(JSContextRef)context
forElement:(JSValueRef)element withValue:(JSValueRef)value;

This is not the right way to hook this up; too much of the code is in WebKit
here. You model for this should be _markerTextForListItem. The argument for the
element can just be a DOMElement * and the argument for the value can just be a
NSString *.

Conversion from a JSValueRef to a DOMElement * and an NSString * can be handled
in the layout test controller.