[webkit-reviews] review granted: [Bug 11645] Table with percentage column widths doesn't scale to fill the entire width of a table containing it : [Attachment 167479] Patch

Tue Oct 16 17:38:01 PDT 2012

Julien Chaffraix <jchaffraix at webkit.org> has granted Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 11645: Table with percentage column widths doesn't scale to fill the entire
width of a table containing it
https://bugs.webkit.org/show_bug.cgi?id=11645

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

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

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:1
> +<!DOCTYPE html>

FWIW, this could have used check-layout.js
(http://lists.webkit.org/pipermail/webkit-dev/2012-October/022490.html). The
upside would be to remove the ref-test that ends up being super close to the
test case.

Note that the ref-test approach works OK in this case too.

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:6
> +<p>No red background color should be visible for the following two
tables.</p>

For the record, you *do* see some red which makes this sentence confusing for
anyone not familiar with tables.

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:10
> +	       <table style="width: 100%; background-color: red;">

Add: style="...; border-spacing: 0px;" to your list...

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:12
> +		       <td>

... and add style="padding: 0px" here too and no red should be shown.

> LayoutTests/fast/table/scale-nested-percent-width-cols.html:13
> +			   <table style="background-color: green;">

I would support moving this common style into a class shared by the 2 examples.

> LayoutTests/platform/chromium/TestExpectations:3758
> +# Requires rebaselining after https://bugs.webkit.org/show_bug.cgi?id=11645
> +webkit.org/b/11645 fast/table/025.html [ Failure ]

You are right to add that.

A couple of comments:
* What happened to gtk, win and efl?
* For Chromium, you really have the choice. Disabling it and doing the
rebaselining yourself with webkit-patch rebaseline-server (or
rebaseline-expectations) or just omitting so that the gardener picks the new
baselines when the patch land.