Bug 87051 - Centralize and clean-up table column iteration
Summary: Centralize and clean-up table column iteration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-21 15:08 PDT by Julien Chaffraix
Modified: 2012-05-22 11:31 PDT (History)
4 users (show)

See Also:


Attachments
Proposed change. Clean-up lots of code and validate our current implementation. (17.79 KB, patch)
2012-05-21 15:25 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-05-21 15:08:09 PDT
Currently there are 2 duplicated functions to iterate over your columns: one in FixedTableLayout and one in RenderTable. This makes some of the code confusing to read as we repeat the functions, also if the next-column logic was on RenderTableCol, we could more easily iterate over them.

Let's change make this code less confusing.
Comment 1 Julien Chaffraix 2012-05-21 15:25:26 PDT
Created attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.
Comment 2 Eric Seidel (no email) 2012-05-21 15:34:43 PDT
Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

LGTM.  Should probably let the EWSes run.
Comment 3 Julien Chaffraix 2012-05-21 15:39:52 PDT
Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

Thanks Eric, clearing the cq until the EWS have had time to run!
Comment 4 Eric Seidel (no email) 2012-05-21 15:45:47 PDT
:)  Well, my bet was that cr-linux would be the slow one, and any of them would set cq- if they failed. :)  But yes.
Comment 5 Julien Chaffraix 2012-05-22 08:58:14 PDT
Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

EWS all green! \o/
Comment 6 WebKit Review Bot 2012-05-22 09:43:05 PDT
Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

Clearing flags on attachment: 143115

Committed r117988: <http://trac.webkit.org/changeset/117988>
Comment 7 WebKit Review Bot 2012-05-22 09:43:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Darin Adler 2012-05-22 10:24:10 PDT
Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

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

> Source/WebCore/rendering/RenderTableCol.cpp:123
> +    if (RenderObject* firstChild = this->firstChild())
> +        return toRenderTableCol(firstChild);

What guarantees the first child of a RenderTableCol is always another RenderTableCol? I think this is missing an isTableCol check.
Comment 9 Julien Chaffraix 2012-05-22 11:31:59 PDT
Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

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

>> Source/WebCore/rendering/RenderTableCol.cpp:123
>> +        return toRenderTableCol(firstChild);
> 
> What guarantees the first child of a RenderTableCol is always another RenderTableCol? I think this is missing an isTableCol check.

I believe this is correct and doesn't need an extra check for the following reasons:
* only column-groups can have children (see RenderTableCol::canHaveChildren).
* for column-groups, we only allow columns to be our children per RenderTableCol::isChildAllowed (the logic is a bit outdated but that's the gist of it).