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

Sun Feb 27 19:57:24 PST 2011

Kent Tamura <tkent at chromium.org> 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 83610: Patch
https://bugs.webkit.org/attachment.cgi?id=83610&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83610&action=review

>> LayoutTests/fast/forms/onchange-setvalueforuser.html:1
>> +<html>
> 
> Should this test live in fast/forms or fast/events?  Both seem to have
precedent.

Either is ok.  I prefer fast/forms because it's a test of a form-specific
feature.

> LayoutTests/fast/forms/onchange-setvalueforuser.html:7
> +    function log(msg) {
> +	   var res = document.getElementById('res');
> +	   res.innerHTML = res.innerHTML + msg + "<br>";
> +    }

We had better use debug() or testPassed() defined in
LayoutTests/fast/js/resources/js-test-pre.js.

>> LayoutTests/fast/forms/onchange-setvalueforuser.html:20
>> +	    tf.blur();
> 
> Do we want to also test that the event is not fired until the blur occurs? 
If so, any suggestions on a nice, straightforward way to do that?

Introducing a global counter for the number of fired 'change' events?

>> LayoutTests/fast/forms/onchange-setvalueforuser.html:22
>> +	</script>
> 
> I've seen other tests have js in a separate file rather than inline -- is
that preferred?

Some developers don't like separating js file.
IMO, either is ok.

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

Wrong indentation.

>> Source/WebKit/mac/WebView/WebView.mm:6271
>> +	String stringValue(jsStringValue->characters(),
jsStringValue->length());
> 
> Do either of these leak memory?  I don't have a good understanding of the
memory ownership model here.

I'm not sure too.