CSS: <position> parser consumes only potentially valid tokens

CSS: <position> parser consumes only potentially valid tokens

When parsing <position>, consumePosition() always attempted to consume
four values, even when it was not possible for the next value to
legally appear in the current <position>.

For example, if the first value is a <percentage> or <length>, the
second value (if present) may only be
  top | center | bottom | <percentage> | <length>
and no third or fourth values are possible.

If the second value is center, no third or fourth values are possible.

left | right  may never be followed by  left | right.

BUG=638055
Committed: https://crrev.com/4c25391a90d84a475e3f4084bfa80cbcb18dc2cb
Cr-Commit-Position: refs/heads/master@{#414006}

[Eric Willigers, 2016-08-17 05:27:59]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 05:28:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Willigers, 2016-08-17 05:55:28]

ericwilligers@chromium.org changed reviewers:
+ shans@chromium.org, timloh@chromium.org

[Eric Willigers, 2016-08-17 05:55:29]

This was motivated by the offset shorthand of the latest Editor's Draft of CSS
Motion Path
https://drafts.fxtf.org/motion-1/
where offset may contain <position> <position>

WIP https://codereview.chromium.org/2247773004/
does not parse the following corner case
  offset: [top | bottom] [<percentage> | <length>]
where offset-position is [top | bottom]
and offset-anchor is [<percentage> | <length>]

But (to my knowledge) all other cases are successfully parsed.

[Timothy Loh, 2016-08-17 06:03:03]

On 2016/08/17 05:55:29, Eric Willigers wrote:
> This was motivated by the offset shorthand of the latest Editor's Draft of CSS
> Motion Path
> https://drafts.fxtf.org/motion-1/
> where offset may contain <position> <position>
> 
> WIP https://codereview.chromium.org/2247773004/
> does not parse the following corner case
>   offset: [top | bottom] [<percentage> | <length>]
> where offset-position is [top | bottom]
> and offset-anchor is [<percentage> | <length>]
> 
> But (to my knowledge) all other cases are successfully parsed.

Can we just.... avoid having <position> <position> in the spec, which pretty
much no one will be able to correctly guess how things will be parsed as?

[commit-bot: I haz the power, 2016-08-17 07:26:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 07:26:35]

Dry run: This issue passed the CQ dry run.

[Eric Willigers, 2016-08-21 21:14:06]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-21 21:14:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 01:39:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 01:39:27]

Dry run: This issue passed the CQ dry run.

[Eric Willigers, 2016-08-22 04:11:22]

On 2016/08/17 06:03:03, Timothy Loh wrote:
> On 2016/08/17 05:55:29, Eric Willigers wrote:
> > This was motivated by the offset shorthand of the latest Editor's Draft of
CSS
> > Motion Path
> > https://drafts.fxtf.org/motion-1/
> > where offset may contain <position> <position>
> > 
> > WIP https://codereview.chromium.org/2247773004/
> > does not parse the following corner case
> >   offset: [top | bottom] [<percentage> | <length>]
> > where offset-position is [top | bottom]
> > and offset-anchor is [<percentage> | <length>]
> > 
> > But (to my knowledge) all other cases are successfully parsed.
> 
> Can we just.... avoid having <position> <position> in the spec, which pretty
> much no one will be able to correctly guess how things will be parsed as?

Raised spec issue https://github.com/w3c/fxtf-drafts/issues/24


Even if the Motion spec changes, this patch fixes an old TODO, and improves test
coverage. (The added tests already passed.)

[Timothy Loh, 2016-08-23 03:51:37]

lgtm

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(left):

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:567: //
shorthand works around it, but we should just fix it here.
Shouldn't we remove the workaround in consumeBackgroundShorthand too? :)

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:476:
static CSSPrimitiveValue* consumePositionComponent(CSSParserTokenRange& range,
CSSParserMode cssParserMode, UnitlessQuirk unitless, bool* horizontalEdge, bool*
verticalEdge)
bool& here

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:490: }
Maybe nicer (less repetitive) to

} else if (id != CSSValueCenter) {
    return nullptr;
}
return consumeIdent(range);

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:579:
CSSPrimitiveValue* value2 = consumePositionComponent(rangeCopy, cssParserMode,
unitless, &horizontalEdge, &verticalEdge);
I think it's clearer to only consume from range and not rangeCopy, so we only
need to use it when we fall back because of a case like "top 10px". Maybe
renaming to something like rangeAfterFirstConsume might help?

[Eric Willigers, 2016-08-24 02:02:24]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 02:03:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Willigers, 2016-08-24 02:03:45]

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(left):

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:567: //
shorthand works around it, but we should just fix it here.
On 2016/08/23 03:51:37, Timothy Loh wrote:
> Shouldn't we remove the workaround in consumeBackgroundShorthand too? :)

I removed the use of rangeCopy in consumeBackgroundShorthand.

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp
(right):

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:476:
static CSSPrimitiveValue* consumePositionComponent(CSSParserTokenRange& range,
CSSParserMode cssParserMode, UnitlessQuirk unitless, bool* horizontalEdge, bool*
verticalEdge)
On 2016/08/23 03:51:37, Timothy Loh wrote:
> bool& here

Done.

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:490: }
On 2016/08/23 03:51:37, Timothy Loh wrote:
> Maybe nicer (less repetitive) to
> 
> } else if (id != CSSValueCenter) {
>     return nullptr;
> }
> return consumeIdent(range);

Done.

https://codereview.chromium.org/2256593002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp:579:
CSSPrimitiveValue* value2 = consumePositionComponent(rangeCopy, cssParserMode,
unitless, &horizontalEdge, &verticalEdge);
On 2016/08/23 03:51:37, Timothy Loh wrote:
> I think it's clearer to only consume from range and not rangeCopy, so we only
> need to use it when we fall back because of a case like "top 10px". Maybe
> renaming to something like rangeAfterFirstConsume might help?

Done.

[commit-bot: I haz the power, 2016-08-24 03:30:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-24 03:30:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Eric Willigers, 2016-08-24 03:37:01]

The CQ bit was checked by ericwilligers@chromium.org

[Eric Willigers, 2016-08-24 03:37:01]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2256593002/#ps40001
(title: "references")

[commit-bot: I haz the power, 2016-08-24 03:37:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 07:37:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-24 07:38:07]

Description was changed from

==========
CSS: <position> parser consumes only potentially valid tokens

When parsing <position>, consumePosition() always attempted to consume
four values, even when it was not possible for the next value to
legally appear in the current <position>.

For example, if the first value is a <percentage> or <length>, the
second value (if present) may only be
  top | center | bottom | <percentage> | <length>
and no third or fourth values are possible.

If the second value is center, no third or fourth values are possible.

left | right  may never be followed by  left | right.



BUG=638055
==========

to

==========
CSS: <position> parser consumes only potentially valid tokens

When parsing <position>, consumePosition() always attempted to consume
four values, even when it was not possible for the next value to
legally appear in the current <position>.

For example, if the first value is a <percentage> or <length>, the
second value (if present) may only be
  top | center | bottom | <percentage> | <length>
and no third or fourth values are possible.

If the second value is center, no third or fourth values are possible.

left | right  may never be followed by  left | right.

BUG=638055

Committed: https://crrev.com/4c25391a90d84a475e3f4084bfa80cbcb18dc2cb
Cr-Commit-Position: refs/heads/master@{#414006}
==========

[commit-bot: I haz the power, 2016-08-24 07:38:08]

Patchset 3 (id:??) landed as
https://crrev.com/4c25391a90d84a475e3f4084bfa80cbcb18dc2cb
Cr-Commit-Position: refs/heads/master@{#414006}