Change the visual appearance of panel on GTK per new UI design.

Change the visual appearance of panel on GTK per new UI design.

In order not to add more panel specific stuff to BrowserTitlebar, I decided to
put all titlebar painting and handling logic in PanelBrowserTitlebarGtk. This
is also needed for the ongoing effort to strip out the BrowserWindow dependency
from panel implementation.

I also moved throbber logic out of BrowserTitlebar into a separate class so that
the code can be shared between BrowserTitlebar and PanelBrowserTitlebarGtk.

BUG=none
TEST=existing tests


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

[jianli, 2012-06-04 19:54:09]

Evan, could you please review all changes related to browser/ui/GTK/*?

Dmitry and Jenn, could you please review all panel related changes?

[jennb, 2012-06-04 22:17:52]

Some of these changes won't be necessary once we stop inheriting from
browser_window_gtk, such as making CreateBrowserTitlebar return TitlebarBase.
Will you be reverting such changes during that part of the refactor?

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar.h:96: void
ShowFaviconMenu(GdkEventButton* event);
Can move this code back inline inside OnFavIconMenuButtonPressed() and delete
from panel_browser_titlebar_gtk.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar_base.h (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_base.h:19: class BrowserTitlebarBase {
Is there anything browser-specific about this titlebar? If not, suggest dropping
Browser from the name so that there's no confusion when we create
browserless-panels that use this class.

Ditto for BrowserTitlebarThrobber.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_base.h:23: // Build the titlebar, the
space above the tab strip, and (maybe) the min,
tab strip is browser-specific. Please put those specifics into the appropriate
subclass. Several other comments in this file also refer to behavior that is
specific to how a subclass might implement the method. Please move those as
well.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/nine_bo...
File chrome/browser/ui/gtk/nine_box.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/nine_bo...
chrome/browser/ui/gtk/nine_box.cc:138: // Note that we still want to do the
painting if there's room in only one
Are you sure this won't mess up non-panel uses of this method?

[Dmitry Titov, 2012-06-04 22:19:57]

It would be better to split the moving constants to panel_constant.h from the
rest of the patch.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:184: *pressed_image_id =
0;
We do have pressed images ("_C")

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:325: window_has_focus_ =
is window_has_focus_ used somewhere?

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:234: custom_frame_border =
new NineBox(IDR_WINDOW_TOP_LEFT_CORNER,
Can Cairo just render a semi-rounded rect? It's a stretch to use NineGrid to
only render 4px thing...

[Evan Stade, 2012-06-04 22:58:19]

gtk wise looks ok to me

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar_throbber.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_throbber.cc:5: #include
"chrome/browser/ui/gtk/browser_titlebar_throbber.h"
aside: Throbber is a confusing name because sometimes it refers to a spinning
thing, but sometimes it refers to the pulsating background color of the tab
(e.g. ThrobAnimation).

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_throbber.cc:72: 
^H

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar_throbber.h (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_throbber.h:20: // so the caller doesn't
need to unref.  |is_waiting| is true if we're
nit: one space after .

[jianli, 2012-06-05 18:48:30]

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar.h:96: void
ShowFaviconMenu(GdkEventButton* event);
On 2012/06/04 22:17:52, jennb wrote:
> Can move this code back inline inside OnFavIconMenuButtonPressed() and delete
> from panel_browser_titlebar_gtk.

Done.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar_base.h (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_base.h:19: class BrowserTitlebarBase {
On 2012/06/04 22:17:52, jennb wrote:
> Is there anything browser-specific about this titlebar? If not, suggest
dropping
> Browser from the name so that there's no confusion when we create
> browserless-panels that use this class.
> 
> Ditto for BrowserTitlebarThrobber.

This base file is a temporary solution so that BrowserWindowGtk can call those
methods provided in BrowserTitlebar/PanelBrowserTitlebarGtk. I will remove this
base file when we implement panel window for gtk that does not depend on
browser. So I rather keep all the names unchanged.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_base.h:23: // Build the titlebar, the
space above the tab strip, and (maybe) the min,
On 2012/06/04 22:17:52, jennb wrote:
> tab strip is browser-specific. Please put those specifics into the appropriate
> subclass. Several other comments in this file also refer to behavior that is
> specific to how a subclass might implement the method. Please move those as
> well.

Ditto. I rather keep as it is now so that we can move the original comments back
to their original file.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar_throbber.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_throbber.cc:5: #include
"chrome/browser/ui/gtk/browser_titlebar_throbber.h"
On 2012/06/04 22:58:19, Evan Stade wrote:
> aside: Throbber is a confusing name because sometimes it refers to a spinning
> thing, but sometimes it refers to the pulsating background color of the tab
> (e.g. ThrobAnimation).

Renamed to TitlebarThrobAnimation.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_throbber.cc:72: 
On 2012/06/04 22:58:19, Evan Stade wrote:
> ^H

Done.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
File chrome/browser/ui/gtk/browser_titlebar_throbber.h (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
chrome/browser/ui/gtk/browser_titlebar_throbber.h:20: // so the caller doesn't
need to unref.  |is_waiting| is true if we're
On 2012/06/04 22:58:19, Evan Stade wrote:
> nit: one space after .

Done.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/nine_bo...
File chrome/browser/ui/gtk/nine_box.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/nine_bo...
chrome/browser/ui/gtk/nine_box.cc:138: // Note that we still want to do the
painting if there's room in only one
On 2012/06/04 22:17:52, jennb wrote:
> Are you sure this won't mess up non-panel uses of this method?

This change is not needed any more since we switched to drawing border via cairo
as suggested.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:184: *pressed_image_id =
0;
On 2012/06/04 22:19:57, Dmitry Titov wrote:
> We do have pressed images ("_C")

Done.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:325: window_has_focus_ =
On 2012/06/04 22:19:57, Dmitry Titov wrote:
> is window_has_focus_ used somewhere?

Removed.

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/panels/pane...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:234: custom_frame_border =
new NineBox(IDR_WINDOW_TOP_LEFT_CORNER,
On 2012/06/04 22:19:57, Dmitry Titov wrote:
> Can Cairo just render a semi-rounded rect? It's a stretch to use NineGrid to
> only render 4px thing...

Done.

[jianli, 2012-06-05 18:49:30]

On 2012/06/04 22:19:57, Dmitry Titov wrote:
> It would be better to split the moving constants to panel_constant.h from the
> rest of the patch.
> 
The enum is needed by PanelBrowserTitlebarGtk since I do not want to introduce a
new enum for it.

[jianli, 2012-06-05 18:50:35]

On 2012/06/04 22:17:52, jennb wrote:
> Some of these changes won't be necessary once we stop inheriting from
> browser_window_gtk, such as making CreateBrowserTitlebar return TitlebarBase.
> Will you be reverting such changes during that part of the refactor?
> 
Yes, I will revert these changes once we rewrite panel window to remove browser
dependency.

[Dmitry Titov, 2012-06-05 19:02:16]

On 2012/06/05 18:49:30, jianli wrote:
> On 2012/06/04 22:19:57, Dmitry Titov wrote:
> > It would be better to split the moving constants to panel_constant.h from
the
> > rest of the patch.
> > 
> The enum is needed by PanelBrowserTitlebarGtk since I do not want to introduce
a
> new enum for it.

I just meant that the enum move could be a first patch, the
PanelBrowserTetlebarGtk could be second...

[Dmitry Titov, 2012-06-05 20:09:37]

LGTM (please restore the comment in nine_box.cc so this file is not in the CL
anymore)

[jennb, 2012-06-05 20:40:35]

LGTM

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.h:279: // Draws the frame.
Comment just repeats method name now. Is the old info no longer valid?

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:114:
browser_window_->browser()->profile());
browser_window_->panel()->profile()

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:211:
UpdateMinimizeRestoreButtonVisibility();
This shouldn't be here anymore. We removed logic that updated buttons on draw a
while ago.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:238: SkBitmap icon =
browser_window_->browser()->GetCurrentPageIcon();
browser_window_->panel()->GetCurrentPageIcon()

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:270:
UpdateMinimizeRestoreButtonVisibility();
Don't think we need this here.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.h (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.h:69: bool BuildButton(const
std::string& button_token);
I don't see BuildButton in panel_browser_titlebar_gtk.cc

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:146: browser()->profile());
Please get the profile() from the panel instead? One less line to refactor.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:359: browser()->profile());
ditto

[jianli, 2012-06-05 21:02:16]

On 2012/06/04 22:58:19, Evan Stade wrote:
> gtk wise looks ok to me
> 
>
http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
> File chrome/browser/ui/gtk/browser_titlebar_throbber.cc (right):
> 
>
http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
> chrome/browser/ui/gtk/browser_titlebar_throbber.cc:5: #include
> "chrome/browser/ui/gtk/browser_titlebar_throbber.h"
> aside: Throbber is a confusing name because sometimes it refers to a spinning
> thing, but sometimes it refers to the pulsating background color of the tab
> (e.g. ThrobAnimation).
> 
>
http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
> chrome/browser/ui/gtk/browser_titlebar_throbber.cc:72: 
> ^H
> 
>
http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
> File chrome/browser/ui/gtk/browser_titlebar_throbber.h (right):
> 
>
http://codereview.chromium.org/10483010/diff/19/chrome/browser/ui/gtk/browser...
> chrome/browser/ui/gtk/browser_titlebar_throbber.h:20: // so the caller doesn't
> need to unref.  |is_waiting| is true if we're
> nit: one space after .

Evan, can you approve it explicitly? Codereview system seems not to recognize
"looks ok". Thanks.

[jianli, 2012-06-05 21:03:40]

Addressed all feedbacks and will land the patch.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.h (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.h:279: // Draws the frame.
On 2012/06/05 20:40:35, jennb wrote:
> Comment just repeats method name now. Is the old info no longer valid?

This is indeed OnCustomFrameExpose that not only calls DrawCustomFrameBorder,
but also draw a bunch of other stuff, like frame background, drop shadow. I
updated the comment to illustrate it in details.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:114:
browser_window_->browser()->profile());
On 2012/06/05 20:40:35, jennb wrote:
> browser_window_->panel()->profile()

Done.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:211:
UpdateMinimizeRestoreButtonVisibility();
On 2012/06/05 20:40:35, jennb wrote:
> This shouldn't be here anymore. We removed logic that updated buttons on draw
a
> while ago.

Done.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:238: SkBitmap icon =
browser_window_->browser()->GetCurrentPageIcon();
On 2012/06/05 20:40:35, jennb wrote:
> browser_window_->panel()->GetCurrentPageIcon()

Done.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.cc:270:
UpdateMinimizeRestoreButtonVisibility();
On 2012/06/05 20:40:35, jennb wrote:
> Don't think we need this here.

Done.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_titlebar_gtk.h (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_titlebar_gtk.h:69: bool BuildButton(const
std::string& button_token);
On 2012/06/05 20:40:35, jennb wrote:
> I don't see BuildButton in panel_browser_titlebar_gtk.cc

Removed.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:146: browser()->profile());
On 2012/06/05 20:40:35, jennb wrote:
> Please get the profile() from the panel instead? One less line to refactor.

Done.

http://codereview.chromium.org/10483010/diff/22001/chrome/browser/ui/panels/p...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:359: browser()->profile());
On 2012/06/05 20:40:35, jennb wrote:
> ditto

Done.

[Evan Stade, 2012-06-05 21:16:18]

lgtm

[commit-bot: I haz the power, 2012-06-05 21:23:18]

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

[commit-bot: I haz the power, 2012-06-05 22:27:32]

Change committed as 140636