[webkit-reviews] review granted: [Bug 204314] Map CSS value ID to system color in the UI process : [Attachment 387048] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 204314: Map CSS value ID to system color in the UI process
https://bugs.webkit.org/show_bug.cgi?id=204314

Attachment 387048: Patch

https://bugs.webkit.org/attachment.cgi?id=387048&action=review




--- Comment #14 from Dean Jackson <dino at apple.com> ---
Comment on attachment 387048
  --> https://bugs.webkit.org/attachment.cgi?id=387048
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.h:63
> +    WEBCORE_EXPORT static const HashMap<CSSValueKey, Color>&
getOrCreateCSSValueToSystemColorMap();

I think this should be just cssValueToSystemColorMap(). The "getOrCreate"
doesn't add much value.

Also, could you define this map type with a using statement, so that it has a
name?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:265
> +   
RenderThemeIOS::setCSSValueToSystemColorMap(WTFMove(parameters.cssValueToSystem
ColorMap));
> +#endif

Why is this only iOS?

> LayoutTests/TestExpectations:26
> +fast/css/ios [ Skip ]

Why is this skipped on iOS? You have code above that sets the mapping on iOS.

> LayoutTests/fast/css/ios/system-color-for-css-value.html:12
> +   
shouldBe("internals.systemColorForCSSValue(\"-apple-system-control-background\"
, true, false)", "\"rgb(0, 0, 0)\"");
> +   
shouldBe("internals.systemColorForCSSValue(\"-apple-system-secondary-grouped-ba
ckground\", true, false)", "\"rgb(28, 28, 30)\"");

You could use `` strings to avoid having to escape the "

Are you testing dark mode anywhere? Also, don't you want to test all the
values?