[webkit-reviews] review canceled: [Bug 81084] Rendering replaced elements with relative dimensions within a table cell (with auto\percent dimensions) : [Attachment 134576] Patch

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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?