Re: [css-grid] Implement grid gutters (issue 1309513008 by svillar@igalia.com)

[cbies...@chromium.org]

lgtm


https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp#newcode1797
Source/core/layout/LayoutGrid.cpp:1797: // // m_rowPositions include
gutters so we need to substract them to get the actual end position for
a given
remove one of the //

https://codereview.chromium.org/1309513008/

[esp...@chromium.org]

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp#newcode378
Source/core/layout/LayoutGrid.cpp:378: DEFINE_STATIC_LOCAL(LayoutUnit,
zero, ());
LayoutUnit is basically just an int, don't define a static local for
zero

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp#newcode380
Source/core/layout/LayoutGrid.cpp:380: return valueForLength(trackGap,
zero) * (span - 1);
just pass 0, it'll be made into a LayoutUnit

if snap is negative what happens here?

Also isn't valueForLength an int, did you want to convert to LayoutUnit
first?

https://codereview.chromium.org/1309513008/

[svi...@igalia.com]

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp
File Source/core/layout/LayoutGrid.cpp (right):

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp#newcode378
Source/core/layout/LayoutGrid.cpp:378: DEFINE_STATIC_LOCAL(LayoutUnit,
zero, ());

On 2015/10/01 08:46:48, esprehn wrote:
> LayoutUnit is basically just an int, don't define a static local for
zero

Acknowledged.

https://codereview.chromium.org/1309513008/diff/40001/Source/core/layout/LayoutGrid.cpp#newcode380
Source/core/layout/LayoutGrid.cpp:380: return valueForLength(trackGap,
zero) * (span - 1);

On 2015/10/01 08:46:48, esprehn wrote:
> just pass 0, it'll be made into a LayoutUnit

> if snap is negative what happens here?

span cannot be negative as it is a size_t. In any case I can add an
assert that it must be >= 1.

> Also isn't valueForLength an int, did you want to convert to
LayoutUnit first?

Not sure what you mean here, valueForLength() returns a LayoutUnit so
the operator* already converts span-1 to another LayoutUnit

https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

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


https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:
chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/106100)

https://codereview.chromium.org/1309513008/

[f...@opera.com]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/CSSProperties.in
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/CSSProperties.in#newcode191
third_party/WebKit/Source/core/css/CSSProperties.in:191: grid-column-gap
runtime_flag=CSSGridLayout, converter=convertLengthSizing
Why convertLengthSizing and not convertLength? (Ditto below)

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
File
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3210
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3210:
bool CSSPropertyParser::parseGridGapShorthand(bool important)
Could you use parseShorthand instead?

https://codereview.chromium.org/1309513008/

[svi...@igalia.com]

Thanks for the review

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/CSSProperties.in
File third_party/WebKit/Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/CSSProperties.in#newcode191
third_party/WebKit/Source/core/css/CSSProperties.in:191: grid-column-gap
runtime_flag=CSSGridLayout, converter=convertLengthSizing

On 2015/10/05 10:53:28, fs wrote:
> Why convertLengthSizing and not convertLength? (Ditto below)

Yeah it should be convertLength()

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
File
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3210
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3210:
bool CSSPropertyParser::parseGridGapShorthand(bool important)

On 2015/10/05 10:53:28, fs wrote:
> Could you use parseShorthand instead?

I don't think so because although ideally the shorthand gets two values
it might get just one so we have to add that specific behavior. AFAIK
that cannot be done with parseShorthand...

https://codereview.chromium.org/1309513008/

[f...@opera.com]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
File
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3210
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3210:
bool CSSPropertyParser::parseGridGapShorthand(bool important)

On 2015/10/05 at 11:58:40, svillar wrote:
> On 2015/10/05 10:53:28, fs wrote:
> > Could you use parseShorthand instead?

> I don't think so because although ideally the shorthand gets two
values it might get just one so we have to add that specific behavior.
AFAIK that cannot be done with parseShorthand...

Right, I figured it might be the column-gap -> row-gap value expansion
(didn't remember if parseShorthand handled that or not.)

https://codereview.chromium.org/1309513008/

[svi...@igalia.com]

So parseShorthand() is indeed able to fill in the missing values with
initial
values but that is not what we want in this case, as I mentioned if just one
value is specified then the other should get the same.

https://codereview.chromium.org/1309513008/

[ru...@opera.com]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
File
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode1178
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1178:
validPrimitive = validUnit(value, FLength | FNonNeg);
The current spec draft doesn't say it's limited to non-negative lengths,
but it probably should, like the spec for column-gap in multicol does.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3222
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3222:
addProperty(CSSPropertyGridColumnGap,
cssValuePool().createImplicitInitialValue(), important);
According to the spec, "normal" is not a valid value for any of the
longhands.

Also, the spec doesn't say what normal mean, afaict.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3235
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3235:
addProperty(CSSPropertyGridRowGap, columnGap, important);
I think it this code might be easier to read if you have the addProperty
calls at the end and have two variables columnGap and rowGap which is
set to the same value if there's only one value in the declaration.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/ComputedStyle.h
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/ComputedStyle.h#newcode799
third_party/WebKit/Source/core/style/ComputedStyle.h:799:
Why the extra empty line?

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/StyleGridData.h
File third_party/WebKit/Source/core/style/StyleGridData.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/StyleGridData.h#newcode49
third_party/WebKit/Source/core/style/StyleGridData.h:49: return
m_gridTemplateColumns == o.m_gridTemplateColumns && m_gridTemplateRows
== o.m_gridTemplateRows && m_gridAutoFlow == o.m_gridAutoFlow &&
m_gridAutoRows == o.m_gridAutoRows && m_gridAutoColumns ==
o.m_gridAutoColumns && m_namedGridColumnLines ==
o.m_namedGridColumnLines && m_namedGridRowLines == o.m_namedGridRowLines
&& m_orderedNamedGridColumnLines == o.m_orderedNamedGridColumnLines &&
m_orderedNamedGridRowLines == o.m_orderedNamedGridRowLines &&
m_namedGridArea == o.m_namedGridArea && m_namedGridArea ==
o.m_namedGridArea && m_namedGridAreaRowCount ==
o.m_namedGridAreaRowCount && m_namedGridAreaColumnCount ==
o.m_namedGridAreaColumnCount && m_gridColumnGap == o.m_gridColumnGap &&
m_gridRowGap == o.m_gridRowGap;
About time to break this into multiple lines.

https://codereview.chromium.org/1309513008/

[svi...@igalia.com]

Thanks for the review

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
File
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode1178
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1178:
validPrimitive = validUnit(value, FLength | FNonNeg);

On 2015/10/05 12:23:39, rune wrote:
> The current spec draft doesn't say it's limited to non-negative
lengths, but it
> probably should, like the spec for column-gap in multicol does.

True, I think it's just an overlook, actually the Mozilla guy that is
going to implement it in Gecko sent this
https://lists.w3.org/Archives/Public/www-style/2015Oct/0028.html to the
list asking for clarifications.

I think that the assumption I made is correct in any case.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3222
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3222:
addProperty(CSSPropertyGridColumnGap,
cssValuePool().createImplicitInitialValue(), important);

On 2015/10/05 12:23:39, rune wrote:
> According to the spec, "normal" is not a valid value for any of the
longhands.

> Also, the spec doesn't say what normal mean, afaict.

That was specified in the previous draft, see
http://www.w3.org/TR/2015/WD-css-grid-1-20150806/#gutters. My first
patch was from 3 weeks ago when that was the latest version.

Anyway in the email I linked above there is another question for
"normal" as well so they'll clarify it for sure.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3235
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3235:
addProperty(CSSPropertyGridRowGap, columnGap, important);

On 2015/10/05 12:23:39, rune wrote:
> I think it this code might be easier to read if you have the
addProperty calls
> at the end and have two variables columnGap and rowGap which is set to
the same
> value if there's only one value in the declaration.

Acknowledged.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/ComputedStyle.h
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/ComputedStyle.h#newcode799
third_party/WebKit/Source/core/style/ComputedStyle.h:799:

On 2015/10/05 12:23:39, rune wrote:
> Why the extra empty line?

Will remove it.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/StyleGridData.h
File third_party/WebKit/Source/core/style/StyleGridData.h (right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/style/StyleGridData.h#newcode49
third_party/WebKit/Source/core/style/StyleGridData.h:49: return
m_gridTemplateColumns == o.m_gridTemplateColumns && m_gridTemplateRows
== o.m_gridTemplateRows && m_gridAutoFlow == o.m_gridAutoFlow &&
m_gridAutoRows == o.m_gridAutoRows && m_gridAutoColumns ==
o.m_gridAutoColumns && m_namedGridColumnLines ==
o.m_namedGridColumnLines && m_namedGridRowLines == o.m_namedGridRowLines
&& m_orderedNamedGridColumnLines == o.m_orderedNamedGridColumnLines &&
m_orderedNamedGridRowLines == o.m_orderedNamedGridRowLines &&
m_namedGridArea == o.m_namedGridArea && m_namedGridArea ==
o.m_namedGridArea && m_namedGridAreaRowCount ==
o.m_namedGridAreaRowCount && m_namedGridAreaColumnCount ==
o.m_namedGridAreaColumnCount && m_gridColumnGap == o.m_gridColumnGap &&
m_gridRowGap == o.m_gridRowGap;

On 2015/10/05 12:23:39, rune wrote:
> About time to break this into multiple lines.

Acknowledged.

https://codereview.chromium.org/1309513008/

[ru...@opera.com]

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
File
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode1178
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:1178:
validPrimitive = validUnit(value, FLength | FNonNeg);

On 2015/10/05 12:36:01, svillar wrote:
> On 2015/10/05 12:23:39, rune wrote:
> > The current spec draft doesn't say it's limited to non-negative
lengths, but
> it
> > probably should, like the spec for column-gap in multicol does.

> True, I think it's just an overlook, actually the Mozilla guy that is
going to
> implement it in Gecko sent this
> https://lists.w3.org/Archives/Public/www-style/2015Oct/0028.html to
the list
> asking for clarifications.

> I think that the assumption I made is correct in any case.

Yes, I just wanted to make sure the spec issue is addressed.

https://codereview.chromium.org/1309513008/diff/100001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3222
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3222:
addProperty(CSSPropertyGridColumnGap,
cssValuePool().createImplicitInitialValue(), important);

On 2015/10/05 12:36:01, svillar wrote:
> On 2015/10/05 12:23:39, rune wrote:
> > According to the spec, "normal" is not a valid value for any of the
longhands.
> >
> > Also, the spec doesn't say what normal mean, afaict.

> That was specified in the previous draft, see
> http://www.w3.org/TR/2015/WD-css-grid-1-20150806/#gutters. My first
patch was
> from 3 weeks ago when that was the latest version.

> Anyway in the email I linked above there is another question for
"normal" as
> well so they'll clarify it for sure.

Ah. So the property used to be shared with multicol. Now that explains
the need for 'normal' to mean something. Now that they're separate
properties, and the initial value is 0, and normal used to map to 0, I
see no point in keeping 'normal'. I suggest that you just drop normal as
a valid value for now and re-introduce it if the spec for some reason
re-introduces it instead of removing it for the shorthand.

https://codereview.chromium.org/1309513008/

[f...@opera.com]

lgtm




https://codereview.chromium.org/1309513008/diff/140001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
File
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1309513008/diff/140001/third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp#newcode3193
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:3193:
RefPtrWillBeRawPtr<CSSPrimitiveValue> columnGap = nullptr;
Nit: With this new structure, I'd propose that the declaration is moved
as close to/as far down as possible (i.e columnGap to where it's
assigned, and rowGap just before the if.)

https://codereview.chromium.org/1309513008/

[ru...@opera.com]

lgtm too

https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/1309513008/140001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1309513008/140001


https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:

cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/63787)
linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/91018)
ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/78498)
ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ninja/builds/120535)
mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/107170)
mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/13819)
mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/123232)

https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/patch-status/1309513008/160001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1309513008/160001


https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

Try jobs failed on following builders:

win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/builds/50556)

https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #9 (id:160001)

https://codereview.chromium.org/1309513008/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 9 (id:??) landed as
https://crrev.com/ad570a7dcd24b813e0dc2400c0bf080caf30a0ce
Cr-Commit-Position: refs/heads/master@{#352839}

https://codereview.chromium.org/1309513008/