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
Peter Kasting
2008-10-23 17:47:56 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.
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. 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 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. :)
|