Titlebar modifications for panels.

Titlebar modifications for panels.

- Do not show minimize and maximize buttons.
- Add a wrench icon that only displays when panel window has focus or mouse is
  in the panel window.

TEST=Manual.  Bring up a panel and verify the above behavior.
BUG=73936
modified:   chrome/browser/ui/gtk/browser_titlebar.cc
modified:   chrome/browser/ui/gtk/browser_titlebar.h

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

[Evan Stade, 2011-06-21 02:58:08]

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:298: 
?

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:340: 
?

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:450: if (!is_type_panel()) {
move into if statement above

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:462: if (!is_type_panel()) {
move into if statement above

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:847: event->x > 0 && event->x <
browser_window_->GetBounds().width() &&
did you try using event->detail ?

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:966: if (is_type_panel() &&
panel_wrench_button_.get()) {
can panel_wrench_button_ actually be null here?

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:976: return
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnablePanels) &&
why is the first part necessary?

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.h:171: bool is_type_panel();
c-style capitalization indicates a simple getter (i.e., something simple enough
to be inlined). This should be C++ style.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.h:213: scoped_ptr<CustomDrawButton>
panel_wrench_button_;
please comment this variable

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.h:221: bool window_has_mouse_;
please comment this variable

[prasadt, 2011-06-21 18:21:06]

Thanks for the feedback!

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:298: 
On 2011/06/21 02:58:08, Evan Stade wrote:
> ?

Done.  Removed empty line, left over from some changes I made but undid.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:340: 
On 2011/06/21 02:58:08, Evan Stade wrote:
> ?

Done.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:450: if (!is_type_panel()) {
On 2011/06/21 02:58:08, Evan Stade wrote:
> move into if statement above

Done.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:462: if (!is_type_panel()) {
On 2011/06/21 02:58:08, Evan Stade wrote:
> move into if statement above

Done.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:847: event->x > 0 && event->x <
browser_window_->GetBounds().width() &&
On 2011/06/21 02:58:08, Evan Stade wrote:
> did you try using event->detail ?

No I didn't notice that field.  I tried it and seems to work well and more
elegant than looking at co-ordinates.  Thanks for the pointer.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:966: if (is_type_panel() &&
panel_wrench_button_.get()) {
On 2011/06/21 02:58:08, Evan Stade wrote:
> can panel_wrench_button_ actually be null here?

On closer look, no.  Removed the check.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.cc:976: return
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnablePanels) &&
On 2011/06/21 02:58:08, Evan Stade wrote:
> why is the first part necessary?

Panel type is enabled by default (scratch pad for example uses it) but panel
behavior is currently under a flag.  Hence the need for that check.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.h:171: bool is_type_panel();
On 2011/06/21 02:58:08, Evan Stade wrote:
> c-style capitalization indicates a simple getter (i.e., something simple
enough
> to be inlined). This should be C++ style.

Done.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.h:213: scoped_ptr<CustomDrawButton>
panel_wrench_button_;
On 2011/06/21 02:58:08, Evan Stade wrote:
> please comment this variable

Done.

http://codereview.chromium.org/7206036/diff/1/chrome/browser/ui/gtk/browser_t...
chrome/browser/ui/gtk/browser_titlebar.h:221: bool window_has_mouse_;
On 2011/06/21 02:58:08, Evan Stade wrote:
> please comment this variable

Done.

[Evan Stade, 2011-06-21 22:19:21]

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.cc:832:
gtk_widget_show(panel_wrench_button_->widget());
shouldn't this check for active?

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:213: // Wrench icon for panels. 
This'll only appear when panel has focus or
s/panel/a panel window/

(to make it clear to someone who doesn't know what a panel is that this doesn't
apply to all types of windows)

also, nit: only one space after period

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:224: // Whether mouse is in the window.
 We show the wrench icon when panel has
ditto

[prasadt, 2011-06-21 23:23:39]

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.cc:832:
gtk_widget_show(panel_wrench_button_->widget());
On 2011/06/21 22:19:21, Evan Stade wrote:
> shouldn't this check for active?

No.  We want to show when the mouse enters the window OR when window has focus.

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:213: // Wrench icon for panels. 
This'll only appear when panel has focus or
On 2011/06/21 22:19:21, Evan Stade wrote:
> s/panel/a panel window/
> 
> (to make it clear to someone who doesn't know what a panel is that this
doesn't
> apply to all types of windows)
> 
> also, nit: only one space after period

Done.  My double space habit comes from when I first learnt typing...on a
typewriter :)  Some light reading -
http://desktoppub.about.com/cs/typespacing/a/onetwospaces.htm

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.h:224: // Whether mouse is in the window.
 We show the wrench icon when panel has
On 2011/06/21 22:19:21, Evan Stade wrote:
> ditto

Done.

[Evan Stade, 2011-06-23 17:05:20]

lgtm

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/7206036/diff/1003/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.cc:832:
gtk_widget_show(panel_wrench_button_->widget());
On 2011/06/21 23:23:39, prasadt wrote:
> On 2011/06/21 22:19:21, Evan Stade wrote:
> > shouldn't this check for active?
> 
> No.  We want to show when the mouse enters the window OR when window has
focus.

oh I see.

[jennb, 2011-06-23 23:09:43]

http://codereview.chromium.org/7206036/diff/3005/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/7206036/diff/3005/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.cc:971: return
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnablePanels) &&
browser.cc converts type to POPUP if --enable-panels is not defined so you
shouldn't need this check here.

[prasadt, 2011-06-24 00:01:11]

http://codereview.chromium.org/7206036/diff/3005/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/7206036/diff/3005/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_titlebar.cc:971: return
CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnablePanels) &&
On 2011/06/23 23:09:43, jennb wrote:
> browser.cc converts type to POPUP if --enable-panels is not defined so you
> shouldn't need this check here.

Chatted offline.
Looks like we convert TYPE_PANEL to TYPE_POPUP in the absence of the flag,
except for ChromeOS.  And this file is not compiled on ChromeOS.  Its not that
obvious from reading the code, so did it this way to be safe.  I can remove the
check on a later CL.