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




--- Comment #23 from Julien Chaffraix <jchaffraix at webkit.org>  2012-04-19 10:05:07 PST ---
(From update of attachment 137859)
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).

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