Remove the ifdef and instead provide a chromium api for setting the focus color. This lets any unittest do overrides w/o forcing other layers to know about layout tests. It also provides the bottom layer so once we can plumb a ipc message for focus colors and send them from the browser over to the renderer processes.
Affected files: A third_party/WebKit/WebCore/platform/graphics/chromium/ColorChromium.h M third_party/WebKit/WebCore/platform/graphics/chromium/ColorChromium.cpp
Remove the ifdef and instead provide a chromium api for setting the focus color. This lets any unittest do overrides w/o forcing other layers to know about layout tests. It also provides the bottom layer so once we can plumb a ipc message for focus colors and send them from the browser over to the renderer processes.
This depends on http://codereview.chromium.org/18424. It then sets the color within the mac test shell during layout tests for the diffs will match.
Affected files: M webkit/tools/test_shell/mac/TestShell.xcodeproj/project.pbxproj M webkit/tools/test_shell/test_shell_mac.mm M webkit/webkit.xcodeproj/project.pbxproj
We are the only port who likely changes the focus-ring color as part of testing, but we're not the only port which needs to respond to focusring color changes as part of OS-theming changes.
The webkit layer should detect the theme changes, and probably call a "setFocusRingColor" function down in WebCore, like you've written. The question is if that function should be part of Color:: or RenderTheme:: Probably color. Then the accessors are Color::focusRingColor() and Color::setFocusRingColor(const Color&) with some nice default.
[2:45pm] <hyatt> eseidel: focus ring colors should be stored in RenderTheme [2:45pm] <hyatt> eseidel: Color is the wrong place [2:46pm] <hyatt> eseidel: selection colors and css system colors are on RenderTheme [2:46pm] <eseidel> hyatt: I'm OK with that. [2:46pm] <hyatt> eseidel: thats where focus ring colors should be too [2:46pm] <eseidel> hyatt: currently they're not, of course [2:46pm] <hyatt> yeah [2:46pm] <hyatt> that stuff predates rendertheme [2:46pm] <hyatt> that Color stuff [2:46pm] <eseidel> hyatt: I think they should probably be explicitly set on RenderTheme though [2:46pm] <eseidel> hyatt: instead of WebCore pulling from WebKit [2:46pm] <eseidel> hyatt: which is what it does now [2:46pm] <hyatt> they don't have to be explicitly set [2:46pm] <hyatt> rendertheme should just cache it when asked for it [2:46pm] <eseidel> hyatt: to handle theme changes, etc. [2:47pm] <hyatt> rendertheme already handles color changes [2:47pm] <hyatt> etc. [2:47pm] <hyatt> focus rings just need to fold into all that existing stuff in rendertheme [2:47pm] <eseidel> hyatt: I guess. although we also need the ability in our RenderTheme to override the focusRing color [2:47pm] <eseidel> which I guess we could do via a RenderTheme caching model too [2:47pm] <hyatt> eseidel: thats the whole point of rendertheme [2:47pm] <hyatt> :) [2:47pm] <hyatt> eseidel: check out selection... the base class caches by querying the derived class [2:47pm] <eseidel> hyatt: your'e suggesting that we supply a different RenderTheme subclass during testing? [2:47pm] <hyatt> and then the base class just clears its cache when colors change [2:48pm] <hyatt> eseidel: i'm suggesting your rendertheme class say if (isTesting()) return oneCOlor; else return otherColor; [2:48pm] <eseidel> that could work too [2:48pm] <eseidel> although that requires hard-coding in the testing values [2:48pm] <eseidel> but yeah [2:48pm] <eseidel> hyatt: k. will update the code review.
http://codereview.chromium.org/18424/diff/12/208#newcode35 Line 35: void setFocusRingColor(const Color &overrideColor); WebKit style would place the & next to the type, and there would be no name to this argument, as the name provides no additional clarification.
http://codereview.chromium.org/18424/diff/12/208#newcode36 Line 36: void setFocusRingColorToDefault(void); no (void) in WebKit style, and I don't think any "reset" function is needed in WebCore. I think that instead the embedder can just call "set". WebCore could expose a Color::defaultFocusRingColor; accessor if desired.
> We are the only port who likely changes the focus-ring color as part of testing, > but we're not the only port which needs to respond to focusring color changes as > part of OS-theming changes.
> [2:45pm] <hyatt> eseidel: focus ring colors should be stored in RenderTheme > [2:45pm] <hyatt> eseidel: Color is the wrong place > [2:46pm] <hyatt> eseidel: selection colors and css system colors are on > RenderTheme > [2:46pm] <eseidel> hyatt: I'm OK with that. > [2:46pm] <hyatt> eseidel: thats where focus ring colors should be too > [2:46pm] <eseidel> hyatt: currently they're not, of course > [2:46pm] <hyatt> yeah > [2:46pm] <hyatt> that stuff predates rendertheme > [2:46pm] <hyatt> that Color stuff > [2:46pm] <eseidel> hyatt: I think they should probably be explicitly set on > RenderTheme though > [2:46pm] <eseidel> hyatt: instead of WebCore pulling from WebKit > [2:46pm] <eseidel> hyatt: which is what it does now > [2:46pm] <hyatt> they don't have to be explicitly set > [2:46pm] <hyatt> rendertheme should just cache it when asked for it > [2:46pm] <eseidel> hyatt: to handle theme changes, etc. > [2:47pm] <hyatt> rendertheme already handles color changes > [2:47pm] <hyatt> etc. > [2:47pm] <hyatt> focus rings just need to fold into all that existing stuff in > rendertheme > [2:47pm] <eseidel> hyatt: I guess. although we also need the ability in our > RenderTheme to override the focusRing color > [2:47pm] <eseidel> which I guess we could do via a RenderTheme caching model too > [2:47pm] <hyatt> eseidel: thats the whole point of rendertheme > [2:47pm] <hyatt> :) > [2:47pm] <hyatt> eseidel: check out selection... the base class caches by > querying the derived class > [2:47pm] <eseidel> hyatt: your'e suggesting that we supply a different > RenderTheme subclass during testing? > [2:47pm] <hyatt> and then the base class just clears its cache when colors > change > [2:48pm] <hyatt> eseidel: i'm suggesting your rendertheme class say if > (isTesting()) return oneCOlor; else return otherColor; > [2:48pm] <eseidel> that could work too > [2:48pm] <eseidel> although that requires hard-coding in the testing values > [2:48pm] <eseidel> but yeah > [2:48pm] <eseidel> hyatt: k. will update the code review.
Seem's sorta wrong (from a design pov) to have the themes having knowledge of layout tests, like we should either have a different "testing theme" or let the tests reach though the layer to change things explicitly. But I'll defer to those that know the code better. :)
To clean up the TODO w/in ColorChromium, should I go ahead w/ comments on the changes, and record this info in the new files (letting test_shell_mac current reach down to this layer)? And as the mac version comes along, we'll move things into the RenderTheme when we build the higher layer plumbing? (we don't currently have the right places to hook in for the color notifications, etc.)
> On 2009/01/20 22:50:25, Eric Seidel wrote: >> Talked to Hyatt as well:
>> [2:45pm] <hyatt> eseidel: focus ring colors should be stored in > RenderTheme >> [2:45pm] <hyatt> eseidel: Color is the wrong place >> [2:46pm] <hyatt> eseidel: selection colors and css system colors are > on >> RenderTheme >> [2:46pm] <eseidel> hyatt: I'm OK with that. >> [2:46pm] <hyatt> eseidel: thats where focus ring colors should be too >> [2:46pm] <eseidel> hyatt: currently they're not, of course >> [2:46pm] <hyatt> yeah >> [2:46pm] <hyatt> that stuff predates rendertheme >> [2:46pm] <hyatt> that Color stuff >> [2:46pm] <eseidel> hyatt: I think they should probably be explicitly > set on >> RenderTheme though >> [2:46pm] <eseidel> hyatt: instead of WebCore pulling from WebKit >> [2:46pm] <eseidel> hyatt: which is what it does now >> [2:46pm] <hyatt> they don't have to be explicitly set >> [2:46pm] <hyatt> rendertheme should just cache it when asked for it >> [2:46pm] <eseidel> hyatt: to handle theme changes, etc. >> [2:47pm] <hyatt> rendertheme already handles color changes >> [2:47pm] <hyatt> etc. >> [2:47pm] <hyatt> focus rings just need to fold into all that existing > stuff in >> rendertheme >> [2:47pm] <eseidel> hyatt: I guess. although we also need the ability > in our >> RenderTheme to override the focusRing color >> [2:47pm] <eseidel> which I guess we could do via a RenderTheme >> caching > model too >> [2:47pm] <hyatt> eseidel: thats the whole point of rendertheme >> [2:47pm] <hyatt> :) >> [2:47pm] <hyatt> eseidel: check out selection... the base class >> caches > by >> querying the derived class >> [2:47pm] <eseidel> hyatt: your'e suggesting that we supply a >> different >> RenderTheme subclass during testing? >> [2:47pm] <hyatt> and then the base class just clears its cache when > colors >> change >> [2:48pm] <hyatt> eseidel: i'm suggesting your rendertheme class say >> if >> (isTesting()) return oneCOlor; else return otherColor; >> [2:48pm] <eseidel> that could work too >> [2:48pm] <eseidel> although that requires hard-coding in the testing > values >> [2:48pm] <eseidel> but yeah >> [2:48pm] <eseidel> hyatt: k. will update the code review.
> Seem's sorta wrong (from a design pov) to have the themes having > knowledge of layout tests, like we should either have a different > "testing theme" or let the tests reach though the layer to change > things > explicitly. But I'll defer to those that know the code better. :)
> To clean up the TODO w/in ColorChromium, should I go ahead w/ comments > on the changes, and record this info in the new files (letting > test_shell_mac current reach down to this layer)? And as the mac > version comes along, we'll move things into the RenderTheme when we > build the higher layer plumbing? (we don't currently have the right > places to hook in for the color notifications, etc.)
