Summary: | The difference between a column and a column group renderer is badly drawn | ||
---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
Component: | Tables | Assignee: | Julien Chaffraix <jchaffraix> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | dglazkov, eric, inferno, robert, tkent, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Attachments: |
Description
Julien Chaffraix
2012-05-25 18:03:02 PDT
Created attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.
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 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 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
Comment on attachment 144178 [details]
Refactor v1: s/isTableCol/isRenderTableCol/, add more helpers and spray them around the code.
Not ready for prime time.
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. 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()' 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. Created attachment 144674 [details]
Proposed fix 2: With the crasher?
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 ? 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? Created attachment 144950 [details]
Patch for landing if no objection.
(In reply to comment #12) > Created an attachment (id=144950) [details] > Patch for landing if no objection. Sounds good. Comment on attachment 144950 [details] Patch for landing if no objection. Clearing flags on attachment: 144950 Committed r119110: <http://trac.webkit.org/changeset/119110> All reviewed patches have been landed. Closing bug. |