border-spacing is doubled between table-row-groups.

border-spacing is doubled between table-row-groups.

Every table-section's rows height is calculating separately. So border spacing
had applied to bottom of the first section and top of the second section.

Now, Only Top most section would apply border spacing on top of it's section.

R=eseidel@chromium.org,jchaffraix@chromium.org
BUG=29502
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157364

[Julien - ping for review, 2013-09-05 17:58:03]

lgtm

https://codereview.chromium.org/23548017/diff/2001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-sections-border-spacing.html (right):

https://codereview.chromium.org/23548017/diff/2001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-sections-border-spacing.html:52:
document.getElementById('console').appendChild(document.createElement('br'));
Looks like you need a function as this line is repeated 6 times.

https://codereview.chromium.org/23548017/diff/2001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-sections-border-spacing.html:53:
shouldBe("(document.getElementById('td1Id').offsetTop -
document.getElementById('thId').offsetTop -
document.getElementById('thId').offsetHeight)", "10");
The logic is similar, maybe we could just renamed the first 'thId' to 'td0Id'
and just have a test for the values 0..4 with a helper function:

function testBorderSpacing(index)
{
    var afterRow = document.getElementById('td' + index + 'Id');
    var beforeRow = document.getElementById('td' + index - 1 + 'Id');
    var borderSpacing = afterRow.offsetTop - beforeRow.offsetTop -
beforeRow.offsetHeight;
    shouldBe(borderSpacing, 10);
}

https://codereview.chromium.org/23548017/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/23548017/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:516: // Because top border spacing
of following sections would be bottom border spacing of it's previous section.
Good comment, let's improve it:

// We ignore the border-spacing on any non-top section as it is already included
in the previous section's last row position.

[a.suchit, 2013-09-06 05:27:18]

https://codereview.chromium.org/23548017/diff/2001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-sections-border-spacing.html (right):

https://codereview.chromium.org/23548017/diff/2001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-sections-border-spacing.html:52:
document.getElementById('console').appendChild(document.createElement('br'));
On 2013/09/05 17:58:03, Julien Chaffraix wrote:
> Looks like you need a function as this line is repeated 6 times.

Done.

https://codereview.chromium.org/23548017/diff/2001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-sections-border-spacing.html:53:
shouldBe("(document.getElementById('td1Id').offsetTop -
document.getElementById('thId').offsetTop -
document.getElementById('thId').offsetHeight)", "10");
On 2013/09/05 17:58:03, Julien Chaffraix wrote:
> The logic is similar, maybe we could just renamed the first 'thId' to 'td0Id'
> and just have a test for the values 0..4 with a helper function:
> 
> function testBorderSpacing(index)
> {
>     var afterRow = document.getElementById('td' + index + 'Id');
>     var beforeRow = document.getElementById('td' + index - 1 + 'Id');
>     var borderSpacing = afterRow.offsetTop - beforeRow.offsetTop -
> beforeRow.offsetHeight;
>     shouldBe(borderSpacing, 10);
> }

Done.

https://codereview.chromium.org/23548017/diff/2001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/23548017/diff/2001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:516: // Because top border spacing
of following sections would be bottom border spacing of it's previous section.
On 2013/09/05 17:58:03, Julien Chaffraix wrote:
> Good comment, let's improve it:
> 
> // We ignore the border-spacing on any non-top section as it is already
included
> in the previous section's last row position.

Done.

[commit-bot: I haz the power, 2013-09-06 05:27:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23548017/9001

[commit-bot: I haz the power, 2013-09-06 08:06:43]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-09-06 08:58:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23548017/9001

[commit-bot: I haz the power, 2013-09-06 12:12:07]

Change committed as 157364