[webkit-reviews] review canceled: [Bug 81084] Fix rendering of replaced elements with relative dimensions within a table cell. : [Attachment 137859] Proposed 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 137859: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=137859&action=review

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


The style complains is just an existing issue on the platform. I overlooked the
tests in my previous review (my apologies). Corrected now...

> Source/WebCore/rendering/RenderReplaced.cpp:556
> +    ASSERT(style);

You don't need this ASSERT, RenderObject::style() cannot be NULL AFAICT (sorry
I forgot to mention it last time).

> Source/WebCore/rendering/RenderReplaced.cpp:558
> +    return (style->height().isAuto() || style->height().isPercent() ||
style->width().isAuto() || style->width().isPercent());

Unneeded parenthesis.

> LayoutTests/ChangeLog:33
> +	   * platform/qt/fast/replaced/table-cell-width-auto-expected.txt:
Added.
> +	   * platform/qt/fast/replaced/table-cell-width-fixed-expected.txt:
Added.
> +	   * platform/qt/fast/replaced/table-cell-width-percent-expected.txt:
Added.
> +	   Expected results for the newly added test cases for qt port.

Those results are not port specific. They should be moved new to the test
(again I missed that in the previous review).

> LayoutTests/fast/replaced/table-cell-width-auto.html:120
> +.greenBgColor {
> +	background-color: green;
> +}
> +.greyBgColor {
> +	background-color: grey;
> +}

Do we *really* need those?

> LayoutTests/fast/replaced/table-cell-width-auto.html:127
> +<table width="600px"><tr><td id="canvas-height-percent"><canvas
class="height100percent greenBgColor"></canvas></td><td><span
class="remove">This is the second table cell</span></td><td width="100%"
greyBgColor"><span class="remove"> </span></td></tr></table>

All your table are 600px width, you should replace that with a tag selector:
table {
    width: 600px;
}

> LayoutTests/fast/replaced/table-cell-width-auto.html:128
> +<table width="600px"><tr><td id="canvas-width-percent"><canvas
class="width100percent greenBgColor"></canvas></td><td><span
class="remove">This is the second table cell</span></td><td width="100%"
greyBgColor"><span class="remove"> </span></td></tr></table>

<td width="100%" greyBgColor"> <- there are some trailing unrelated stuffs (not
repeated).

The non-breaking space is unneeded AFAICT. As is the remove class.

> LayoutTests/fast/replaced/table-cell-width-fixed.html:3
> +<title>Test for Buzilla Bug 81084: Fix rendering of replaced elements with
relative dimensions within a table cell.</title>

My preference is for the bug id to be dumped in the output as it makes it
easier for port maintainer to find errors. Bonus point if it includes the URL
in a clickable link.

> LayoutTests/fast/replaced/table-cell-width-fixed.html:8
> +if (window.layoutTestController) {
> +    layoutTestController.dumpAsText();
> +}

AFAICT you don't need that as js-test-pre.js already does it for you.

> LayoutTests/fast/replaced/table-cell-width-fixed.html:14
> +    if (!element) {
> +	   return null;
> +    }

Style violation. Do you really want this to fail silently if you have no
elements?

> LayoutTests/fast/replaced/table-cell-width-fixed.html:101
> +		elements[0].parentNode.removeChild(elements[0]);
> +		elements = document.getElementsByTagName('table');

We prefer tests to follow WebKit coding style (4 spaces tab).