Make parsing of <transform> more strict

Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking
that something valid follows after a comma.

BUG=593321
Committed: https://crrev.com/04dfd7b740446fe6cead94132546d4dfdb69c2e8
Cr-Commit-Position: refs/heads/master@{#381864}

[rwlbuis, 2016-03-17 16:36:05]

Description was changed from

==========
593321 WIP

BUG=
==========

to

==========
593321 WIP

BUG=593321
==========

[rwlbuis, 2016-03-17 16:37:31]

Description was changed from

==========
593321 WIP

BUG=593321
==========

to

==========
Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking that
something valid follows after a comma.

BUG=593321
==========

[rwlbuis, 2016-03-17 16:37:49]

Description was changed from

==========
Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking that
something valid follows after a comma.

BUG=593321
==========

to

==========
Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking
that something valid follows after a comma.

BUG=593321
==========

[rwlbuis, 2016-03-17 16:40:12]

rob.buis@samsung.com changed reviewers:
+ ericwilligers@chromium.org

[rwlbuis, 2016-03-17 16:40:12]

PTAL.

[Timothy Loh, 2016-03-17 23:23:31]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2016-03-17 23:23:32]

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

https://codereview.chromium.org/1811953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1846:
parsedValue = consumeLength(args, cssParserMode, ValueRangeAll);
Is this case right? Maybe it'd be better to make the cases which call out to
e.g. consumeNumbers always return, while the bottom of the switch returns
nullptr if we didn't set parsedValue?

[rwlbuis, 2016-03-17 23:29:48]

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

https://codereview.chromium.org/1811953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1846:
parsedValue = consumeLength(args, cssParserMode, ValueRangeAll);
On 2016/03/17 23:23:32, Timothy Loh wrote:
> Is this case right? Maybe it'd be better to make the cases which call out to
> e.g. consumeNumbers always return, while the bottom of the switch returns
> nullptr if we didn't set parsedValue?

This should be correct, since the no args case is handled above, and any failure
to consume
a length should cause the atEnd() check at the bottom of this method to fail.
There should also already be tests for this (at least the empty args one).

[Timothy Loh, 2016-03-17 23:56:45]

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

https://codereview.chromium.org/1811953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1846:
parsedValue = consumeLength(args, cssParserMode, ValueRangeAll);
On 2016/03/17 23:29:48, rwlbuis wrote:
> On 2016/03/17 23:23:32, Timothy Loh wrote:
> > Is this case right? Maybe it'd be better to make the cases which call out to
> > e.g. consumeNumbers always return, while the bottom of the switch returns
> > nullptr if we didn't set parsedValue?
> 
> This should be correct, since the no args case is handled above, and any
failure
> to consume
> a length should cause the atEnd() check at the bottom of this method to fail.
> There should also already be tests for this (at least the empty args one).

Ah right. lgtm then, I just realised my suggestion doesn't work too well because
the cases which don't use parsedValue still need to check atEnd...

[rwlbuis, 2016-03-18 00:06:09]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2016-03-18 00:06:42]

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

[commit-bot: I haz the power, 2016-03-18 01:20:50]

Description was changed from

==========
Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking
that something valid follows after a comma.

BUG=593321
==========

to

==========
Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking
that something valid follows after a comma.

BUG=593321
==========

[commit-bot: I haz the power, 2016-03-18 01:20:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-18 01:23:37]

Description was changed from

==========
Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking
that something valid follows after a comma.

BUG=593321
==========

to

==========
Make parsing of <transform> more strict

Make parsing of <transform> more strict by checking
that something valid follows after a comma.

BUG=593321

Committed: https://crrev.com/04dfd7b740446fe6cead94132546d4dfdb69c2e8
Cr-Commit-Position: refs/heads/master@{#381864}
==========

[commit-bot: I haz the power, 2016-03-18 01:23:38]

Patchset 2 (id:??) landed as
https://crrev.com/04dfd7b740446fe6cead94132546d4dfdb69c2e8
Cr-Commit-Position: refs/heads/master@{#381864}