Increases the limit of bookmarks shown in the menu to 10,000

The ids now start at 0xDFFF. Since the menus are lazily loaded there isn't a need to limit how much is shown.

R=pkasting@chromium.org
BUG=374312
TEST=none

Review URL: https://codereview.chromium.org/1005873012

Cr-Commit-Position: refs/heads/master@{#322935}

9 files changed, 149 insertions, 214 deletions

diff --git a/chrome/app/chrome_command_ids.h b/chrome/app/chrome_command_ids.h
index 6db8daf..3e91a9e 100644
--- a/chrome/app/chrome_command_ids.h
+++ b/chrome/app/chrome_command_ids.h
@@ -361,4 +361,11 @@
 // NOTE: The last valid command value is 57343 (0xDFFF)
 // See http://msdn.microsoft.com/en-us/library/t2zechd4(VS.71).aspx
 
+// Starting command id for menus showing bookmarks (such as the wrench menu).
+// While command ids passed to Windows functions must not be higher than 0xDFFF,
+// these IDs are not exposed to the native system and thus can be in this
+// otherwise-reserved range. No command used in a menu (such as the wrench menu)
+// should be higher than this, otherwise it'll conflict.
+#define IDC_FIRST_BOOKMARK_MENU 0xE000
+
 #endif  // CHROME_APP_CHROME_COMMAND_IDS_H_
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.h b/chrome/browser/ui/toolbar/wrench_menu_model.h
index da4c431..aa6e8b2 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.h
+++ b/chrome/browser/ui/toolbar/wrench_menu_model.h
@@ -128,13 +128,7 @@ class WrenchMenuModel : public ui::SimpleMenuModel,
                         public TabStripModelObserver,
                         public content::NotificationObserver {
  public:
-  // Range of command ID's to use for the items representing bookmarks in the
-  // bookmark menu, must not overlap with that for recent tabs submenu.
-  static const int kMinBookmarkCommandId = 1;
-  static const int kMaxBookmarkCommandId = 1000;
-
-  // Range of command ID's to use for the items in the recent tabs submenu, must
-  // not overlap with that for bookmarks.
+  // Range of command IDs to use for the items in the recent tabs submenu.
   static const int kMinRecentTabsCommandId = 1001;
   static const int kMaxRecentTabsCommandId = 1200;
 
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
index b01a8c0..02d3a11 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc
@@ -31,9 +31,7 @@ BookmarkMenuController::BookmarkMenuController(Browser* browser,
                                                const BookmarkNode* node,
                                                int start_child_index,
                                                bool for_drop)
-    : menu_delegate_(
-        new BookmarkMenuDelegate(browser, page_navigator, parent, 1,
-                                 kint32max)),
+    : menu_delegate_(new BookmarkMenuDelegate(browser, page_navigator, parent)),
       node_(node),
       observer_(NULL),
       for_drop_(for_drop),
@@ -186,6 +184,10 @@ int BookmarkMenuController::GetMaxWidthForMenu(MenuItemView* view) {
   return menu_delegate_->GetMaxWidthForMenu(view);
 }
 
+void BookmarkMenuController::WillShowMenu(MenuItemView* menu) {
+  menu_delegate_->WillShowMenu(menu);
+}
+
 void BookmarkMenuController::BookmarkModelChanged() {
   if (!menu_delegate_->is_mutating_model())
     menu()->Cancel();
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.h b/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.h
index 86ea7e9..5c78008 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.h
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.h
@@ -112,6 +112,7 @@ class BookmarkMenuController : public bookmarks::BaseBookmarkModelObserver,
                                       bool* has_mnemonics,
                                       views::MenuButton** button) override;
   int GetMaxWidthForMenu(views::MenuItemView* view) override;
+  void WillShowMenu(views::MenuItemView* menu) override;
 
   // bookmarks::BaseBookmarkModelObserver:
   void BookmarkModelChanged() override;
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
index b6cb2db..ec43608 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
@@ -6,6 +6,7 @@
 
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/chrome_bookmark_client.h"
 #include "chrome/browser/bookmarks/chrome_bookmark_client_factory.h"
@@ -42,21 +43,18 @@ static const int kMaxMenuWidth = 400;
 
 BookmarkMenuDelegate::BookmarkMenuDelegate(Browser* browser,
                                            PageNavigator* navigator,
-                                           views::Widget* parent,
-                                           int first_menu_id,
-                                           int max_menu_id)
+                                           views::Widget* parent)
     : browser_(browser),
       profile_(browser->profile()),
       page_navigator_(navigator),
       parent_(parent),
       menu_(NULL),
       parent_menu_item_(NULL),
-      next_menu_id_(first_menu_id),
-      min_menu_id_(first_menu_id),
-      max_menu_id_(max_menu_id),
+      next_menu_id_(IDC_FIRST_BOOKMARK_MENU),
       real_delegate_(NULL),
       is_mutating_model_(false),
-      location_(BOOKMARK_LAUNCH_LOCATION_NONE) {}
+      location_(BOOKMARK_LAUNCH_LOCATION_NONE) {
+}
 
 BookmarkMenuDelegate::~BookmarkMenuDelegate() {
   GetBookmarkModel()->RemoveObserver(this);
@@ -91,12 +89,12 @@ void BookmarkMenuDelegate::Init(views::MenuDelegate* real_delegate,
     if (has_children && initial_count > 0)
       parent->AppendSeparator();
     if (show_managed)
-      BuildMenuForManagedNode(parent, &next_menu_id_);
+      BuildMenuForManagedNode(parent);
     if (show_supervised)
-      BuildMenuForSupervisedNode(parent, &next_menu_id_);
-    BuildMenu(node, start_child_index, parent, &next_menu_id_);
+      BuildMenuForSupervisedNode(parent);
+    BuildMenu(node, start_child_index, parent);
     if (show_options == SHOW_PERMANENT_FOLDERS)
-      BuildMenusForPermanentNodes(parent, &next_menu_id_);
+      BuildMenusForPermanentNodes(parent);
   } else {
     menu_ = CreateMenu(node, start_child_index, show_options);
   }
@@ -347,6 +345,13 @@ int BookmarkMenuDelegate::GetMaxWidthForMenu(MenuItemView* menu) {
   return kMaxMenuWidth;
 }
 
+void BookmarkMenuDelegate::WillShowMenu(MenuItemView* menu) {
+  auto iter = menu_id_to_node_map_.find(menu->GetCommand());
+  if ((iter != menu_id_to_node_map_.end()) && iter->second->child_count() &&
+      !menu->GetSubmenu()->GetMenuItemCount())
+    BuildMenu(iter->second, 0, menu);
+}
+
 void BookmarkMenuDelegate::BookmarkModelChanged() {
 }
 
@@ -430,45 +435,37 @@ MenuItemView* BookmarkMenuDelegate::CreateMenu(const BookmarkNode* parent,
                                                ShowOptions show_options) {
   MenuItemView* menu = new MenuItemView(real_delegate_);
   menu->SetCommand(next_menu_id_++);
-  menu_id_to_node_map_[menu->GetCommand()] = parent;
+  AddMenuToMaps(menu, parent);
   menu->set_has_icons(true);
   bool show_permanent = show_options == SHOW_PERMANENT_FOLDERS;
   if (show_permanent && parent == GetBookmarkModel()->bookmark_bar_node()) {
-    BuildMenuForManagedNode(menu, &next_menu_id_);
-    BuildMenuForSupervisedNode(menu, &next_menu_id_);
+    BuildMenuForManagedNode(menu);
+    BuildMenuForSupervisedNode(menu);
   }
-  BuildMenu(parent, start_child_index, menu, &next_menu_id_);
+  BuildMenu(parent, start_child_index, menu);
   if (show_permanent)
-    BuildMenusForPermanentNodes(menu, &next_menu_id_);
+    BuildMenusForPermanentNodes(menu);
   return menu;
 }
 
 void BookmarkMenuDelegate::BuildMenusForPermanentNodes(
-    views::MenuItemView* menu,
-    int* next_menu_id) {
+    views::MenuItemView* menu) {
   BookmarkModel* model = GetBookmarkModel();
   bool added_separator = false;
   BuildMenuForPermanentNode(model->other_node(), IDR_BOOKMARK_BAR_FOLDER, menu,
-                            next_menu_id, &added_separator);
+                            &added_separator);
   BuildMenuForPermanentNode(model->mobile_node(), IDR_BOOKMARK_BAR_FOLDER, menu,
-                            next_menu_id, &added_separator);
+                            &added_separator);
 }
 
 void BookmarkMenuDelegate::BuildMenuForPermanentNode(
     const BookmarkNode* node,
     int icon_resource_id,
     MenuItemView* menu,
-    int* next_menu_id,
     bool* added_separator) {
   if (!node->IsVisible() || node->GetTotalNodeCount() == 1)
     return;  // No children, don't create a menu.
 
-  int id = *next_menu_id;
-  // Don't create the submenu if its menu ID will be outside the range allowed.
-  if (IsOutsideMenuIdRange(id))
-    return;
-  (*next_menu_id)++;
-
   if (!*added_separator) {
     *added_separator = true;
     menu->AppendSeparator();
@@ -476,65 +473,55 @@ void BookmarkMenuDelegate::BuildMenuForPermanentNode(
 
   ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
   gfx::ImageSkia* folder_icon = rb->GetImageSkiaNamed(icon_resource_id);
-  MenuItemView* submenu = menu->AppendSubMenuWithIcon(
-      id, node->GetTitle(), *folder_icon);
-  BuildMenu(node, 0, submenu, next_menu_id);
-  menu_id_to_node_map_[id] = node;
+  AddMenuToMaps(menu->AppendSubMenuWithIcon(next_menu_id_++, node->GetTitle(),
+                                            *folder_icon),
+                node);
 }
 
-void BookmarkMenuDelegate::BuildMenuForManagedNode(MenuItemView* menu,
-                                                   int* next_menu_id) {
+void BookmarkMenuDelegate::BuildMenuForManagedNode(MenuItemView* menu) {
   // Don't add a separator for this menu.
   bool added_separator = true;
   const BookmarkNode* node = GetChromeBookmarkClient()->managed_node();
   BuildMenuForPermanentNode(node, IDR_BOOKMARK_BAR_FOLDER_MANAGED, menu,
-                            next_menu_id, &added_separator);
+                            &added_separator);
 }
 
-void BookmarkMenuDelegate::BuildMenuForSupervisedNode(MenuItemView* menu,
-                                                      int* next_menu_id) {
+void BookmarkMenuDelegate::BuildMenuForSupervisedNode(MenuItemView* menu) {
   // Don't add a separator for this menu.
   bool added_separator = true;
   const BookmarkNode* node = GetChromeBookmarkClient()->supervised_node();
   BuildMenuForPermanentNode(node, IDR_BOOKMARK_BAR_FOLDER_SUPERVISED, menu,
-                            next_menu_id, &added_separator);
+                            &added_separator);
 }
 
 void BookmarkMenuDelegate::BuildMenu(const BookmarkNode* parent,
                                      int start_child_index,
-                                     MenuItemView* menu,
-                                     int* next_menu_id) {
-  node_to_menu_map_[parent] = menu;
+                                     MenuItemView* menu) {
   DCHECK(parent->empty() || start_child_index < parent->child_count());
   ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
   for (int i = start_child_index; i < parent->child_count(); ++i) {
     const BookmarkNode* node = parent->GetChild(i);
-    const int id = *next_menu_id;
-    // Don't create the item if its menu ID will be outside the range allowed.
-    if (IsOutsideMenuIdRange(id))
-      break;
-
-    (*next_menu_id)++;
-
-    menu_id_to_node_map_[id] = node;
+    const int id = next_menu_id_++;
+    MenuItemView* child_menu_item;
     if (node->is_url()) {
       const gfx::Image& image = GetBookmarkModel()->GetFavicon(node);
       const gfx::ImageSkia* icon = image.IsEmpty() ?
           rb->GetImageSkiaNamed(IDR_DEFAULT_FAVICON) : image.ToImageSkia();
-      node_to_menu_map_[node] =
+      child_menu_item =
           menu->AppendMenuItemWithIcon(id, node->GetTitle(), *icon);
-    } else if (node->is_folder()) {
+    } else {
+      DCHECK(node->is_folder());
       gfx::ImageSkia* folder_icon =
           rb->GetImageSkiaNamed(IDR_BOOKMARK_BAR_FOLDER);
-      MenuItemView* submenu = menu->AppendSubMenuWithIcon(
-          id, node->GetTitle(), *folder_icon);
-      BuildMenu(node, 0, submenu, next_menu_id);
-    } else {
-      NOTREACHED();
+      child_menu_item =
+          menu->AppendSubMenuWithIcon(id, node->GetTitle(), *folder_icon);
     }
+    AddMenuToMaps(child_menu_item, node);
   }
 }
 
-bool BookmarkMenuDelegate::IsOutsideMenuIdRange(int menu_id) const {
-  return menu_id < min_menu_id_ || menu_id > max_menu_id_;
+void BookmarkMenuDelegate::AddMenuToMaps(MenuItemView* menu,
+                                         const BookmarkNode* node) {
+  menu_id_to_node_map_[menu->GetCommand()] = node;
+  node_to_menu_map_[node] = menu;
 }
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h
index 3c28ff4..7b44bd9 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h
@@ -52,9 +52,7 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
 
   BookmarkMenuDelegate(Browser* browser,
                        content::PageNavigator* navigator,
-                       views::Widget* parent,
-                       int first_menu_id,
-                       int max_menu_id);
+                       views::Widget* parent);
   ~BookmarkMenuDelegate() override;
 
   // Creates the menus from the model.
@@ -119,6 +117,7 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   void WriteDragData(views::MenuItemView* sender, ui::OSExchangeData* data);
   int GetDragOperations(views::MenuItemView* sender);
   int GetMaxWidthForMenu(views::MenuItemView* menu);
+  void WillShowMenu(views::MenuItemView* menu);
 
   // BookmarkModelObserver methods.
   void BookmarkModelChanged() override;
@@ -131,6 +130,8 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   void DidRemoveBookmarks() override;
 
  private:
+  friend class BookmarkMenuDelegateTest;
+
   typedef std::map<int, const bookmarks::BookmarkNode*> MenuIDToNodeMap;
   typedef std::map<const bookmarks::BookmarkNode*, views::MenuItemView*>
       NodeToMenuMap;
@@ -142,8 +143,7 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
 
   // Invokes BuildMenuForPermanentNode() for the permanent nodes (excluding
   // 'other bookmarks' folder).
-  void BuildMenusForPermanentNodes(views::MenuItemView* menu,
-                                   int* next_menu_id);
+  void BuildMenusForPermanentNodes(views::MenuItemView* menu);
 
   // If |node| has children a new menu is created and added to |menu| to
   // represent it. If |node| is not empty and |added_separator| is false, a
@@ -152,22 +152,20 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   void BuildMenuForPermanentNode(const bookmarks::BookmarkNode* node,
                                  int icon_resource_id,
                                  views::MenuItemView* menu,
-                                 int* next_menu_id,
                                  bool* added_separator);
 
-  void BuildMenuForManagedNode(views::MenuItemView* menu, int* next_menu_id);
-  void BuildMenuForSupervisedNode(views::MenuItemView* menu, int* next_menu_id);
+  void BuildMenuForManagedNode(views::MenuItemView* menu);
+  void BuildMenuForSupervisedNode(views::MenuItemView* menu);
 
   // Creates an entry in menu for each child node of |parent| starting at
   // |start_child_index|.
   void BuildMenu(const bookmarks::BookmarkNode* parent,
                  int start_child_index,
-                 views::MenuItemView* menu,
-                 int* next_menu_id);
+                 views::MenuItemView* menu);
 
-  // Returns true if |menu_id_| is outside the range of minimum and maximum menu
-  // ID's allowed.
-  bool IsOutsideMenuIdRange(int menu_id) const;
+  // Registers the necessary mappings for |menu| and |node|.
+  void AddMenuToMaps(views::MenuItemView* menu,
+                     const bookmarks::BookmarkNode* node);
 
   Browser* browser_;
   Profile* profile_;
@@ -198,10 +196,6 @@ class BookmarkMenuDelegate : public bookmarks::BaseBookmarkModelObserver,
   // ID of the next menu item.
   int next_menu_id_;
 
-  // Minimum and maximum ID's to use for menu items.
-  const int min_menu_id_;
-  const int max_menu_id_;
-
   views::MenuDelegate* real_delegate_;
 
   // Is the model being changed?
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
index 571f387..d7dd524b 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
@@ -46,30 +46,52 @@ class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
   }
 
  protected:
-  void NewDelegate(int min_menu_id, int max_menu_id) {
+  void NewDelegate() {
     // Destroy current menu if available, see comments in TearDown().
     if (bookmark_menu_delegate_.get())
       views::MenuRunner menu_runner(bookmark_menu_delegate_->menu(), 0);
 
     bookmark_menu_delegate_.reset(
-        new BookmarkMenuDelegate(browser(), NULL, NULL,
-                                 min_menu_id, max_menu_id));
+        new BookmarkMenuDelegate(browser(), nullptr, nullptr));
   }
 
-  void NewAndInitDelegateForPermanent(int min_menu_id,
-                                      int max_menu_id) {
+  void NewAndInitDelegateForPermanent() {
     const BookmarkNode* node = model_->bookmark_bar_node();
-    NewDelegate(min_menu_id, max_menu_id);
+    NewDelegate();
     bookmark_menu_delegate_->Init(&test_delegate_, NULL, node, 0,
                                   BookmarkMenuDelegate::SHOW_PERMANENT_FOLDERS,
                                   BOOKMARK_LAUNCH_LOCATION_NONE);
   }
 
+  const BookmarkNode* GetNodeForMenuItem(views::MenuItemView* menu) {
+    const auto& node_map = bookmark_menu_delegate_->menu_id_to_node_map_;
+    auto iter = node_map.find(menu->GetCommand());
+    return (iter == node_map.end()) ? nullptr : iter->second;
+  }
+
+  int next_menu_id() { return bookmark_menu_delegate_->next_menu_id_; }
+
+  // Forces all the menus to load by way of invoking WillShowMenu() on all menu
+  // items of tyep SUBMENU.
+  void LoadAllMenus() { LoadAllMenus(bookmark_menu_delegate_->menu()); }
+
   BookmarkModel* model_;
 
   scoped_ptr<BookmarkMenuDelegate> bookmark_menu_delegate_;
 
  private:
+  void LoadAllMenus(views::MenuItemView* menu) {
+    EXPECT_EQ(views::MenuItemView::SUBMENU, menu->GetType());
+
+    for (int i = 0; i < menu->GetSubmenu()->GetMenuItemCount(); ++i) {
+      views::MenuItemView* child = menu->GetSubmenu()->GetMenuItemAt(i);
+      if (child->GetType() == views::MenuItemView::SUBMENU) {
+        bookmark_menu_delegate_->WillShowMenu(child);
+        LoadAllMenus(child);
+      }
+    }
+  }
+
   std::string base_path() const { return "file:///c:/tmp/"; }
 
   // Creates the following structure:
@@ -107,124 +129,60 @@ class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
   DISALLOW_COPY_AND_ASSIGN(BookmarkMenuDelegateTest);
 };
 
+TEST_F(BookmarkMenuDelegateTest, VerifyLazyLoad) {
+  NewAndInitDelegateForPermanent();
+  views::MenuItemView* root_item = bookmark_menu_delegate_->menu();
+  ASSERT_TRUE(root_item->HasSubmenu());
+  EXPECT_EQ(4, root_item->GetSubmenu()->GetMenuItemCount());
+  EXPECT_EQ(5, root_item->GetSubmenu()->child_count());  // Includes separator.
+  views::MenuItemView* f1_item = root_item->GetSubmenu()->GetMenuItemAt(1);
+  ASSERT_TRUE(f1_item->HasSubmenu());
+  // f1 hasn't been loaded yet.
+  EXPECT_EQ(0, f1_item->GetSubmenu()->GetMenuItemCount());
+  // Will show triggers a load.
+  int next_id_before_load = next_menu_id();
+  bookmark_menu_delegate_->WillShowMenu(f1_item);
+  // f1 should have loaded its children.
+  EXPECT_EQ(next_id_before_load + 2, next_menu_id());
+  ASSERT_EQ(2, f1_item->GetSubmenu()->GetMenuItemCount());
+  const BookmarkNode* f1_node = model_->bookmark_bar_node()->GetChild(1);
+  EXPECT_EQ(f1_node->GetChild(0),
+            GetNodeForMenuItem(f1_item->GetSubmenu()->GetMenuItemAt(0)));
+  EXPECT_EQ(f1_node->GetChild(1),
+            GetNodeForMenuItem(f1_item->GetSubmenu()->GetMenuItemAt(1)));
+
+  // F11 shouldn't have loaded yet.
+  views::MenuItemView* f11_item = f1_item->GetSubmenu()->GetMenuItemAt(1);
+  ASSERT_TRUE(f11_item->HasSubmenu());
+  EXPECT_EQ(0, f11_item->GetSubmenu()->GetMenuItemCount());
+
+  next_id_before_load = next_menu_id();
+  bookmark_menu_delegate_->WillShowMenu(f11_item);
+  // Invoke WillShowMenu() twice to make sure the second call doesn't cause
+  // problems.
+  bookmark_menu_delegate_->WillShowMenu(f11_item);
+  // F11 should have loaded its single child (f11a).
+  EXPECT_EQ(next_id_before_load + 1, next_menu_id());
+
+  ASSERT_EQ(1, f11_item->GetSubmenu()->GetMenuItemCount());
+  const BookmarkNode* f11_node = f1_node->GetChild(1);
+  EXPECT_EQ(f11_node->GetChild(0),
+            GetNodeForMenuItem(f11_item->GetSubmenu()->GetMenuItemAt(0)));
+}
+
 // Verifies WillRemoveBookmarks() doesn't attempt to access MenuItemViews that
 // have since been deleted.
 TEST_F(BookmarkMenuDelegateTest, RemoveBookmarks) {
   views::MenuDelegate test_delegate;
   const BookmarkNode* node = model_->bookmark_bar_node()->GetChild(1);
-  NewDelegate(0, kint32max);
+  NewDelegate();
   bookmark_menu_delegate_->Init(&test_delegate, NULL, node, 0,
                                 BookmarkMenuDelegate::HIDE_PERMANENT_FOLDERS,
                                 BOOKMARK_LAUNCH_LOCATION_NONE);
+  LoadAllMenus();
   std::vector<const BookmarkNode*> nodes_to_remove;
   nodes_to_remove.push_back(node->GetChild(1));
   bookmark_menu_delegate_->WillRemoveBookmarks(nodes_to_remove);
   nodes_to_remove.clear();
   bookmark_menu_delegate_->DidRemoveBookmarks();
 }
-
-// Verifies menu ID's of items in menu fall within the specified range.
-TEST_F(BookmarkMenuDelegateTest, MenuIdRange) {
-  // Start with maximum menu Id of 10 - the number of items that AddTestData()
-  // populated.  Everything should be created.
-  NewAndInitDelegateForPermanent(0, 10);
-  views::MenuItemView* root_item = bookmark_menu_delegate_->menu();
-  ASSERT_TRUE(root_item->HasSubmenu());
-  EXPECT_EQ(4, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(5, root_item->GetSubmenu()->child_count());  // Includes separator.
-  views::MenuItemView* F1_item = root_item->GetSubmenu()->GetMenuItemAt(1);
-  ASSERT_TRUE(F1_item->HasSubmenu());
-  EXPECT_EQ(2, F1_item->GetSubmenu()->GetMenuItemCount());
-  views::MenuItemView* F11_item = F1_item->GetSubmenu()->GetMenuItemAt(1);
-  ASSERT_TRUE(F11_item->HasSubmenu());
-  EXPECT_EQ(1, F11_item->GetSubmenu()->GetMenuItemCount());
-  views::MenuItemView* other_item = root_item->GetSubmenu()->GetMenuItemAt(3);
-  ASSERT_TRUE(other_item->HasSubmenu());
-  EXPECT_EQ(2, other_item->GetSubmenu()->GetMenuItemCount());
-  views::MenuItemView* OF1_item = other_item->GetSubmenu()->GetMenuItemAt(1);
-  ASSERT_TRUE(OF1_item->HasSubmenu());
-  EXPECT_EQ(1, OF1_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum 9.  "of1a" item should not be created.
-  NewAndInitDelegateForPermanent(0, 9);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(4, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(5, root_item->GetSubmenu()->child_count());  // Includes separator.
-  other_item = root_item->GetSubmenu()->GetMenuItemAt(3);
-  OF1_item = other_item->GetSubmenu()->GetMenuItemAt(1);
-  EXPECT_EQ(0, OF1_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum 8.  "OF1" submenu should not be created.
-  NewAndInitDelegateForPermanent(0, 8);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(4, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(5, root_item->GetSubmenu()->child_count());  // Includes separator.
-  other_item = root_item->GetSubmenu()->GetMenuItemAt(3);
-  EXPECT_EQ(1, other_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum 7.  "Other" submenu should be empty.
-  NewAndInitDelegateForPermanent(0, 7);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(4, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(5, root_item->GetSubmenu()->child_count());  // Includes separator.
-  other_item = root_item->GetSubmenu()->GetMenuItemAt(3);
-  EXPECT_EQ(0, other_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum to 6.  "Other" submenu should not be created, and no
-  // separator.
-  NewAndInitDelegateForPermanent(0, 6);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(3, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(3, root_item->GetSubmenu()->child_count());  // No separator.
-
-  // Reduce maximum 5.  "F2" and "Other" submenus shouldn't be created.
-  NewAndInitDelegateForPermanent(0, 5);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(2, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(2, root_item->GetSubmenu()->child_count());  // No separator.
-  F1_item = root_item->GetSubmenu()->GetMenuItemAt(1);
-  F11_item = F1_item->GetSubmenu()->GetMenuItemAt(1);
-  EXPECT_EQ(1, F11_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum to 4.  "f11a" item and "F2" and "Other" submenus should
-  // not be created.
-  NewAndInitDelegateForPermanent(0, 4);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(2, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(2, root_item->GetSubmenu()->child_count());  // No separator.
-  F1_item = root_item->GetSubmenu()->GetMenuItemAt(1);
-  F11_item = F1_item->GetSubmenu()->GetMenuItemAt(1);
-  EXPECT_EQ(0, F11_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum to 3.  "F11", "F2" and "Other" submenus should not be
-  // created.
-  NewAndInitDelegateForPermanent(0, 3);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(2, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(2, root_item->GetSubmenu()->child_count());  // No separator.
-  F1_item = root_item->GetSubmenu()->GetMenuItemAt(1);
-  EXPECT_EQ(views::MenuItemView::SUBMENU, F1_item->GetType());
-  EXPECT_EQ(1, F1_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum 2.  Only "a" item and empty "F1" submenu should be created.
-  NewAndInitDelegateForPermanent(0, 2);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(2, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(2, root_item->GetSubmenu()->child_count());  // No separator.
-  F1_item = root_item->GetSubmenu()->GetMenuItemAt(1);
-  EXPECT_EQ(views::MenuItemView::SUBMENU, F1_item->GetType());
-  EXPECT_EQ(0, F1_item->GetSubmenu()->GetMenuItemCount());
-
-  // Reduce maximum to 1.  Only "a" item should be created.
-  NewAndInitDelegateForPermanent(0, 1);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(1, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(1, root_item->GetSubmenu()->child_count());  // No separator.
-
-  // Verify correct handling of integer overflow with range, set kint32max as
-  // maximum and 1 less as minimum.  Only "a" item should be created.
-  NewAndInitDelegateForPermanent(kint32max - 1, kint32max);
-  root_item = bookmark_menu_delegate_->menu();
-  EXPECT_EQ(1, root_item->GetSubmenu()->GetMenuItemCount());
-  EXPECT_EQ(1, root_item->GetSubmenu()->child_count());  // No separator.
-}
diff --git a/chrome/browser/ui/views/toolbar/wrench_menu.cc b/chrome/browser/ui/views/toolbar/wrench_menu.cc
index fb972ec..00d9a7a 100644
--- a/chrome/browser/ui/views/toolbar/wrench_menu.cc
+++ b/chrome/browser/ui/views/toolbar/wrench_menu.cc
@@ -91,8 +91,7 @@ const int kZoomLabelHorizontalPadding = 2;
 
 // Returns true if |command_id| identifies a bookmark menu item.
 bool IsBookmarkCommand(int command_id) {
-  return command_id >= WrenchMenuModel::kMinBookmarkCommandId &&
-      command_id <= WrenchMenuModel::kMaxBookmarkCommandId;
+  return command_id >= IDC_FIRST_BOOKMARK_MENU;
 }
 
 // Returns true if |command_id| identifies a recent tabs menu item.
@@ -817,13 +816,6 @@ void WrenchMenu::Init(ui::MenuModel* model) {
                                // so we get the taller menu style.
   PopulateMenu(root_, model);
 
-#if !defined(NDEBUG)
-  // Verify that the reserved command ID's for bookmarks menu are not used.
-  for (int i = WrenchMenuModel::kMinBookmarkCommandId;
-       i <= WrenchMenuModel::kMaxBookmarkCommandId; ++i)
-    DCHECK(command_id_to_entry_.find(i) == command_id_to_entry_.end());
-#endif  // !defined(NDEBUG)
-
   int32 types = views::MenuRunner::HAS_MNEMONICS;
   if (for_drop()) {
     // We add NESTED_DRAG since currently the only operation to open the wrench
@@ -1055,6 +1047,8 @@ bool WrenchMenu::GetAccelerator(int command_id,
 void WrenchMenu::WillShowMenu(MenuItemView* menu) {
   if (menu == bookmark_menu_)
     CreateBookmarkMenu();
+  else if (bookmark_menu_delegate_)
+    bookmark_menu_delegate_->WillShowMenu(menu);
 }
 
 void WrenchMenu::WillHideMenu(MenuItemView* menu) {
@@ -1064,9 +1058,9 @@ void WrenchMenu::WillHideMenu(MenuItemView* menu) {
   if (menu->HasSubmenu() &&
       ((feedback_menu_item_ && feedback_menu_item_->IsSelected()) ||
        (screenshot_menu_item_ && screenshot_menu_item_->IsSelected()))) {
-    // It's okay to just turn off the animation and no to take care the
-    // animation back because the menu widget will be recreated next time
-    // it's opened. See ToolbarView::RunMenu() and Init() of this class.
+    // It's okay to just turn off the animation and not turn it back on because
+    // the menu widget will be recreated next time it's opened. See
+    // ToolbarView::RunMenu() and Init() of this class.
     menu->GetSubmenu()->GetWidget()->
         SetVisibilityChangedAnimationsEnabled(false);
   }
@@ -1197,6 +1191,7 @@ MenuItemView* WrenchMenu::AddMenuItem(MenuItemView* parent,
   DCHECK(command_id > -1 ||
          (command_id == -1 &&
           model->GetTypeAt(model_index) == MenuModel::TYPE_SEPARATOR));
+  DCHECK_LT(command_id, IDC_FIRST_BOOKMARK_MENU);
 
   if (command_id > -1) {  // Don't add separators to |command_id_to_entry_|.
     // All command ID's should be unique except for IDC_SHOW_HISTORY which is
@@ -1249,11 +1244,7 @@ void WrenchMenu::CreateBookmarkMenu() {
   views::Widget* parent = views::Widget::GetWidgetForNativeWindow(
       browser_->window()->GetNativeWindow());
   bookmark_menu_delegate_.reset(
-      new BookmarkMenuDelegate(browser_,
-                               browser_,
-                               parent,
-                               WrenchMenuModel::kMinBookmarkCommandId,
-                               WrenchMenuModel::kMaxBookmarkCommandId));
+      new BookmarkMenuDelegate(browser_, browser_, parent));
   bookmark_menu_delegate_->Init(this,
                                 bookmark_menu_,
                                 model->bookmark_bar_node(),
diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
index 7e77b13..7cc4955 100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -1515,6 +1515,7 @@ void MenuController::OpenMenuImpl(MenuItemView* item, bool show) {
     item->GetDelegate()->WillShowMenu(item);
     if (old_count != item->GetSubmenu()->child_count()) {
       // If the number of children changed then we may need to add empty items.
+      item->RemoveEmptyMenus();
       item->AddEmptyMenus();
     }
   }