Bug 81854

Summary: RadioNodeList support in HTMLFormElement::elements
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Rakesh <rakeshchaitan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arv, dglazkov, haraken, japhet, kling, ojan, rakeshchaitan, rakuco, rniwa, webkit.review.bot
Priority: P2    
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

Comment 1 Rakesh 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
Comment 2 Early Warning System Bot 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
Comment 3 Early Warning System Bot 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
Comment 4 Build Bot 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
Comment 5 Gyuyoung Kim 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
Comment 6 Early Warning System Bot 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
Comment 7 Early Warning System Bot 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
Comment 8 Build Bot 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
Comment 9 Kent Tamura 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.
Comment 10 Rakesh 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)
Comment 11 Rakesh 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?
Comment 12 WebKit Review Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Gyuyoung Kim 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
Comment 16 WebKit Review Bot 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
Comment 17 Rakesh 2012-03-29 02:42:50 PDT
Created attachment 134534 [details]
Updated patch

Hopefully it passes all builds.
Comment 18 Kent Tamura 2012-04-01 21:51:47 PDT
Add NodeList experts to Cc.
Comment 19 Ryosuke Niwa 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?
Comment 20 Kent Tamura 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>
Comment 21 Kent Tamura 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.
Comment 22 Ryosuke Niwa 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?
Comment 23 Rakesh 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?
Comment 24 Rakesh 2012-04-02 07:27:45 PDT
Created attachment 135095 [details]
Updated patch

Updated patch with more test coverage.
Comment 25 Ryosuke Niwa 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.
Comment 26 Darin Adler 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.
Comment 27 Rakesh 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.
Comment 28 Rakesh 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.
Comment 29 Ryosuke Niwa 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.
Comment 30 Rakesh 2012-04-09 07:43:54 PDT
Created attachment 136223 [details]
Updated patch

Modified to use DynamicSubtreeNodeList.
Comment 31 Rakesh 2012-04-09 07:53:54 PDT
Created attachment 136224 [details]
Updated patch

Missed cleanup in previous patch
Comment 32 Rakesh 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.
Comment 33 Ryosuke Niwa 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.
Comment 34 Rakesh 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.
Comment 35 Rakesh 2012-04-10 03:48:31 PDT
Created attachment 136420 [details]
Updated patch

Updated patch using DynamicSubtreeNodeList and test coverage for non subtree element.
Comment 36 Ryosuke Niwa 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.
Comment 37 Rakesh 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');
Comment 38 Rakesh 2012-04-10 19:59:15 PDT
Created attachment 136614 [details]
Updated patch

Updated patch with review comments incorporated
Comment 39 Ryosuke Niwa 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.
Comment 40 Ryosuke Niwa 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.
Comment 41 Ryosuke Niwa 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.
Comment 42 Rakesh 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.
Comment 43 Ryosuke Niwa 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).
Comment 44 Rakesh 2012-04-10 21:00:25 PDT
Created attachment 136618 [details]
Updated patch

Updated patch, making the implementation form specific to avoid untested code.
Comment 45 Build Bot 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
Comment 46 Build Bot 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
Comment 47 Rakesh 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.
Comment 48 Ryosuke Niwa 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.
Comment 49 Ryosuke Niwa 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.
Comment 50 Rakesh 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.
Comment 51 Rakesh 2012-04-12 01:58:59 PDT
Created attachment 136855 [details]
Updated patch

Updated patch with changes as per review comments
Comment 52 WebKit Review Bot 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
Comment 53 WebKit Review Bot 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
Comment 54 WebKit Review Bot 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
Comment 55 WebKit Review Bot 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
Comment 56 Rakesh 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.
Comment 57 Ryosuke Niwa 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.
Comment 58 Rakesh 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.
Comment 59 Ryosuke Niwa 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).
Comment 60 Rakesh 2012-04-23 14:28:30 PDT
Created attachment 138426 [details]
PatchForLanding

Fixed the nits.
Comment 61 WebKit Review Bot 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
Comment 62 WebKit Review Bot 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
Comment 63 Nayan Kumar K 2012-04-23 23:41:56 PDT
Created attachment 138499 [details]
Patch for landing
Comment 64 WebKit Review Bot 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>
Comment 65 Erik Arvidsson 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?
Comment 66 Ryosuke Niwa 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.
Comment 67 Ryosuke Niwa 2012-04-24 14:29:04 PDT
Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/115112.
Comment 68 Rakesh 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
Comment 69 Ryosuke Niwa 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?
Comment 70 Rakesh 2012-04-27 06:35:56 PDT
Created attachment 139190 [details]
Updated patch

Updated patch with changes and more test coverage
Comment 71 Rakesh 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.
Comment 72 Ryosuke Niwa 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.
Comment 73 Rakesh 2012-05-02 13:12:35 PDT
Created attachment 139868 [details]
Updated patch

Updated patch with changes as per review comments.
Comment 74 Rakesh 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.
Comment 75 Ryosuke Niwa 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.
Comment 76 Rakesh 2012-05-08 08:00:45 PDT
Created attachment 140720 [details]
patch for landing

Patch for landing, fixed review comments.
Comment 77 Kent Tamura 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.
Comment 78 Rakesh 2012-05-08 18:57:13 PDT
Created attachment 140847 [details]
patch for landing

Updated reviewer.
Comment 79 WebKit Review Bot 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>
Comment 80 WebKit Review Bot 2012-05-08 20:23:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 81 Kent Tamura 2012-05-08 21:10:02 PDT
I landed a follow-up patch: http://trac.webkit.org/changeset/116491