[webkit-reviews] review canceled: [Bug 81084] Fix rendering of replaced elements with relative dimensions within a table cell. : [Attachment 135815] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled
arpitabahuguna at gmail.com's request for review:
Bug 81084: Fix rendering of replaced elements with relative dimensions within a
table cell.
https://bugs.webkit.org/show_bug.cgi?id=81084

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

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


Sorry for the delay.

It looks better, more comments but I like the new change.

> Source/WebCore/rendering/RenderReplaced.cpp:561
> +    bool isTableRelative = false;

I still don't like this variable name. Whenever you have a variable, think of
how you would explain its role in English. Reading this one makes me think that
|this| is a relative (sized? positioned?) table which it is not!

Better namings: hasAutoOrPercentContainingTable,
hasContainingTableWithRelativeDimensions

> Source/WebCore/rendering/RenderReplaced.cpp:564
> +    while (containingBlock && !containingBlock->isRenderView() &&
(containingBlock->style()->height().isAuto() ||
containingBlock->style()->height().isPercent()
> +	      || containingBlock->style()->width().isAuto() ||
containingBlock->style()->width().isPercent())) {

It's difficult to read because of all the conditions. You could split the
hasRelativeWidthOrHeight logic into its own static function (taking the style)
and maybe even switch to a |for| loop here:

for (const RenderBlock* containingBlock = this->containingBlock;
containingBlock && !containingBlock->isRenderView() &&
hasRelativeWidthOrHeight(containingBlock->style()); containingBlock =
containingBlock->containingBlock())

(it should be |const| as you don't expect it to be modified)

> LayoutTests/ChangeLog:8
> +	   * fast/replaced/table-cell-width-auto.html: Added.

You are missing the expected results for your 3 tests.