[webkit-reviews] review denied: [Bug 26141] Implement onformchange and onforminput event handlers : [Attachment 78913] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 17 17:25:47 PST 2011


Kent Tamura <tkent at chromium.org> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 26141: Implement onformchange and onforminput event handlers
https://bugs.webkit.org/show_bug.cgi?id=26141

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

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

> LayoutTests/fast/forms/script-tests/formchange-event.js:25
> +var results = document.createElement('ul');
> +document.body.appendChild(results);
> +var result1 = document.createElement('li');
> +result1.setAttribute('id', 'result1');
> +results.appendChild(result1);
> +var result2 = document.createElement('li');
> +result2.setAttribute('id', 'result2');
> +results.appendChild(result2);
> +var result3 = document.createElement('li');
> +result3.setAttribute('id', 'result3');
> +results.appendChild(result3);

I don't think we need to use a DOM tree to represent these results.
Three global variables, or one object with three properties should be enough.

> LayoutTests/fast/forms/script-tests/formchange-event.js:29
> +form.innerHTML = "<ul><li id='li1'></li><li id='li2'></li><li
id='li3'></li><li id='li4'></li><li id='li5'></li><li id='li6'></li><li
id='li7'></li></ul>";

Do we need these <li> elements?  Why don't you build these and the following
input element by just one .innerHTML?

> LayoutTests/fast/forms/script-tests/formchange-event.js:33
> +document.getElementById('li1').innerHTML = '<input type="number" id="input1"
value="28"
onformchange="document.getElementById(\'result1\').innerText=\'foo\'" />';
> +document.getElementById('li2').innerHTML = '<input type="number" id="input2"
value="12" />';
> +document.getElementById('li3').innerHTML = '<input type="text" id="input3"
value="some"
onformchange="document.getElementById(\'result3\').innerText=\'bar\'" />';

Please set meaningful strings instead of "foo" "bar".
For example, innerText = 'input1-formchange-called'.

> LayoutTests/fast/forms/script-tests/formchange-event.js:44
> +var input52 = document.getElementById('input52');

nit: If you call document.getElmentById() many times, you may define a function
like
function $(id) { return document.getElementById(id); }
It helps to make the code cleaner.

> LayoutTests/fast/forms/script-tests/formchange-event.js:168
> +var successfullyParsed = true;

We need more tests:
 - Remove an associated element in an event handler
 - Remove the form element from the DOM tree in an event handler

> LayoutTests/fast/forms/script-tests/forminput-event.js:1
> +description('Test for forminput events.');

Same comments as formchange-event.js

> Source/WebCore/html/HTMLElement.cpp:879
> +    RefPtr<HTMLFormElement> ownerForm;
> +    Node* ancestorNode = shadowAncestorNode();
> +    if (ancestorNode) {
> +	   if (!ancestorNode->isHTMLElement())
> +	       return;
> +	   HTMLElement* ancestorHTML = static_cast<HTMLElement*>(ancestorNode);

> +	   if (!ancestorHTML)
> +	       return;
> +	   ownerForm = ancestorHTML->form();
> +    } else
> +	   ownerForm = form();

This part is identical to dispatchChangeEvents().  We had better introduce a
new function for the common part.

> Source/WebCore/html/HTMLFormElement.cpp:604
> +	   RefPtr<FormAssociatedElement> formAssociatedElement = elements[i];

We don't need to use RefPtr<> and a raw pointer is enough because elements[i]
holds a reference count.

> Source/WebCore/html/HTMLFormElement.cpp:607
> +	   RefPtr<HTMLFormControlElement> formElement =
static_pointer_cast<HTMLFormControlElement>(formAssociatedElement);

ditto.	RefPtr<> is not needed.

> Source/WebCore/html/HTMLFormElement.cpp:628
> +	   if
(!formElement->dispatchEvent(Event::create(eventNames().formchangeEvent, false,
false)))

The difference from dispatchFormInput() is only the event name.  We had better
have a new function like broadcastFormEvent(const AtomicString& eventName).


More information about the webkit-reviews mailing list