Move text-shadow/box-shadow properties into CSSPropertyParser

Move text-shadow/box-shadow properties into CSSPropertyParser

Move text-shadow/box-shadow properties from
LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780
Committed: https://crrev.com/0b45a3d62817bd54483b29ee0357c9880ab3f06b
Cr-Commit-Position: refs/heads/master@{#357021}

[rwlbuis, 2015-10-28 16:38:51]

Description was changed from

==========
shadow WIP


Move two color related properties into CSSPropertyParser

Move two color related properties handling from
LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780

patch from issue 1415953002 at patchset 60001
(http://crrev.com/1415953002#ps60001)
==========

to

==========
shadow WIP


Move two color related properties into CSSPropertyParser

Move two color related properties handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

[rwlbuis, 2015-10-28 17:10:29]

Patchset #5 (id:80001) has been deleted

[rwlbuis, 2015-10-28 18:03:07]

Patchset #5 (id:100001) has been deleted

[rwlbuis, 2015-10-28 18:48:54]

Description was changed from

==========
shadow WIP


Move two color related properties into CSSPropertyParser

Move two color related properties handling from
LegacyCSSPropertyParser into CSSPropertyParser.

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

to

==========
Move text-shadow/box-shadow properties into CSSPropertyParser
    
Move text-shadow/box-shadow properties from
LegacyCSSPropertyParser into CSSPropertyParser.
    
BUG=499780
==========

[rwlbuis, 2015-10-28 18:50:52]

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

[rwlbuis, 2015-10-28 18:50:52]

PTAL. parseSingleShadow will be used by the filter property too.

[Timothy Loh, 2015-10-29 02:53:27]

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

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1340:
WillBeHeapVector<RefPtrWillBeMember<CSSPrimitiveValue>, 4> lengths;
Probably nicer to just have four separate variables horizontalOffset,
verticalOffset, blurRadius, spreadDistance.

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1369: if
(!allowSpread)
I think it makes more sense to not try and consumeLength at all if allowSpread
is false

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1376: if
(RefPtrWillBeRawPtr<CSSValue> colorValue = consumeColor(range, context)) {
Can't we write:
if (!color)
    color = consumeColor(..);

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1399: if
(RefPtrWillBeRawPtr<CSSShadowValue> shadowValue = parseSingleShadow(range,
context, isBoxShadowProperty, isBoxShadowProperty))
Doesn't this allow extraneous commas everywhere?
text-shadow: <shadow>,,,,,
text-shadow: ,,,<shadow>,,

I think we can just put an else clause which returns nullptr here (this removes
the need for an empty list check later).

[rwlbuis, 2015-10-29 20:48:14]

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

[rwlbuis, 2015-10-29 20:48:14]

PTAL.

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

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1340:
WillBeHeapVector<RefPtrWillBeMember<CSSPrimitiveValue>, 4> lengths;
On 2015/10/29 02:53:27, Timothy Loh wrote:
> Probably nicer to just have four separate variables horizontalOffset,
> verticalOffset, blurRadius, spreadDistance.

That actually makes the code much more compact, done.

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1369: if
(!allowSpread)
On 2015/10/29 02:53:26, Timothy Loh wrote:
> I think it makes more sense to not try and consumeLength at all if allowSpread
> is false

Done.

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1376: if
(RefPtrWillBeRawPtr<CSSValue> colorValue = consumeColor(range, context)) {
On 2015/10/29 02:53:27, Timothy Loh wrote:
> Can't we write:
> if (!color)
>     color = consumeColor(..);

Done.

https://codereview.chromium.org/1419223002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1399: if
(RefPtrWillBeRawPtr<CSSShadowValue> shadowValue = parseSingleShadow(range,
context, isBoxShadowProperty, isBoxShadowProperty))
On 2015/10/29 02:53:26, Timothy Loh wrote:
> Doesn't this allow extraneous commas everywhere?
> text-shadow: <shadow>,,,,,
> text-shadow: ,,,<shadow>,,

Ouch! Fixed now.
I noticed box-shadow-interpolation.html using one comma at the end in two cases,
I assume this was an error (Firefox sees this as invalid) so I fixed the test,
adding alancutter to check if the original ending comma's were intended.

> I think we can just put an else clause which returns nullptr here (this
removes
> the need for an empty list check later).

Done.

[Timothy Loh, 2015-10-30 00:19:28]

lgtm

[rwlbuis, 2015-10-30 00:22:59]

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

[commit-bot: I haz the power, 2015-10-30 00:23:36]

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

[commit-bot: I haz the power, 2015-10-30 02:12:09]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-10-30 02:13:02]

Patchset 7 (id:??) landed as
https://crrev.com/0b45a3d62817bd54483b29ee0357c9880ab3f06b
Cr-Commit-Position: refs/heads/master@{#357021}