Adding "send to mobile" and "add bookmark" to the action box menu and wiring to their respective ac…

Adding "send to mobile" and "add bookmark" to the action box menu and wiring to their respective action on Mac.

BUG=138118
125307


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

[beaudoin, 2012-08-13 18:32:35]

Follow-up to last week CL, hooking up action box menu with recent additions by
Yefim.

[Scott Hess - ex-Googler, 2012-08-13 18:54:43]

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:124: profile_));
Make this call consistent WRT use of parameter values versus member variables. 
I'm not sure if we have a style guideline, but I just noticed that it was
inconsistent.

Also ... would browser_->profile() work for PlusDecoration instead?

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/plus_decoration.h (right):

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/plus_decoration.h:14: class Profile;
Alphabetize.

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/action_box_menu_model.cc (right):

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_menu_model.cc:20: static const int
kFirstExtensionCommandId = 0xE000;
No need for static in anonymous namespace.

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_menu_model.cc:32: 
No need for this empty line.

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_menu_model.cc:43: if
(action_box_items.size() > 0)
Might as well put the for() loop in here at this point.  Also,
!action_box_items.empty() is a more straight-forward phrasing, though it's your
pref as to which you use.

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_menu_model.cc:44: InsertSeparatorAt(2);
Could this be coded as "Insert separator at the end"?  The earlier index
constants are presumably temporary while you're fleshing things out, but this
one will probably need adjusting.

http://codereview.chromium.org/10823289/diff/2001/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_menu_model.cc:53: }
Does SimpleMenuModel() support clearing the delegate?  If ~SimpleMenuModel()
makes any delegate calls, the results will go to the abstract base class (ie,
crash).

[beaudoin, 2012-08-17 20:04:21]

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:124: profile_));
On 2012/08/13 18:54:43, shess wrote:
> Make this call consistent WRT use of parameter values versus member variables.

> I'm not sure if we have a style guideline, but I just noticed that it was
> inconsistent.
> 
> Also ... would browser_->profile() work for PlusDecoration instead?

Good question, I did not know of browser_->profile(). Any idea why
LocationBarViewMac takes both Browser and Profile in this case?

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

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/plus_decoration.h:14: class Profile;
On 2012/08/13 18:54:43, shess wrote:
> Alphabetize.

Profile is gone.

Done.

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

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:20: static const int
kFirstExtensionCommandId = 0xE000;
On 2012/08/13 18:54:43, shess wrote:
> No need for static in anonymous namespace.

Done.

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:32: 
On 2012/08/13 18:54:43, shess wrote:
> No need for this empty line.

Done.

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:43: if
(action_box_items.size() > 0)
On 2012/08/13 18:54:43, shess wrote:
> Might as well put the for() loop in here at this point.  Also,
> !action_box_items.empty() is a more straight-forward phrasing, though it's
your
> pref as to which you use.

Done.

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:44: InsertSeparatorAt(2);
On 2012/08/13 18:54:43, shess wrote:
> Could this be coded as "Insert separator at the end"?  The earlier index
> constants are presumably temporary while you're fleshing things out, but this
> one will probably need adjusting.

Done.

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/t...
chrome/browser/ui/toolbar/action_box_menu_model.cc:53: }
On 2012/08/13 18:54:43, shess wrote:
> Does SimpleMenuModel() support clearing the delegate?  If ~SimpleMenuModel()
> makes any delegate calls, the results will go to the abstract base class (ie,
> crash).

Done.

[Scott Hess - ex-Googler, 2012-08-17 20:39:15]

lgtm

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/c...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10823289/diff/2001/chrome/browser/ui/c...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:124: profile_));
On 2012/08/17 20:04:21, beaudoin wrote:
> On 2012/08/13 18:54:43, shess wrote:
> > Make this call consistent WRT use of parameter values versus member
variables.
> 
> > I'm not sure if we have a style guideline, but I just noticed that it was
> > inconsistent.
> > 
> > Also ... would browser_->profile() work for PlusDecoration instead?
> 
> Good question, I did not know of browser_->profile(). Any idea why
> LocationBarViewMac takes both Browser and Profile in this case?

LocationBarView* dates from deepest darkest times.  IOW, I don't know.  It just
looks like lots of stuff would break if browser_->profile() doesn't work.

[commit-bot: I haz the power, 2012-08-17 21:11:57]

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

[commit-bot: I haz the power, 2012-08-17 21:12:01]

Presubmit check for 10823289-9001 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

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[beaudoin, 2012-08-17 21:14:22]

@sky Friendly ping for the non-cocoa stuff in there. Thanks!

On 2012/08/17 21:12:01, I haz the power (commit-bot) wrote:
> Presubmit check for 10823289-9001 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
> 
> Presubmit checks took 1.1s to calculate.
> 
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!

[beaudoin, 2012-08-20 14:24:47]

+ben for owner review on chrome/browser/ui stuff.

[Ben Goodger (Google), 2012-08-20 17:07:33]

lgtm

[commit-bot: I haz the power, 2012-08-20 18:19:17]

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

[commit-bot: I haz the power, 2012-08-20 22:44:11]

Change committed as 152416