[webkit-reviews] review denied: [Bug 14858] <col> width ignored when not tied to a single cell : [Attachment 47516] Proposed fix v2

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Dmitry Gorbik
<socket.h at gmail.com>'s request for review:
Bug 14858: <col> width ignored when not tied to a single cell
https://bugs.webkit.org/show_bug.cgi?id=14858

Attachment 47516: Proposed fix v2
https://bugs.webkit.org/attachment.cgi?id=47516&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> Index: WebCore/rendering/RenderTableCell.cpp
> -}
> +}		       

Unneeded whitespace change here.

> Index: LayoutTests/fast/table/col-width-span-expand-expected.txt
> ===================================================================
> --- LayoutTests/fast/table/col-width-span-expand-expected.txt (revision 0)
> +++ LayoutTests/fast/table/col-width-span-expand-expected.txt (revision 0)
> @@ -0,0 +1,35 @@
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x8
> +  RenderBlock {HTML} at (0,0) size 800x8
> +    RenderBody {BODY} at (8,8) size 784x0
> +layer at (350,16) size 36x18
> +  RenderBlock (positioned) {DIV} at (350,16) size 36x18 [color=#FF0000]
> +    RenderText {#text} at (0,0) size 36x18
> +	 text run at (0,0) width 36: "FAIL"
> +layer at (16,16) size 500x18
> +  RenderTable {TABLE} at (16,16) size 500x18
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableCol {COL} at (0,0) size 0x0
> +    RenderTableSection {TBODY} at (0,0) size 500x18
> +	 RenderTableRow {TR} at (0,0) size 500x18
> +	   RenderTableCell {TD} at (0,0) size 50x18 [bgcolor=#FFFFFF] [r=0 c=0
rs=1 cs=1]
> +	     RenderText {#text} at (0,0) size 4x18
> +	       text run at (0,0) width 4: " "
> +	   RenderTableCell {TD} at (50,0) size 250x18 [bgcolor=#FFFFFF] [r=0
c=1 rs=1 cs=2]
> +	     RenderText {#text} at (0,0) size 4x18
> +	       text run at (0,0) width 4: " "
> +	   RenderTableCell {TD} at (300,0) size 200x18 [bgcolor=#FFFFFF] [r=0
c=3 rs=1 cs=1]
> +	     RenderText {#text} at (0,0) size 4x18
> +	       text run at (0,0) width 4: " "
> +layer at (16,40) size 469x36
> +  RenderBlock (positioned) {DIV} at (16,40) size 469x36
> +    RenderText {#text} at (0,0) size 469x18
> +	 text run at (0,0) width 469: "Visual test: if you do not see the red
word \"FAIL\" then your have passed. "
> +    RenderBR {BR} at (469,0) size 0x18
> +    RenderInline {A} at (0,0) size 133x18 [color=#0000EE]
> +	 RenderText {#text} at (0,18) size 133x18
> +	   text run at (0,18) width 133: "WebKit Bug #14858"
> +    RenderText {#text} at (0,0) size 0x0

(In reply to comment #9)
> David, I do not think pixel or javascript tests are necessary here. Width of
> the cell is dumped in the RenderTree.

You are correct about the widths appearing in the render tree dump, but
unfortunately this is not the way text-only tests work in WebKit.  If you don't
use layoutTestController.dumpAsText(), run-webkit-tests and DumpRenderTree
expect that there are pixel test results.

This test should still be turned into a text-only test.  There are many
examples of text-only tests in the tree.  Start by searching for "dumpAsText()"
in another test that appears to check something similar.

We also still need a domain expert in rendering to review the change, although
my opinion is that the WebCore changes look good.

r- to fix the whitespace change above and to make the layout test a text-only
test.