[webkit-reviews] review denied: [Bug 5515] Border collapse problem with rowspan/colspan cells : [Attachment 161153] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 5515: Border collapse problem with rowspan/colspan cells
https://bugs.webkit.org/show_bug.cgi?id=5515

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

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


> Source/WebCore/ChangeLog:9
> +	   Our handling of colspans and rowspans while resolving collapsed
borders
> +	   on a table is not proper.

Globally this sentence doesn't add anything. How does this sentence help
someone unfamiliar with your change understand what you did?

Adding a one line short / TL;DR description is totally welcome but think how
you can pack *relevant* information in one sentence.

> Source/WebCore/ChangeLog:17
> +	   The left border (or the start border) specified for the first cell
> +	   adjacent to a spanned cell (rowspan) was being applied for the
entire length
> +	   of the spanned cell's right (end border) border.
> +
> +	   Similarly, the top border (or the before border) specified for the
first
> +	   cell below a spanned cell (colspan) was being applied for the entire

> +	   length of the spanned cell's bottom (after border) border.

Be careful how you phrase the change. start / before are not strictly equal to
left / top but this ChangeLog mixes the 2 and makes people wonder if you know
the difference.

> Source/WebCore/ChangeLog:21
> +	   collapsed borders.

Nowhere do you mention in your ChangeLog that this change is a work-around,
please do and mention the bug with the right fix.

> Source/WebCore/rendering/RenderTableCell.cpp:342
> +static bool isValidCellForColRowSpanBorderResolution(const RenderTableCell*
currCell, bool isStartOrEndBorder, const RenderTableCell* adjacentCell = 0)

Please NO BOOLEAN parameters. They make the call sites difficult to read and
every time you add one you should ask yourself if it's needed. See
http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html among
other resources.

On top of that, here they are unnecessary and confusing. You should have 2
function dedicated to each logical direction.

> Source/WebCore/rendering/RenderTableCell.cpp:346
> +	   if (currCell->rowSpan() > 1)
> +	       return adjacentCell ?  adjacentCell->rowSpan() > 1 : false;

First unrelated but this could be written: return adjacentCell &&
adjacentCell->rowSpan() > 1.

Second, I don't see the point of doing this comparison with an empty
|adjacentCell|.

Finally, I really don't understand what the point of this check is. I would
rather keep the alignment case as our heuristic here. Both choices are
arbitrary but it matches other browsers on more cases and AFAICT yours requires
a lot more rebaselining.

> Source/WebCore/rendering/RenderTableCell.cpp:509
> +    if (!isEndColumn && isValidCellForColRowSpanBorderResolution(this,
true)) {

NO! Changing this condition means that you now wrongly fall back to checking
the |row|'s end border for middle rows if the cells has a rowspan.

>
LayoutTests/fast/table/border-collapsing/collapsed-border-with-colspan.html:51
> +<!-- Red, green and blue colored borders should be displayed per cell. -->

Red should be reserved for failures. See our wiki on how to write good layout
tests:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

Preferably this should be a ref-test.