Bug 81854

Summary:

RadioNodeList support in HTMLFormElement::elements

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Forms

Assignee:

Rakesh <rakeshchaitan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, arv, dglazkov, haraken, japhet, kling, ojan, rakeshchaitan, rakuco, rniwa, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

19264, 80110

Attachments:

Description	Flags
intial_patch	none
updated patch	none
Updated patch	none
Updated patch	none
Updated patch	none
Updated patch	none
Updated patch	none
Updated patch	none
Updated patch	none
Updated patch	none
Updated patch	none
Archive of layout-test-results from ec2-cq-02	none
Archive of layout-test-results from ec2-cr-linux-02	none
Updated patch	none
Update patch	rniwa: review+, rniwa: commit-queue-
PatchForLanding	none
Patch for landing	none
Updated patch	none
Updated patch	none
Updated patch	none
patch for landing	none
patch for landing	none

Kent Tamura

Reported 2012-03-21 18:01:47 PDT

http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#radionodelist

Attachments
intial_patch (15.42 KB, patch) 2012-03-22 08:22 PDT, Rakesh	no flags	Details Formatted Diff Diff
updated patch (26.55 KB, patch) 2012-03-27 07:46 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.87 KB, patch) 2012-03-29 02:42 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (36.04 KB, patch) 2012-04-02 07:27 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.71 KB, patch) 2012-04-09 07:43 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.29 KB, patch) 2012-04-09 07:53 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.53 KB, patch) 2012-04-10 03:48 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.54 KB, patch) 2012-04-10 19:59 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.44 KB, patch) 2012-04-10 21:00 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.46 KB, patch) 2012-04-10 23:06 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (34.31 KB, patch) 2012-04-12 01:58 PDT, Rakesh	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cq-02 (6.44 MB, application/zip) 2012-04-12 03:23 PDT, WebKit Review Bot	no flags	Details
Archive of layout-test-results from ec2-cr-linux-02 (6.31 MB, application/zip) 2012-04-12 03:30 PDT, WebKit Review Bot	no flags	Details
Updated patch (34.42 KB, patch) 2012-04-12 07:20 PDT, Rakesh	no flags	Details Formatted Diff Diff
Update patch (35.19 KB, patch) 2012-04-18 08:28 PDT, Rakesh	rniwa: review+ rniwa: commit-queue-	Details Formatted Diff Diff
PatchForLanding (34.17 KB, patch) 2012-04-23 14:28 PDT, Rakesh	no flags	Details Formatted Diff Diff
Patch for landing (35.60 KB, patch) 2012-04-23 23:41 PDT, Nayan Kumar K	no flags	Details Formatted Diff Diff
Updated patch (41.27 KB, patch) 2012-04-25 09:34 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (41.98 KB, patch) 2012-04-27 06:35 PDT, Rakesh	no flags	Details Formatted Diff Diff
Updated patch (43.20 KB, patch) 2012-05-02 13:12 PDT, Rakesh	no flags	Details Formatted Diff Diff
patch for landing (42.00 KB, patch) 2012-05-08 08:00 PDT, Rakesh	no flags	Details Formatted Diff Diff
patch for landing (42.00 KB, patch) 2012-05-08 18:57 PDT, Rakesh	no flags	Details Formatted Diff Diff
Show Obsolete (19) View All Add attachment proposed patch, testcase, etc.

Rakesh

Comment 1 2012-03-22 08:22:15 PDT

Created attachment 133268 [details] intial_patch This is an basic implmentation just to confirm that approach used is fine. Layout tests, changes for other platforms other than GTK are yet to be added

Early Warning System Bot

Comment 2 2012-03-22 08:34:22 PDT

Comment on attachment 133268 [details] intial_patch Attachment 133268 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12090999

Early Warning System Bot

Comment 3 2012-03-22 08:39:03 PDT

Comment on attachment 133268 [details] intial_patch Attachment 133268 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12093016

Build Bot

Comment 4 2012-03-22 08:49:50 PDT

Comment on attachment 133268 [details] intial_patch Attachment 133268 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12122013

Gyuyoung Kim

Comment 5 2012-03-22 09:05:49 PDT

Comment on attachment 133268 [details] intial_patch Attachment 133268 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12117065

Early Warning System Bot

Comment 6 2012-03-22 09:34:21 PDT

Comment on attachment 133268 [details] intial_patch Attachment 133268 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12123025

Early Warning System Bot

Comment 7 2012-03-22 09:40:04 PDT

Comment on attachment 133268 [details] intial_patch Attachment 133268 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12123027

Build Bot

Comment 8 2012-03-22 11:14:37 PDT

Comment on attachment 133268 [details] intial_patch Attachment 133268 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12114106

Kent Tamura

Comment 9 2012-03-22 21:34:58 PDT

Comment on attachment 133268 [details] intial_patch View in context: https://bugs.webkit.org/attachment.cgi?id=133268&action=review > Source/WebCore/bindings/js/JSHTMLCollectionCustom.cpp:58 > + if (collection->impl()->type() == FormControls) > + return toJS(exec, collection->globalObject(), RadioNodeList::adopt(namedItems).get()); RadioNodeList must be a "live" node list, and it has a snapshot of matched nodes in this code.

Rakesh

Comment 10 2012-03-27 07:46:39 PDT

Created attachment 134059 [details] updated patch Considering liveness of the form elements. This patch hopefully passes most builds except mac and maybe efl. Help needed for mac, I added RadioNodeList.cpp, .h and .idl in html after opening WebCore.xproj on mac. I am not sure where to add the generated JSRadioNodeList.h, cpp files in project. (Newbie to mac and XCode)

Rakesh

Comment 11 2012-03-27 07:54:24 PDT

(In reply to comment #9) > > RadioNodeList must be a "live" node list, and it has a snapshot of matched nodes in this code. Thanks for your inputs. Changed implementation to be live. Got stuck with adding JS files to WebCore.xproj, is there specific way to add them as adding from release/debug folder may not be proper?

WebKit Review Bot

Comment 12 2012-03-27 08:06:22 PDT

Comment on attachment 134059 [details] updated patch Attachment 134059 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12147523

Build Bot

Comment 13 2012-03-27 08:15:38 PDT

Comment on attachment 134059 [details] updated patch Attachment 134059 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12140590

Build Bot

Comment 14 2012-03-27 08:20:51 PDT

Comment on attachment 134059 [details] updated patch Attachment 134059 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12148481

Gyuyoung Kim

Comment 15 2012-03-27 08:39:50 PDT

Comment on attachment 134059 [details] updated patch Attachment 134059 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12142560

WebKit Review Bot

Comment 16 2012-03-27 09:21:50 PDT

Comment on attachment 134059 [details] updated patch Attachment 134059 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12142578

Rakesh

Comment 17 2012-03-29 02:42:50 PDT

Created attachment 134534 [details] Updated patch Hopefully it passes all builds.

Kent Tamura

Comment 18 2012-04-01 21:51:47 PDT

Add NodeList experts to Cc.

Ryosuke Niwa

Comment 19 2012-04-01 21:54:38 PDT

Comment on attachment 134534 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=134534&action=review > Source/WebCore/ChangeLog:8 > + > + Test: fast/forms/form-collection-radio-node-list.html What's the rationale for this change? Where is this feature spec'ed?

Kent Tamura

Comment 20 2012-04-01 21:58:41 PDT

Comment on attachment 134534 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=134534&action=review > LayoutTests/fast/forms/form-collection-radio-node-list.html:71 > + > +</script> We need more test cases. e.g. Getting a RadioNodeList, then removing one of the members from the owner <form>, or adding another member to the owner <form>

Kent Tamura

Comment 21 2012-04-01 22:04:20 PDT

Comment on attachment 134534 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=134534&action=review > Source/WebCore/html/RadioNodeList.cpp:83 > + for (size_t i = 0; i < namedItems.size(); ++i) { > + Node* node = namedItems[i].get(); > + if (node->isElementNode()) { > + Element* element = toElement(node); > + if (element->hasTagName(inputTag)) { > + const HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(element); > + if (inputElement->isRadioButton() && !inputElement->value().isEmpty() && inputElement->checked()) > + return inputElement->value(); > + } > + } We prefer early-exit like: for (size_t ... Node* node = ... if (!node->isElementNode()) continue; Element*... if (!element->hasTagName(inputTag)) continue; .... > Source/WebCore/html/RadioNodeList.cpp:99 > + for (size_t i = 0; i < namedItems.size(); ++i) { > + Node* node = namedItems[i].get(); > + if (node->isElementNode()) { > + Element* element = toElement(node); > + if (element->hasTagName(inputTag)) { > + HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(element); > + if (inputElement->isRadioButton() && !inputElement->value().isEmpty() && inputElement->value() == val) ditto. > Source/WebCore/html/RadioNodeList.h:48 > + virtual unsigned length() const; > + virtual Node* item(unsigned index) const; > + virtual Node* itemWithName(const AtomicString&) const; Add OVERRIDE > Source/WebCore/html/RadioNodeList.h:59 > + HTMLCollection* m_collection; What happens if m_collection is deleted.

Ryosuke Niwa

Comment 22 2012-04-01 22:06:42 PDT

Comment on attachment 134534 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=134534&action=review r- due to inefficient implementation. > Source/WebCore/html/RadioNodeList.cpp:49 > + std::sort(namedItems.begin(), namedItems.end(), compareTreeOrder); Why do we need to sort it here? Doesn't namedItems already give you a list of nodes in the tree order? > Source/WebCore/html/RadioNodeList.cpp:57 > + Vector<RefPtr<Node> > namedItems; > + m_collection->namedItems(m_name, namedItems); > + std::sort(namedItems.begin(), namedItems.end(), compareTreeOrder); Ditto. > Source/WebCore/html/RadioNodeList.cpp:64 > + size_t length = namedItems.size(); > + for (size_t i = 0; i < length; ++i) { > + Node* node = namedItems[i].get(); > + if (node->isElementNode() && toElement(node)->getIdAttribute() == elementId) > + return node; > + } This seems like backwards. We should be calling document()->getElementById first, and only fallback to slower algorithm. > Source/WebCore/html/RadioNodeList.cpp:73 > + Vector<RefPtr<Node> > namedItems; > + m_collection->namedItems(m_name, namedItems); > + std::sort(namedItems.begin(), namedItems.end(), compareTreeOrder); Same issue. > Source/WebCore/html/RadioNodeList.cpp:92 > + Vector<RefPtr<Node> > namedItems; > + m_collection->namedItems(m_name, namedItems); > + std::sort(namedItems.begin(), namedItems.end(), compareTreeOrder); Ditto. > Source/WebCore/html/RadioNodeList.h:39 > +class RadioNodeList : public NodeList { Don't we want to inherit from DynamicSubtreeNodeList?

Rakesh

Comment 23 2012-04-02 00:26:37 PDT

Thanks for reviewing. (In reply to comment #19) > > Source/WebCore/ChangeLog:8 > > + > > + Test: fast/forms/form-collection-radio-node-list.html > > What's the rationale for this change? Where is this feature spec'ed? This feature is spec'ed at http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#radionodelist. (In reply to comment #20) > We need more test cases. > e.g. Getting a RadioNodeList, then removing one of the members from the owner <form>, or adding another member to the owner <form> Yes, will add more. (In reply to comment #21) > We prefer early-exit like: > for (size_t ... > Node* node = ... > if (!node->isElementNode()) > continue; > Element*... > if (!element->hasTagName(inputTag)) > continue; > .... > > + virtual Node* itemWithName(const AtomicString&) const; > > Add OVERRIDE Will add this also. > > Source/WebCore/html/RadioNodeList.h:59 > > + HTMLCollection* m_collection; > > What happens if m_collection is deleted. Can I ref the collection? (In reply to comment #22) > > Source/WebCore/html/RadioNodeList.cpp:49 > > + std::sort(namedItems.begin(), namedItems.end(), compareTreeOrder); > > Why do we need to sort it here? Doesn't namedItems already give you a list of nodes in the tree order? namedItems does not give the list in tree order but it gives list which contains first the elements which match the 'id' and then the 'name' matched elements. Hence we have to sort the tree. > > Source/WebCore/html/RadioNodeList.cpp:64 > > + size_t length = namedItems.size(); > > + for (size_t i = 0; i < length; ++i) { > > + Node* node = namedItems[i].get(); > > + if (node->isElementNode() && toElement(node)->getIdAttribute() == elementId) > > + return node; > > + } > > This seems like backwards. We should be calling document()->getElementById first, and only fallback to slower algorithm. Yes, can be done. > > > Source/WebCore/html/RadioNodeList.h:39 > > +class RadioNodeList : public NodeList { > > Don't we want to inherit from DynamicSubtreeNodeList? I am not sure, could you please mention what could be the benefits?

Rakesh

Comment 24 2012-04-02 07:27:45 PDT

Created attachment 135095 [details] Updated patch Updated patch with more test coverage.

Ryosuke Niwa

Comment 25 2012-04-02 10:10:00 PDT

(In reply to comment #23) > Thanks for reviewing. > > (In reply to comment #19) > > > Source/WebCore/ChangeLog:8 > > > + > > > + Test: fast/forms/form-collection-radio-node-list.html > > > > What's the rationale for this change? Where is this feature spec'ed? > > This feature is spec'ed at http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#radionodelist. You need to state that in the change log. > (In reply to comment #22) > > > Source/WebCore/html/RadioNodeList.cpp:49 > > > + std::sort(namedItems.begin(), namedItems.end(), compareTreeOrder); > > > > Why do we need to sort it here? Doesn't namedItems already give you a list of nodes in the tree order? > > namedItems does not give the list in tree order but it gives list which contains first the elements which match the 'id' and then the 'name' matched elements. Hence we have to sort the tree. Then you can't use namedItems. Sorting elements like this is extremely slow and unacceptable. > > > Source/WebCore/html/RadioNodeList.h:39 > > > +class RadioNodeList : public NodeList { > > > > Don't we want to inherit from DynamicSubtreeNodeList? > > I am not sure, could you please mention what could be the benefits? You can use all the machineries of DynamicSubtreeNodeList then. Really, this class is a dynamic node list, and should be implemented as one. The fact it relies on HTMLCollection to do work and then sort its results is a bad design.

Darin Adler

Comment 26 2012-04-02 12:44:31 PDT

Comment on attachment 135095 [details] Updated patch Ryosuke is right. This design where we get and re-sort the list each time is going to be pathologically slow given how expensive the compareTreeOrder function is. This needs to be redone in a different want that gets the elements in tree order in the first place.

Rakesh

Comment 27 2012-04-02 22:42:19 PDT

(In reply to comment #25) > > You can use all the machineries of DynamicSubtreeNodeList then. Really, this class is a dynamic node list, and should be implemented as one. The fact it relies on HTMLCollection to do work and then sort its results is a bad design. (In reply to comment #26) > (From update of attachment 135095 [details]) > Ryosuke is right. This design where we get and re-sort the list each time is going to be pathologically slow given how expensive the compareTreeOrder function is. This needs to be redone in a different want that gets the elements in tree order in the first place. Thanks Ryosuke and Darin for your valuable inputs, I will change the implementation to have a collection in tree orders avoiding sorting and use DynamicSubtreeNodeList.

Rakesh

Comment 28 2012-04-06 23:54:23 PDT

(In reply to comment #25) > You can use all the machineries of DynamicSubtreeNodeList then. Really, this class is a dynamic node list, and should be implemented as one. The fact it relies on HTMLCollection to do work and then sort its results is a bad design. Using DynamicSubtreeNodeList makes things easier but AFAIK it can have only Nodes under the sub tree but in case of FormCollection, FormAssociated element need not be necessarily child of form element. With DynamicSubtreeNodeList traversing we may not access the all the elements whose form owner is this particular form element. May be RadioNodeList can rely on FormCollection but should sort the matching elements once before caching the nodes in RadioNodeList. Also spec says: "create a new RadioNodeList object representing a live view of the HTMLFormControlsCollection object, further filtered so that the only nodes in the RadioNodeList object are those that have either an id attribute or a name attribute equal to name." Please let me know your inputs.

Ryosuke Niwa

Comment 29 2012-04-07 10:10:43 PDT

(In reply to comment #28) > (In reply to comment #25) > > You can use all the machineries of DynamicSubtreeNodeList then. Really, this class is a dynamic node list, and should be implemented as one. The fact it relies on HTMLCollection to do work and then sort its results is a bad design. > > Using DynamicSubtreeNodeList makes things easier but AFAIK it can have only Nodes under the sub tree but in case of FormCollection, FormAssociated element need not be necessarily child of form element. With DynamicSubtreeNodeList traversing we may not access the all the elements whose form owner is this particular form element. See http://trac.webkit.org/browser/trunk/Source/WebCore/html/LabelsNodeList.cpp.

Rakesh

Comment 30 2012-04-09 07:43:54 PDT

Created attachment 136223 [details] Updated patch Modified to use DynamicSubtreeNodeList.

Rakesh

Comment 31 2012-04-09 07:53:54 PDT

Created attachment 136224 [details] Updated patch Missed cleanup in previous patch

Rakesh

Comment 32 2012-04-09 08:00:36 PDT

(In reply to comment #29) > > See http://trac.webkit.org/browser/trunk/Source/WebCore/html/LabelsNodeList.cpp. Thanks for your inputs. The issue I was thinking about was formcontrols whose owner is form but yet they may not be children of that form. For eg: newElement1.form = owner; document.body.appendChild(newElement1); But this issue I think is due to rootNode of FormCollection being form where as specs says FormCollection should be rooted at document. "The elements IDL attribute must return an HTMLFormControlsCollection rooted at the Document node" May be that can be addressed in another bug.

Ryosuke Niwa

Comment 33 2012-04-09 09:33:03 PDT

(In reply to comment #32) > (In reply to comment #29) > > > > See http://trac.webkit.org/browser/trunk/Source/WebCore/html/LabelsNodeList.cpp. > > Thanks for your inputs. > > The issue I was thinking about was formcontrols whose owner is form but yet they may not be children of that form. For eg: > > newElement1.form = owner; > document.body.appendChild(newElement1); That can be also true for labels node list. Did you even read LabelsNodeList.cpp? LabelsNodeList doesn't just traverse through the subtree of the form element. It traverses the entire document to find all labels that matches. So whether an element is in the subtree of the form element or not is completely irrelevant here.

Rakesh

Comment 34 2012-04-09 12:11:57 PDT

(In reply to comment #33) > That can be also true for labels node list. Did you even read LabelsNodeList.cpp? LabelsNodeList doesn't just traverse through the subtree of the form element. It traverses the entire document to find all labels that matches. So whether an element is in the subtree of the form element or not is completely irrelevant here. My mistake, I did not observe "DynamicSubtreeNodeList(forNode->document()) , m_forNode(forNode)" carefully, I was more keen to look at nodeMatches. I may need to pass rootNode for DynamicSubtreeNodeList conditionally as for fieldset we don't want to traverse the complete document.

Rakesh

Comment 35 2012-04-10 03:48:31 PDT

Created attachment 136420 [details] Updated patch Updated patch using DynamicSubtreeNodeList and test coverage for non subtree element.

Ryosuke Niwa

Comment 36 2012-04-10 09:44:49 PDT

Comment on attachment 136420 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=136420&action=review > Source/WebCore/html/RadioNodeList.cpp:39 > + : DynamicSubtreeNodeList(forNode->hasTagName(formTag) ? forNode->document() : forNode) When can forNode not be formTag? As far as I know, HTMLFormElement::elements is the only function that creates HTMLFormCollection. It's better to assert that forNode is indeed a HTML form element. Also, please rename m_forNode to m_formElement. r- because of this. > Source/WebCore/html/RadioNodeList.cpp:56 > + if (!node->isElementNode()) > + continue; This continue will never be executed because nodeMatches returns false for non-element nodes. It's better to assert in this case. > Source/WebCore/html/RadioNodeList.cpp:79 > + Element* element = toElement(node); > + if (!element->hasTagName(inputTag)) > + continue; > + HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(element); > + if (!inputElement->isRadioButton() || inputElement->value().isEmpty() || inputElement->value() != val) > + continue; We're repeating this logic here. Please extract it as a helper function. > LayoutTests/fast/forms/form-collection-radio-node-list.html:16 > +container.innerHTML = '<form id="form1">' + > + '<button id=button1></button>' + You said there are cases RadioNodeList needs to return an element that's not necessarily in the subtree for form. If that's really the case, then we definitely need a test case for that. But looking at your code and also the spec, I don't see how that's possible.

Rakesh

Comment 37 2012-04-10 11:07:00 PDT

Comment on attachment 136420 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=136420&action=review Thanks for reviewing this patch. >> Source/WebCore/html/RadioNodeList.cpp:39 >> + : DynamicSubtreeNodeList(forNode->hasTagName(formTag) ? forNode->document() : forNode) > > When can forNode not be formTag? As far as I know, HTMLFormElement::elements is the only function that creates HTMLFormCollection. > It's better to assert that forNode is indeed a HTML form element. Also, please rename m_forNode to m_formElement. > r- because of this. HTMLFieldset element also has a elements attribute which has to create HTMLFormCollection (It is not yet implemented, this bug blocks 80110). As forNode can be formElement or fieldset element, maybe generic name would be better. >> Source/WebCore/html/RadioNodeList.cpp:56 >> + continue; > > This continue will never be executed because nodeMatches returns false for non-element nodes. > It's better to assert in this case. Will do this change. >> Source/WebCore/html/RadioNodeList.cpp:79 >> + continue; > > We're repeating this logic here. Please extract it as a helper function. Yes, will do this also. >> LayoutTests/fast/forms/form-collection-radio-node-list.html:16 >> + '<button id=button1></button>' + > > You said there are cases RadioNodeList needs to return an element that's not necessarily in the subtree for form. > If that's really the case, then we definitely need a test case for that. > But looking at your code and also the spec, I don't see how that's possible. It is covered here: LayoutTests/fast/forms/form-collection-radio-node-list.html:16 shouldBe('document.body.appendChild(nonSubtreeElement); radioNodeList.length', '5');

Rakesh

Comment 38 2012-04-10 19:59:15 PDT

Created attachment 136614 [details] Updated patch Updated patch with review comments incorporated

Ryosuke Niwa

Comment 39 2012-04-10 20:05:34 PDT

(In reply to comment #37) > (From update of attachment 136420 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136420&action=review > Thanks for reviewing this patch. > > >> Source/WebCore/html/RadioNodeList.cpp:39 > >> + : DynamicSubtreeNodeList(forNode->hasTagName(formTag) ? forNode->document() : forNode) > > > > When can forNode not be formTag? As far as I know, HTMLFormElement::elements is the only function that creates HTMLFormCollection. > > It's better to assert that forNode is indeed a HTML form element. Also, please rename m_forNode to m_formElement. > > r- because of this. > > HTMLFieldset element also has a elements attribute which has to create HTMLFormCollection (It is not yet implemented, this bug blocks 80110). > As forNode can be formElement or fieldset element, maybe generic name would be better. "forNode" is definitely not generic. The only reason it's named "for" in labels node list is because it IS the element specified by the "for" content attribute.

Ryosuke Niwa

Comment 40 2012-04-10 20:10:33 PDT

(In reply to comment #37) > (From update of attachment 136420 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136420&action=review > Thanks for reviewing this patch. > > >> Source/WebCore/html/RadioNodeList.cpp:39 > >> + : DynamicSubtreeNodeList(forNode->hasTagName(formTag) ? forNode->document() : forNode) > > > > When can forNode not be formTag? As far as I know, HTMLFormElement::elements is the only function that creates HTMLFormCollection. > > It's better to assert that forNode is indeed a HTML form element. Also, please rename m_forNode to m_formElement. > > r- because of this. > > HTMLFieldset element also has a elements attribute which has to create HTMLFormCollection (It is not yet implemented, this bug blocks 80110). Also, we should not over-generalize the logic before implementing this feature. We should make necessary changes in the bug 80110 instead of pre-emptively adding the logic here. In general, every code we add in a patch should be tested by a test in the patch.

Ryosuke Niwa

Comment 41 2012-04-10 20:11:52 PDT

Comment on attachment 136614 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=136614&action=review > Source/WebCore/html/RadioNodeList.cpp:39 > + : DynamicSubtreeNodeList(forNode->hasTagName(formTag) ? forNode->document() : forNode) This condition is not needed at the moment since we don't implement one for html fieldset elements. r- due to this over-generalization. > Source/WebCore/html/RadioNodeList.cpp:93 > + if (m_forNode->hasTagName(formTag)) { Also, this should always be true for now.

Rakesh

Comment 42 2012-04-10 20:17:56 PDT

(In reply to comment #41) > (From update of attachment 136614 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136614&action=review > > > Source/WebCore/html/RadioNodeList.cpp:39 > > + : DynamicSubtreeNodeList(forNode->hasTagName(formTag) ? forNode->document() : forNode) > > This condition is not needed at the moment since we don't implement one for html fieldset elements. > r- due to this over-generalization. > > > Source/WebCore/html/RadioNodeList.cpp:93 > > + if (m_forNode->hasTagName(formTag)) { > > Also, this should always be true for now. You mean to say add these changes when implementing for fieldset's elements attribute.

Ryosuke Niwa

Comment 43 2012-04-10 20:19:04 PDT

(In reply to comment #42) > You mean to say add these changes when implementing for fieldset's elements attribute. Right. Otherwise, we'll be adding a bunch of un-tested, un-used code (at least for now).

Rakesh

Comment 44 2012-04-10 21:00:25 PDT

Created attachment 136618 [details] Updated patch Updated patch, making the implementation form specific to avoid untested code.

Build Bot

Comment 45 2012-04-10 22:23:26 PDT

Comment on attachment 136618 [details] Updated patch Attachment 136618 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12386190

Build Bot

Comment 46 2012-04-10 22:27:17 PDT

Comment on attachment 136618 [details] Updated patch Attachment 136618 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12385459

Rakesh

Comment 47 2012-04-10 23:06:20 PDT

Created attachment 136627 [details] Updated patch Updated patch with compilation fix for mac, helper function is static now.

Ryosuke Niwa

Comment 48 2012-04-10 23:22:03 PDT

Comment on attachment 136627 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=136627&action=review > Source/WebCore/html/RadioNodeList.cpp:38 > +RadioNodeList::RadioNodeList(const AtomicString& name, Node* forNode) Please rename forNode to formElement. > Source/WebCore/html/RadioNodeList.cpp:41 > + , m_formNode(forNode) Ditto. It should be an Element. > Source/WebCore/html/RadioNodeList.cpp:57 > + Element* element = toElement(node); > + if (!element->hasTagName(inputTag)) > + return false; > + const HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(element); There is toInputElement() function that returns 0 when it's not an input element. > Source/WebCore/html/RadioNodeList.cpp:69 > + if (!isNodeRadioButton(node)) > + continue; > + const HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(node); > + if (!inputElement->checked()) > + continue; Would have been better if isNodeRadioButton was instead toRadioButtonInputElement and returned HTMLInputElement* so that we don't have to do this casting here. > Source/WebCore/html/RadioNodeList.cpp:83 > + if (!isNodeRadioButton(node)) > + continue; > + HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(node); > + if (inputElement->value() != val) > + continue; Ditto. > Source/WebCore/html/RadioNodeList.cpp:94 > + if (!formElement || (formElement != m_formNode)) You don't need parenthesis around formElement != m_formNode. > Source/WebCore/html/RadioNodeList.h:39 > + static PassRefPtr<RadioNodeList> create(const AtomicString& name, Node* forNode) Again, formElement, and it should be HTMLFormElement*. > Source/WebCore/html/RadioNodeList.h:56 > + RefPtr<Node> m_formNode; Ditto about the variable name. > LayoutTests/fast/forms/form-collection-radio-node-list.html:15 > +var container = document.createElement('div'); > +document.body.appendChild(container); > + > +container.innerHTML = '<form id="form1">' + Why do you have to create this dynamically? You can just put this outside of the script element and put some id on the div to grab it inside the script. That'll make the test a lot more easier to read.

Ryosuke Niwa

Comment 49 2012-04-11 12:11:20 PDT

Comment on attachment 136627 [details] Updated patch I think we need to address at least some of my review comments.

Rakesh

Comment 50 2012-04-11 23:12:39 PDT

(In reply to comment #49) > (From update of attachment 136627 [details]) > I think we need to address at least some of my review comments. Yes, surely. (In reply to comment #48) > Again, formElement, and it should be HTMLFormElement*. > AS we are not having HTMLFormElement dependency as such expect to compare that particular node's form is the same HTMLFormElement. Also in future it can be HTMLFieldsetElement hence I think Node* should be better. > Why do you have to create this dynamically? You can just put this outside of the script element and put some id on the div to grab it inside the script. > That'll make the test a lot more easier to read. If you think that helps in readability then will do this.

Rakesh

Comment 51 2012-04-12 01:58:59 PDT

Created attachment 136855 [details] Updated patch Updated patch with changes as per review comments

WebKit Review Bot

Comment 52 2012-04-12 03:23:42 PDT

Comment on attachment 136855 [details] Updated patch Rejecting attachment 136855 [details] from commit-queue. New failing tests: fast/forms/form-collection-radio-node-list.html Full output: http://queues.webkit.org/results/12396137

WebKit Review Bot

Comment 53 2012-04-12 03:23:49 PDT

Created attachment 136866 [details] Archive of layout-test-results from ec2-cq-02 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

WebKit Review Bot

Comment 54 2012-04-12 03:30:49 PDT

Comment on attachment 136855 [details] Updated patch Attachment 136855 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12393292 New failing tests: fast/forms/form-collection-radio-node-list.html

WebKit Review Bot

Comment 55 2012-04-12 03:30:57 PDT

Created attachment 136868 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Rakesh

Comment 56 2012-04-12 07:20:11 PDT

Created attachment 136904 [details] Updated patch We are getting an text diff of space when the form content is specified in html but when added dynamically added through script text diff is not observed. This issue is happening only in cr-linux build. Updated the patch to have form content added dynamically so that layout tests pass in all platforms.

Ryosuke Niwa

Comment 57 2012-04-12 10:16:19 PDT

(In reply to comment #56) > Created an attachment (id=136904) [details] > Updated patch > > We are getting an text diff of space when the form content is specified in html but when added dynamically added through script text diff is not observed. This issue is happening only in cr-linux build. Updated the patch to have form content added dynamically so that layout tests pass in all platforms. The failure probably happens on all platforms. It's just that only cr-linux bot run tests. All other EWS bots never run tests. Again, please don't generate form content dynamically.

Rakesh

Comment 58 2012-04-18 08:28:46 PDT

Created attachment 137700 [details] Update patch Updated the expected.txt with the one generated with chromium port. Veried with mac port, expected.txt generated for mac and chromium port are same. I think some issue with my GTK build, it is not able to run Dumprendertree properly, may be some issue of dependencies.

Ryosuke Niwa

Comment 59 2012-04-21 19:12:05 PDT

Comment on attachment 137700 [details] Update patch View in context: https://bugs.webkit.org/attachment.cgi?id=137700&action=review The patch looks fantastic now. Please fix the nits below before you land it or I'll gladly cq+ if you upload a new patch. > Source/WebCore/html/RadioNodeList.cpp:72 > +void RadioNodeList::setValue(const String& val) Please don't use abbreviations like val. > LayoutTests/fast/forms/form-collection-radio-node-list.html:41 > + </form> You should output this form for consistency. > LayoutTests/fast/forms/form-collection-radio-node-list.html:68 > +debug("Checking value IDL attribute on the RadioNodeList"); > +shouldBe('radioNodeList.value', '""'); > +shouldBe('radioNodeList.value = "inputRadioValue"; radioNodeList[2].checked', 'true'); Maybe we should also check the name of class? (i.e. prototype name).

Rakesh

Comment 60 2012-04-23 14:28:30 PDT

Created attachment 138426 [details] PatchForLanding Fixed the nits.

WebKit Review Bot

Comment 61 2012-04-23 19:10:55 PDT

Comment on attachment 138426 [details] PatchForLanding Rejecting attachment 138426 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: atching file Source/WebCore/CMakeLists.txt patching file Source/WebCore/WebCore.xcodeproj/project.pbxproj patching file LayoutTests/fast/forms/form-collection-radio-node-list-expected.txt patching file LayoutTests/fast/forms/form-collection-radio-node-list.html patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 1. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Ryosuke Ni..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12527106

WebKit Review Bot

Comment 62 2012-04-23 20:22:43 PDT

Nayan Kumar K

Comment 63 2012-04-23 23:41:56 PDT

Created attachment 138499 [details] Patch for landing

WebKit Review Bot

Comment 64 2012-04-24 01:11:47 PDT

Comment on attachment 138499 [details] Patch for landing Clearing flags on attachment: 138499 Committed r115009: <http://trac.webkit.org/changeset/115009>

Erik Arvidsson

Comment 65 2012-04-24 11:51:06 PDT

Does this work correctly when you change the name/id of an input? I don't see where you invalidate the node list?

Ryosuke Niwa

Comment 66 2012-04-24 12:16:25 PDT

(In reply to comment #65) > Does this work correctly when you change the name/id of an input? I don't see where you invalidate the node list? Ugh.. my review fail. It won't work at all. We need to add a new list on NodeListsNodeData (in NodeRareData.h) and then add a new method on Node that wraps ::create and add it to that list. We then need to modify ~RadioNodeList to remove this from the list. I'm revering the patch now.

Ryosuke Niwa

Comment 67 2012-04-24 14:29:04 PDT

Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/115112.

Rakesh

Comment 68 2012-04-25 09:34:35 PDT

Created attachment 138825 [details] Updated patch Updated patch with suggested changes and test coverage for change in id

Ryosuke Niwa

Comment 69 2012-04-26 01:13:59 PDT

Comment on attachment 138825 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=138825&action=review > Source/WebCore/dom/Node.cpp:2333 > + I think you'll also need to modify invalidateNodeListsCacheAfterAttributeChanged and add idAttr to the list of attributes it checks. > Source/WebCore/dom/Node.cpp:2975 > + AtomicString nameAtom = name; Why are we making a copy here? > Source/WebCore/html/RadioNodeList.cpp:95 > + if (HTMLInputElement* inputElement = testElement->toInputElement()) > + if (inputElement->isImageButton()) > + return false; You need curly brackets around the outer if (no curly brackets for the inner if). > Source/WebCore/html/RadioNodeList.cpp:97 > + return equalIgnoringCase(testElement->getIdAttribute(), m_name) || equalIgnoringCase(testElement->getNameAttribute(), m_name); Don't we need to check that it's a radio button?

Rakesh

Comment 70 2012-04-27 06:35:56 PDT

Created attachment 139190 [details] Updated patch Updated patch with changes and more test coverage

Rakesh

Comment 71 2012-04-27 06:38:01 PDT

(In reply to comment #69) > > > Source/WebCore/html/RadioNodeList.cpp:97 > > + return equalIgnoringCase(testElement->getIdAttribute(), m_name) || equalIgnoringCase(testElement->getNameAttribute(), m_name); > > Don't we need to check that it's a radio button? Here we want to match all the input elements, but only while setting/getting the value attribute we want check for radio button.

Ryosuke Niwa

Comment 72 2012-04-29 16:04:02 PDT

Comment on attachment 139190 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=139190&action=review > Source/WebCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). This line should appear before the description (i.e. lines 6 & 7) but still surrounded by blank lines. > Source/WebCore/html/RadioNodeList.cpp:80 > + if (!inputElement || inputElement->value() != value) > + continue; > + inputElement->setChecked(true); This is going to make all radio button with the specified value checked, which is not what the spec says. You need to bail out from the loop after making the first element checked. r- because of this bug. > LayoutTests/fast/forms/form-collection-radio-node-list.html:6 > +<head> > +<meta charset="utf-8"> > +<script src="../js/resources/js-test-pre.js"></script> > +</head> We don't need head for this test. You can just move the script element to the body.

Rakesh

Comment 73 2012-05-02 13:12:35 PDT

Created attachment 139868 [details] Updated patch Updated patch with changes as per review comments.

Rakesh

Comment 74 2012-05-02 13:16:32 PDT

Thanks for reviewing. > > + inputElement->setChecked(true); > > This is going to make all radio button with the specified value checked, which is not what the spec says. > You need to bail out from the loop after making the first element checked. > r- because of this bug. Yes, we need to bail out after first element is checked, uploaded new patch with this change.

Ryosuke Niwa

Comment 75 2012-05-07 13:33:40 PDT

Comment on attachment 139868 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=139868&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:24860 > - FD8C46EC154608E700A5910C /* AudioScheduledSourceNode.h in Headers */, > - 71A57DF2154BE25C0009D120 /* SVGPathUtilities.h in Headers */, > - 78D02BC6154A18DF00B62D05 /* CSSPropertyAnimation.h in Headers */, > - FD629EA3154B47160006D026 /* AudioBasicInspectorNode.h in Headers */, > + FD8C46EC154608E700A5910C /* AudioScheduledSourceNode.h in Headers */, > + 71A57DF2154BE25C0009D120 /* SVGPathUtilities.h in Headers */, > + 78D02BC6154A18DF00B62D05 /* CSSPropertyAnimation.h in Headers */, > + FD629EA3154B47160006D026 /* AudioBasicInspectorNode.h in Headers */, > + B658FFA21522EF3A00DD5595 /* JSRadioNodeList.h in Headers */, > + B658FFA61522EFAA00DD5595 /* RadioNodeList.h in Headers */, You should be using tabs instead of spaces here. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:27879 > - FD8C46EB154608E700A5910C /* AudioScheduledSourceNode.cpp in Sources */, > - 71A57DF1154BE25C0009D120 /* SVGPathUtilities.cpp in Sources */, > - 78D02BC5154A18DF00B62D05 /* CSSPropertyAnimation.cpp in Sources */, > - FD629EA4154B47160006D026 /* AudioBasicInspectorNode.cpp in Sources */, > + FD8C46EB154608E700A5910C /* AudioScheduledSourceNode.cpp in Sources */, > + 71A57DF1154BE25C0009D120 /* SVGPathUtilities.cpp in Sources */, > + 78D02BC5154A18DF00B62D05 /* CSSPropertyAnimation.cpp in Sources */, > + FD629EA4154B47160006D026 /* AudioBasicInspectorNode.cpp in Sources */, > + B658FFA11522EF3A00DD5595 /* JSRadioNodeList.cpp in Sources */, > + B658FFA51522EFAA00DD5595 /* RadioNodeList.cpp in Sources */, Ditto. > Source/WebCore/html/RadioNodeList.cpp:52 > +static HTMLInputElement* toRadioButtonInputElement(Node* node) You should add inline keyword here.

Rakesh

Comment 76 2012-05-08 08:00:45 PDT

Created attachment 140720 [details] patch for landing Patch for landing, fixed review comments.

Kent Tamura

Comment 77 2012-05-08 17:16:11 PDT

Comment on attachment 140720 [details] patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=140720&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). You need to fill this line with the actual reviewer name to land this patch without additional review. > LayoutTests/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). ditto.

Rakesh

Comment 78 2012-05-08 18:57:13 PDT

Created attachment 140847 [details] patch for landing Updated reviewer.

WebKit Review Bot

Comment 79 2012-05-08 20:22:31 PDT

Comment on attachment 140847 [details] patch for landing Clearing flags on attachment: 140847 Committed r116487: <http://trac.webkit.org/changeset/116487>

WebKit Review Bot

Comment 80 2012-05-08 20:23:04 PDT

All reviewed patches have been landed. Closing bug.

Kent Tamura

Comment 81 2012-05-08 21:10:02 PDT

I landed a follow-up patch: http://trac.webkit.org/changeset/116491

Note You need to log in before you can comment on or make changes to this bug.