[webkit-reviews] review canceled: [Bug 20040] Vertical border spacing is doubled between table row groups : [Attachment 130887] Patch to remove extra vertical spacing across table sections

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Kishore Bolisetty
<kbolisetty at innominds.com>'s request for review:
Bug 20040: Vertical border spacing is doubled between table row groups
https://bugs.webkit.org/show_bug.cgi?id=20040

Attachment 130887: Patch to remove extra vertical spacing across table sections
https://bugs.webkit.org/attachment.cgi?id=130887&action=review

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


Please, don't include unneeded rebaselines (I am responsible for the layer
disappearances - see r110072). Those changes you are adding are neat but
irrelevant to this bug and hides the important change. We prefer to have very
focused patches so that we can catch regressions easily. Those irrelevant
baselines should be postponed to another bug that I would happily review!

> Source/WebCore/ChangeLog:9
> +	   A default 2px vertical border-spacing is used while calculating row
logical heights with in a section.
> +	   This border-spacing is being added both at the top and bottom of a
section, thus leaving a gap of double border-spacing across
> +	   any two adjacent sections.This changes ensures proper border-spacing
across any two adjacent sections.

As mentioned, I don't understand why you mention the default border-spacing
here. It has very little to do with the problem and hides the fact that we used
to count the border-spacing regardless of it.

Also there should be a line between 'Reviewed by' and you description.

And while at it, 
* with in -> within
* top / bottom are ambiguous here and matches your reading direction
(horizontally I would bet), if you were for example japanese (reading
vertically), those would be right / left so we prefer the name: before / after
to be writing-mode agnostic.

> LayoutTests/platform/efl/Skipped:2871
> +# Needs baseline

Nit: EFL does support new-run-webkit-tests and thus normally those changes
should have gone into test_expectations.txt.