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.
Created attachment 143115 [details] Proposed change. Clean-up lots of code and validate our current implementation.
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 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!
:) 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 on attachment 143115 [details] Proposed change. Clean-up lots of code and validate our current implementation. EWS all green! \o/
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>
All reviewed patches have been landed. Closing bug.
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 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).