Bug 204314 – Map CSS value ID to system color in the UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Currently, RenderThemeIOS is mapping CSS value IDs to system colors in the WebContent process. This mapping leads to invoking selectors on UITraitCollection and UIColor, which will send messages to the runningboard daemon. Since we will be blocking access to this daemon in the WebContent process, this mapping should be moved to the UI process.

[Radar WebKit Bug Importer]

<rdar://problem/57295392>

[Per Arne Vollan]

Created attachment 383791 [details]
Patch

[Brent Fulgham]

Comment on attachment 383791 [details]
Patch

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

r=me, but please consider the improvements I suggested.

> Source/WebCore/rendering/RenderThemeIOS.h:-42
> -    

Whitespace change isn't needed, but maybe the editor just does this automatically.

> Source/WebCore/rendering/RenderThemeIOS.mm:1476
> +        { CSSValueAppleSystemYellow, @selector(systemYellowColor) }

Is there any way to help future developers know that they should update this list if they need new CSS color values?

> Source/WebCore/rendering/RenderThemeIOS.mm:1489
> +        for (unsigned i = 0; i < size; i++) {

Could this be a modern C++ loop?

> Source/WebCore/rendering/RenderThemeIOS.mm:1524
> +                map.add(CSSValueKey { cssValueIDSelectors[i].cssValueID, useDarkAppearance, useElevatedUserInterfaceLevel }, *color);

I wonder if this would be clearer if we made a setter function and just called it four times:

'createCSSColorValueForOptions(unsigned valueID, bool useDarkAppearance, bool userElevatedUserInterfaceLevel)'

Then the loop would just be once over the selectors:

for (auto valueSelectors : cssValueIDSelectors) {
    createCSSColorValueForOptions(valueSelectors.cssValueID, false, false);
    createCSSColorValueForOptions(valueSelectors.cssValueID, false, true);
    createCSSColorValueForOptions(valueSelectors.cssValueID, true, false);
    createCSSColorValueForOptions(valueSelectors.cssValueID, true, true);
}

[Per Arne Vollan]

Created attachment 385624 [details]
Patch

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 383791 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383791&action=review
> 
> r=me, but please consider the improvements I suggested.
> 
> > Source/WebCore/rendering/RenderThemeIOS.h:-42
> > -    
> 
> Whitespace change isn't needed, but maybe the editor just does this
> automatically.
> 

Fixed.

> > Source/WebCore/rendering/RenderThemeIOS.mm:1476
> > +        { CSSValueAppleSystemYellow, @selector(systemYellowColor) }
> 
> Is there any way to help future developers know that they should update this
> list if they need new CSS color values?
> 

That's a good point. Perhaps we could add a comment?

> > Source/WebCore/rendering/RenderThemeIOS.mm:1489
> > +        for (unsigned i = 0; i < size; i++) {
> 
> Could this be a modern C++ loop?
> 

Fixed.

> > Source/WebCore/rendering/RenderThemeIOS.mm:1524
> > +                map.add(CSSValueKey { cssValueIDSelectors[i].cssValueID, useDarkAppearance, useElevatedUserInterfaceLevel }, *color);
> 
> I wonder if this would be clearer if we made a setter function and just
> called it four times:
> 
> 'createCSSColorValueForOptions(unsigned valueID, bool useDarkAppearance,
> bool userElevatedUserInterfaceLevel)'
> 
> Then the loop would just be once over the selectors:
> 
> for (auto valueSelectors : cssValueIDSelectors) {
>     createCSSColorValueForOptions(valueSelectors.cssValueID, false, false);
>     createCSSColorValueForOptions(valueSelectors.cssValueID, false, true);
>     createCSSColorValueForOptions(valueSelectors.cssValueID, true, false);
>     createCSSColorValueForOptions(valueSelectors.cssValueID, true, true);
> }

I used the following loop construct in the latest patch:

for (bool useDarkAppearance : { false, true }) ...

I hope this will make it easier to read.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 385688 [details]
Patch

[Per Arne Vollan]

Created attachment 385694 [details]
Patch

[Per Arne Vollan]

Created attachment 385699 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 385699 [details]
Patch

Clearing flags on attachment: 385699

Committed r253530: <https://trac.webkit.org/changeset/253530>

[Per Arne Vollan]

Landed in <https://trac.webkit.org/changeset/253530/webkit>

[Per Arne Vollan]

Rolled out in <https://trac.webkit.org/changeset/253784/webkit>

[Per Arne Vollan]

Created attachment 387045 [details]
Patch

[Per Arne Vollan]

Created attachment 387048 [details]
Patch

[Dean Jackson]

Comment on attachment 387048 [details]
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.cssValueToSystemColorMap));
> +#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-background\", 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?

[Per Arne Vollan]

Created attachment 387362 [details]
Patch

[Per Arne Vollan]

(In reply to Dean Jackson from comment #14)
> Comment on attachment 387048 [details]
> 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.
> 

Fixed.

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

Done.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:265
> > +    RenderThemeIOS::setCSSValueToSystemColorMap(WTFMove(parameters.cssValueToSystemColorMap));
> > +#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.
> 

The tests are skipped globally, but then enabled for iOS in the iOS platform directory.

> > 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-background\", true, false)", "\"rgb(28, 28, 30)\"");
> 
> You could use `` strings to avoid having to escape the "
> 

Fixed.

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

This is only testing dark mode (the first boolean parameter to internals.systemColorForCSSValue). I have added tests of the rest of the values.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 387385 [details]
Patch

[Per Arne Vollan]

Committed r254373: <https://trac.webkit.org/changeset/254373/webkit>.