Bug 21844

Summary: Remove setFocusRingColorChangeFunction()
Product: WebKit Reporter: Peter Kasting <pkasting>
Component: WebCore Misc.Assignee: Peter Kasting <pkasting>
Status: RESOLVED FIXED    
Severity: Minor    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
patch v1 hyatt: review+

Description Peter Kasting 2008-10-23 17:47:56 PDT
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.
Comment 1 Peter Kasting 2008-10-23 18:34:26 PDT
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.
Comment 2 Peter Kasting 2008-10-24 10:07:41 PDT
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.
Comment 3 Peter Kasting 2008-10-24 10:49:23 PDT
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?
Comment 4 Dave Hyatt 2008-10-24 11:29:18 PDT
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. :)
Comment 5 Peter Kasting 2008-10-24 13:43:30 PDT
Fixed in r37858.