[webkit-reviews] review granted: [Bug 16811] Row size/position is wrongly calculated when table having overlapping rowspan cell and colspan cell : [Attachment 151527] Patch

Thu Jul 12 11:35:06 PDT 2012

Julien Chaffraix <jchaffraix at webkit.org> has granted Pravin D
<pravind.2k4 at gmail.com>'s request for review:
Bug 16811: Row size/position is wrongly calculated when table having
overlapping rowspan cell and colspan cell
https://bugs.webkit.org/show_bug.cgi?id=16811

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

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

> Source/WebCore/rendering/RenderTableSection.cpp:380
> +		       }

I think it's fine to compute the baseline in the loop. We are taking the max so
it shouldn't impact the result in most cases.

>
LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.h
tml:1
> +<html>

Unless you are testing quirksmode behavior, test case *should* include a
doctype:

<!DOCTYPE html>

This is all the more true for a test involving an auto-table layout.

>
LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.h
tml:3
> +  <title> Ref Test for bug https://bugs.webkit.org/show_bug.cgi?id=16811
</title>

I prefer to dump the bug (including a link) when text is allowed in the change
(like here). A title doesn't add much as you wouldn't see it in DRT output (on
top of not being clickable).

>
LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.h
tml:6
> +  <p> The Height of the green rectangle should be same as the yellow
rectangle and the green rectangle must be contained

it would be nice to add a description of 'what' you are testing. Example:

<p>This test checks that overlapping cells get sized properly.</p>

>
LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.h
tml:9
> +  <table  style="border:1px solid yellow;" cellspacing="0" cellpadding="0"
align="center">

Couldn't we remove more styling from here? The align="center" for example seems
like it could be removed. Arguably same for the other attributes.

>
LayoutTests/fast/table/last-cell-of-rowspan-overlapping-colspan-cell-expected.h
tml:19
> +	 <td  >

Unneeded spaces (not repeated for the other cases)