Bug 21844 – Remove setFocusRingColorChangeFunction()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Peter Kasting]

setFocusRingColorChangeFunction() seems totally useless, as far as I can tell.

Only the Mac port implements it at all.  In ColorMac.mm, it sets a file-static variable -- which is then never used by anyone.

[Peter Kasting]

Created attachment 24630 [details]
patch v1

Removing this function causes the Mac port to no longer hook up an observer until focusRingColor() is called.  I don't think that can have a practical effect, since focusRingColor() is the only way callers can actually get the color the observer sets.  If anything, this makes the code (insignificantly) more efficient.

[Peter Kasting]

I've been thinking about this code more, and I'm not sure this patch is precisely right.  It maintains what the current code does, but I think the current code may be broken.

There are two questions at stake:
(1) Is the custom CSS style to get the focus ring color used by any consumers?
(2) Should this style and/or the focus ring itself redraw immediately in response to system theme changes, or can such changes only happen when the browser isn't running?

If (1) is "no", then focusRingColor() can disappear and the remaining code can move into the GraphicsContext code that draws the focus ring.

If (2) is "yes", then instead of removing setFocusRingColorChangeFunction(), the color change observer needs to be updated to call that function, and either that function or some other function the observer calls needs to redraw the focus ring.

Only if the answers are, respectively, "yes" and "no" is this patch precisely what we want.

[Peter Kasting]

On IRC, Maciej notes that the default focus ring is just a CSS rule:
  outline: auto 5px -webkit-focus-ring-color

In this case, the answer to (1) is definitely "yes".  But it makes me wonder why GraphicsContext::drawFocusRing() is needed if the focus ring is just a CSS outline.  Maybe one is used in some scenarios and one in others?

[Dave Hyatt]

Comment on attachment 24630 [details]
patch v1

r=me, should go even further and move focusRingColor stuff into the Theme eventually.  This should wait until i have theme refactored though. :)

[Peter Kasting]

Fixed in r37858.