[webkit-reviews] review canceled: [Bug 81084] Rendering replaced elements with percentage height within a table cell (with auto height) : [Attachment 132759] 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 percentage height within a table
cell (with auto height)
https://bugs.webkit.org/show_bug.cgi?id=81084

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

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


The patch cannot be applied by our tools due to the SVN property (which doesn't
look right to me anyway).

> Source/WebCore/rendering/RenderReplaced.cpp:586
> +    while (cb && !cb->isRenderView() && (cb->style()->height().isAuto() ||
cb->style()->height().isPercent())) {
> +	   if (cb->isTableCell())
> +	       isTableCellHeightAuto = true;
> +	   cb = cb->containingBlock();
> +    }

You are making hasRelativeDimensions O(n) instead of O(1) as it was before. I
think that's fine as it's rarely used.

Please add a break statement if you find a table cell as it's useless to
continue walking your ancestor's chain.

> Source/WebCore/rendering/RenderReplaced.cpp:590
> +    return (style()->width().isPercent() || (style()->height().isPercent()
&& !isTableCellHeightAuto)
> +	   || style()->maxWidth().isPercent() ||
(style()->maxHeight().isPercent() && !isTableCellHeightAuto)
> +	   || style()->minWidth().isPercent() ||
(style()->minHeight().isPercent() && !isTableCellHeightAuto));

I don't understand why you single out height here. If you encountered a cell's
containing block with auto or percent width, wouldn't you need the same
exception?

Your testing would need to cover that too.

> Source/WebCore/rendering/RenderReplaced.h:41
> +    virtual bool hasRelativeDimensions() const;

Add the 'OVERRIDE' keyword at the end.

> LayoutTests/ChangeLog:9
> +	   * platform/win/fast/replaced/table-cell-width-expected.txt: Added.

I don't think the output is platform-specific as you have a done a good job of
making it dumpAsText.

> LayoutTests/fast/replaced/table-cell-width.html:35
> +function parsePixelValue(str)

Used only by is75PercentOf so unused.

> LayoutTests/fast/replaced/table-cell-width.html:44
> +function is75PercentOf(expression75, expression100)

Unused function AFAICT.

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

No need for this. DRT dumps after the load event listener was run by default.

> LayoutTests/fast/replaced/table-cell-width.html:133
> +<table><tr><td id="canvas-75"><canvas style="background-color: #00ff00;
height: 75%;"></canvas></td><td>This is the second table cell</td><td
style="background-color: grey;"
width="100%"><span> </span></td></tr></table>
> +<table><tr><td id="canvas-100"><canvas style="background-color: #00ff00;
height: 100%;"></canvas></td><td>This is the second table cell</td><td
style="background-color: grey;"
width="100%"><span> </span></td></tr></table>

Please use some CSS class selector on those. That would make it way easier to
read the output. For example:
.75percentHeight {
   height: 75%
}

canvas {
   background-color: #0F0;
}

...

> LayoutTests/platform/win/fast/replaced/table-cell-width-expected.txt:30
> +This is the second table cell	 
> +This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +
> +This is the second table cell	 
> +
> +This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +Button	This is the second table cell	 
> +Button	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 
> +	This is the second table cell	 

Not a huge fan of those. You could wrap all of them in an element and remove it
before at the end of your 'load' event listener.

> LayoutTests/platform/win/fast/replaced/table-cell-width-expected.txt:36
> +PASS getWidth('canvas-75') is '224px'

This test (among others) is failing in FF. I haven't thought much about it but
setting the size on your tables (or wrapping them in a fixed sized container)
may help here.