[webkit-reviews] review denied: [Bug 73969] NodeChildList shouldn't be in NodeListNodeData : [Attachment 118613] Updated per Darin's comment; also fixed ChildNodeList::itemWithName

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 73969: NodeChildList shouldn't be in NodeListNodeData
https://bugs.webkit.org/show_bug.cgi?id=73969

Attachment 118613: Updated per Darin's comment; also fixed
ChildNodeList::itemWithName
https://bugs.webkit.org/attachment.cgi?id=118613&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118613&action=review


review- because of the offset problem

> Source/WebCore/dom/ChildNodeList.cpp:114
> +    if (m_node->isDocumentNode() || m_node->inDocument()) {

Why do we need to check isDocumentNode()? Doesn't inDocument() return true for
document nodes themselves?

> Source/WebCore/dom/ChildNodeList.cpp:118
> +	   // In the case of multiple nodes with the same name, just fall
through.

Shouldn’t this comment say “the same ID”?

> Source/WebCore/dom/ChildNodeList.cpp:121
> +    unsigned offset = 0;

I think you forgot the code that updates offset. It’s always zero!

Why didn’t any test case catch this? I think we may need to add test cases.

> Source/WebCore/dom/Node.cpp:1016
>      ASSERT(rareData());

Shouldn’t this assertion say ASSERT(hasRareData())?