Remove print and move bookmark to the top of the action box.

Remove print and move bookmark to the top of the action box.

This change removes the print menu entry from the action box,
and moves the bookmark entry so it becomes the first entry in
the list.

Also moved responsibility for populating the menu model into the
controller class, instead of having it split between the model
and the controller.

BUG=226803
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194758

[Ben Goodger (Google), 2013-04-10 16:35:49]

Can you get someone else from the frontend team to pre-review these kind of
changes. This excludes sky@. Both of us have pretty heavy loads. I'd like to
focus on overall design/integration.

[Rune Fevang, 2013-04-10 18:59:14]

Sure, added wittman.

[Mike Wittman, 2013-04-10 20:44:04]

lgtm

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.h:49:
scoped_ptr<ActionBoxMenuModel> CreateMenuModel();
make private and expose via a test api.  see e.g. ui::Event::TestApi

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_menu_model_unittest.cc (right):

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_menu_model_unittest.cc:38:
controller_.reset(new ActionBoxButtonController(browser(), this));
destroy the controller explicitly in a TearDown function, so it gets destroyed
in the same test run where it is created

[Rune Fevang, 2013-04-10 21:17:48]

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.h:49:
scoped_ptr<ActionBoxMenuModel> CreateMenuModel();
On 2013/04/10 20:44:04, Mike Wittman wrote:
> make private and expose via a test api.  see e.g. ui::Event::TestApi

I've ended up adding this function a couple of times (for now abandoned CLs), so
the main motivation for adding it was not to facilitate testing (I could have
called OnButtonClicked and implemented ShowMenu for that).

I actually considered getting rid of the Delegate class and OnButtonClicked and
having callers instead calling CreateMenuModel and showing the menu themselves.
What do you think? I didn't do it because I wasn't really sure why it was done
this way in the first place, and was worried it was wrong from an idealized MVC
perspective.

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_menu_model_unittest.cc (right):

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_menu_model_unittest.cc:38:
controller_.reset(new ActionBoxButtonController(browser(), this));
On 2013/04/10 20:44:04, Mike Wittman wrote:
> destroy the controller explicitly in a TearDown function, so it gets destroyed
> in the same test run where it is created

Done.

[Mike Wittman, 2013-04-10 22:16:01]

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

https://codereview.chromium.org/13983002/diff/6001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.h:49:
scoped_ptr<ActionBoxMenuModel> CreateMenuModel();
On 2013/04/10 21:17:48, Rune Fevang wrote:
> On 2013/04/10 20:44:04, Mike Wittman wrote:
> > make private and expose via a test api.  see e.g. ui::Event::TestApi
> 
> I've ended up adding this function a couple of times (for now abandoned CLs),
so
> the main motivation for adding it was not to facilitate testing (I could have
> called OnButtonClicked and implemented ShowMenu for that).
> 
> I actually considered getting rid of the Delegate class and OnButtonClicked
and
> having callers instead calling CreateMenuModel and showing the menu
themselves.
> What do you think? I didn't do it because I wasn't really sure why it was done
> this way in the first place, and was worried it was wrong from an idealized
MVC
> perspective.

The delegate doesn't appear to be providing a lot of value in the current code. 
But if the controller were to get more complicated, it seems like a useful
mechanism to separate the cross-platform and platform-dependent input handling.

[Rune Fevang, 2013-04-10 22:23:00]

Thanks Mike.
Ben - could you do the OWNERS review please?

[Ben Goodger (Google), 2013-04-16 16:12:29]

lgtm

[commit-bot: I haz the power, 2013-04-18 00:14:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rfevang@chromium.org/13983002/15001

[commit-bot: I haz the power, 2013-04-18 03:23:56]

Change committed as 194758