Bug 87556 – The difference between a column and a column group renderer is badly drawn

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently when someone uses a RenderTableCol, we don't know if they expect to get a column or a column-group renderer.

The cause is that we re-use the same class for both objects but also the current functions don't draw the attention on what they return (column vs column group). There is not even a way to know what type of RenderTableCol you have at the moment and the only isTableCol() "type" getter is super confusing.

[Julien Chaffraix]

Created attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.

[Build Bot]

Comment on attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.

Attachment 144178 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12792668

[WebKit Review Bot]

Comment on attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.

Attachment 144178 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12795704

New failing tests:
css2.1/20110323/border-conflict-element-025.htm
css2.1/20110323/border-conflict-element-030.htm
css2.1/20110323/border-conflict-element-026.htm
css2.1/20110323/border-conflict-element-028.htm
css2.1/20110323/border-conflict-element-027.htm
css2.1/20110323/border-conflict-element-035.htm
css2.1/20110323/border-conflict-element-016.htm
css2.1/20110323/border-conflict-element-034.htm
css2.1/20110323/border-conflict-element-008.htm
css2.1/20110323/border-conflict-element-007.htm
css2.1/20110323/border-conflict-element-022.htm
css2.1/20110323/border-conflict-element-036.htm
http/tests/inspector/network/network-initiator-from-console.html
http/tests/inspector/network/network-sidebar-width.html
css2.1/20110323/border-conflict-element-037.htm
css2.1/20110323/border-conflict-element-032.htm
css2.1/20110323/border-conflict-element-010.htm
css2.1/20110323/border-conflict-element-029.htm
css2.1/20110323/border-conflict-element-033.htm
css2.1/20110323/border-conflict-element-031.htm

[WebKit Review Bot]

Created attachment 144186 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Julien Chaffraix]

Comment on attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.

Not ready for prime time.

[Abhishek Arya]

Comment on attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.

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

> Source/WebCore/dom/Text.cpp:-202
> -    if (par->isTable() || par->isTableRow() || par->isTableSection() || par->isTableCol() || par->isFrameSet())

par is a crazy variable name used in this function, can we change it to parent please.

> Source/WebCore/editing/htmlediting.cpp:-379
> -    return (r && (r->isTableCell() || r->isTableRow() || r->isTableSection() || r->isTableCol()));

s/r/renderer and can you please the incorrect location of '*' in line above. Good for the cleanup. Also, i think it does need to cover captions, so we can use isTablePart here. Otherwise, a FIXME will be good.

> Source/WebCore/html/HTMLFormElement.cpp:119
>          || (parentRenderer->isTableCell() && node->hasTagName(trTag));

I think this code is missing the table caption check since a few lines below, it does check for caption display.

bool formIsTablePart = display == TABLE || display == INLINE_TABLE || display == TABLE_ROW_GROUP
        || display == TABLE_HEADER_GROUP || display == TABLE_FOOTER_GROUP || display == TABLE_ROW
        || display == TABLE_COLUMN_GROUP || display == TABLE_COLUMN || display == TABLE_CELL
        || display == TABLE_CAPTION;

> Source/WebCore/rendering/RenderObject.cpp:275
> +        RenderTableCol* newTableColumn = toRenderTableCol(newChild);

newTableColumn is a little confusing since it can be a column group. How about we just keep it as newTableCol

> Source/WebCore/rendering/RenderObject.cpp:276
> +        bool isColumnInColumnGroup = newTableColumn->isTableColumn() && isRenderTableCol();

Should we add '&& toRenderTableCol(this)->isTableColumnGroup()' ?

> Source/WebCore/rendering/RenderTableCol.cpp:74
> +    ASSERT(isTableColumnGroup());

What is guaranteeing this ? Why can't a table column add a table column, is our parser ensuring that never happens ? Should we do two HARD ifs here to check which children go for isTableColumn(no children probably) and isTableColumnGroup(just table column children).

> Source/WebCore/rendering/RenderTableCol.cpp:126
> +    ASSERT(parentColumnGroup->isTableColumnGroup());

Will this always work, right now RenderTableCol::isChildAllowed just checks for isTableColumn ?

> Source/WebCore/rendering/RenderTableCol.cpp:127
> +    ASSERT(isTableColumn());

This should probably be moved to the start of function.

[Abhishek Arya]

We should also virtualize isTableColumn and isTableColumnGroup to RenderObject so that statements like these 'child->isRenderTableCol() && toRenderTableCol(child)->isTableColumn()' will be simplified to 'child->isTableColumn()'

[Julien Chaffraix]

Comment on attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.

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

> We should also virtualize isTableColumn and isTableColumnGroup to RenderObject so that statements like these 'child->isRenderTableCol() && toRenderTableCol(child)->isTableColumn()' will be simplified to 'child->isTableColumn()'

I am not a huge fan of that. First you can achieve the same results without adding more virtual work:

bool isTableColumn() const { return isRenderTableCol() && toRenderTableCol(this)->isTableColumn(); }

But mostly, the only code that cares about the difference between column and column-group is already manipulating a RenderTableCol.

>> Source/WebCore/dom/Text.cpp:-202
>> -    if (par->isTable() || par->isTableRow() || par->isTableSection() || par->isTableCol() || par->isFrameSet())
> 
> par is a crazy variable name used in this function, can we change it to parent please.

Good catch, will do. It really looks like those checks should be isTablePart() as nothing indicated that captions should be ignored.

>> Source/WebCore/html/HTMLFormElement.cpp:119
>>          || (parentRenderer->isTableCell() && node->hasTagName(trTag));
> 
> I think this code is missing the table caption check since a few lines below, it does check for caption display.
> 
> bool formIsTablePart = display == TABLE || display == INLINE_TABLE || display == TABLE_ROW_GROUP
>         || display == TABLE_HEADER_GROUP || display == TABLE_FOOTER_GROUP || display == TABLE_ROW
>         || display == TABLE_COLUMN_GROUP || display == TABLE_COLUMN || display == TABLE_CELL
>         || display == TABLE_CAPTION;

You are totally right. I will put a FIXME about it.

>> Source/WebCore/rendering/RenderObject.cpp:276
>> +        bool isColumnInColumnGroup = newTableColumn->isTableColumn() && isRenderTableCol();
> 
> Should we add '&& toRenderTableCol(this)->isTableColumnGroup()' ?

No. Only column groups can have any children. If isColumnInColumnGroup is true, I would expect |this| to be a table column group. I poundered over adding the following ASSERT:

ASSERT(!isColumnInColumnGroup || toRenderTableCol(this)->isTableColumnGroup());

but it looked messy.

>> Source/WebCore/rendering/RenderTableCol.cpp:74
>> +    ASSERT(isTableColumnGroup());
> 
> What is guaranteeing this ? Why can't a table column add a table column, is our parser ensuring that never happens ? Should we do two HARD ifs here to check which children go for isTableColumn(no children probably) and isTableColumnGroup(just table column children).

canHaveChildren will return false for table column (see below), which *should* ensure that we never call isChildAllowed for table column.

>> Source/WebCore/rendering/RenderTableCol.cpp:126
>> +    ASSERT(parentColumnGroup->isTableColumnGroup());
> 
> Will this always work, right now RenderTableCol::isChildAllowed just checks for isTableColumn ?

Yes it will. We check that our parent() is a RenderTableCol. The only possibility is if it's a table column group. You misread the logic: canHaveChildren ensures that only table column groups can have children and isChildAllowed only allows column groups to have children that are columns.

>> Source/WebCore/rendering/RenderTableCol.cpp:127
>> +    ASSERT(isTableColumn());
> 
> This should probably be moved to the start of function.

Actually no. It's valid to call enclosingColumnGroup on a column group.

[Julien Chaffraix]

Created attachment 144674 [details]
Proposed fix 2: With the crasher?

[Abhishek Arya]

Comment on attachment 144674 [details]
Proposed fix 2: With the crasher?

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

Looks like you are accidently changing a acid3 png ? can you check.

> Source/WebCore/editing/htmlediting.cpp:379
> +    return (renderer && (renderer->isTableCell() || renderer->isTableRow() || renderer->isTableSection() || renderer->isRenderTableCol()));

Do we want to use isTablePart here ?

> Source/WebCore/rendering/AutoTableLayout.cpp:173
> +        if (column->isTableColumn() && !column->nextSibling())

What guarantees that our parent is a column group. can this not be an independent table column without being enclosed in column group. Original code does a parent check for isTableCol before changing groupLogicalWidth ?

[Julien Chaffraix]

Comment on attachment 144674 [details]
Proposed fix 2: With the crasher?

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

>> Source/WebCore/editing/htmlediting.cpp:379
>> +    return (renderer && (renderer->isTableCell() || renderer->isTableRow() || renderer->isTableSection() || renderer->isRenderTableCol()));
> 
> Do we want to use isTablePart here ?

Most of the code checking the different table parts may want to use the helper function (I don't know editing or forms enough to know that). I don't want to change that now as it is an obvious change in behavior (we match more or less elements which may have visible changes).

>> Source/WebCore/rendering/AutoTableLayout.cpp:173
>> +        if (column->isTableColumn() && !column->nextSibling())
> 
> What guarantees that our parent is a column group. can this not be an independent table column without being enclosed in column group. Original code does a parent check for isTableCol before changing groupLogicalWidth ?

You are totally right: nothing guarantees that our parent is a column group.

It will match a column without any next sibling but I expect this situation to be rare (this means you have no rows or cells so it's an empty table anyway). Also it properly resets the column group width in the case that interests us.

I can add && parent()->isRenderTableCol() if you prefer?

[Julien Chaffraix]

Created attachment 144950 [details]
Patch for landing if no objection.

[Abhishek Arya]

(In reply to comment #12)
> Created an attachment (id=144950) [details]
> Patch for landing if no objection.

Sounds good.

[WebKit Review Bot]

Comment on attachment 144950 [details]
Patch for landing if no objection.

Clearing flags on attachment: 144950

Committed r119110: <http://trac.webkit.org/changeset/119110>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.