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

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> 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 383791: Patch

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




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 383791
  --> https://bugs.webkit.org/attachment.cgi?id=383791
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);
}