Parse border-radius shorthand in CSSPropertyParser with CSSParserTokens

Parse border-radius shorthand in CSSPropertyParser with CSSParserTokens

Move border-radius property handling from LegacyCSSPropertyParser
into CSSPropertyParser. Also move its related longhands.

BUG=499780
Committed: https://crrev.com/1aa3ac45628b36da02fb46f3cc47b6fc09544155
Cr-Commit-Position: refs/heads/master@{#369680}

[rwlbuis, 2016-01-11 16:07:37]

Description was changed from

==========
border-radius WIP


Merge parseBorderRadius and parseInsetRoundedCorners

Merge parseBorderRadius and parseInsetRoundedCorners into one
method called parseRadii, since they do roughly the same thing.
Since parseRadii does not add properties, do that when handling
the CSSPropertyBorderRadius case.

BUG=499780

patch from issue 1311413004 at patchset 80001
(http://crrev.com/1311413004#ps80001)
==========

to

==========
border-radius WIP

BUG=499780
==========

[rwlbuis, 2016-01-13 03:17:55]

Description was changed from

==========
border-radius WIP

BUG=499780
==========

to

==========
Parse border-radius shorthand in CSSPropertyParser with CSSParserTokens

Move border-radius property handling from LegacyCSSPropertyParser
into CSSPropertyParser. Also move its related longhands.

BUG=499780
==========

[rwlbuis, 2016-01-13 03:18:53]

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

[rwlbuis, 2016-01-13 03:18:53]

PTAL.

[Timothy Loh, 2016-01-14 06:38:45]

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3675: static
bool consumeRadii(RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[4],
RefPtrWillBeRawPtr<CSSPrimitiveValue> radii2[4], CSSParserTokenRange& range,
CSSParserMode cssParserMode, CSSPropertyID unresolvedProperty)
I'd rather if the last argument was useLegacyParsing. Also maybe radii ->
horizontalRadii and radii2 -> verticalRadii?

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3687: if
(range.atEnd() && i == 1 && unresolvedProperty ==
CSSPropertyAliasWebkitBorderRadius) {
I think this code should be adjacent to the code handling the rest of the
no-slash cases, instead of having the slash case in between.

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3705: break;
The first loop returns false in this case, can we make these consistent with
each other?

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3836:
ImplicitScope implicitScope(this);
I think this isn't needed.

[rwlbuis, 2016-01-14 22:45:50]

PTAL.

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3675: static
bool consumeRadii(RefPtrWillBeRawPtr<CSSPrimitiveValue> radii[4],
RefPtrWillBeRawPtr<CSSPrimitiveValue> radii2[4], CSSParserTokenRange& range,
CSSParserMode cssParserMode, CSSPropertyID unresolvedProperty)
On 2016/01/14 06:38:45, Timothy Loh wrote:
> I'd rather if the last argument was useLegacyParsing. Also maybe radii ->
> horizontalRadii and radii2 -> verticalRadii?

Done.

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3687: if
(range.atEnd() && i == 1 && unresolvedProperty ==
CSSPropertyAliasWebkitBorderRadius) {
On 2016/01/14 06:38:45, Timothy Loh wrote:
> I think this code should be adjacent to the code handling the rest of the
> no-slash cases, instead of having the slash case in between.

Actually it is hard to find a nice structure... Hope my new version is ok.

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3705: break;
On 2016/01/14 06:38:45, Timothy Loh wrote:
> The first loop returns false in this case, can we make these consistent with
> each other?

Done.

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3836:
ImplicitScope implicitScope(this);
On 2016/01/14 06:38:45, Timothy Loh wrote:
> I think this isn't needed.

Done.

[Timothy Loh, 2016-01-15 01:11:57]

lgtm

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1535463002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3687: if
(range.atEnd() && i == 1 && unresolvedProperty ==
CSSPropertyAliasWebkitBorderRadius) {
On 2016/01/14 22:45:50, rwlbuis wrote:
> On 2016/01/14 06:38:45, Timothy Loh wrote:
> > I think this code should be adjacent to the code handling the rest of the
> > no-slash cases, instead of having the slash case in between.
> 
> Actually it is hard to find a nice structure... Hope my new version is ok.

I know, otherwise I would've suggested something explicitly :S

Anyway, I think it's an improvement.

[rwlbuis, 2016-01-15 01:22:34]

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

[commit-bot: I haz the power, 2016-01-15 01:23:23]

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

[commit-bot: I haz the power, 2016-01-15 01:29:25]

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

[commit-bot: I haz the power, 2016-01-15 01:29:26]

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

[rwlbuis, 2016-01-15 02:39:30]

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

[commit-bot: I haz the power, 2016-01-15 02:39:58]

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

[commit-bot: I haz the power, 2016-01-15 04:06:47]

Description was changed from

==========
Parse border-radius shorthand in CSSPropertyParser with CSSParserTokens

Move border-radius property handling from LegacyCSSPropertyParser
into CSSPropertyParser. Also move its related longhands.

BUG=499780
==========

to

==========
Parse border-radius shorthand in CSSPropertyParser with CSSParserTokens

Move border-radius property handling from LegacyCSSPropertyParser
into CSSPropertyParser. Also move its related longhands.

BUG=499780
==========

[commit-bot: I haz the power, 2016-01-15 04:06:47]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-15 04:08:40]

Description was changed from

==========
Parse border-radius shorthand in CSSPropertyParser with CSSParserTokens

Move border-radius property handling from LegacyCSSPropertyParser
into CSSPropertyParser. Also move its related longhands.

BUG=499780
==========

to

==========
Parse border-radius shorthand in CSSPropertyParser with CSSParserTokens

Move border-radius property handling from LegacyCSSPropertyParser
into CSSPropertyParser. Also move its related longhands.

BUG=499780

Committed: https://crrev.com/1aa3ac45628b36da02fb46f3cc47b6fc09544155
Cr-Commit-Position: refs/heads/master@{#369680}
==========

[commit-bot: I haz the power, 2016-01-15 04:08:41]

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