[Webkit-unassigned] [Bug 81084] Fix rendering of replaced elements with relative dimensions within a table cell.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=81084


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #135815|review?                     |
               Flag|                            |




--- Comment #17 from Julien Chaffraix <jchaffraix at webkit.org>  2012-04-12 09:44:30 PST ---
(From update of attachment 135815)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.