Bug 120222 - Reduce use of Node in HTMLTableRowsCollection, and use modern traversal idiom
Summary: Reduce use of Node in HTMLTableRowsCollection, and use modern traversal idiom
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-23 12:27 PDT by Darin Adler
Modified: 2013-08-23 16:56 PDT (History)
6 users (show)

See Also:

Attachments
Patch (14.27 KB, patch)
2013-08-23 12:31 PDT, Darin Adler 		no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2013-08-23 12:48 PDT, Darin Adler 		koivisto: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (918.88 KB, application/zip)
2013-08-23 13:22 PDT, Build Bot 		no flags Details
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2013-08-23 12:27:17 PDT 
Reduce use of Node in HTMLTableRowsCollection, and use modern traversal idiom
Comment 1 Darin Adler 2013-08-23 12:31:10 PDT 
Created attachment 209492 [details]
Patch
Comment 2 Darin Adler 2013-08-23 12:48:16 PDT 
Created attachment 209495 [details]
Patch
Comment 3 Antti Koivisto 2013-08-23 12:53:28 PDT 
Comment on attachment 209495 [details]
Patch

r=me
Comment 4 Build Bot 2013-08-23 13:22:24 PDT 
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
Comment 5 Build Bot 2013-08-23 13:22:25 PDT 
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
Comment 6 Darin Adler 2013-08-23 13:22:28 PDT 
Committed r154515: <http://trac.webkit.org/changeset/154515>
Comment 7 Benjamin Poulain 2013-08-23 13:41:11 PDT 
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.
Comment 8 Darin Adler 2013-08-23 13:46:24 PDT 
(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.
Comment 9 Tim Horton 2013-08-23 15:25:07 PDT 
fast/dom/HTMLTableElement/rows.html seems to be timing out on all the bots now, seems likely this is related?
Comment 10 Darin Adler 2013-08-23 16:08:55 PDT 
(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.
Comment 11 Ryosuke Niwa 2013-08-23 16:56:07 PDT 
Fixed in http://trac.webkit.org/changeset/154532.