[webkit-reviews] review granted: [Bug 76246] Incorrect rendering of borders on <col> with span > 1 : [Attachment 167480] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Arpita Bahuguna
<arpitabahuguna at gmail.com>'s request for review:
Bug 76246: Incorrect rendering of borders on <col> with span > 1
https://bugs.webkit.org/show_bug.cgi?id=76246

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

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


I will fix the patch for landing.

> We seems to be repeating much of this code 4 times.  Is there no way to share
it?

That would be neat but I don't see a good way to do that: each repetition is
indeed similar but also slightly different from the other ones. This makes
sharing difficult. The code is also confusingly complex due to equal border
resolution (all other parameters being equals, the start-most border wins)

> Source/WebCore/rendering/RenderTableCell.cpp:532
> +	       // we apply its start border. This is as per the specification
which states that

HTML5 instead of "the specification" here. This has an importance as CSS tables
don't have the concept of spanning cell.

> Source/WebCore/rendering/RenderTableCell.cpp:539
> +	       // Next, apply the start border of the enclosing colgroup but
only if it is
> +	       // adjacent to the cell's edge.

This could be on one line.

> Source/WebCore/rendering/RenderTableCell.cpp:632
> +	       // This is as per the specification which states that "For the
purposes of the CSS table model,

Same comment here.

> Source/WebCore/rendering/RenderTableCell.cpp:639
> +	       // Next, if it has a parent colgroup then we apply its end
border but only if it is
> +	       // adjacent to the cell.

Ditto.

> LayoutTests/ChangeLog:12
> +	   Existing tests modified. This is because previously, while computing
collpased

Typo: collpased.