Only show chrome2mobile in action box if it is enabled

Only show chrome2mobile in action box if it is enabled.

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

[Cait (Slow), 2012-08-29 18:46:57]

sky:  please take a look at the views/ changes.
shess:  another one-line change to location_bar
beaudoin and msw: cc'ing you on this in case it conflicts with ccr/10885024.

Thanks all,
Cait

[msw, 2012-08-29 19:27:04]

Should the menu actually add/remove items while open or should they simply be
enabled/disabled (via dimming, etc.) or perhaps even just no-op when clicked if
the state changes while open?

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:45:
browser_->profile()->IsSyncAccessible() &&
nit: I think we should drop the IsSyncAccessible condition.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:56:
command_updater_->RemoveCommandObserver(this);
Will this work if the ctor skips adding the observer?

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:104:
command_updater_->UpdateCommandEnabled(IDC_CHROME_TO_MOBILE_PAGE, enabled);
This seems unnecessary.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:111: bool c2m_enabled =
ChromeToMobileService::IsChromeToMobileEnabled() &&
nit: nix |c2m_enabled|, inline conditionals into the if().
Ideally this is the command enabled state, but this seems ok.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:115:
InsertItemWithStringIdAt(next_item_index, IDC_CHROME_TO_MOBILE_PAGE,
nit: perhaps use AddItem instead and nix next_item_index?

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:116:
IDS_CHROME_TO_MOBILE_BUBBLE_TOOLTIP);
nit: indent

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:38:
ActionBoxButtonView::ActionBoxButtonView(Browser* browser, Profile* profile,
nit: one arg per line.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:19: explicit
ActionBoxButtonView(Browser* browser, Profile* profile,
nit: one arg per line.

[Scott Hess - ex-Googler, 2012-08-29 20:03:39]

lgtm for one-line change to cocoa/!

[Cait (Slow), 2012-08-29 20:26:13]

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:45:
browser_->profile()->IsSyncAccessible() &&
On 2012/08/29 19:27:04, msw wrote:
> nit: I think we should drop the IsSyncAccessible condition.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:56:
command_updater_->RemoveCommandObserver(this);
On 2012/08/29 19:27:04, msw wrote:
> Will this work if the ctor skips adding the observer?

Yeah, I think it is just a no-op in that case.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:104:
command_updater_->UpdateCommandEnabled(IDC_CHROME_TO_MOBILE_PAGE, enabled);
On 2012/08/29 19:27:04, msw wrote:
> This seems unnecessary.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:111: bool c2m_enabled =
ChromeToMobileService::IsChromeToMobileEnabled() &&
On 2012/08/29 19:27:04, msw wrote:
> nit: nix |c2m_enabled|, inline conditionals into the if().
> Ideally this is the command enabled state, but this seems ok.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:115:
InsertItemWithStringIdAt(next_item_index, IDC_CHROME_TO_MOBILE_PAGE,
On 2012/08/29 19:27:04, msw wrote:
> nit: perhaps use AddItem instead and nix next_item_index?

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:116:
IDS_CHROME_TO_MOBILE_BUBBLE_TOOLTIP);
On 2012/08/29 19:27:04, msw wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:38:
ActionBoxButtonView::ActionBoxButtonView(Browser* browser, Profile* profile,
On 2012/08/29 19:27:04, msw wrote:
> nit: one arg per line.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1007/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:19: explicit
ActionBoxButtonView(Browser* browser, Profile* profile,
On 2012/08/29 19:27:04, msw wrote:
> nit: one arg per line.

Done.

[sky, 2012-08-29 21:27:00]

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:19: explicit
ActionBoxButtonView(Browser* browser,
no explicit.
Why do you need these three args? Once you have a browser you can get everything
else.

[msw, 2012-08-29 22:05:42]

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:44: if
(!browser_->profile()->IsOffTheRecord() &&
I think this should just always observe; sorry for the churn.

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:112:
SetIcon(GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE),
Ah, sorry my advice necessitated this roundabout way of getting the index to set
the icon here and below. It's too bad you can't just Add(command, name, icon).
This seems okay, but you can revert to using the explicit index if you prefer
that.

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:67: void UpdateMenu();
nit: Add a comment.

[Cait (Slow), 2012-08-29 23:03:47]

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:44: if
(!browser_->profile()->IsOffTheRecord() &&
On 2012/08/29 22:05:42, msw wrote:
> I think this should just always observe; sorry for the churn.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:67: void UpdateMenu();
On 2012/08/29 22:05:42, msw wrote:
> nit: Add a comment.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/8002/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:19: explicit
ActionBoxButtonView(Browser* browser,
On 2012/08/29 21:27:01, sky wrote:
> no explicit.
> Why do you need these three args? Once you have a browser you can get
everything
> else.

Done.

[sky, 2012-08-30 00:09:21]

Did you forget to upload a new patch?

[Cait (Slow), 2012-08-30 00:31:08]

On 2012/08/30 00:09:21, sky wrote:
> Did you forget to upload a new patch?

Should be up now -- sorry about that.

[msw, 2012-08-30 00:41:43]

Cleanup comments.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/plus_decoration.h:22: Browser* browser);
nit: fits on line above (or indent to match open paren).

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:7: #include
"chrome/browser/extensions/extension_system.h"
remove unnecessary include.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:26:
PlusDecoration::PlusDecoration(LocationBarViewMac* owner, Browser* browser)
nit: one arg per line when the initializer list doesn't also fit.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:11: #include
"chrome/browser/extensions/extension_service.h"
Remove these two extension includes with the out-of-line definition below.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:13: #include
"chrome/browser/ui/browser.h"
The Browser forward decl will suffice with the out-of-line definition below.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:14: #include
"chrome/browser/ui/browser_command_controller.h"
browser_command_controller.h and browser_commands.h seem unnecessary.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:19: class Profile;
Remove this forward decl with the out-of-line definition below.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:51: const
extensions::ExtensionList& action_box_menu_items() {
This shouldn't be defined inline anymore (also needs to be renamed with
CamelCase as [Get]ActionBoxMenuItems or similar).

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:13: class
CommandUpdater;
Remove this unnecessary forward decl.

[Cait (Slow), 2012-08-30 01:33:29]

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/plus_decoration.h:22: Browser* browser);
On 2012/08/30 00:41:43, msw wrote:
> nit: fits on line above (or indent to match open paren).

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:7: #include
"chrome/browser/extensions/extension_system.h"
On 2012/08/30 00:41:43, msw wrote:
> remove unnecessary include.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:26:
PlusDecoration::PlusDecoration(LocationBarViewMac* owner, Browser* browser)
On 2012/08/30 00:41:43, msw wrote:
> nit: one arg per line when the initializer list doesn't also fit.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
File chrome/browser/ui/toolbar/action_box_menu_model.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:11: #include
"chrome/browser/extensions/extension_service.h"
On 2012/08/30 00:41:43, msw wrote:
> Remove these two extension includes with the out-of-line definition below.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:13: #include
"chrome/browser/ui/browser.h"
On 2012/08/30 00:41:43, msw wrote:
> The Browser forward decl will suffice with the out-of-line definition below.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:14: #include
"chrome/browser/ui/browser_command_controller.h"
On 2012/08/30 00:41:43, msw wrote:
> browser_command_controller.h and browser_commands.h seem unnecessary.
browser_commands.h is needed, browser_command_controller.h isn't.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:19: class Profile;
On 2012/08/30 00:41:43, msw wrote:
> Remove this forward decl with the out-of-line definition below.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.h:51: const
extensions::ExtensionList& action_box_menu_items() {
On 2012/08/30 00:41:43, msw wrote:
> This shouldn't be defined inline anymore (also needs to be renamed with
> CamelCase as [Get]ActionBoxMenuItems or similar).

Done.

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/1008/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:13: class
CommandUpdater;
On 2012/08/30 00:41:43, msw wrote:
> Remove this unnecessary forward decl.

Done.

[msw, 2012-08-30 01:47:01]

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:7: #include
"chrome/browser/extensions/extension_system.h"
repeat nit: remove unnecessary include.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:26:
PlusDecoration::PlusDecoration(LocationBarViewMac* owner, Browser* browser)
repeat nit: one arg per line when the initializer list doesn't also fit (move
"Browser* browser" to the line below).

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.cc:95: const
extensions::ExtensionList& ActionBoxMenuModel::GetActionBoxMenuItems() {
This belongs below ExecuteCommand to match declaration order.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/action_box_menu_model.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.h:10: #include
"chrome/browser/extensions/extension_service.h"
nit: use chrome/common/extensions/extension.h instead (needed for const
extensions::ExtensionList&) and you can remove the Extension forward decl below.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.h:17: class Extension;
nit: remove this when including extension.h instead of extension_service.h

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:12: class Profile;
Remove unnecessary forward decl.

[Scott Hess - ex-Googler, 2012-08-30 03:09:43]

LGTM for removing lines from cocoa/

[Cait (Slow), 2012-08-30 21:46:03]

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.mm (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:7: #include
"chrome/browser/extensions/extension_system.h"
On 2012/08/30 01:47:01, msw wrote:
> repeat nit: remove unnecessary include.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/cocoa/location_bar/plus_decoration.mm:26:
PlusDecoration::PlusDecoration(LocationBarViewMac* owner, Browser* browser)
On 2012/08/30 01:47:01, msw wrote:
> repeat nit: one arg per line when the initializer list doesn't also fit (move
> "Browser* browser" to the line below).

Done.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.cc:95: const
extensions::ExtensionList& ActionBoxMenuModel::GetActionBoxMenuItems() {
On 2012/08/30 01:47:01, msw wrote:
> This belongs below ExecuteCommand to match declaration order.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/action_box_menu_model.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.h:10: #include
"chrome/browser/extensions/extension_service.h"
On 2012/08/30 01:47:01, msw wrote:
> nit: use chrome/common/extensions/extension.h instead (needed for const
> extensions::ExtensionList&) and you can remove the Extension forward decl
below.

Done.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.h:17: class Extension;
On 2012/08/30 01:47:01, msw wrote:
> nit: remove this when including extension.h instead of extension_service.h

Done.

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/10887029/diff/15001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:12: class Profile;
On 2012/08/30 01:47:01, msw wrote:
> Remove unnecessary forward decl.

Done.

[msw, 2012-08-30 22:21:09]

LGTM with one nit, thanks for the tremendous cleanup!

https://chromiumcodereview.appspot.com/10887029/diff/20002/chrome/browser/ui/...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/20002/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.cc:42: if
(ChromeToMobileService::IsChromeToMobileEnabled() &&
nit: You can nix IsChromeToMobileEnabled, it's 1:1 with action box now.

[sky, 2012-08-30 23:22:49]

LGTM

[Cait (Slow), 2012-08-31 15:38:12]

https://chromiumcodereview.appspot.com/10887029/diff/20002/chrome/browser/ui/...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/20002/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.cc:42: if
(ChromeToMobileService::IsChromeToMobileEnabled() &&
On 2012/08/30 22:21:09, msw wrote:
> nit: You can nix IsChromeToMobileEnabled, it's 1:1 with action box now.

Done.

[commit-bot: I haz the power, 2012-08-31 15:38:24]

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

[msw, 2012-08-31 17:15:36]

One nit. Still, LGTM, in case you want to keep this CQ run going and fix later.

https://chromiumcodereview.appspot.com/10887029/diff/26001/chrome/browser/ui/...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

https://chromiumcodereview.appspot.com/10887029/diff/26001/chrome/browser/ui/...
chrome/browser/ui/toolbar/action_box_menu_model.cc:43: HasMobiles()) {
nit: indent 4 more spaces.

[commit-bot: I haz the power, 2012-08-31 17:36:27]

Change committed as 154458