Move background related longhands into CSSPropertyParser

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
Committed: https://crrev.com/f508558913b0628d8c009b3240a9c1587e12f015
Cr-Commit-Position: refs/heads/master@{#376677}

[rwlbuis, 2016-02-17 03:34:55]

Description was changed from

==========
backgrounds longhands WIP

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

to

==========
Move background related longhands into CSSPropertyParser

Move the following background related longhands from
LegacyCSSPropertyParser into CSSPropertyParser:

border-image-outset, webkit-mask-box-image-outset
border-image-repeat, webkit-mask-box-image-repeat
border-image-slice, webkit-mask-box-image-slice
border-image-width, webkit-mask-box-image-width

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

[rwlbuis, 2016-02-17 03:44:17]

Description was changed from

==========
Move background related longhands into CSSPropertyParser

Move the following background related longhands from
LegacyCSSPropertyParser into CSSPropertyParser:

border-image-outset, webkit-mask-box-image-outset
border-image-repeat, webkit-mask-box-image-repeat
border-image-slice, webkit-mask-box-image-slice
border-image-width, webkit-mask-box-image-width

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

to

==========
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
==========

[rwlbuis, 2016-02-17 04:00:41]

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

[rwlbuis, 2016-02-17 04:00:41]

PTAL. Note that some tests had to be rebased because of the new <position>
parsing logic.

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

On 2016/02/17 04:00:41, rwlbuis wrote:
> PTAL. Note that some tests had to be rebased because of the new <position>
> parsing logic.

Hmm, looks like https://codereview.chromium.org/1708173002/ should go in first.
I can easily adjust this patch after that one has landed though.

[Timothy Loh, 2016-02-19 04:11:14]

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

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3402: if
(identMatches<CSSValueNormal, CSSValueOverlay>(range.peek().id()))
I think it'd be clearer to not use these helpers here, e.g.

CSSValueKeyword id = range.peek().id();
if (id == CSSValueNormal || id == CSSValueOverlay || (CSSValueMultiply <= id &&
id <= CSSValueLuminosity)
    return consumeIdent(range);
return nullptr;

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3412: static
PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundOrigin(CSSParserTokenRange&
range)
Can we merge this and consumeBackgroundClip and name it consumeBox or
consumeBackgroundBox? The spec calls these a <box> type.

https://drafts.csswg.org/css-backgrounds-3/#the-background-clip

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3428: static
PassRefPtrWillBeRawPtr<CSSValue> consumeClipOrOrigin(CSSPropertyID property,
CSSParserTokenRange& range, const CSSParserContext& context)
consumePrefixedBackgroundBox?

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3430: //
CSSValueBorder, CSSValuePadding and CSSValueContent are deprecated and do not
apply to the version of the property that has the -webkit- prefix removed.
Maybe better to talk about the actual values and not enum values, e.g.

// The values 'border', 'padding' and 'content' ...

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3449: if
(identMatches<CSSValueAuto>(range.peek().id())) // `auto' is the default
Why not just range.peek().id() == CSSValueAuto?

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3454: // For
backwards compatibility we set the second value to the first if it is omitted.
I don't think this needs to talk about masks here. I'd just write e.g. 

// Legacy syntax: "-webkit-background-size: 10px" is equivalent to
"background-size: 10px 10px"

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3464: static
PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundComponent(CSSPropertyID
property, CSSParserTokenRange& range, const CSSParserContext& context)
property -> unresolvedProperty

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3506: static
void addBackgroundValue(RefPtrWillBeRawPtr<CSSValue>& lvalue,
PassRefPtrWillBeRawPtr<CSSValue> rvalue)
I think it'd help to write a comment like
// To conserve memory we don't actually wrap a single value in a list

Also maybe better names are list and value? Not sure about list, but I think
it's an improvement.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3509: if
(lvalue->isBaseValueList()) {
How about

if (!list->isBaseValueList()) {
    RefPtrWillBeRawPtr<CSSValueList> firstValue = list.release();
    list = CSSValueList::createCommaSeparated();
    list->append(firstValue.release());
}
list->append(value);

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3525:
RefPtrWillBeRawPtr<CSSValue> ret = nullptr;
ret -> result

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3868: case
CSSPropertyBackgroundClip:
Probably looks better sorted ;)

[rwlbuis, 2016-02-19 16:24:59]

Description was changed from

==========
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
==========

to

==========
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
==========

[rwlbuis, 2016-02-21 18:22:08]

PTAL :)

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

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3402: if
(identMatches<CSSValueNormal, CSSValueOverlay>(range.peek().id()))
On 2016/02/19 04:11:13, Timothy Loh wrote:
> I think it'd be clearer to not use these helpers here, e.g.
> 
> CSSValueKeyword id = range.peek().id();
> if (id == CSSValueNormal || id == CSSValueOverlay || (CSSValueMultiply <= id
&&
> id <= CSSValueLuminosity)
>     return consumeIdent(range);
> return nullptr;

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3412: static
PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundOrigin(CSSParserTokenRange&
range)
On 2016/02/19 04:11:13, Timothy Loh wrote:
> Can we merge this and consumeBackgroundClip and name it consumeBox or
> consumeBackgroundBox? The spec calls these a <box> type.
> 
> https://drafts.csswg.org/css-backgrounds-3/#the-background-clip

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3428: static
PassRefPtrWillBeRawPtr<CSSValue> consumeClipOrOrigin(CSSPropertyID property,
CSSParserTokenRange& range, const CSSParserContext& context)
On 2016/02/19 04:11:13, Timothy Loh wrote:
> consumePrefixedBackgroundBox?

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3430: //
CSSValueBorder, CSSValuePadding and CSSValueContent are deprecated and do not
apply to the version of the property that has the -webkit- prefix removed.
On 2016/02/19 04:11:13, Timothy Loh wrote:
> Maybe better to talk about the actual values and not enum values, e.g.
> 
> // The values 'border', 'padding' and 'content' ...

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3449: if
(identMatches<CSSValueAuto>(range.peek().id())) // `auto' is the default
On 2016/02/19 04:11:13, Timothy Loh wrote:
> Why not just range.peek().id() == CSSValueAuto?

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3454: // For
backwards compatibility we set the second value to the first if it is omitted.
On 2016/02/19 04:11:13, Timothy Loh wrote:
> I don't think this needs to talk about masks here. I'd just write e.g. 
> 
> // Legacy syntax: "-webkit-background-size: 10px" is equivalent to
> "background-size: 10px 10px"

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3464: static
PassRefPtrWillBeRawPtr<CSSValue> consumeBackgroundComponent(CSSPropertyID
property, CSSParserTokenRange& range, const CSSParserContext& context)
On 2016/02/19 04:11:13, Timothy Loh wrote:
> property -> unresolvedProperty

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3506: static
void addBackgroundValue(RefPtrWillBeRawPtr<CSSValue>& lvalue,
PassRefPtrWillBeRawPtr<CSSValue> rvalue)
On 2016/02/19 04:11:13, Timothy Loh wrote:
> I think it'd help to write a comment like
> // To conserve memory we don't actually wrap a single value in a list

Done.

> Also maybe better names are list and value? Not sure about list, but I think
> it's an improvement.

Right, list is not completely accurate, but probably good enough.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3509: if
(lvalue->isBaseValueList()) {
On 2016/02/19 04:11:13, Timothy Loh wrote:
> How about
> 
> if (!list->isBaseValueList()) {
>     RefPtrWillBeRawPtr<CSSValueList> firstValue = list.release();
>     list = CSSValueList::createCommaSeparated();
>     list->append(firstValue.release());
> }
> list->append(value);

Done, except that I have to cast to CSSValueList :)

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3525:
RefPtrWillBeRawPtr<CSSValue> ret = nullptr;
On 2016/02/19 04:11:13, Timothy Loh wrote:
> ret -> result

Done.

https://codereview.chromium.org/1706573002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3868: case
CSSPropertyBackgroundClip:
On 2016/02/19 04:11:13, Timothy Loh wrote:
> Probably looks better sorted ;)

Done.

[Timothy Loh, 2016-02-21 23:49:17]

lgtm

[rwlbuis, 2016-02-22 00:01:03]

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

[commit-bot: I haz the power, 2016-02-22 00:03:39]

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

[commit-bot: I haz the power, 2016-02-22 02:26:03]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-02-22 02:26:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-22 02:27:30]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/f508558913b0628d8c009b3240a9c1587e12f015
Cr-Commit-Position: refs/heads/master@{#376677}
==========

[commit-bot: I haz the power, 2016-02-22 02:27:31]

Patchset 6 (id:??) landed as
https://crrev.com/f508558913b0628d8c009b3240a9c1587e12f015
Cr-Commit-Position: refs/heads/master@{#376677}