Theme window control buttons....

Theme window control buttons.

BUG= http://crbug.com/14579
TEST= Install a theme with themed window control buttons.  Observe that WCB's are themed.


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

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

[Glen Murphy, 2009-06-29 19:31:44]

http://codereview.chromium.org/150041/diff/1/11
File chrome/browser/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/150041/diff/1/11#newcode123
Line 123: SkBitmap* background = tp->GetBitmapNamed(
I would forget about defining this variable and just include the
tp->GetBitmap... in the SetBackground call, as you have to redefine it each time
anyway.

http://codereview.chromium.org/150041/diff/1/9
File chrome/browser/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/150041/diff/1/9#newcode46
Line 46: TabStrip(TabStripModel* model, ThemeProvider* theme_provider);
TabStrip is a View, so it should be able to call GetThemeProvider() to get the
theme provider from the view hierarchy it's in rather than having it passed in.
If this isn't the case (and in a few cases it isn't), let me know and we can
poke at it.

[Ben Goodger (Google), 2009-06-29 19:35:27]

http://codereview.chromium.org/150041/diff/1/10
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/150041/diff/1/10#newcode1395
Line 1395: tabstrip_ = new TabStrip(browser_->tabstrip_model(),
GetThemeProvider());
Why? you can get the theme provider from the widget.

[Miranda Callahan, 2009-06-29 19:41:57]

http://codereview.chromium.org/150041/diff/1/10
File chrome/browser/views/frame/browser_view.cc (right):

http://codereview.chromium.org/150041/diff/1/10#newcode1395
Line 1395: tabstrip_ = new TabStrip(browser_->tabstrip_model(),
GetThemeProvider());
On 2009/06/29 19:35:27, Ben Goodger wrote:
> Why? you can get the theme provider from the widget.

See response to Glen's comment; the initialization process is concurrent with
the creation of the tabstrip_, so no ThemeProvider is available to the tabstrip_
yet at this point.

http://codereview.chromium.org/150041/diff/1/11
File chrome/browser/views/frame/opaque_browser_frame_view.cc (right):

http://codereview.chromium.org/150041/diff/1/11#newcode123
Line 123: SkBitmap* background = tp->GetBitmapNamed(
On 2009/06/29 19:31:44, Glen Murphy wrote:
> I would forget about defining this variable and just include the
> tp->GetBitmap... in the SetBackground call, as you have to redefine it each
time
> anyway.

Done.

http://codereview.chromium.org/150041/diff/1/9
File chrome/browser/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/150041/diff/1/9#newcode46
Line 46: TabStrip(TabStripModel* model, ThemeProvider* theme_provider);
On 2009/06/29 19:31:44, Glen Murphy wrote:
> TabStrip is a View, so it should be able to call GetThemeProvider() to get the
> theme provider from the view hierarchy it's in rather than having it passed
in.
> If this isn't the case (and in a few cases it isn't), let me know and we can
> poke at it.

The problem is that the TabStrip is initialized when it is created, and
therefore isn't made a child of the BrowserView until after initialization is
complete; this means that the ThemeProvider it gets on a call to
GetThemeProvider is initially null.

I passed in the ThemeProvider from the BrowserView rather than rearrange the
initialization process because I didn't want to get involved in an untangling
that might break other dependencies.

[Miranda Callahan, 2009-06-29 22:06:01]

Unhooked image loading from initialization, now calling it after TabStrip has
access to the ThemeProvider.

[Glen Murphy, 2009-07-01 01:39:43]

Whups - late catch - you don't need five different background images - the theme
author will only ever specify one background to be used for all of them. Replace
the IDR_THEME_*_BUTTON_BACKGROUNDs with a single
IDR_THEME_WINDOW_CONTROL_BACKGROUND

[Glen Murphy, 2009-07-01 17:40:32]

LGTM with naming nits discussed in person.

[Miranda Callahan, 2009-07-02 19:51:31]

Please take another look; previous code failed unit_tests because
test_browser_window cannot access ThemeProvider, as it does not have a RootView.
 I fixed with a test in TabStrip::LoadNewTabButtonImage for a NULL
ThemeProvider, and also by hardcoding the size of the NewTabButton as a pair of
constants.  This also allows us to remove the size calculation from the image
loading, which makes sense.

[Miranda Callahan, 2009-07-06 16:20:27]

ping!  (should be a short review -- not much of a change from the original
LGTM.)

On 2009/07/02 19:51:31, Miranda Callahan wrote:
> Please take another look; previous code failed unit_tests because
> test_browser_window cannot access ThemeProvider, as it does not have a
RootView.
>  I fixed with a test in TabStrip::LoadNewTabButtonImage for a NULL
> ThemeProvider, and also by hardcoding the size of the NewTabButton as a pair
of
> constants.  This also allows us to remove the size calculation from the image
> loading, which makes sense.

[Glen Murphy, 2009-07-06 17:53:15]

LG