[css-grid] Percentage columns shouldn't include border and padding

[css-grid] Percentage columns shouldn't include border and padding

Subtract border and padding when we're calculating the breadth of the
columns in LayoutGrid::computeUsedBreadthOfSpecifiedLength().

Added test to check the behavior for both columns and rows.

BUG=529203
TEST=fast/css-grid-layout/grid-percent-track-margin-border-padding.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201909

[Manuel Rego, 2015-09-08 10:33:50]

rego@igalia.com changed reviewers:
+ cbiesinger@chromium.org, jfernandez@igalia.com, svillar@igalia.com

[svillar, 2015-09-08 10:43:37]

Nice catch! lgtm

Just in case you want to provide a super-awesome patch, please ensure that
borders, margins, paddings are big enough so that they're clearly visible by
humans :) (perhaps they're now, just that 1,2,3 seem small to me)

[Manuel Rego, 2015-09-08 10:51:17]

On 2015/09/08 10:43:37, svillar wrote:
> Nice catch! lgtm

Thanks for the quick review!

> Just in case you want to provide a super-awesome patch, please ensure that
> borders, margins, paddings are big enough so that they're clearly visible by
> humans :) (perhaps they're now, just that 1,2,3 seem small to me)

Ok, I'm using 5, 10 and 20 now :-)

[Manuel Rego, 2015-09-08 10:51:23]

The CQ bit was checked by rego@igalia.com

[Manuel Rego, 2015-09-08 10:51:23]

The patchset sent to the CQ was uploaded after l-g-t-m from svillar@igalia.com
Link to the patchset: https://codereview.chromium.org/1321723008/#ps20001
(title: "New version increasing pixels for margin, border and padding")

[commit-bot: I haz the power, 2015-09-08 10:51:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1321723008/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1321723008/20001

[commit-bot: I haz the power, 2015-09-08 12:00:14]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201909

[cbiesinger, 2015-10-08 20:51:24]

https://codereview.chromium.org/1321723008/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1321723008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:528: return valueForLength(trackLength,
direction == ForColumns ? logicalWidth() - borderAndPaddingLogicalWidth() :
std::max(LayoutUnit(), computeContentLogicalHeight(MainOrPreferredSize,
style()->logicalHeight(), -1)));
I'm way too late here, but... maybe use contentLogicaLWidth() instead of
logicalWidth() - borderAndPaddingLogicalWidth(), which would also take care of
the scrollbar width

[Manuel Rego, 2015-10-09 13:16:25]

https://codereview.chromium.org/1321723008/diff/20001/Source/core/layout/Layo...
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1321723008/diff/20001/Source/core/layout/Layo...
Source/core/layout/LayoutGrid.cpp:528: return valueForLength(trackLength,
direction == ForColumns ? logicalWidth() - borderAndPaddingLogicalWidth() :
std::max(LayoutUnit(), computeContentLogicalHeight(MainOrPreferredSize,
style()->logicalHeight(), -1)));
On 2015/10/08 20:51:24, cbiesinger wrote:
> I'm way too late here, but... maybe use contentLogicaLWidth() instead of
> logicalWidth() - borderAndPaddingLogicalWidth(), which would also take care of
> the scrollbar width

Yeah, that was changed a few days later in:
https://codereview.chromium.org/1338703002/

[cbiesinger, 2015-10-09 14:28:00]

On 2015/10/09 13:16:25, Manuel Rego wrote:
>
https://codereview.chromium.org/1321723008/diff/20001/Source/core/layout/Layo...
> File Source/core/layout/LayoutGrid.cpp (right):
> 
>
https://codereview.chromium.org/1321723008/diff/20001/Source/core/layout/Layo...
> Source/core/layout/LayoutGrid.cpp:528: return valueForLength(trackLength,
> direction == ForColumns ? logicalWidth() - borderAndPaddingLogicalWidth() :
> std::max(LayoutUnit(), computeContentLogicalHeight(MainOrPreferredSize,
> style()->logicalHeight(), -1)));
> On 2015/10/08 20:51:24, cbiesinger wrote:
> > I'm way too late here, but... maybe use contentLogicaLWidth() instead of
> > logicalWidth() - borderAndPaddingLogicalWidth(), which would also take care
of
> > the scrollbar width
> 
> Yeah, that was changed a few days later in:
> https://codereview.chromium.org/1338703002/

Ah good :) Sorry, I'm slowly catching up on older emails.