[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