Display action box menu on Linux/gtk.

Display action box menu on Linux/gtk.

Known issues:
- The send-to-mobile bubble should not have an arrow and should be anchored to the action box button.

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

[beaudoin, 2012-08-30 02:16:10]

This is part of the port of the action box UI to Linux/GTK.

In addition to wiring the menu, this patch dynamically hides the location bar
star icon when the page is not bookmarked. This seems to cause
gtk_widget_get_allocation to return the wrong rectangle when first called. I
have unsuccessfully tried various workarounds, but if someone with more GTK
experience than I knows the right way to do this, I'd be grateful!

[msw, 2012-08-30 03:39:22]

Huzzah! Thanks for doing this! I just have some minor nits.
Seems worth addressing the bookmark star/bubble known issue.
+cc erg in case he has any suggestions.

https://chromiumcodereview.appspot.com/10889052/diff/1004/chrome/browser/ui/g...
File chrome/browser/ui/gtk/action_box_button_gtk.h (right):

https://chromiumcodereview.appspot.com/10889052/diff/1004/chrome/browser/ui/g...
chrome/browser/ui/gtk/action_box_button_gtk.h:16: class MenuGtk;
nit: forward decl is redundant with include.

https://chromiumcodereview.appspot.com/10889052/diff/1004/chrome/browser/ui/g...
chrome/browser/ui/gtk/action_box_button_gtk.h:22: class ActionBoxButtonGtk :
MenuGtk::Delegate {
nit: explicitly specify public inheritance.

https://chromiumcodereview.appspot.com/10889052/diff/1004/chrome/browser/ui/g...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10889052/diff/1004/chrome/browser/ui/g...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1575: bool visible =
star_enabled;
nit: mutate |star_enabled| (after UpdateCommandEnabled) and nix |visible|.

https://chromiumcodereview.appspot.com/10889052/diff/1004/chrome/browser/ui/g...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1583:
gtk_widget_show_all(star_.get());
nit: any reason to re-order this after the two preceding statements?

[Evan Stade, 2012-08-30 20:42:56]

On 2012/08/30 02:16:10, beaudoin wrote:
> This is part of the port of the action box UI to Linux/GTK.
> 
> In addition to wiring the menu, this patch dynamically hides the location bar
> star icon when the page is not bookmarked. This seems to cause
> gtk_widget_get_allocation to return the wrong rectangle when first called. I
> have unsuccessfully tried various workarounds, but if someone with more GTK
> experience than I knows the right way to do this, I'd be grateful!

not sure i understand what is going on but it sounds like you need to delay
showing the bubble until after the next "size-allocate" signal to the bookmark
star.

[beaudoin, 2012-08-30 20:55:05]

Any idea on how to fix the initial popup problem are welcome. As a workaround we
could leave the star visible at all time, but it's not the behavior on
Mac/Windows.

http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/actio...
File chrome/browser/ui/gtk/action_box_button_gtk.h (right):

http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/actio...
chrome/browser/ui/gtk/action_box_button_gtk.h:16: class MenuGtk;
On 2012/08/30 03:39:22, msw wrote:
> nit: forward decl is redundant with include.

Done.

http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/actio...
chrome/browser/ui/gtk/action_box_button_gtk.h:22: class ActionBoxButtonGtk :
MenuGtk::Delegate {
On 2012/08/30 03:39:22, msw wrote:
> nit: explicitly specify public inheritance.

Done.

http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1575: bool visible =
star_enabled;
On 2012/08/30 03:39:22, msw wrote:
> nit: mutate |star_enabled| (after UpdateCommandEnabled) and nix |visible|.

Done.

http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1583:
gtk_widget_show_all(star_.get());
On 2012/08/30 03:39:22, msw wrote:
> nit: any reason to re-order this after the two preceding statements?

Probably left-over from my experimentations. :)

Done.

[beaudoin1, 2012-08-30 20:57:34]

Thanks Evan. I'll try to do this. I'm worried it will make the code more
complex though. Do we have any command queue that could help me do this?


On Thu, Aug 30, 2012 at 4:55 PM, <beaudoin@chromium.org> wrote:

> Any idea on how to fix the initial popup problem are welcome. As a
> workaround we
> could leave the star visible at all time, but it's not the behavior on
> Mac/Windows.
>
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/action_box_**button_gtk.h<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/action_box_button_gtk.h>
> File chrome/browser/ui/gtk/action_**box_button_gtk.h (right):
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/action_box_**button_gtk.h#newcode16<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/action_box_button_gtk.h#newcode16>
> chrome/browser/ui/gtk/action_**box_button_gtk.h:16: class MenuGtk;
>
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: forward decl is redundant with include.
>>
>
> Done.
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/action_box_**button_gtk.h#newcode22<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/action_box_button_gtk.h#newcode22>
>
> chrome/browser/ui/gtk/action_**box_button_gtk.h:22: class
> ActionBoxButtonGtk : MenuGtk::Delegate {
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: explicitly specify public inheritance.
>>
>
> Done.
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/location_bar_**view_gtk.cc<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/location_bar_view_gtk.cc>
> File chrome/browser/ui/gtk/**location_bar_view_gtk.cc (right):
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/location_bar_**view_gtk.cc#newcode1575<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/location_bar_view_gtk.cc#newcode1575>
>
> chrome/browser/ui/gtk/**location_bar_view_gtk.cc:1575: bool visible =
> star_enabled;
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: mutate |star_enabled| (after UpdateCommandEnabled) and nix
>>
> |visible|.
>
> Done.
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/location_bar_**view_gtk.cc#newcode1583<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/location_bar_view_gtk.cc#newcode1583>
> chrome/browser/ui/gtk/**location_bar_view_gtk.cc:1583:
> gtk_widget_show_all(star_.get(**));
>
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: any reason to re-order this after the two preceding statements?
>>
>
> Probably left-over from my experimentations. :)
>
> Done.
>
>
http://codereview.chromium.**org/10889052/<http://codereview.chromium.org/108...
>

[beaudoin1, 2012-08-30 20:57:34]

Thanks Evan. I'll try to do this. I'm worried it will make the code more
complex though. Do we have any command queue that could help me do this?


On Thu, Aug 30, 2012 at 4:55 PM, <beaudoin@chromium.org> wrote:

> Any idea on how to fix the initial popup problem are welcome. As a
> workaround we
> could leave the star visible at all time, but it's not the behavior on
> Mac/Windows.
>
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/action_box_**button_gtk.h<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/action_box_button_gtk.h>
> File chrome/browser/ui/gtk/action_**box_button_gtk.h (right):
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/action_box_**button_gtk.h#newcode16<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/action_box_button_gtk.h#newcode16>
> chrome/browser/ui/gtk/action_**box_button_gtk.h:16: class MenuGtk;
>
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: forward decl is redundant with include.
>>
>
> Done.
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/action_box_**button_gtk.h#newcode22<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/action_box_button_gtk.h#newcode22>
>
> chrome/browser/ui/gtk/action_**box_button_gtk.h:22: class
> ActionBoxButtonGtk : MenuGtk::Delegate {
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: explicitly specify public inheritance.
>>
>
> Done.
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/location_bar_**view_gtk.cc<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/location_bar_view_gtk.cc>
> File chrome/browser/ui/gtk/**location_bar_view_gtk.cc (right):
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/location_bar_**view_gtk.cc#newcode1575<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/location_bar_view_gtk.cc#newcode1575>
>
> chrome/browser/ui/gtk/**location_bar_view_gtk.cc:1575: bool visible =
> star_enabled;
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: mutate |star_enabled| (after UpdateCommandEnabled) and nix
>>
> |visible|.
>
> Done.
>
> http://codereview.chromium.**org/10889052/diff/1004/chrome/**
>
browser/ui/gtk/location_bar_**view_gtk.cc#newcode1583<http://codereview.chromium.org/10889052/diff/1004/chrome/browser/ui/gtk/location_bar_view_gtk.cc#newcode1583>
> chrome/browser/ui/gtk/**location_bar_view_gtk.cc:1583:
> gtk_widget_show_all(star_.get(**));
>
> On 2012/08/30 03:39:22, msw wrote:
>
>> nit: any reason to re-order this after the two preceding statements?
>>
>
> Probably left-over from my experimentations. :)
>
> Done.
>
>
http://codereview.chromium.**org/10889052/<http://codereview.chromium.org/108...
>

[Evan Stade, 2012-08-30 21:17:15]

On 2012/08/30 20:57:34, beaudoin1 wrote:
> Thanks Evan. I'll try to do this. I'm worried it will make the code more
> complex though. Do we have any command queue that could help me do this?

It will increase the complexity but probably not substantially so. You can just
keep a flag for whether the bubble needs to be shown on the next size-allocate.

[beaudoin, 2012-08-30 21:42:06]

On 2012/08/30 21:17:15, Evan Stade wrote:
> On 2012/08/30 20:57:34, beaudoin1 wrote:
> > Thanks Evan. I'll try to do this. I'm worried it will make the code more
> > complex though. Do we have any command queue that could help me do this?
> 
> It will increase the complexity but probably not substantially so. You can
just
> keep a flag for whether the bubble needs to be shown on the next
size-allocate.

It's a bit more complex than I would have hoped as I need to keep a structure to
record the parameters of BookmarkBubbleGtk::Show.

The most recent patch fixes that issue, please review the changes.

[Evan Stade, 2012-08-30 22:32:24]

http://codereview.chromium.org/10889052/diff/14002/chrome/browser/ui/gtk/loca...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10889052/diff/14002/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:389: class
LocationBarViewGtk::ShowStarBubbleAction {
I'd just use a pair. At the very least it should be a struct, and defined in the
header file.

The other option would be to create a base::Closure on the spot, then choose to
fire it now or save it for later.

http://codereview.chromium.org/10889052/diff/14002/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1514: if (star_sized_)
curlies

http://codereview.chromium.org/10889052/diff/14002/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1577: !starred_)
curlies

[beaudoin, 2012-08-31 13:27:09]

Ah! My relative Chrome inexperience is showing up. The base::Closure solution
makes everything smell super fresh. :)

Thanks Evan!

http://codereview.chromium.org/10889052/diff/14002/chrome/browser/ui/gtk/loca...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10889052/diff/14002/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1514: if (star_sized_)
On 2012/08/30 22:32:25, Evan Stade wrote:
> curlies

Done.

http://codereview.chromium.org/10889052/diff/14002/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1577: !starred_)
On 2012/08/30 22:32:25, Evan Stade wrote:
> curlies

Done.

[Evan Stade, 2012-08-31 19:47:43]

lgtm.

https://chromiumcodereview.appspot.com/10889052/diff/14008/chrome/browser/ui/...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10889052/diff/14008/chrome/browser/ui/...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1474: on_star_sized_.Reset();
you could put this inside the condtional

[beaudoin, 2012-08-31 21:00:53]

http://codereview.chromium.org/10889052/diff/14008/chrome/browser/ui/gtk/loca...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10889052/diff/14008/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1474: on_star_sized_.Reset();
On 2012/08/31 19:47:44, Evan Stade wrote:
> you could put this inside the condtional

Done.

[commit-bot: I haz the power, 2012-08-31 21:00:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10889052/9011

[commit-bot: I haz the power, 2012-08-31 21:41:33]

Try job failure for 10889052-9011 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-03 16:30:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10889052/11007

[commit-bot: I haz the power, 2012-09-03 16:30:48]

Presubmit check for 10889052-11007 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

[beaudoin, 2012-09-03 16:33:08]

@sky or pkasting: 1 line bug fix in c/b/ui/toolbar/action_box_menu_model.cc
needs an LGTM.

[skare_, 2012-09-03 22:56:33]

On 2012/09/03 16:33:08, beaudoin wrote:
> @sky or pkasting: 1 line bug fix in c/b/ui/toolbar/action_box_menu_model.cc
> needs an LGTM.
Philippe, 10910055 is in the CQ if you want to submit a prior patchset out of
/ui/gtk

[commit-bot: I haz the power, 2012-09-04 15:26:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10889052/19002

[commit-bot: I haz the power, 2012-09-04 16:49:03]

Try job failure for 10889052-19002 (retry) on linux_rel for step
"interactive_ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-09-04 19:54:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/beaudoin@chromium.org/10889052/19002

[commit-bot: I haz the power, 2012-09-04 22:15:33]

Change committed as 154822