[Webkit-unassigned] [Bug 81084] Rendering replaced elements with relative dimensions within a table cell (with auto\percent dimensions)

[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 #134576|review?                     |
               Flag|                            |




--- Comment #13 from Julien Chaffraix <jchaffraix at webkit.org>  2012-04-02 11:32:56 PST ---
(From update of attachment 134576)
View in context: https://bugs.webkit.org/attachment.cgi?id=134576&action=review

> Source/WebCore/ChangeLog:7
> +

ChangeLogs SHOULD be filled. Look at other ChangeLogs on how to do that.

> Source/WebCore/rendering/RenderReplaced.cpp:430
> +    // In case containing block is a table with auto/percent dimensions, the max preferred logical width should be used

This definitely needs a 'why' explanation. Normally table cells shrink to fit their internal content. Here you are forcing them to use our preferred logical width.

> Source/WebCore/rendering/RenderReplaced.cpp:432
> +    RenderBlock* cb = this->containingBlock();

Please don't abbreviate.

> Source/WebCore/rendering/RenderReplaced.cpp:433
> +    bool isTableAuto = false;

This name is bad: auto in table has different meaning than for any other element (auto table layout).

> Source/WebCore/rendering/RenderReplaced.cpp:436
> +        if (cb->isTable()) {

Shouldn't we be looking for a table cell not a table here?

> Source/WebCore/rendering/RenderReplaced.cpp:443
> +    if (hasRelativeDimensions() && !isTableAuto)

We really would like our containing block's chain crawling to happen only if we *do* have relative dimension. This means that the code above should be moved in an helper function: hasTableWithRelativeDimensionsInContainingBlockChain or something.

> LayoutTests/ChangeLog:3
> +        Rendering replaced elements with relative dimensions within a table cell (with auto\percent dimensions)

The bug name could made more explicit here: Fix rendering of replaced elements with relative dimensions within a table cell.

> LayoutTests/ChangeLog:15
> +        * platform/qt/fast/replaced/width100percent-image-expected.png:
> +        * platform/qt/fast/replaced/width100percent-image-expected.txt:

You should explain if / why this is a progression.

This test needs to be skipped on all other platforms. This is what is making the cr-linux EWS unhappy. Currently, you should test_expectations.txt for Chromium and Skipped for all other platforms (mac, mac-wk2, qt, gtk, efl, win-cairo)

> LayoutTests/fast/replaced/table-cell-width-percent.html:102
> +	var elements = document.getElementsByClassName('remove');
> +	for (i = 0; i < elements.length; i++) {
> +		if (elements[i])
> +			elements[i].innerText = "";
> +	}

Very neat trick. I think you could remove the leading empty lines by just doing:
var elements = document.getElementByTagName('table');
for (var i = 0; i < elements.length; ++i)
    elements[i].parentNode.removeChild(elements[i]);

> LayoutTests/platform/qt/fast/replaced/table-cell-width-percent-expected.txt:30
> +FAIL getWidth('canvas-td-width-percent') should be 300px. Was 530px.

Why is it failing? What are we testing here that we do not match? Is this expected?

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