Bug 91334

Created attachment 152449 [details] Patch

My attempt to unify HTMLCollection & DynamicNodeList revealed a regression.

Comment on attachment 152449 [details] Patch 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.

Comment on attachment 152449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review >> Source/WebCore/html/HTMLCollection.cpp:254 >> + 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; That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node.

Committed r122689: <http://trac.webkit.org/changeset/122689>

(In reply to comment #4) > (From update of attachment 152449 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review > > >> Source/WebCore/html/HTMLCollection.cpp:254 > >> + 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; > > That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node. Isn't that what this code does? I don't see where the crash is.

(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 152449 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review > > > > >> Source/WebCore/html/HTMLCollection.cpp:254 > > >> + 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; > > > > That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node. > > Isn't that what this code does? I don't see where the crash is. You're initializing node to 0. The first call to node->lastChild() will result in a null pointer access.

Comment on attachment 152449 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152449&action=review >>>>> Source/WebCore/html/HTMLCollection.cpp:254 >>>>> + 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; >>> >>> That'll just crash. Note that we need to return 0 when node->lastChild() is 0, not node. >> >> Isn't that what this code does? I don't see where the crash is. > > You're initializing node to 0. The first call to node->lastChild() will result in a null pointer access. lol. whoops.