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.
<rdar://problem/57295392>
Created attachment 383791 [details] Patch
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); }
Created attachment 385624 [details] Patch
(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!
Created attachment 385688 [details] Patch
Created attachment 385694 [details] Patch
Created attachment 385699 [details] Patch
Comment on attachment 385699 [details] Patch Clearing flags on attachment: 385699 Committed r253530: <https://trac.webkit.org/changeset/253530>
Landed in <https://trac.webkit.org/changeset/253530/webkit>
Rolled out in <https://trac.webkit.org/changeset/253784/webkit>
Created attachment 387045 [details] Patch
Created attachment 387048 [details] Patch
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?
Created attachment 387362 [details] Patch
(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!
Created attachment 387385 [details] Patch
Committed r254373: <https://trac.webkit.org/changeset/254373/webkit>.