Bug 204314 - Map CSS value ID to system color in the UI process
Summary: Map CSS value ID to system color in the UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-18 13:30 PST by Per Arne Vollan
Modified: 2020-01-10 15:41 PST (History)
14 users (show)

See Also:


Attachments
Patch (26.00 KB, patch)
2019-11-18 14:40 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.13 KB, patch)
2019-12-13 11:51 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (21.08 KB, patch)
2019-12-14 08:32 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.08 KB, patch)
2019-12-14 11:18 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (25.31 KB, patch)
2019-12-14 13:48 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (27.78 KB, patch)
2020-01-07 15:37 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (32.20 KB, patch)
2020-01-07 16:05 PST, Per Arne Vollan
dino: review+
Details | Formatted Diff | Diff
Patch (39.40 KB, patch)
2020-01-10 12:16 PST, Per Arne Vollan
pvollan: commit-queue-
Details | Formatted Diff | Diff
Patch (38.70 KB, patch)
2020-01-10 15:35 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-11-18 13:30:46 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-11-18 13:31:09 PST
<rdar://problem/57295392>
Comment 2 Per Arne Vollan 2019-11-18 14:40:29 PST
Created attachment 383791 [details]
Patch
Comment 3 Brent Fulgham 2019-12-05 10:02:52 PST
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);
}
Comment 4 Per Arne Vollan 2019-12-13 11:51:07 PST
Created attachment 385624 [details]
Patch
Comment 5 Per Arne Vollan 2019-12-13 12:00:21 PST
(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!
Comment 6 Per Arne Vollan 2019-12-14 08:32:21 PST
Created attachment 385688 [details]
Patch
Comment 7 Per Arne Vollan 2019-12-14 11:18:25 PST
Created attachment 385694 [details]
Patch
Comment 8 Per Arne Vollan 2019-12-14 13:48:55 PST
Created attachment 385699 [details]
Patch
Comment 9 WebKit Commit Bot 2019-12-14 15:37:22 PST
Comment on attachment 385699 [details]
Patch

Clearing flags on attachment: 385699

Committed r253530: <https://trac.webkit.org/changeset/253530>
Comment 10 Per Arne Vollan 2020-01-06 15:21:14 PST
Landed in <https://trac.webkit.org/changeset/253530/webkit>
Comment 11 Per Arne Vollan 2020-01-06 15:34:39 PST
Rolled out in <https://trac.webkit.org/changeset/253784/webkit>
Comment 12 Per Arne Vollan 2020-01-07 15:37:56 PST
Created attachment 387045 [details]
Patch
Comment 13 Per Arne Vollan 2020-01-07 16:05:49 PST
Created attachment 387048 [details]
Patch
Comment 14 Dean Jackson 2020-01-09 11:19:57 PST
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?
Comment 15 Per Arne Vollan 2020-01-10 12:16:09 PST
Created attachment 387362 [details]
Patch
Comment 16 Per Arne Vollan 2020-01-10 12:19:26 PST
(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!
Comment 17 Per Arne Vollan 2020-01-10 15:35:53 PST
Created attachment 387385 [details]
Patch
Comment 18 Per Arne Vollan 2020-01-10 15:41:53 PST
Committed r254373: <https://trac.webkit.org/changeset/254373/webkit>.