[webkit-reviews] review denied: [Bug 20040] Vertical border spacing is doubled between table row groups : [Attachment 113154] patch for "Vertical border spacing doubled between table row groups" with test_expectations.txt updated for all platforms and added TEXT+IMAGE at non bottom place

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Vamshi Krishna N
<vnampally 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 113154: patch for "Vertical border spacing doubled between table row
groups" with test_expectations.txt updated for all platforms and added
TEXT+IMAGE at non bottom place 
https://bugs.webkit.org/attachment.cgi?id=113154&action=review

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


r- mostly for the missing baselines & test cases. Also there may be a better
way to solve the bug.

> Source/WebCore/ChangeLog:7
> +	   

Where are the test cases? The bug has a test case that would be a nice addition
if it's not properly tested by our existing cases.

> Source/WebCore/rendering/RenderTableSection.cpp:330
> +    if (this == table()->topSection())
> +	   m_rowPos[0] = spacing;
> +    else
> +	   m_rowPos[0] = 0;

>From the ChangeLog explanation, it looks like the issue is that we shouldn't be
happily adding the vertical spacing twice inside a section.

If it's the case, this fix is wrong and there should be a way to create a test
case where a section overflows where it shouldn't due to the double spacing.

> LayoutTests/ChangeLog:18
> +	   * platform/chromium/test_expectations.txt:
> +	   * platform/gtk/test_expectations.txt:
> +	   * platform/mac/test_expectations.txt:
> +	   * platform/qt/test_expectations.txt:
> +	   * platform/win/test_expectations.txt:

I would like to see a baseline for one platform before saying if it makes sense
to skip (and eventually rebaseline) tons of tests on all platforms.