Add button tinting to the toolbar buttons.

Add button tinting to the toolbar buttons.

Make CustomDrawButtonBase aware of the ThemeProvider.  If the
theme changes, we reload new pixbufs from the ThemeProvider.

This breaks RTL of the toolbar buttons, I'll add that back
in a follow up change to BrowserThemeProvider.

BUG=15506
TEST=Change themes or set the GTK+ theme.  The star button and go button should be tinted according to the theme (other buttons as well if not using GTK+ buttons).

[Elliot Glaysher, 2009-07-09 23:56:08]

LGTM. It's too bad a CustomButton will have half of its state changed by one
method and the other half by another.

[tony, 2009-07-10 00:02:29]

Oh, you mean CustomDrawButton::SetUseSystemTheme?  Hmm, I see.

I could rename the method to UserChangedTheme(bool use_gtk) and pass that from
CustomDrawButton to CustomDrawButtonBase.

Alternately, I could get rid of UserChangedTheme and use the notification
instead.  Either change is fine with me.

On 2009/07/09 23:56:08, Elliot Glaysher wrote:
> LGTM. It's too bad a CustomButton will have half of its state changed by one
> method and the other half by another.

[Elliot Glaysher, 2009-07-10 00:12:35]

It might be better to just use a UserChangedTheme; that's what I'm
doing instead of having half the interface subscribed to
BROWSER_THEME_CHANGED.

-- Elliot

On Thu, Jul 9, 2009 at 5:02 PM, <tony@chromium.org> wrote:
> Oh, you mean CustomDrawButton::SetUseSystemTheme? =A0Hmm, I see.
>
> I could rename the method to UserChangedTheme(bool use_gtk) and pass
> that from CustomDrawButton to CustomDrawButtonBase.
>
> Alternately, I could get rid of UserChangedTheme and use the
> notification instead. =A0Either change is fine with me.
>
> On 2009/07/09 23:56:08, Elliot Glaysher wrote:
>>
>> LGTM. It's too bad a CustomButton will have half of its state changed
>
> by one
>>
>> method and the other half by another.
>
>
>
> http://codereview.chromium.org/149432
>

[tony, 2009-07-10 00:20:48]

On 2009/07/10 00:12:35, Elliot Glaysher wrote:
> It might be better to just use a UserChangedTheme; that's what I'm
> doing instead of having half the interface subscribed to
> BROWSER_THEME_CHANGED.

Ok, I will remove the notification observer and do that.  New patch coming
soon...

[Evan Stade, 2009-07-10 00:21:37]

On 2009/07/10 00:02:29, tony wrote:
> Oh, you mean CustomDrawButton::SetUseSystemTheme?  Hmm, I see.
> 
> I could rename the method to UserChangedTheme(bool use_gtk) and pass that from
> CustomDrawButton to CustomDrawButtonBase.
> 
> Alternately, I could get rid of UserChangedTheme and use the notification
> instead.  Either change is fine with me.
> 
> On 2009/07/09 23:56:08, Elliot Glaysher wrote:
> > LGTM. It's too bad a CustomButton will have half of its state changed by one
> > method and the other half by another.

using the notification will let you get rid of a bunch of calls from
CustomButton clients right? that gets my vote.

[tony, 2009-07-10 17:05:37]

Ok, I looked at this a bit more.

If I remove the notification stuff, then I need to add new calls that change the
theme for the go button and the star button.  In the limit, we need to plumb
through update calls for all themes that change where as with notifications, we
get updates for free.

On the other hand, I could switch the existing SetUseSystemTheme to use
notifications, but that would add a NotificationObserver to both
CustomDrawButtonBase and CustomDrawButton.  This is ok, but seems like overkill
in CustomDrawButton because most buttons can't be GTK+ themed (there's no icon
for it).  There's currently only the 4 toolbar buttons, although you can imagine
that eventually the menus would be some GTK+ icons as well.  There are as many
CustomDrawButtons that probably won't be GTK+ themed (tab close button, find bar
next/prev, new tab button, min/max/restore/close).  Since only some
CustomDrawButtons need to know about GTK+ themes, it seems wasteful to put a
NotificationObserver there.

In conclusion, I think we should just stick with the hybrid mode we have now. 
Most CustomDrawButtonBase classes will need to be updated with new pixbufs when
the theme changes.  A few CustomDrawButtons will need to be updated to GTK+
style when the theme changes.  The former uses notifications and the latter we
just call directly.

Thoughts?

[Elliot Glaysher, 2009-07-10 17:47:26]

Go right ahead and keep it the way it was originally proposed.

FYI: I'm looking at replacing the CustomButton stuff for redoing the
star/omnibox/gostop part, which I already have a prototype of (blocked
on Glen making icons).

-- Elliot

On Fri, Jul 10, 2009 at 10:05 AM, <tony@chromium.org> wrote:
> Ok, I looked at this a bit more.
>
> If I remove the notification stuff, then I need to add new calls that
> change the theme for the go button and the star button. =A0In the limit,
> we need to plumb through update calls for all themes that change where
> as with notifications, we get updates for free.
>
> On the other hand, I could switch the existing SetUseSystemTheme to use
> notifications, but that would add a NotificationObserver to both
> CustomDrawButtonBase and CustomDrawButton. =A0This is ok, but seems like
> overkill in CustomDrawButton because most buttons can't be GTK+ themed
> (there's no icon for it). =A0There's currently only the 4 toolbar buttons=
,
> although you can imagine that eventually the menus would be some GTK+
> icons as well. =A0There are as many CustomDrawButtons that probably won't
> be GTK+ themed (tab close button, find bar next/prev, new tab button,
> min/max/restore/close). =A0Since only some CustomDrawButtons need to know
> about GTK+ themes, it seems wasteful to put a NotificationObserver
> there.
>
> In conclusion, I think we should just stick with the hybrid mode we have
> now. =A0Most CustomDrawButtonBase classes will need to be updated with ne=
w
> pixbufs when the theme changes. =A0A few CustomDrawButtons will need to b=
e
> updated to GTK+ style when the theme changes. =A0The former uses
> notifications and the latter we just call directly.
>
> Thoughts?
>
> http://codereview.chromium.org/149432
>

[Evan Stade, 2009-07-10 18:19:40]

The notification observer approach seems the cleanest from a code
standpoint. Sure it's wasteful in some sense but we won't get a
noticeable performance hit, and I don't know how much we are concerned
with theme changing performance anyway. I think this is a case where
readability/fewer lines of code trumps performance microseconds.


On Fri, Jul 10, 2009 at 10:05 AM, <tony@chromium.org> wrote:
> Ok, I looked at this a bit more.
>
> If I remove the notification stuff, then I need to add new calls that
> change the theme for the go button and the star button. =A0In the limit,
> we need to plumb through update calls for all themes that change where
> as with notifications, we get updates for free.
>
> On the other hand, I could switch the existing SetUseSystemTheme to use
> notifications, but that would add a NotificationObserver to both
> CustomDrawButtonBase and CustomDrawButton. =A0This is ok, but seems like
> overkill in CustomDrawButton because most buttons can't be GTK+ themed
> (there's no icon for it). =A0There's currently only the 4 toolbar buttons=
,
> although you can imagine that eventually the menus would be some GTK+
> icons as well. =A0There are as many CustomDrawButtons that probably won't
> be GTK+ themed (tab close button, find bar next/prev, new tab button,
> min/max/restore/close). =A0Since only some CustomDrawButtons need to know
> about GTK+ themes, it seems wasteful to put a NotificationObserver
> there.
>
> In conclusion, I think we should just stick with the hybrid mode we have
> now. =A0Most CustomDrawButtonBase classes will need to be updated with ne=
w
> pixbufs when the theme changes. =A0A few CustomDrawButtons will need to b=
e
> updated to GTK+ style when the theme changes. =A0The former uses
> notifications and the latter we just call directly.
>
> Thoughts?
>
> http://codereview.chromium.org/149432
>