Set the focus ring color to match the Gtk theme focus color.

Set the focus ring color to match the Gtk theme focus color.

BUG=8540
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25278

[Evan Stade, 2009-09-01 18:32:22]

this will always set the focus ring color to match your gtk theme. On the other
hand, we could leave the focus ring alone for chrome themes and only use the
focus ring for gtk theme mode, which makes sense to me, but doesn't seem to jive
with our intent to put gtk-drawn widgets in the render view.

[Evan Martin, 2009-09-01 18:33:22]

+joel, who's working in this area

[Evan Stade, 2009-09-01 18:35:55]

oh, and there's a webkit side: http://codereview.chromium.org/180069/show

which I will try to upstream after the review for this patch is finished.

[Elliot Glaysher, 2009-09-01 18:40:42]

> but doesn't seem to jive with our intent to put gtk-drawn widgets in the
render view.

I thought that we explicitly didn't want to do that. IIRC, EvanM said that he
talked to Owen Tyler and he asked us not to do that until there was specific
infrastructure in place in GTK for do this.

http://codereview.chromium.org/173642/diff/16/21
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/173642/diff/16/21#newcode359
Line 359: void InitRendererPrefsFromGtkSettings(RendererPreferences* prefs) {
I assume this is called once, and that we don't ferry the RendererPrefrences
over to currently existing on configuration change events, i.e. theme change?
(We should  do this anyway to detect system changes in xft preferences...)

If not, could you file a bug against me to go ahead and do that?

[Evan Stade, 2009-09-01 18:44:44]

>I thought that we explicitly didn't want to do that. IIRC, EvanM said that he
>talked to Owen Tyler and he asked us not to do that until there was specific
>infrastructure in place in GTK for do this.

right, well, long term intent, but intent nonetheless

http://codereview.chromium.org/173642/diff/16/21
File chrome/common/gtk_util.cc (right):

http://codereview.chromium.org/173642/diff/16/21#newcode359
Line 359: void InitRendererPrefsFromGtkSettings(RendererPreferences* prefs) {
On 2009/09/01 18:40:42, Elliot Glaysher wrote:
> I assume this is called once, and that we don't ferry the RendererPrefrences
> over to currently existing on configuration change events, i.e. theme change?
> (We should  do this anyway to detect system changes in xft preferences...)
> 
> If not, could you file a bug against me to go ahead and do that?

once per render view, yes. I will file a bug.

[tony, 2009-09-01 18:49:25]

http://codereview.chromium.org/173642/diff/16/22
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/173642/diff/16/22#newcode316
Line 316: renderer_prefs.focus_ring_color_g,
Can you move this to OnSetRendererPrefs?

[Evan Stade, 2009-09-01 18:51:57]

http://codereview.chromium.org/173642/diff/16/22
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/173642/diff/16/22#newcode316
Line 316: renderer_prefs.focus_ring_color_g,
On 2009/09/01 18:49:25, tony wrote:
> Can you move this to OnSetRendererPrefs?

no because the webview() hasn't been created by then.

[Evan Stade, 2009-09-01 18:52:48]

ah but I see your point. It will be necessary for when we change themes. hm. Do
you know if it's ok to move OnSetRendererPrefs to after webview creation?

[tony, 2009-09-01 18:53:24]

On 2009/09/01 18:40:42, Elliot Glaysher wrote:
> > but doesn't seem to jive with our intent to put gtk-drawn widgets in the
> render view.
> 
> I thought that we explicitly didn't want to do that. IIRC, EvanM said that he
> talked to Owen Tyler and he asked us not to do that until there was specific
> infrastructure in place in GTK for do this.

I don't think we explicitly don't want GTK theming in the renderer, it's just a
lot of work and not really how GTK is intended to be used.  That said, I think
we should plumb the easy stuff (focus ring, selection color, etc), and punt on
the harder stuff (buttons, scrollbars, etc) until GTK provides an API for it.

[tony, 2009-09-01 18:56:09]

On 2009/09/01 18:52:48, Evan Stade wrote:
> ah but I see your point. It will be necessary for when we change themes. hm.
Do
> you know if it's ok to move OnSetRendererPrefs to after webview creation?

Yes, that should be ok.  See RenderView::OnThemeChanged which just forces a
relayout/paint after invalidating the old theme handles on Windows.

[Evan Stade, 2009-09-01 19:42:23]

OK, I updated it to sync the renderer prefs whenever the theme changes (although
I think this won't work for when the gtk theme changes, but we are using the
chrome theme).

[Elliot Glaysher, 2009-09-01 19:45:18]

On 2009/09/01 19:42:23, Evan Stade wrote:
> OK, I updated it to sync the renderer prefs whenever the theme changes
(although
> I think this won't work for when the gtk theme changes, but we are using the
> chrome theme).

LGTM. Getting the correct selected colors should be really easy now.

[darin (slow to review), 2009-09-10 06:14:47]

http://codereview.chromium.org/173642/diff/32/35
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/173642/diff/32/35#newcode1784
Line 1784: void WebViewImpl::SetThemeFocusRingColor(int r, int g, int b) {
why is this a method on WebView when it doesn't have anything to do with the
state of a WebView?

this should really be using the WebKit::setNamedColors interface.  was there a
reason why that was not done?

[Evan Stade, 2009-09-10 18:29:02]

> this should really be using the WebKit::setNamedColors interface.  was there a
> reason why that was not done?

ignorance on my part. Joel, Elliot, are you working in this area? If so, are you
going to change how this stuff works soon, or would it be worth it for me to go
back and fix it?

[Elliot Glaysher, 2009-09-10 18:37:12]

I stopped working on this since there were already two people (estade
and joe) touching it so I abandoned it; I didn't want to duplicate
work.

-- Elliot

On Thu, Sep 10, 2009 at 11:29 AM, <estade@chromium.org> wrote:
>> this should really be using the WebKit::setNamedColors interface. =A0was
>> there a
>> reason why that was not done?
>
> ignorance on my part. Joel, Elliot, are you working in this area? If so, =
are
> you
> going to change how this stuff works soon, or would it be worth it for me=
 to
> go
> back and fix it?
>
> http://codereview.chromium.org/173642
>

[darin (slow to review), 2009-09-10 18:57:30]

It would be great if someone could help with this.  I think we should only
have one way to set colors in the WebKit API.  In the future, please feel
free to run WebKit API changes by me.  I'm happy to help.  I try my best to
watch relevant code reviews, but sometimes I miss them.
Thanks,
-Darin

On Thu, Sep 10, 2009 at 11:36 AM, Elliot Glaysher (Chromium) <
erg@chromium.org> wrote:

> I stopped working on this since there were already two people (estade
> and joe) touching it so I abandoned it; I didn't want to duplicate
> work.
>
> -- Elliot
>
> On Thu, Sep 10, 2009 at 11:29 AM, <estade@chromium.org> wrote:
> >> this should really be using the WebKit::setNamedColors interface.  was
> >> there a
> >> reason why that was not done?
> >
> > ignorance on my part. Joel, Elliot, are you working in this area? If so,
> are
> > you
> > going to change how this stuff works soon, or would it be worth it for me
> to
> > go
> > back and fix it?
> >
> > http://codereview.chromium.org/173642
> >
>

[Evan Stade, 2009-09-10 20:37:32]

filed http://code.google.com/p/chromium/issues/detail?id=21521

[Evan Stade, 2009-09-10 20:52:58]

It appears that WebKit::setNamedColors is stubbed out.

[darin (slow to review), 2009-09-10 20:58:15]

Right.  It would need to be completed.  It is okay to just add support for
setting the focus ring color for now since this API isn't used yet.-Darin

On Thu, Sep 10, 2009 at 1:52 PM, <estade@chromium.org> wrote:

> It appears that WebKit::setNamedColors is stubbed out.
>
>
> http://codereview.chromium.org/173642
>

[Evan Stade, 2009-09-10 22:32:12]

updated

[darin (slow to review), 2009-09-10 22:50:15]

http://codereview.chromium.org/173642/diff/32/39
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/173642/diff/32/39#newcode1577
Line 1577: WriteParam(m, p.focus_ring_color_r);
why not just send the WebKit::WebColor over IPC?  it is just an unsigned int, so
you won't have to write ParamTraits for it.  you could also use a SkColor which
is equivalent to WebColor.

[Evan Stade, 2009-09-10 22:53:28]

(note that the previous "updated" msg was sent in error)

http://codereview.chromium.org/173642/diff/32/39
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/173642/diff/32/39#newcode1577
Line 1577: WriteParam(m, p.focus_ring_color_r);
On 2009/09/10 22:50:15, darin wrote:
> why not just send the WebKit::WebColor over IPC?  it is just an unsigned int,
so
> you won't have to write ParamTraits for it.  you could also use a SkColor
which
> is equivalent to WebColor.

well, I can change the format of how the color is sent, but I believe it does
belong in RenderPreferences, so I would still need to edit the ParamTraits.

[darin (slow to review), 2009-09-10 23:00:11]

On Thu, Sep 10, 2009 at 3:53 PM, <estade@chromium.org> wrote:

> (note that the previous "updated" msg was sent in error)
>
>
> http://codereview.chromium.org/173642/diff/32/39
> File chrome/common/render_messages.h (right):
>
> http://codereview.chromium.org/173642/diff/32/39#newcode1577
> Line 1577: WriteParam(m, p.focus_ring_color_r);
> On 2009/09/10 22:50:15, darin wrote:
>
>> why not just send the WebKit::WebColor over IPC?  it is just an
>>
> unsigned int, so
>
>> you won't have to write ParamTraits for it.  you could also use a
>>
> SkColor which
>
>> is equivalent to WebColor.
>>
>
> well, I can change the format of how the color is sent, but I believe it
> does belong in RenderPreferences, so I would still need to edit the
> ParamTraits.
>
> sorry, yes.  i meant that you wouldn't need to write special param traits
for the WebColor type.
-darin



>
> http://codereview.chromium.org/173642
>

[Joel Stanley (old), 2009-09-10 23:55:33]

On 2009/09/10 18:29:02, Evan Stade wrote:
> > this should really be using the WebKit::setNamedColors interface.  was there
a
> > reason why that was not done?
> 
> ignorance on my part. Joel, Elliot, are you working in this area? If so, are
you
> going to change how this stuff works soon, or would it be worth it for me to
go
> back and fix it?

My patch was backed out as it turned valgrind red, and I haven't had a chance to
look at it yet: http://codereview.chromium.org/186009/show

I'm happy to fix it if someone (Elliot?) can point out the correct way to do it.