[webkit-reviews] review denied: [Bug 26691] Cleanup: Move focusRingColor to RenderTheme : [Attachment 31945] patch 3

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Jeremy Moskovich
<playmobil at google.com>'s request for review:
Bug 26691: Cleanup: Move focusRingColor to RenderTheme
https://bugs.webkit.org/show_bug.cgi?id=26691

Attachment 31945: patch 3
https://bugs.webkit.org/attachment.cgi?id=31945&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Style error:
 175 Color RenderThemeChromiumMac::focusRingColor() const {

I worry that we're no longer caching the focus ring color.
 425		 color = convertNSColorToColor([NSColor
keyboardFocusIndicatorColor]);
could be expensive to call every time if it goes down the "allocate a bitmap"
case.

Also, are you sure that keyboardFocusIndicatorColor will respect focus ring
changes?  i.e. have you tested changing the focus ring color from System
Prefernces while Safari is open and verified that the focus ring colors still
update?  (Actually they probably don't update until they're redrawn.)

I take it nsColor(color) is no longer used?

In general this looks fine.  A PLT run would tell us if the focus ring color
caching has caused a regression.

r- for the possible performance regression.  We need to investigate that
keyboardFocusIndicatorColor properly updates on focus color change in the
Appearance control panel.