[webkit-reviews] review denied: [Bug 109462] REGRESSION(r130774): table isn't sized after content : [Attachment 188114] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Pravin D
<pravind.2k4 at gmail.com>'s request for review:
Bug 109462: REGRESSION(r130774): table isn't sized after content
https://bugs.webkit.org/show_bug.cgi?id=109462

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

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


I don't think you are fixing the right place. At line 256, there is an 'if
(!numAuto || totalWidth > tableLogicalWidth) {'. This 'if' looks suspicious to
me as I don't see how both clauses are related.

I would be better with the following change:
* make sure tableLogicalWidth can accomodate totalWidth (probably renamed
totalColumnContentWidths).
* after the if (...) else (...) (or at the end) always write tableLogicalWidth
back to the table.

> Source/WebCore/ChangeLog:3
> +	   REGRESSION(r130774): table isn't sized after content

Please rename the bug to something more explicit. I filed it without really
understanding the issue.

> Source/WebCore/rendering/FixedTableLayout.cpp:274
> +			   calcWidth[i] = m_width[i].percent() * max(0,
(tableLogicalWidth - totalFixedWidth)) / totalPercent;

One test for that seems too low. Could you have more testing to this to ensure
we don't regress other cases. I would like at least:
* several percentage.
* mix of fixed, percentage + sometimes auto.

> LayoutTests/fast/table/css-table-max-width-expected.txt:17
> +PASS maxWidthZeroFixedLayout.getBoundingClientRect().width is 2

Would be nice to have an explanation for the 2 (basically the default padding).