Bug 81854 – RadioNodeList support in HTMLFormElement::elements

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

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

[Rakesh]

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 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 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 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 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 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 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 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 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]

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]

(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 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 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 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 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 on attachment 134059 [details]
updated patch

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

[Rakesh]

Created attachment 134534 [details]
Updated patch

Hopefully it passes all builds.

[Kent Tamura]

Add NodeList experts to Cc.

[Ryosuke Niwa]

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 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 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 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]

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]

Created attachment 135095 [details]
Updated patch

Updated patch with more test coverage.

[Ryosuke Niwa]

(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 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]

(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]

(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]

(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]

Created attachment 136223 [details]
Updated patch

Modified to use DynamicSubtreeNodeList.

[Rakesh]

Created attachment 136224 [details]
Updated patch

Missed cleanup in previous patch

[Rakesh]

(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]

(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]

(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]

Created attachment 136420 [details]
Updated patch

Updated patch using DynamicSubtreeNodeList and test coverage for non subtree element.

[Ryosuke Niwa]

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 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]

Created attachment 136614 [details]
Updated patch

Updated patch with review comments incorporated

[Ryosuke Niwa]

(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]

(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 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]

(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]

(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]

Created attachment 136618 [details]
Updated patch

Updated patch, making the implementation form specific to avoid untested code.

[Build Bot]

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 on attachment 136618 [details]
Updated patch

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

[Rakesh]

Created attachment 136627 [details]
Updated patch

Updated patch with compilation fix for mac, helper function is static now.

[Ryosuke Niwa]

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 on attachment 136627 [details]
Updated patch

I think we need to address at least some of my review comments.

[Rakesh]

(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]

Created attachment 136855 [details]
Updated patch

Updated patch with changes as per review comments

[WebKit Review Bot]

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]

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 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]

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]

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]

(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]

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 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]

Created attachment 138426 [details]
PatchForLanding

Fixed the nits.

[WebKit Review Bot]

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 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]

Created attachment 138499 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 138499 [details]
Patch for landing

Clearing flags on attachment: 138499

Committed r115009: <http://trac.webkit.org/changeset/115009>

[Erik Arvidsson]

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]

(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]

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

[Rakesh]

Created attachment 138825 [details]
Updated patch

Updated patch with suggested changes and test coverage for change in id

[Ryosuke Niwa]

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]

Created attachment 139190 [details]
Updated patch

Updated patch with changes and more test coverage

[Rakesh]

(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 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]

Created attachment 139868 [details]
Updated patch

Updated patch with changes as per review comments.

[Rakesh]

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 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]

Created attachment 140720 [details]
patch for landing

Patch for landing, fixed review comments.

[Kent Tamura]

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]

Created attachment 140847 [details]
patch for landing

Updated reviewer.

[WebKit Review Bot]

Comment on attachment 140847 [details]
patch for landing

Clearing flags on attachment: 140847

Committed r116487: <http://trac.webkit.org/changeset/116487>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

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