Move background/webkit-mask shorthands into CSSPropertyParser

Move background/webkit-mask shorthand into CSSPropertyParser

Move the background/webkit-mask related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780
Committed: https://crrev.com/415fcc3e0defda31ddd412d266b37c567936c15d
Cr-Commit-Position: refs/heads/master@{#379752}

[rwlbuis, 2016-03-01 21:50:50]

Description was changed from

==========
background WIP

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

to

==========
background WIP

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

[rwlbuis, 2016-03-02 03:12:32]

Description was changed from

==========
background WIP

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

to

==========
Move background/webkit-mask shorthand into CSSPropertyParser

Move the background/webkit-mask related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser.

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

[rwlbuis, 2016-03-02 03:54:42]

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

[rwlbuis, 2016-03-02 03:54:43]

PTAL.

This code will never be pretty, but I think at least it is better than before :)

[Timothy Loh, 2016-03-02 04:07:01]

On 2016/03/02 03:54:43, rwlbuis wrote:
> PTAL.
> 
> This code will never be pretty, but I think at least it is better than before
:)

Can't we remove a whole heap of the old code now?

[Timothy Loh, 2016-03-02 04:50:28]

This is way better than before :)

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

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4574: bool
seenClip = false;
Doesn't seem right, this should be per-layer.

background: url(a) content-box padding-box, url(b) border-box;

should set
background-origin: content-box, border-box;
background-clip: padding-box, border-box;

but we end up setting background-clip: padding-box, initial;

Can we fix this (and add a test)?

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4594: } else if
(((property == CSSPropertyBackgroundSize || property ==
CSSPropertyWebkitMaskSize) && !consumeSlashIncludingWhitespace(m_range))
Slash handling looks wrong here, since we could consume a slash and not have a
<bg-size>, but then keep going. Can we just put the size case after the position
code just above? Then we can remove the seenPosition logic below.

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4599: if
(!value)
can this be an else?

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4621: if
(property == CSSPropertyBackgroundColor && m_range.peek().type() == CommaToken)
{
maybe clearer to write !m_range.atEnd() instead of checking for a comma?

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4623: return
false; // Color is not allowed except as the last item in a list for
backgrounds.
Sounds like it has to be the very last thing. Maybe something like
// Colors are only allowed in the last layer

[rwlbuis, 2016-03-02 15:45:42]

Patchset #29 (id:560001) has been deleted

[rwlbuis, 2016-03-07 02:50:04]

PTAL.

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

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4574: bool
seenClip = false;
On 2016/03/02 04:50:27, Timothy Loh wrote:
> Doesn't seem right, this should be per-layer.
> 
> background: url(a) content-box padding-box, url(b) border-box;
> 
> should set
> background-origin: content-box, border-box;
> background-clip: padding-box, border-box;
> 
> but we end up setting background-clip: padding-box, initial;
> 
> Can we fix this (and add a test)?

Done.

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4594: } else if
(((property == CSSPropertyBackgroundSize || property ==
CSSPropertyWebkitMaskSize) && !consumeSlashIncludingWhitespace(m_range))
On 2016/03/02 04:50:27, Timothy Loh wrote:
> Slash handling looks wrong here, since we could consume a slash and not have a
> <bg-size>, but then keep going. Can we just put the size case after the
position
> code just above? Then we can remove the seenPosition logic below.

Not sure if you meant it the way I implemented it in my latest patch :) Anyway
it works, glad to see the seenPosition logic go.

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4599: if
(!value)
On 2016/03/02 04:50:27, Timothy Loh wrote:
> can this be an else?

Done.

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4621: if
(property == CSSPropertyBackgroundColor && m_range.peek().type() == CommaToken)
{
On 2016/03/02 04:50:27, Timothy Loh wrote:
> maybe clearer to write !m_range.atEnd() instead of checking for a comma?

Done.

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4623: return
false; // Color is not allowed except as the last item in a list for
backgrounds.
On 2016/03/02 04:50:27, Timothy Loh wrote:
> Sounds like it has to be the very last thing. Maybe something like
> // Colors are only allowed in the last layer

Done.

[Timothy Loh, 2016-03-07 03:55:08]

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

https://codereview.chromium.org/1676513003/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4594: } else if
(((property == CSSPropertyBackgroundSize || property ==
CSSPropertyWebkitMaskSize) && !consumeSlashIncludingWhitespace(m_range))
On 2016/03/07 02:50:04, rwlbuis wrote:
> On 2016/03/02 04:50:27, Timothy Loh wrote:
> > Slash handling looks wrong here, since we could consume a slash and not have
a
> > <bg-size>, but then keep going. Can we just put the size case after the
> position
> > code just above? Then we can remove the seenPosition logic below.
> 
> Not sure if you meant it the way I implemented it in my latest patch :) Anyway
> it works, glad to see the seenPosition logic go.

Wasn't really what I meant, but it works.

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html
(right):

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html:6:
description("Tests a flag to make background shorthand property not override
background-size when it's already set.");
needs update

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html:11:
//e.style.background = "url('a') border-box";
remove this?

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html:14:
debug("")
does this do anything?

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

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4603: } else if
(!value) {
else

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4624:
CSSPropertyID property = shorthand.properties()[i];
Can we just start with
if (!parsedLonghand[i])
    continue;

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4636:
parsedLonghand[i] = false;
don't need this, the array will get re-initialized every (outer) loop iteration
anyway

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:371: void
CSSPropertyParser::addFillValue(RefPtrWillBeRawPtr<CSSValue>& lval,
PassRefPtrWillBeRawPtr<CSSValue> rval)
shouldn't we remove all the background parsing logic here in this patch? :)

[Timothy Loh, 2016-03-07 06:25:37]

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

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4624:
CSSPropertyID property = shorthand.properties()[i];
On 2016/03/07 03:55:07, Timothy Loh wrote:
> Can we just start with
> if (!parsedLonghand[i])
>     continue;

Err.. wait I misread the code. Nevermind.

[rwlbuis, 2016-03-08 02:51:49]

PTAL.

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html
(right):

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html:6:
description("Tests a flag to make background shorthand property not override
background-size when it's already set.");
On 2016/03/07 03:55:07, Timothy Loh wrote:
> needs update

Done.

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html:6:
description("Tests a flag to make background shorthand property not override
background-size when it's already set.");
On 2016/03/07 03:55:07, Timothy Loh wrote:
> needs update

Done.

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html:11:
//e.style.background = "url('a') border-box";
On 2016/03/07 03:55:07, Timothy Loh wrote:
> remove this?

Done.

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/backgrounds/background-clip-per-layer.html:14:
debug("")
On 2016/03/07 03:55:07, Timothy Loh wrote:
> does this do anything?

Just adds a newline to expected result, removed.

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

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4603: } else if
(!value) {
On 2016/03/07 03:55:07, Timothy Loh wrote:
> else

Done.

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:4636:
parsedLonghand[i] = false;
On 2016/03/07 03:55:07, Timothy Loh wrote:
> don't need this, the array will get re-initialized every (outer) loop
iteration
> anyway

Done.

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp
(right):

https://codereview.chromium.org/1676513003/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/LegacyCSSPropertyParser.cpp:371: void
CSSPropertyParser::addFillValue(RefPtrWillBeRawPtr<CSSValue>& lval,
PassRefPtrWillBeRawPtr<CSSValue> rval)
On 2016/03/07 03:55:07, Timothy Loh wrote:
> shouldn't we remove all the background parsing logic here in this patch? :)

I prefer to do that in a separate patch, I already prepared it and the change
(mostly removal with some code moving) will be huge :)

[Timothy Loh, 2016-03-08 03:04:49]

lgtm

[rwlbuis, 2016-03-08 03:33:20]

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

[commit-bot: I haz the power, 2016-03-08 03:33:39]

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

[commit-bot: I haz the power, 2016-03-08 05:08:47]

Description was changed from

==========
Move background/webkit-mask shorthand into CSSPropertyParser

Move the background/webkit-mask related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser.

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

to

==========
Move background/webkit-mask shorthand into CSSPropertyParser

Move the background/webkit-mask related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser.

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

[commit-bot: I haz the power, 2016-03-08 05:08:47]

Committed patchset #32 (id:640001)

[commit-bot: I haz the power, 2016-03-08 05:09:56]

Description was changed from

==========
Move background/webkit-mask shorthand into CSSPropertyParser

Move the background/webkit-mask related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser.

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

to

==========
Move background/webkit-mask shorthand into CSSPropertyParser

Move the background/webkit-mask related shorthands from
LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780

Committed: https://crrev.com/415fcc3e0defda31ddd412d266b37c567936c15d
Cr-Commit-Position: refs/heads/master@{#379752}
==========

[commit-bot: I haz the power, 2016-03-08 05:09:57]

Patchset 32 (id:??) landed as
https://crrev.com/415fcc3e0defda31ddd412d266b37c567936c15d
Cr-Commit-Position: refs/heads/master@{#379752}