Summary: | Map CSS value ID to system color in the UI process | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, dino, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, pdr, timothy, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=213396 | ||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2019-11-18 13:30:46 PST
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>. |