Bug 87051 – Centralize and clean-up table column iteration

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

[Eric Seidel (no email)]

Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

LGTM.  Should probably let the EWSes run.

[Julien Chaffraix]

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!

[Eric Seidel (no email)]

:)  Well, my bet was that cr-linux would be the slow one, and any of them would set cq- if they failed. :)  But yes.

[Julien Chaffraix]

Comment on attachment 143115 [details]
Proposed change. Clean-up lots of code and validate our current implementation.

EWS all green! \o/

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Julien Chaffraix]

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).