[webkit-reviews] review canceled: [Bug 86671] Standalone table-columns should be wrapped in anonymous tables : [Attachment 142383] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 86671: Standalone table-columns should be wrapped in anonymous tables
https://bugs.webkit.org/show_bug.cgi?id=86671

Attachment 142383: Patch
https://bugs.webkit.org/attachment.cgi?id=142383&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142383&action=review


You will need to investigate why fast/table/form-with-table-style.html has such
a weird change in sizing (a quick glance didn't give me any insight of what's
going wrong).

> Source/WebCore/rendering/RenderObject.cpp:279
>      if (newChild->isTableCol() && newChild->style()->display() ==
TABLE_COLUMN_GROUP)
>	   needsTable = !isTable();
> +    else if (newChild->isTableCol())
> +	   needsTable = !isTable() && style()->display() != TABLE_COLUMN_GROUP;


I think we could squash those 2 ifs with the following:

if (newChild->isTableCol())
    needsTable = !isTable();

isTableCol() returns true for both TABLE_COLUMN_GROUP and TABLE_COLUMN which is
what you need to cover.

> LayoutTests/fast/table/table-column-generates-anonymous-table.html:4
> +

Please, please, please, don't forget to include the following in your test:
* bug id (even better *clickable* link to this bug).
* bug title.

> LayoutTests/fast/table/table-column-generates-anonymous-table.html:6
> +<div style="display:table-column; background-color: green;"></div>
> +<div style="display:table-row;"><div style="display:table-cell;">This box
should be green</div></div>

If you don't need to use CSS tables (which is the case here AFAICT), you can
just use the html equivalent:
* <table>
* <col> for columns
* <tr> for rows
* <td>, <th> for cells

> LayoutTests/fast/table/table-column-generates-anonymous-table.html:7
> +

Also let's remove the unneeded space.