Bug 221881 – Prepare for adding relative color support

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Prepare for adding relative color support

[Sam Weinig]

Created attachment 420256 [details]
Patch

[Sam Weinig]

Created attachment 420258 [details]
Patch

[Sam Weinig]

Created attachment 420260 [details]
Patch

[Sam Weinig]

Created attachment 420261 [details]
Patch

[Sam Weinig]

Created attachment 420265 [details]
Patch

[Sam Weinig]

Created attachment 420267 [details]
Patch

[Darin Adler]

Comment on attachment 420267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420267&action=review

> Source/WebCore/css/parser/CSSParser.cpp:120
> -    return CSSPropertyParserHelpers::consumeColorWorkerSafe(range, HTMLStandardMode);
> +    return CSSPropertyParserHelpers::consumeColorWorkerSafe(range, strictCSSParserContext());

Rationale for going from "standard mode" to "strict"?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:699
> +        return clampTo<double>(*alphaParameter, 0.0, 1.0);

Should not need the <double> here.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:712
> +    if (auto number = consumeNumberRaw(range))
> +        return *number;
> +
> +    return WTF::nullopt;

Doesn’t seem like we need an if here. Just a return.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:723
> +enum class RGBComponentType {
> +    Number,
> +    Percentage
> +};

Looks better on a single line?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:736
> +enum class RGBOrHSLSeparatorSyntax {
> +    Commas,
> +    WhitespaceSlash
> +};

Looks better on a single line?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:760
> +    if (auto alphaParameter = consumeNumberOrPercentDividedBy100Raw(args))
> +        return *alphaParameter;
> +
> +    return WTF::nullopt;

Doesn’t seem like we need an if here. Just a return.

> Source/WebCore/editing/cocoa/DataDetection.mm:603
> +                        hsla.lightness = 50.0f;

How do you know you found all the places that depend on the old range?

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[Darin Adler]

Comment on attachment 420267 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420267&action=review

>> Source/WebCore/css/parser/CSSParser.cpp:120
>> +    return CSSPropertyParserHelpers::consumeColorWorkerSafe(range, strictCSSParserContext());
> 
> Rationale for going from "standard mode" to "strict"?

Oh, wait, I guess those are two names for the same sort of thing. Somehow I thought "standard" was the non-strict.

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> Comment on attachment 420267 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420267&action=review
> 
> > Source/WebCore/css/parser/CSSParser.cpp:120
> > -    return CSSPropertyParserHelpers::consumeColorWorkerSafe(range, HTMLStandardMode);
> > +    return CSSPropertyParserHelpers::consumeColorWorkerSafe(range, strictCSSParserContext());
> 
> Rationale for going from "standard mode" to "strict"?

strictCSSParserContext() is just a CSSParserContext with HTMLStandardMode. 

const CSSParserContext& strictCSSParserContext()
{
    static MainThreadNeverDestroyed<CSSParserContext> strictContext(HTMLStandardMode);
    return strictContext;
}

So it should behave the same.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:699
> > +        return clampTo<double>(*alphaParameter, 0.0, 1.0);
> 
> Should not need the <double> here.

Fixed.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:712
> > +    if (auto number = consumeNumberRaw(range))
> > +        return *number;
> > +
> > +    return WTF::nullopt;
> 
> Doesn’t seem like we need an if here. Just a return.

Good point. Fixed.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:723
> > +enum class RGBComponentType {
> > +    Number,
> > +    Percentage
> > +};
> 
> Looks better on a single line?

Fixed.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:736
> > +enum class RGBOrHSLSeparatorSyntax {
> > +    Commas,
> > +    WhitespaceSlash
> > +};
> 
> Looks better on a single line?

Fixed.

> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:760
> > +    if (auto alphaParameter = consumeNumberOrPercentDividedBy100Raw(args))
> > +        return *alphaParameter;
> > +
> > +    return WTF::nullopt;
> 
> Doesn’t seem like we need an if here. Just a return.

Fixed.

> 
> > Source/WebCore/editing/cocoa/DataDetection.mm:603
> > +                        hsla.lightness = 50.0f;
> 
> How do you know you found all the places that depend on the old range?

Searched for all uses of HSLA in the code. Since you explicit conversion is needed it's pretty easy to find them all.

Thanks for the review.

[Sam Weinig]

Created attachment 420330 [details]
Patch

[EWS]

Committed r272870: <https://commits.webkit.org/r272870>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420330 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74358043>

[Michael Catanzaro]

Reopening to attach new patch.

[Michael Catanzaro]

Created attachment 420353 [details]
Patch for landing

[EWS]

Committed r272876: <https://commits.webkit.org/r272876>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420353 [details].

[Fujii Hironori]

Comment on attachment 420330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420330&action=review

> Source/WebCore/css/parser/CSSParser.cpp:120
> +    return CSSPropertyParserHelpers::consumeColorWorkerSafe(range, strictCSSParserContext());

You can't use strictCSSParserContext in parseColorWorkerSafe. See Bug 222156.