Prepare for adding relative color support
Created attachment 420256 [details] Patch
Created attachment 420258 [details] Patch
Created attachment 420260 [details] Patch
Created attachment 420261 [details] Patch
Created attachment 420265 [details] Patch
Created attachment 420267 [details] Patch
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?
ChangeLog entry in Tools/ChangeLog contains OOPS!.
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.
(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.
Created attachment 420330 [details] Patch
Committed r272870: <https://commits.webkit.org/r272870> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420330 [details].
<rdar://problem/74358043>
Reopening to attach new patch.
Created attachment 420353 [details] Patch for landing
Committed r272876: <https://commits.webkit.org/r272876> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420353 [details].
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.