RESOLVED FIXED 81854
RadioNodeList support in HTMLFormElement::elements
https://bugs.webkit.org/show_bug.cgi?id=81854
Summary RadioNodeList support in HTMLFormElement::elements
Attachments
intial_patch (15.42 KB, patch)
2012-03-22 08:22 PDT, Rakesh
no flags
updated patch (26.55 KB, patch)
2012-03-27 07:46 PDT, Rakesh
no flags
Updated patch (34.87 KB, patch)
2012-03-29 02:42 PDT, Rakesh
no flags
Updated patch (36.04 KB, patch)
2012-04-02 07:27 PDT, Rakesh
no flags
Updated patch (34.71 KB, patch)
2012-04-09 07:43 PDT, Rakesh
no flags
Updated patch (34.29 KB, patch)
2012-04-09 07:53 PDT, Rakesh
no flags
Updated patch (34.53 KB, patch)
2012-04-10 03:48 PDT, Rakesh
no flags
Updated patch (34.54 KB, patch)
2012-04-10 19:59 PDT, Rakesh
no flags
Updated patch (34.44 KB, patch)
2012-04-10 21:00 PDT, Rakesh
no flags
Updated patch (34.46 KB, patch)
2012-04-10 23:06 PDT, Rakesh
no flags
Updated patch (34.31 KB, patch)
2012-04-12 01:58 PDT, Rakesh
no flags
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
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
Updated patch (34.42 KB, patch)
2012-04-12 07:20 PDT, Rakesh
no flags
Update patch (35.19 KB, patch)
2012-04-18 08:28 PDT, Rakesh
rniwa: review+
rniwa: commit-queue-
PatchForLanding (34.17 KB, patch)
2012-04-23 14:28 PDT, Rakesh
no flags
Patch for landing (35.60 KB, patch)
2012-04-23 23:41 PDT, Nayan Kumar K
no flags
Updated patch (41.27 KB, patch)
2012-04-25 09:34 PDT, Rakesh
no flags
Updated patch (41.98 KB, patch)
2012-04-27 06:35 PDT, Rakesh
no flags
Updated patch (43.20 KB, patch)
2012-05-02 13:12 PDT, Rakesh
no flags
patch for landing (42.00 KB, patch)
2012-05-08 08:00 PDT, Rakesh
no flags
patch for landing (42.00 KB, patch)
2012-05-08 18:57 PDT, Rakesh
no flags
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
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/12524144
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.