Move background related shorthands into CSSPropertyParser

Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position

BUG=499780
Committed: https://crrev.com/4d1a91b7a318da0de7eda393d3e0e23647135c6e
Cr-Commit-Position: refs/heads/master@{#378219}

[rwlbuis, 2016-02-23 01:18:35]

Description was changed from

==========
background-repeat WIP


Move background related longhands into CSSPropertyParser

Move the following background related longhands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-blend-mode, background-attachment
background-clip, -webkit-background-clip
-webkit-background-composite, background-image
background-origin, mask-source-type
-webkit-background-origin, background-color
background-position-x, background-position-y
background-size, -webkit-mask-clip
-webkit-mask-composite, Webkit-mask-image
-webkit-mask-origin,  -webkit-mask-size
-webkit-mask-position-x, -webkit-mask-position-y
-webkit-mask-repeat-x, -webkit-mask-repeat-y
background-color

BUG=499780

patch from issue 1706573002 at patchset 40001
(http://crrev.com/1706573002#ps40001)
==========

to

==========
background-repeat WIP

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

[rwlbuis, 2016-02-23 02:15:41]

Description was changed from

==========
background-repeat WIP

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

to

==========
Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position


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

[rwlbuis, 2016-02-23 02:17:00]

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

[rwlbuis, 2016-02-23 02:17:00]

PTAL. Unfortunately I see no way around handling the implicit stuff this way...

[Timothy Loh, 2016-02-23 05:53:15]

Seems ok. I didn't look at the implicit stuff yet.

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/backgrounds/background-position-parsing-2.html
(right):

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/backgrounds/background-position-parsing-2.html:248:
style.backgroundPosition = "";
Might as well swap this and the next one, so that all the invalid values end up
as empty instead of left 20% 0%

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

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4533: if
(positionY)
I think both always get set if consumePosition returns true

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4536: return
resultX;
return true?

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4560: value2 =
cssValuePool().createIdentifierValue(id);
isn't this just value1?

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4572: break;
Any reason for this to be different than consumeBackgroundPosition?

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4729: if
(resultY)
There shouldn't be any case where a shorthand only sets some of its longhands
(except for that one quirk that we have, see bug 277157). Same below.

[rwlbuis, 2016-02-23 14:51:06]

Description was changed from

==========
Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position


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

to

==========
Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position

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

[rwlbuis, 2016-02-23 20:16:42]

PTAL. The layout test changes are mostly due to the <position> changes.

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/backgrounds/background-position-parsing-2.html
(right):

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/backgrounds/background-position-parsing-2.html:248:
style.backgroundPosition = "";
On 2016/02/23 05:53:14, Timothy Loh wrote:
> Might as well swap this and the next one, so that all the invalid values end
up
> as empty instead of left 20% 0%

Done.

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

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4533: if
(positionY)
On 2016/02/23 05:53:15, Timothy Loh wrote:
> I think both always get set if consumePosition returns true

Done.

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4536: return
resultX;
On 2016/02/23 05:53:14, Timothy Loh wrote:
> return true?

Done, also in consumeRepeatStyle.

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4560: value2 =
cssValuePool().createIdentifierValue(id);
On 2016/02/23 05:53:14, Timothy Loh wrote:
> isn't this just value1?

True, I changed it.

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4572: break;
On 2016/02/23 05:53:15, Timothy Loh wrote:
> Any reason for this to be different than consumeBackgroundPosition?

Maybe at some point, but probably not anymore, fixed.

https://codereview.chromium.org/1715513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4729: if
(resultY)
On 2016/02/23 05:53:14, Timothy Loh wrote:
> There shouldn't be any case where a shorthand only sets some of its longhands
> (except for that one quirk that we have, see bug 277157). Same below.

Good point! Fixed.

[Timothy Loh, 2016-02-29 05:45:25]

lgtm

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

https://codereview.chromium.org/1715513002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4507: if
(!consumePosition(range, context.mode(), unitless, positionX, positionY))
not sure if I said this yet, but careful with the patch adding the background
shorthand because consumePosition might consume tokens even if it fails.

[rwlbuis, 2016-02-29 12:42:39]

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

[commit-bot: I haz the power, 2016-02-29 12:42:46]

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

[commit-bot: I haz the power, 2016-02-29 12:44:55]

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

[commit-bot: I haz the power, 2016-02-29 12:44:55]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[rwlbuis, 2016-02-29 15:39:44]

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

[rwlbuis, 2016-02-29 15:39:44]

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/1715513002/#ps120001
(title: "Rebase test")

[commit-bot: I haz the power, 2016-02-29 15:39:55]

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

[rwlbuis, 2016-02-29 16:10:00]

On 2016/02/29 05:45:25, Timothy Loh wrote:
>
https://codereview.chromium.org/1715513002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4507: if
> (!consumePosition(range, context.mode(), unitless, positionX, positionY))
> not sure if I said this yet, but careful with the patch adding the background
> shorthand because consumePosition might consume tokens even if it fails.

Sure, will pay attention to this.

[commit-bot: I haz the power, 2016-02-29 16:45:48]

Description was changed from

==========
Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position

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

to

==========
Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position

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

[commit-bot: I haz the power, 2016-02-29 16:45:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-29 16:46:54]

Description was changed from

==========
Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position

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

to

==========
Move background related shorthands into CSSPropertyParser

Move the following background related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser:

background-repeat, background-position,
-webkit-mask-repeat, -webkit-mask-position

BUG=499780

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

[commit-bot: I haz the power, 2016-02-29 16:46:56]

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