Move two color related properties into CSSPropertyParser

Move two color related properties into CSSPropertyParser

Move two color related properties handling from
LegacyCSSPropertyParser into CSSPropertyParser.

BUG=499780
Committed: https://crrev.com/8e65bfbb9a84a701f5db97bd4fbf28f8a8c097aa
Cr-Commit-Position: refs/heads/master@{#356539}

[rwlbuis, 2015-10-20 21:09:43]

Description was changed from

==========
tap-highlight-color WIP

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

to

==========
Move two color related properties into CSSPropertyParser
    
Move two color related properties handling from
LegacyCSSPropertyParser into CSSPropertyParser.
    
BUG=499780
==========

[rwlbuis, 2015-10-21 02:42:30]

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

[rwlbuis, 2015-10-21 02:42:30]

PTAL.

[Timothy Loh, 2015-10-26 05:23:09]

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

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:362: {
Both IE and FF round instead of truncating, and multiply by 255 instead of 256
(as per spec). Also, why don't we just take a CSSValue here?

static int clampForRGBColor(const CSSValue& value) {
    double result = value.getDoubleValue();
    // TODO(timloh): Multiply by 2.55 and round instead of floor.
    if (value.isPercentage())
        result *= 2.56;
    return clampTo<int>(result, 0, 255);
}

Probably also should be using clampTo everywhere else too.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:380:
ASSERT(range.peek().type() == FunctionToken);
I think consumeFunction asserts this

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:394:
colorParameter = consumeInteger(args);
I'd prefer:

= isPercent ? consumePercent(args, ValueRangeAll) : consumeInteger(args);

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:417: // Get the
first value
I don't think this comment is useful

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:423: //
normalize the Hue value and change it to be between 0 and 1.0
I don't think this comment is useful either

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:424:
colorArray[0] = (((static_cast<int>(hslValue->getDoubleValue()) % 360) + 360) %
360) / 360.0;
we can probably drop the static_cast here

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:458: String
color = token.value();
Don't we need to prepend the numeric value to this?

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:468: if
(color.setNamedColor(token.value()))
isn't this covered by isColorKeyword in the calling function?

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:517: if
(!parseColorFromValue(range, c, acceptQuirkyColors))
There are a few places where we're updating the range but returning false (e.g.
#foo, rgb(bla)). We probably don't want to modify the range in these cases.

[rwlbuis, 2015-10-26 22:21:24]

PTAL. I now realize this is an important patch since shadow and filter patches
rely on it :)

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

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:362: {
On 2015/10/26 05:23:09, Timothy Loh wrote:
> Both IE and FF round instead of truncating, and multiply by 255 instead of 256
> (as per spec). Also, why don't we just take a CSSValue here?
> 
> static int clampForRGBColor(const CSSValue& value) {
>     double result = value.getDoubleValue();
>     // TODO(timloh): Multiply by 2.55 and round instead of floor.
>     if (value.isPercentage())
>         result *= 2.56;
>     return clampTo<int>(result, 0, 255);
> }
> 
> Probably also should be using clampTo everywhere else too.

Done.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:380:
ASSERT(range.peek().type() == FunctionToken);
On 2015/10/26 05:23:09, Timothy Loh wrote:
> I think consumeFunction asserts this

Done.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:394:
colorParameter = consumeInteger(args);
On 2015/10/26 05:23:09, Timothy Loh wrote:
> I'd prefer:
> 
> = isPercent ? consumePercent(args, ValueRangeAll) : consumeInteger(args);

Way better, done.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:417: // Get the
first value
On 2015/10/26 05:23:09, Timothy Loh wrote:
> I don't think this comment is useful

Done.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:423: //
normalize the Hue value and change it to be between 0 and 1.0
On 2015/10/26 05:23:09, Timothy Loh wrote:
> I don't think this comment is useful either 

Done.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:424:
colorArray[0] = (((static_cast<int>(hslValue->getDoubleValue()) % 360) + 360) %
360) / 360.0;
On 2015/10/26 05:23:09, Timothy Loh wrote:
> we can probably drop the static_cast here

You mean using getIntValue right? Done.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:458: String
color = token.value();
On 2015/10/26 05:23:09, Timothy Loh wrote:
> Don't we need to prepend the numeric value to this?

Yeah, sorry. I changes color-quirk.html test to catch this.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:468: if
(color.setNamedColor(token.value()))
On 2015/10/26 05:23:09, Timothy Loh wrote:
> isn't this covered by isColorKeyword in the calling function?

Well spotted! I guess the original code has the same issue. Done.

https://codereview.chromium.org/1415953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:517: if
(!parseColorFromValue(range, c, acceptQuirkyColors))
On 2015/10/26 05:23:09, Timothy Loh wrote:
> There are a few places where we're updating the range but returning false
(e.g.
> #foo, rgb(bla)). We probably don't want to modify the range in these cases.

Done.

[Timothy Loh, 2015-10-27 03:52:21]

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

https://codereview.chromium.org/1415953002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:370: static bool
parseColorParameters(CSSParserTokenRange& range, int* colorArray, bool
parseAlpha)
parseColorParameters -> parseRGBParameters?

IMO this and the other function should be returning into an RGBA32 instead of an
int array.

https://codereview.chromium.org/1415953002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:498: if
(!parseColorFromValue(range, color, acceptQuirkyColors))
I don't think the current split makes sense here. Maybe split it up into
functions for keywords/functions/hash colors/quirky colors?

[rwlbuis, 2015-10-27 15:58:01]

Patchset #11 (id:200001) has been deleted

[rwlbuis, 2015-10-27 20:07:50]

PTAL. I don't think we can make it any more compact :)

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

https://codereview.chromium.org/1415953002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:370: static bool
parseColorParameters(CSSParserTokenRange& range, int* colorArray, bool
parseAlpha)
On 2015/10/27 03:52:21, Timothy Loh wrote:
> parseColorParameters -> parseRGBParameters?

Done.

> IMO this and the other function should be returning into an RGBA32 instead of
an
> int array.

Good point, done.

https://codereview.chromium.org/1415953002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:498: if
(!parseColorFromValue(range, color, acceptQuirkyColors))
On 2015/10/27 03:52:21, Timothy Loh wrote:
> I don't think the current split makes sense here. Maybe split it up into
> functions for keywords/functions/hash colors/quirky colors?

Sounds good, done.

[Timothy Loh, 2015-10-27 23:57:39]

lgtm

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

https://codereview.chromium.org/1415953002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:393:
RefPtrWillBeRawPtr<CSSPrimitiveValue> alphaValue = consumeNumber(args,
ValueRangeAll);
consumeNumberRaw probably works here

https://codereview.chromium.org/1415953002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:398:
colorArray[3] = static_cast<int>(clampTo<double>(alphaValue->getDoubleValue(),
0.0, 1.0) * nextafter(256.0, 0.0));
I'd rather this was just a variable scoped to this block instead of part of the
array (similar for hsl)

https://codereview.chromium.org/1415953002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:427: hslValue =
consumeNumber(args, ValueRangeAll);
consumeNumberRaw here too

[rwlbuis, 2015-10-28 00:52:56]

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

[rwlbuis, 2015-10-28 00:52:56]

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/1415953002/#ps280001
(title: "Patch for landing")

[commit-bot: I haz the power, 2015-10-28 00:54:11]

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

[commit-bot: I haz the power, 2015-10-28 02:09:08]

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

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

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

[rwlbuis, 2015-10-28 02:59:32]

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

https://codereview.chromium.org/1415953002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:393:
RefPtrWillBeRawPtr<CSSPrimitiveValue> alphaValue = consumeNumber(args,
ValueRangeAll);
On 2015/10/27 23:57:39, Timothy Loh wrote:
> consumeNumberRaw probably works here

Good idea! Done. BTW I see now that this is very useful. In fact I think we can
use the same idea for percentage and maybe others.

[rwlbuis, 2015-10-28 11:20:44]

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

[commit-bot: I haz the power, 2015-10-28 11:21:08]

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

[commit-bot: I haz the power, 2015-10-28 12:26:00]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-10-28 12:26:31]

Patchset 14 (id:??) landed as
https://crrev.com/8e65bfbb9a84a701f5db97bd4fbf28f8a8c097aa
Cr-Commit-Position: refs/heads/master@{#356539}