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

Review URL: https://chromiumcodereview.appspot.com/13983002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@194758 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller_unittest.mm

@@ -5,6 +5,7 @@
 #import "chrome/browser/ui/cocoa/location_bar/action_box_menu_bubble_controller.h"
 
 #include "base/command_line.h"
+#include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
@@ -136,7 +137,7 @@ class ActionBoxMenuBubbleControllerTest : public CocoaProfileTest {
 
 TEST_F(ActionBoxMenuBubbleControllerTest, CreateMenuWithExtensions) {
   scoped_ptr<ActionBoxMenuModel> model(new ActionBoxMenuModel(
-      browser(), &menu_delegate_));
+      profile(), &menu_delegate_));
   AddPageLauncherExtension(model.get(), "Launch extension 1", 0);
   AddPageLauncherExtension(model.get(), "Launch extension 2", 1);
   CreateController(model.Pass());
@@ -169,8 +170,9 @@ TEST_F(ActionBoxMenuBubbleControllerTest, CreateMenuWithExtensions) {
 
 TEST_F(ActionBoxMenuBubbleControllerTest, CheckSeparatorWithShortExtensions) {
   scoped_ptr<ActionBoxMenuModel> model(new ActionBoxMenuModel(
-      browser(), &menu_delegate_));
-  AddPageLauncherExtension(model.get(), "Short", 0);
+      profile(), &menu_delegate_));
+  model->AddItem(0, ASCIIToUTF16("Bookmark this page"));
+  AddPageLauncherExtension(model.get(), "Short", 1);
   CreateController(model.Pass());
 
   // The width of the menu is dictated by the widest item which in this case
@@ -181,9 +183,10 @@ TEST_F(ActionBoxMenuBubbleControllerTest, CheckSeparatorWithShortExtensions) {
 
 TEST_F(ActionBoxMenuBubbleControllerTest, CheckSeparatorWithLongExtensions) {
   scoped_ptr<ActionBoxMenuModel> model(new ActionBoxMenuModel(
-      browser(), &menu_delegate_));
+      profile(), &menu_delegate_));
+  model->AddItem(0, ASCIIToUTF16("Bookmark this page"));
   AddPageLauncherExtension(model.get(),
-      "This is a long page launcher extension title...", 0);
+      "This is a long page launcher extension title...", 1);
   CreateController(model.Pass());
 
   // The width of the menu is dictated by the widest item which in this case

chrome/browser/ui/toolbar/action_box_button_controller.cc

@@ -8,10 +8,12 @@
 #include "base/metrics/histogram.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/chrome_to_mobile_service.h"
 #include "chrome/browser/extensions/api/page_launcher/page_launcher_api.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/bookmarks/bookmark_tab_helper.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -25,6 +27,10 @@
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
+#include "grit/generated_resources.h"
+#include "grit/theme_resources.h"
+#include "ui/base/resource/resource_bundle.h"
+
 
 using content::UserMetricsAction;
 using content::WebContents;
@@ -48,26 +54,52 @@ ActionBoxButtonController::ActionBoxButtonController(Browser* browser,
 
 ActionBoxButtonController::~ActionBoxButtonController() {}
 
-void ActionBoxButtonController::OnButtonClicked() {
+scoped_ptr<ActionBoxMenuModel> ActionBoxButtonController::CreateMenuModel() {
   // Build a menu model and display the menu.
   scoped_ptr<ActionBoxMenuModel> menu_model(
-      new ActionBoxMenuModel(browser_, this));
+      new ActionBoxMenuModel(browser_->profile(), this));
+  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
+
+  // In some unit tests, GetActiveWebContents can return NULL.
+  bool starred = browser_->tab_strip_model()->GetActiveWebContents() &&
+      BookmarkTabHelper::FromWebContents(browser_->tab_strip_model()->
+          GetActiveWebContents())->is_starred();
+  menu_model->AddItemWithStringId(
+      IDC_BOOKMARK_PAGE_FROM_STAR,
+      starred ? IDS_TOOLTIP_STARRED : IDS_TOOLTIP_STAR);
+  menu_model->SetIcon(
+      menu_model->GetIndexOfCommandId(IDC_BOOKMARK_PAGE_FROM_STAR),
+      rb.GetNativeImageNamed(starred ? IDR_STAR_LIT : IDR_STAR));
+
+  // TODO(msw): Show the item as disabled for chrome: and file: scheme pages?
+  if (ChromeToMobileService::UpdateAndGetCommandState(browser_)) {
+    menu_model->AddItemWithStringId(IDC_CHROME_TO_MOBILE_PAGE,
+                                    IDS_CHROME_TO_MOBILE_BUBBLE_TOOLTIP);
+    menu_model->SetIcon(
+        menu_model->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE),
+        rb.GetNativeImageNamed(IDR_MOBILE));
+  }
 
-  const ExtensionSet* extensions =
+  ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(browser_->profile())->
-          extension_service()->extensions();
-  for (ExtensionSet::const_iterator it = extensions->begin();
-       it != extensions->end(); ++it) {
-    const extensions::Extension* extension = *it;
-    if (ActionInfo::GetPageLauncherInfo(extension)) {
-      int command_id = GetCommandIdForExtension(*extension);
-      menu_model->AddExtension(*extension, command_id);
+          extension_service();
+  if (extension_service) {
+    const ExtensionSet* extensions = extension_service->extensions();
+    for (ExtensionSet::const_iterator it = extensions->begin();
+         it != extensions->end(); ++it) {
+      const extensions::Extension* extension = *it;
+      if (ActionInfo::GetPageLauncherInfo(extension)) {
+        int command_id = GetCommandIdForExtension(*extension);
+        menu_model->AddExtension(*extension, command_id);
+      }
     }
   }
-  content::RecordAction(UserMetricsAction("ActionBox.ClickButton"));
+  return menu_model.Pass();
+}
 
-  // And show the menu.
-  delegate_->ShowMenu(menu_model.Pass());
+void ActionBoxButtonController::OnButtonClicked() {
+  content::RecordAction(UserMetricsAction("ActionBox.ClickButton"));
+  delegate_->ShowMenu(CreateMenuModel());
 }
 
 bool ActionBoxButtonController::IsCommandIdChecked(int command_id) const {

chrome/browser/ui/toolbar/action_box_button_controller.h

@@ -44,6 +44,10 @@ class ActionBoxButtonController : public ui::SimpleMenuModel::Delegate,
   ActionBoxButtonController(Browser* browser, Delegate* delegate);
   virtual ~ActionBoxButtonController();
 
+  // Creates and populates an ActionBoxMenuModel according to the current
+  // state of the browser.
+  scoped_ptr<ActionBoxMenuModel> CreateMenuModel();
+
   // Notifies this that the action box button has been clicked.
   // Methods on the Delegate may be called re-entrantly.
   void OnButtonClicked();

chrome/browser/ui/toolbar/action_box_menu_model.cc

@@ -6,53 +6,22 @@
 
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
-#include "chrome/app/chrome_command_ids.h"
-#include "chrome/browser/chrome_to_mobile_service.h"
-#include "chrome/browser/chrome_to_mobile_service_factory.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_toolbar_model.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/bookmarks/bookmark_tab_helper.h"
-#include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_commands.h"
-#include "chrome/browser/ui/browser_command_controller.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/extensions/api/extension_action/action_info.h"
-#include "chrome/common/url_constants.h"
-#include "grit/generated_resources.h"
-#include "grit/theme_resources.h"
-#include "ui/base/resource/resource_bundle.h"
+
 
 using extensions::ActionInfo;
 
 ////////////////////////////////////////////////////////////////////////////////
 // ActionBoxMenuModel
 
-ActionBoxMenuModel::ActionBoxMenuModel(Browser* browser,
+ActionBoxMenuModel::ActionBoxMenuModel(Profile* profile,
                                        ui::SimpleMenuModel::Delegate* delegate)
     : ui::SimpleMenuModel(delegate),
-      browser_(browser) {
-  ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-  // TODO(msw): Show the item as disabled for chrome: and file: scheme pages?
-  if (ChromeToMobileService::UpdateAndGetCommandState(browser_)) {
-    AddItemWithStringId(IDC_CHROME_TO_MOBILE_PAGE,
-                        IDS_CHROME_TO_MOBILE_BUBBLE_TOOLTIP);
-    SetIcon(GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE),
-            rb.GetNativeImageNamed(IDR_MOBILE));
-  }
-
-  // In some unit tests, GetActiveWebContents can return NULL.
-  bool starred = browser_->tab_strip_model()->GetActiveWebContents() &&
-      BookmarkTabHelper::FromWebContents(browser_->tab_strip_model()->
-          GetActiveWebContents())->is_starred();
-
-  AddItemWithStringId(IDC_BOOKMARK_PAGE_FROM_STAR,
-                      starred ? IDS_TOOLTIP_STARRED : IDS_TOOLTIP_STAR);
-  SetIcon(GetIndexOfCommandId(IDC_BOOKMARK_PAGE_FROM_STAR),
-          rb.GetNativeImageNamed(starred ? IDR_STAR_LIT : IDR_STAR));
+      profile_(profile) {
 
-  AddItemWithStringId(IDC_PRINT, IDS_PRINT);
 }
 
 ActionBoxMenuModel::~ActionBoxMenuModel() {
@@ -84,8 +53,7 @@ const extensions::Extension* ActionBoxMenuModel::GetExtensionAt(int index) {
   CHECK_LT(index_in_extension_ids, static_cast<int>(extension_ids_.size()));
 
   ExtensionService* extension_service =
-      extensions::ExtensionSystem::Get(browser_->profile())->
-          extension_service();
+      extensions::ExtensionSystem::Get(profile_)->extension_service();
   return extension_service->extensions()->GetByID(
       extension_ids_[index_in_extension_ids]);
 }

chrome/browser/ui/toolbar/action_box_menu_model.h

@@ -11,7 +11,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "ui/base/models/simple_menu_model.h"
 
-class Browser;
+class Profile;
 
 // A menu model that builds the contents of the action box menu. Effectively,
 // a ui::SimpleMenuModel with methods specifically for dealing with extension
@@ -21,7 +21,7 @@ class Browser;
 // the browser at creation time.
 class ActionBoxMenuModel : public ui::SimpleMenuModel {
  public:
-  ActionBoxMenuModel(Browser* browser, ui::SimpleMenuModel::Delegate* delegate);
+  ActionBoxMenuModel(Profile* profile, ui::SimpleMenuModel::Delegate* delegate);
   virtual ~ActionBoxMenuModel();
 
   // Adds an extension to the model with a given command ID.
@@ -43,7 +43,7 @@ class ActionBoxMenuModel : public ui::SimpleMenuModel {
   // total items in the model if there are no extensions installed.
   int GetFirstExtensionIndex();
 
-  Browser* browser_;
+  Profile* profile_;
 
   // The list of extensions added to the menu, in order, if any.
   extensions::ExtensionIdList extension_ids_;

chrome/browser/ui/toolbar/action_box_menu_model_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/toolbar/action_box_menu_model.h"
 
+#include "base/memory/scoped_ptr.h"
 #include "base/prefs/testing_pref_service.h"
 #include "base/values.h"
 #include "chrome/app/chrome_command_ids.h"
@@ -12,6 +13,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_command_controller.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/toolbar/action_box_button_controller.h"
 #include "chrome/common/extensions/feature_switch.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/browser_with_test_window_test.h"
@@ -27,25 +29,23 @@
 using extensions::FeatureSwitch;
 
 class ActionBoxMenuModelTest : public BrowserWithTestWindowTest,
-                               public ui::SimpleMenuModel::Delegate {
+                               public ActionBoxButtonController::Delegate {
  public:
   ActionBoxMenuModelTest() {}
 
-  // Testing overrides to ui::SimpleMenuModel::Delegate:
-  virtual bool IsCommandIdChecked(int command_id) const OVERRIDE {
-    return false;
+  virtual void SetUp() OVERRIDE {
+    BrowserWithTestWindowTest::SetUp();
+    controller_.reset(new ActionBoxButtonController(browser(), this));
   }
 
-  virtual bool IsCommandIdEnabled(int command_id) const OVERRIDE {
-    return false;
+  virtual void TearDown() OVERRIDE {
+    controller_.reset();
+    BrowserWithTestWindowTest::TearDown();
   }
 
-  virtual void ExecuteCommand(int command_id, int event_flags) OVERRIDE {}
-
-    // Don't handle accelerators.
-  virtual bool GetAcceleratorForCommandId(
-      int command_id,
-      ui::Accelerator* accelerator) OVERRIDE { return false; }
+  scoped_ptr<ActionBoxMenuModel> CreateModel() {
+    return controller_->CreateMenuModel();
+  }
 
   void InitProfile(){
     profile()->set_incognito(true);
@@ -83,6 +83,8 @@ class ActionBoxMenuModelTest : public BrowserWithTestWindowTest,
   }
 
  private:
+  scoped_ptr<ActionBoxButtonController> controller_;
+
   DISALLOW_COPY_AND_ASSIGN(ActionBoxMenuModelTest);
 };
 
@@ -91,20 +93,18 @@ TEST_F(ActionBoxMenuModelTest, IncongnitoNoMobiles) {
   InitProfile();
 
   NavigateToLocalPage();
-  // Create model.
-  ActionBoxMenuModel model(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model = CreateModel();
 
   // Expect no c2m command in model.
-  EXPECT_EQ(-1, model.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 
   NavigateToBookmarkablePage();
 
-  // Create model.
-  ActionBoxMenuModel model2(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model2 = CreateModel();
 
   // Expect c2m command not in model.
-  EXPECT_EQ(-1, model2.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model2->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 }
 
@@ -115,20 +115,17 @@ TEST_F(ActionBoxMenuModelTest, IncongnitoHasMobiles) {
 
   NavigateToLocalPage();
 
-  // Create model.
-  ActionBoxMenuModel model(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model = CreateModel();
 
   // Expect no c2m command in model.
-  EXPECT_EQ(-1, model.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 
   NavigateToBookmarkablePage();
 
-  // Create model.
-
-  ActionBoxMenuModel model2(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model2 = CreateModel();
   // Expect c2m command not in model.
-  EXPECT_EQ(-1, model2.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model2->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 }
 
@@ -141,20 +138,18 @@ TEST_F(ActionBoxMenuModelTest, OnRecordNoMobiles) {
 
   NavigateToLocalPage();
 
-  // Create model.
-  ActionBoxMenuModel model(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model = CreateModel();
 
   // Expect no c2m command in model.
-  EXPECT_EQ(-1, model.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 
   NavigateToBookmarkablePage();
 
-  // Create model.
-  ActionBoxMenuModel model2(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model2 = CreateModel();
 
   // Expect c2m command not in model.
-  EXPECT_EQ(-1, model2.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model2->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 }
 
@@ -169,30 +164,27 @@ TEST_F(ActionBoxMenuModelTest, HasMobilesOnRecordOrIncognito) {
 
   NavigateToLocalPage();
 
-  // Create model.
-  ActionBoxMenuModel model(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model = CreateModel();
 
   // Expect no c2m command in model.
-  EXPECT_EQ(-1, model.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 
   NavigateToBookmarkablePage();
 
-  // Create model.
-  ActionBoxMenuModel model2(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model2 = CreateModel();
 
   // Expect c2m command in model.
-  EXPECT_NE(-1, model2.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_NE(-1, model2->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 
   // Incognito-ize profile.
   profile()->set_incognito(true);
 
-  // Create another model.
-  ActionBoxMenuModel model3(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model3 = CreateModel();
 
   // Expect no c2m command in this model.
-  EXPECT_EQ(-1, model3.GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
+  EXPECT_EQ(-1, model3->GetIndexOfCommandId(IDC_CHROME_TO_MOBILE_PAGE));
   EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_CHROME_TO_MOBILE_PAGE));
 
   // Un-incognito-ize for shutdown.
@@ -211,19 +203,18 @@ TEST_F(ActionBoxMenuModelTest, BookmarkedPage) {
   GURL url1("http://www.google.com");
   AddTab(browser(), url1);
 
-  // Create model.
-  ActionBoxMenuModel model(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model = CreateModel();
 
   // Bokomark item should be in menu.
-  int bookmark_item_index = model.GetIndexOfCommandId(
+  int bookmark_item_index = model->GetIndexOfCommandId(
       IDC_BOOKMARK_PAGE_FROM_STAR);
-  EXPECT_NE(-1, bookmark_item_index);
+  ASSERT_NE(-1, bookmark_item_index);
 
   gfx::Image bookmark_icon;
   gfx::Image unlit_icon;
   gfx::Image lit_icon;
 
-  model.GetIconAt(bookmark_item_index, &bookmark_icon);
+  model->GetIconAt(bookmark_item_index, &bookmark_icon);
   unlit_icon =
       ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed(IDR_STAR);
 
@@ -240,10 +231,9 @@ TEST_F(ActionBoxMenuModelTest, BookmarkedPage) {
   // Now bookmark it.
   chrome::BookmarkCurrentPage(browser());
 
-  // Create model.
-  ActionBoxMenuModel model2(browser(), this);
+  scoped_ptr<ActionBoxMenuModel> model2 = CreateModel();
 
-  model2.GetIconAt(bookmark_item_index, &bookmark_icon);
+  model2->GetIconAt(bookmark_item_index, &bookmark_icon);
   lit_icon =
       ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed(IDR_STAR_LIT);