[webkit-reviews] review granted: [Bug 91334] REGRESSION(r122660): Cannot iterate over HTMLCollection that contains non-child descendent nodes in some conditions : [Attachment 152449] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 91334: REGRESSION(r122660): Cannot iterate over HTMLCollection that
contains non-child descendent nodes in some conditions
https://bugs.webkit.org/show_bug.cgi?id=91334

Attachment 152449: Patch
https://bugs.webkit.org/attachment.cgi?id=152449&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review


Please make the test actually assert that we're iterating in the right order
before committing.

> Source/WebCore/ChangeLog:10
> +	   which is visited traverseNextNode immeidatley before reaching the
root node.

typo: immediately

> Source/WebCore/html/HTMLCollection.cpp:254
> +    node = node->lastChild();
> +    for (Node* current = node; current; current = current->lastChild())
> +	   node = current;
> +    return node;

nit: i think this would be more readable as a while loop:
Node* node = 0;
while(Node* next = node->lastChild())
    node = next;
return node;

>
LayoutTests/fast/dom/htmlcollection-backwards-subtree-iteration-expected.txt:1
> +Tests that HTMLCollection of a subtree (as supposed to direct children) can
be iterated backwards.

s/supposed/opposed

> LayoutTests/fast/dom/htmlcollection-backwards-subtree-iteration.html:10
> +    images[0].class = 'foo';
> +    images[i - 1].class = 'foo';

Would be nice if you asserted that you're getting the image elements in the
right order. You could put an ID on each one in order to verify it's location.