Bug 21844 - Remove setFocusRingColorChangeFunction()
Summary: Remove setFocusRingColorChangeFunction()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Minor
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-23 17:47 PDT by Peter Kasting
Modified: 2008-10-24 13:43 PDT (History)
0 users

See Also:


Attachments
patch v1 (4.51 KB, patch)
2008-10-23 18:34 PDT, Peter Kasting
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.