Bug 120222 – Reduce use of Node in HTMLTableRowsCollection, and use modern traversal idiom

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Darin Adler]

Reduce use of Node in HTMLTableRowsCollection, and use modern traversal idiom

[Darin Adler]

Created attachment 209492 [details]
Patch

[Darin Adler]

Created attachment 209495 [details]
Patch

[Antti Koivisto]

Comment on attachment 209495 [details]
Patch

r=me

[Build Bot]

Comment on attachment 209495 [details]
Patch

Attachment 209495 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1555322

New failing tests:
fast/dom/HTMLTableElement/rows.html

[Build Bot]

Created attachment 209502 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Darin Adler]

Committed r154515: <http://trac.webkit.org/changeset/154515>

[Benjamin Poulain]

Comment on attachment 209495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209495&action=review

> Source/WebCore/html/HTMLTableRowsCollection.cpp:74
> +        if (auto row = Traversal<HTMLTableRowElement>::nextSibling(previous))
> +            return row;

Hum, I am not sure I like the "auto" type.

It simplify the code for the author, you knows what type you are dealing with. But reading this two lines, I think it makes it harder to read the code.
When I read "auto", my brain start looking for clues to resolve the type before continuing to read.

[Darin Adler]

(In reply to comment #7)
> > Source/WebCore/html/HTMLTableRowsCollection.cpp:74
> > +        if (auto row = Traversal<HTMLTableRowElement>::nextSibling(previous))
> > +            return row;
> 
> Hum, I am not sure I like the "auto" type.
> 
> It simplify the code for the author, you knows what type you are dealing with. But reading this two lines, I think it makes it harder to read the code.
> When I read "auto", my brain start looking for clues to resolve the type before continuing to read.

Maybe part of this is that you are not used to auto yet.

The type is right there, HTMLTableRowElement. If I did not use auto I would have had to repeat HTMLTableRowElement twice on the same line of code.

[Tim Horton]

fast/dom/HTMLTableElement/rows.html seems to be timing out on all the bots now, seems likely this is related?

[Darin Adler]

(In reply to comment #9)
> fast/dom/HTMLTableElement/rows.html seems to be timing out on all the bots now, seems likely this is related?

Oops. OK to roll this out, and I’ll look into it later when I have a chance.

[Ryosuke Niwa]

Fixed in http://trac.webkit.org/changeset/154532.