[webkit-reviews] review granted: [Bug 106571] REGRESSION(r120616): Cell's logical height wrongly computed with vertical-align: baseline and rowspan : [Attachment 183808] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Robert Hogan
<robert at webkit.org>'s request for review:
Bug 106571: REGRESSION(r120616): Cell's logical height wrongly computed with
vertical-align: baseline and rowspan
https://bugs.webkit.org/show_bug.cgi?id=106571

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

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


r=me, if you can make the test not ref-tests, you get extra bonus points but I
will let you see if my suggestions work / make any sense.

>> Source/WebCore/rendering/RenderTableSection.cpp:331
>> +			    m_rowPos[cellStartRow + 1] =
max<int>(m_rowPos[cellStartRow + 1], m_rowPos[cellStartRow] +
m_grid[cellStartRow].baseline + baselineDescent);
> 
> Is this not open to race conditions as baselineDescent could increase *after*
you compute your next row's position? Adding some testing about that would be a
good addition regardless.
> 
> The old code was doing the sizing after walking over all the cells in a row
which prevents that (on top of being more efficient).

Walking through the code over, this is my misunderstanding. There is no race
condition as we keep track of the maximums and update them properly.

> LayoutTests/ChangeLog:11
> +	   * fast/css/vertical-align-baseline-rowspan-011.html: Added.

I agree it's better to group them (especially since they are not in fast/table
:() even if I still don't like the naming.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-010.html:25
> +		<td>XXX</td><td>XXX</td><td
rowspan="2"><img></td><td>XXX</td><td>XXX</td>
> +	   </tr>
> +	   <tr>
> +		<td>XXX</td><td>XXX</td><td>XXX</td><td>XXX</td>
> +	   </tr>

Not totally sure about this one but you could check that the first row (the
first td XXX) matches the height of the image as this is required for baseline
alignment.

> LayoutTests/fast/css/vertical-align-baseline-rowspan-011.html:21
> +		<td>XXX</td><td>XXX</td><td rowspan="2"><img
src="resources/greenbox-100px.png"></td><td>XXX</td><td>XXX</td>

This could be converted to a layout-only test easily: you want the first row to
match the size of the rowspan (the second is effectively 0 sized).