Add PanelBrowserTitlebarGtk for panels on GTK.

Add PanelBrowserTitlebarGtk for panels on GTK.

PanelBrowserTitlebarGtk, derived from BrowserTitlebar, can allow us to add
panel-specific logic easily, instead of mixing everything in BrowserTitlebar.

This is needed when we're going to add custom button support for panels. We need to paint close and minimize button different from the standard ones and we also need to add unminimize button support.

BUG=none
TEST=existing tests due to no new functionaility


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

[jennb, 2012-04-20 21:22:30]

Now would be a good time to move the existing IsTypePanel() logic in the base
titlebar class into the new subclass to demonstrate why we even need a subclass.

Any new functionality should wait for a separate patch though.

[jianli, 2012-04-20 21:49:41]

On 2012/04/20 21:22:30, jennb wrote:
> Now would be a good time to move the existing IsTypePanel() logic in the base
> titlebar class into the new subclass to demonstrate why we even need a
subclass.
> 
> Any new functionality should wait for a separate patch though.

Done.

[jennb, 2012-04-20 22:26:32]

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_titlebar.cc:466: } else if (button_token ==
"maximize") {
Should we add a "restore" token and separate the building of maximize_button_
and restore_button_ now as we already know that panel needs a restore button but
not a maximize button?

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:19: if (button_token ==
"minimize" || button_token == "maximize")
Better to check for the ones we do show in panel and build those. Otherwise, if
new buttons panel doesn't care about are added to tokenizer list in base class,
they will automatically show up for panels.

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:25: void
PanelBrowserTitlebarGtk::ShowFaviconMenu(GdkEventButton* event) {
Should subclass OnFavIconMenuButtonPressed instead?

[jianli, 2012-04-20 23:00:29]

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_titlebar.cc:466: } else if (button_token ==
"maximize") {
On 2012/04/20 22:26:32, jennb wrote:
> Should we add a "restore" token and separate the building of maximize_button_
> and restore_button_ now as we already know that panel needs a restore button
but
> not a maximize button?

We can't do that because the passing string to BuildButtons do not have restore.

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:19: if (button_token ==
"minimize" || button_token == "maximize")
On 2012/04/20 22:26:32, jennb wrote:
> Better to check for the ones we do show in panel and build those. Otherwise,
if
> new buttons panel doesn't care about are added to tokenizer list in base
class,
> they will automatically show up for panels.

Done.

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:25: void
PanelBrowserTitlebarGtk::ShowFaviconMenu(GdkEventButton* event) {
On 2012/04/20 22:26:32, jennb wrote:
> Should subclass OnFavIconMenuButtonPressed instead?

Can't do this because OnFavIconMenuButtonPressed is defined by the macro and I
can't add virtual to it.

[jennb, 2012-04-21 01:04:49]

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_titlebar.cc:466: } else if (button_token ==
"maximize") {
On 2012/04/20 23:00:29, jianli wrote:
> On 2012/04/20 22:26:32, jennb wrote:
> > Should we add a "restore" token and separate the building of
maximize_button_
> > and restore_button_ now as we already know that panel needs a restore button
> but
> > not a maximize button?
> 
> We can't do that because the passing string to BuildButtons do not have
restore.

You could modify the passed in string, kDefaultButtonString.

Also, do we care about the GConf case? BuildButtons is conditional on !gconf...

[jianli, 2012-04-23 21:32:15]

On 2012/04/21 01:04:49, jennb wrote:
>
http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
> File chrome/browser/ui/gtk/browser_titlebar.cc (right):
> 
>
http://codereview.chromium.org/10169019/diff/8002/chrome/browser/ui/gtk/brows...
> chrome/browser/ui/gtk/browser_titlebar.cc:466: } else if (button_token ==
> "maximize") {
> On 2012/04/20 23:00:29, jianli wrote:
> > On 2012/04/20 22:26:32, jennb wrote:
> > > Should we add a "restore" token and separate the building of
> maximize_button_
> > > and restore_button_ now as we already know that panel needs a restore
button
> > but
> > > not a maximize button?
> > 
> > We can't do that because the passing string to BuildButtons do not have
> restore.
> 
> You could modify the passed in string, kDefaultButtonString.
> 
> Also, do we care about the GConf case? BuildButtons is conditional on
!gconf...

Yes, we do care about GConf case and need to ensure it works as before. For
GConf, we get the stored value from /apps/metacity/general/button_layout and
pass it to BrowserTitlebar::BuildButtons and the format will be like what we
define in kDefaultButtonString (see
http://people.gnome.org/~bmsmith/gconf-docs/C/metacity.html). So we do not want
to modify the passing string to make GConf case break.

[Evan Stade, 2012-04-23 22:50:41]

BrowserTitlebar split lgtm

[jennb, 2012-04-23 22:54:01]

lgtm

[commit-bot: I haz the power, 2012-04-23 22:56:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10169019/1009

[commit-bot: I haz the power, 2012-04-23 23:37:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10169019/23001

[commit-bot: I haz the power, 2012-04-24 01:17:04]

Change committed as 133586