[webkit-reviews] review denied: [Bug 81854] RadioNodeList support in HTMLFormElement::elements : [Attachment 134534] Updated patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Rakesh <rakesh.kn at motorola.com>'s
request for review:
Bug 81854: RadioNodeList support in HTMLFormElement::elements
https://bugs.webkit.org/show_bug.cgi?id=81854

Attachment 134534: Updated patch
https://bugs.webkit.org/attachment.cgi?id=134534&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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?