Implement basic theming for panel titlebars.

Implement basic theming for panel titlebars.

This patch adds the plumbing to use the theme provider for panel windows:
- Updated the xib so the created window is a ThemedBrowserWindow, which knows how to delegate theming related calls to the window delegate.
- Implemented -[ThemedWindow themeProvider] for PanelWindowController.

BUG=none
TEST=no more yellow panel titlebars.

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

[dcheng, 2011-08-24 22:00:09]

Would it make sense to split ChromeBrowserWindow up into ThemedBrowserWIndow
(with just the ThemedWindow support) and ChromeBrowserWindow (with all the other
stuff)?

[Avi (use Gerrit), 2011-08-24 22:02:39]

On 2011/08/24 22:00:09, dcheng wrote:
> Would it make sense to split ChromeBrowserWindow up into ThemedBrowserWIndow
> (with just the ThemedWindow support) and ChromeBrowserWindow (with all the
other
> stuff)?

I wouldn't mind.

[dcheng, 2011-08-24 22:22:21]

On 2011/08/24 22:02:39, Avi wrote:
> On 2011/08/24 22:00:09, dcheng wrote:
> > Would it make sense to split ChromeBrowserWindow up into ThemedBrowserWIndow
> > (with just the ThemedWindow support) and ChromeBrowserWindow (with all the
> other
> > stuff)?
> 
> I wouldn't mind.

Done.

[Dmitry Titov, 2011-08-25 00:08:21]

Initial general comments:

Please describe the .xib file changes in the description of the patch (sometimes
changes to xib files are hard to read because they are IB-generated).

Also, if ThemedBrowserWindow delegates everything to its delegate anyways, do we
need to inherit from it or just implement the needed methods on the
PanelWindowControllerCocoa and just call them directly from the
PanelTitlebarViewCocoa?

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:54: [self
drawBackgroundWithOpaque:YES];
It seems there should be more to it. See, for example, drawRect method in
chrome/browser/ui/cocoa/tabs/tab_view.mm

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:34: -
(ui::ThemeProvider*)themeProvider;
ThemedBrowserWindow looks for 3 selectors on the controller, why implement only
one? It seems at least themePatternPhase is already used in this patch. I think
themedWindowStyle will also be needed.

[dcheng, 2011-08-25 18:48:54]

On 2011/08/25 00:08:21, Dmitry Titov wrote:
> Initial general comments:
> 
> Please describe the .xib file changes in the description of the patch
(sometimes
> changes to xib files are hard to read because they are IB-generated).
> 

Done.

> Also, if ThemedBrowserWindow delegates everything to its delegate anyways, do
we
> need to inherit from it or just implement the needed methods on the
> PanelWindowControllerCocoa and just call them directly from the
> PanelTitlebarViewCocoa?
> 

We don't /have/ to but I suspect if we want to share code with any other
windows/views that use themes (e.g. BackgroundGradientView), we'll want to take
this approach.

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:54: [self
drawBackgroundWithOpaque:YES];
On 2011/08/25 00:08:21, Dmitry Titov wrote:
> It seems there should be more to it. See, for example, drawRect method in
> chrome/browser/ui/cocoa/tabs/tab_view.mm

I agree, but I would be more comfortable splitting it up into small patches.
This patch adds in the basic plumbing we need retrieve theme colors/bitmaps and
subclasses from BackgroundGradientView to start using that functionality.

Known things that are still a work in progress:
- Active window tracking
- Updating the theme when the global theme is changed
- Titlebars (I started working on this yesterday and it appears we have no
window/titlebar view when UpdatePanelTitleBar is first called...)
- Wrench icon

The hover glow effect and other things in tab_view.mm would definitely be nice
to add, but I think that should be in followup patches since we'd probably want
to refactor it to be able to share the code between panels and tab views. I also
see it as lower priority, since all the other stuff is more required =)

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:34: -
(ui::ThemeProvider*)themeProvider;
On 2011/08/25 00:08:21, Dmitry Titov wrote:
> ThemedBrowserWindow looks for 3 selectors on the controller, why implement
only
> one? It seems at least themePatternPhase is already used in this patch. I
think
> themedWindowStyle will also be needed.

We probably need all 3 eventually, but as I mentioned, I'm starting with just
the basics.

[Dmitry Titov, 2011-08-25 19:24:51]

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:54: [self
drawBackgroundWithOpaque:YES];
On 2011/08/25 18:48:54, dcheng wrote:
> I agree, but I would be more comfortable splitting it up into small patches.

Ok, I'm fine with smaller patches.

> Known things that are still a work in progress:
> - Active window tracking
> - Updating the theme when the global theme is changed
> - Titlebars (I started working on this yesterday and it appears we have no
> window/titlebar view when UpdatePanelTitleBar is first called...)
> - Wrench icon

Good list.

> 
> The hover glow effect and other things in tab_view.mm would definitely be nice
> to add, but I think that should be in followup patches since we'd probably
want
> to refactor it to be able to share the code between panels and tab views. I
also
> see it as lower priority, since all the other stuff is more required =)

I suspect you'll have to use some code from tab_view in order to render active
color... Anyways, agree to leave it for future patches.

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:34: -
(ui::ThemeProvider*)themeProvider;
On 2011/08/25 18:48:54, dcheng wrote:
> On 2011/08/25 00:08:21, Dmitry Titov wrote:
> > ThemedBrowserWindow looks for 3 selectors on the controller, why implement
> only
> > one? It seems at least themePatternPhase is already used in this patch. I
> think
> > themedWindowStyle will also be needed.
> 
> We probably need all 3 eventually, but as I mentioned, I'm starting with just
> the basics.

Since you are adding plumbing in this patch, I think we should add all 3 (and a
unittest to test that they return some non-null values etc).
For example, you already use phase info in the drawing code but plumbing for it
is not complete. I agree to get more advanced painting in later patches but lets
get plumbing for themeProvider and its friends here and a test for it added to
panel_browser_window_cocoa_unittest.cc

[dcheng, 2011-08-26 23:04:42]

I also fixed the find bar so it can draw on top of our custom title bar in this
patch.

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/7734003/diff/7001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:34: -
(ui::ThemeProvider*)themeProvider;
On 2011/08/25 19:24:51, Dmitry Titov wrote:
> On 2011/08/25 18:48:54, dcheng wrote:
> > On 2011/08/25 00:08:21, Dmitry Titov wrote:
> > > ThemedBrowserWindow looks for 3 selectors on the controller, why implement
> > only
> > > one? It seems at least themePatternPhase is already used in this patch. I
> > think
> > > themedWindowStyle will also be needed.
> > 
> > We probably need all 3 eventually, but as I mentioned, I'm starting with
just
> > the basics.
> 
> Since you are adding plumbing in this patch, I think we should add all 3 (and
a
> unittest to test that they return some non-null values etc).
> For example, you already use phase info in the drawing code but plumbing for
it
> is not complete. I agree to get more advanced painting in later patches but
lets
> get plumbing for themeProvider and its friends here and a test for it added to
> panel_browser_window_cocoa_unittest.cc

I've implemented the other two methods. There's some duplication in
themePatternPhase which I'm not sure how to resolve, and themedWindowStyle is
completely unused at the moment (BackgroundGradientView doesn't use it). What's
our story for incognito panels? Is it possible to have one?

[Dmitry Titov, 2011-08-27 20:30:58]

Still needs a unittest that verifies pulling those theming objects from the
panel window works as expected.

http://codereview.chromium.org/7734003/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:50: // TODO(dcheng):
These constants and the following logic are duplicated from
Lets just share this code in this patch the new util file that Jenn has created,
before it got any further out of sync.

[Dmitry Titov, 2011-08-27 20:32:52]

On 2011/08/26 23:04:42, dcheng wrote:
> I've implemented the other two methods. There's some duplication in
> themePatternPhase which I'm not sure how to resolve, and themedWindowStyle is
> completely unused at the moment (BackgroundGradientView doesn't use it).
What's
> our story for incognito panels? Is it possible to have one?

There will be incognito panels and panels from different profiles. We should
show them differently.

[dcheng, 2011-08-31 00:05:04]

Refactored two pieces of functionality into BrowserWindowUtils and added a
simple test to verify that the theme provider plumbing works.

http://codereview.chromium.org/7734003/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:50: // TODO(dcheng):
These constants and the following logic are duplicated from
On 2011/08/27 20:30:58, Dmitry Titov wrote:
> Lets just share this code in this patch the new util file that Jenn has
created,
> before it got any further out of sync.

Done.

[Dmitry Titov, 2011-08-31 01:01:30]

Almost there.

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:206: [BrowserWindowUtils
delayedReplace:pending_window_title_.get()
Naming suggestion: Usually 'delayedFoo' is associated with literal time-based
delay... How about:
[BrowserWindowUtils scheduleTitleChangeForWindow:window newTitle:newTitle
previouslyScheduledTitle:previousTitle]?

It can also return a new copy of the title, if that makes code more readable :-)

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa_unittest.mm (right):

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa_unittest.mm:322: }
We also need a test now that verifies that titlebar is actually updated when
Panel->UpdateTitleBar is called.  you can pull it from native window.

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:87: -
(void)setTitle:(NSString*)newTitle {
could you add TODO here?

[dcheng, 2011-08-31 23:58:02]

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:206: [BrowserWindowUtils
delayedReplace:pending_window_title_.get()
On 2011/08/31 01:01:30, Dmitry Titov wrote:
> Naming suggestion: Usually 'delayedFoo' is associated with literal time-based
> delay... How about:
> [BrowserWindowUtils scheduleTitleChangeForWindow:window newTitle:newTitle
> previouslyScheduledTitle:previousTitle]?
> 
> It can also return a new copy of the title, if that makes code more readable
:-)

Done. Now with a longer more verbose name.

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa_unittest.mm (right):

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa_unittest.mm:322: }
On 2011/08/31 01:01:30, Dmitry Titov wrote:
> We also need a test now that verifies that titlebar is actually updated when
> Panel->UpdateTitleBar is called.  you can pull it from native window.

Done.

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right):

http://codereview.chromium.org/7734003/diff/16001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:87: -
(void)setTitle:(NSString*)newTitle {
On 2011/08/31 01:01:30, Dmitry Titov wrote:
> could you add TODO here?

Done.

[Dmitry Titov, 2011-09-01 01:44:12]

LGTM with one nit:

http://codereview.chromium.org/7734003/diff/27003/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa_unittest.mm (right):

http://codereview.chromium.org/7734003/diff/27003/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa_unittest.mm:335:
EXPECT_NSEQ(@"Untitled", [[native_window->controller_ window] title]);
It would be nice to check for previous value (which is probbaly different from
"Untitled") so that it's obvious that calling updateTitleBar actually changed
the title.

[Nico, 2013-03-04 14:07:02]

https://codereview.chromium.org/7734003/diff/29001/chrome/browser/ui/cocoa/br...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

https://codereview.chromium.org/7734003/diff/29001/chrome/browser/ui/cocoa/br...
chrome/browser/ui/cocoa/browser_window_utils.mm:140: return [newTitle copy];
Doesn't this leak? Who frees this?