Revert 227675 "reland "views: change WrenchMenu to use each mode..."

> reland "views: change WrenchMenu to use each model's command ID's when creating MenuItemView's"
> 
> the original cl (hhttps://codereview.chromium.org/25667005/) was reverted because of crash in bug 304756, this cl relands it with the fixes. 
> 
> this is in preparation for dynamic recent tabs model and menu: 
> 
> * WrenchMenu: 
> - for each menu item, MenuItemView::GetCommand() used to be separate and different from MenuModel::GetCommandIdAt(model_index); they are the same now. 
> - different range of command id's are reserved for recent tabs and bookmarks. 
> - command id's of all items in the wrench menu and all its submenus can't clash, except for separator and IDC_SHOW_HISTORY, the latter of which is in both wrench menu and recent tabs submenu. 
> 
> * RecentTabsSubMenuModel: 
> - make all command ids within range, use different ids among device name headers. 
> 
> * BookmarkMenuDelegate 
> - makes sure only maximum number of menu items allowed is created. 
> - add test. 
> 
> BUG=256750
> TEST=wrench menu and all its submenus still work.
> 
> Review URL: https://codereview.chromium.org/26455004

The new test has memory leaks. When the previous menu delegate is freed it doesn't
free the menu it creates in Init, I suspect this normally is handled by the views
system itself.

Example of leaks:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28valgrind%29%286%29/builds/21625

You can reproduce the leaks by building for chromeos and running under valgrind:
http://www.chromium.org/developers/how-tos/using-valgrind

TBR=kuan@chromium.org

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

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

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc

@@ -24,7 +24,6 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/browser/ui/toolbar/wrench_menu_model.h"
 #include "chrome/common/favicon/favicon_types.h"
 #include "chrome/common/pref_names.h"
 #include "grit/browser_resources.h"
@@ -53,13 +52,8 @@ namespace {
 // These values must be bigger than the maximum possible number of items in
 // menu, so that index of last menu item doesn't clash with this value when menu
 // items are retrieved via GetIndexOfCommandId.
-// The range of all command ID's used in RecentTabsSubMenuModel must be between
-// WrenchMenuModel::kMinRecentTabsCommandId i.e. 1001 and 1200
-// (WrenchMenuModel::kMaxRecentTabsCommandId) inclusively.
-const int kFirstTabCommandId = WrenchMenuModel::kMinRecentTabsCommandId;
-const int kFirstWindowCommandId = 1051;
-const int kMinDeviceNameCommandId = 1100;
-const int kMaxDeviceNameCommandId = 1110;
+const int kFirstTabCommandId = 100;
+const int kFirstWindowCommandId = 200;
 
 // The maximum number of recently closed entries to be shown in the menu.
 const int kMaxRecentlyClosedEntries = 8;
@@ -88,11 +82,6 @@ bool IsWindowModelCommandId(int command_id) {
          command_id < RecentTabsSubMenuModel::kRecentlyClosedHeaderCommandId;
 }
 
-bool IsDeviceNameCommandId(int command_id) {
-  return command_id >= kMinDeviceNameCommandId &&
-      command_id <= kMaxDeviceNameCommandId;
-}
-
 // Convert |tab_model_index| to command id of menu item.
 int TabModelIndexToCommandId(int tab_model_index) {
   int command_id = tab_model_index + kFirstTabCommandId;
@@ -157,8 +146,9 @@ struct RecentTabsSubMenuModel::TabNavigationItem {
   GURL url;
 };
 
-const int RecentTabsSubMenuModel::kRecentlyClosedHeaderCommandId = 1120;
-const int RecentTabsSubMenuModel::kDisabledRecentlyClosedHeaderCommandId = 1121;
+const int RecentTabsSubMenuModel::kRecentlyClosedHeaderCommandId = 500;
+const int RecentTabsSubMenuModel::kDisabledRecentlyClosedHeaderCommandId = 501;
+const int RecentTabsSubMenuModel::kDeviceNameCommandId = 1000;
 
 RecentTabsSubMenuModel::RecentTabsSubMenuModel(
     ui::AcceleratorProvider* accelerator_provider,
@@ -202,8 +192,8 @@ bool RecentTabsSubMenuModel::IsCommandIdChecked(int command_id) const {
 bool RecentTabsSubMenuModel::IsCommandIdEnabled(int command_id) const {
   if (command_id == kRecentlyClosedHeaderCommandId ||
       command_id == kDisabledRecentlyClosedHeaderCommandId ||
-      command_id == IDC_RECENT_TABS_NO_DEVICE_TABS ||
-      IsDeviceNameCommandId(command_id)) {
+      command_id == kDeviceNameCommandId ||
+      command_id == IDC_RECENT_TABS_NO_DEVICE_TABS) {
     return false;
   }
   return true;
@@ -234,8 +224,8 @@ void RecentTabsSubMenuModel::ExecuteCommand(int command_id, int event_flags) {
     return;
   }
 
+  DCHECK_NE(kDeviceNameCommandId, command_id);
   DCHECK_NE(IDC_RECENT_TABS_NO_DEVICE_TABS, command_id);
-  DCHECK(!IsDeviceNameCommandId(command_id));
 
   WindowOpenDisposition disposition =
       ui::DispositionFromEventFlags(event_flags);
@@ -292,8 +282,8 @@ void RecentTabsSubMenuModel::ExecuteCommand(int command_id, int event_flags) {
 
 const gfx::Font* RecentTabsSubMenuModel::GetLabelFontAt(int index) const {
   int command_id = GetCommandIdAt(index);
-  if (command_id == kRecentlyClosedHeaderCommandId ||
-      IsDeviceNameCommandId(command_id)) {
+  if (command_id == kDeviceNameCommandId ||
+      command_id == kRecentlyClosedHeaderCommandId) {
     return &ResourceBundle::GetSharedInstance().GetFont(
         ResourceBundle::BoldFont);
   }
@@ -441,7 +431,7 @@ void RecentTabsSubMenuModel::BuildDevices() {
     // Add the header for the device session.
     DCHECK(!session->session_name.empty());
     AddSeparator(ui::NORMAL_SEPARATOR);
-    AddItem(kMinDeviceNameCommandId + i, UTF8ToUTF16(session->session_name));
+    AddItem(kDeviceNameCommandId, UTF8ToUTF16(session->session_name));
     AddDeviceFavicon(GetItemCount() - 1, session->device_type);
 
     // Build tab menu items from sorted session tabs.

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h

@@ -39,11 +39,6 @@ class AcceleratorProvider;
 class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
                                public ui::SimpleMenuModel::Delegate {
  public:
-  // Command Id for recently closed items header or disabled item to which the
-  // accelerator string will be appended.
-  static const int kRecentlyClosedHeaderCommandId;
-  static const int kDisabledRecentlyClosedHeaderCommandId;
-
   // If |associator| is NULL, default associator for |browser|'s profile will
   // be used.  Testing may require a specific |associator|.
   RecentTabsSubMenuModel(ui::AcceleratorProvider* accelerator_provider,
@@ -65,6 +60,14 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
                                     std::string* url,
                                     string16* title) const;
 
+  // Command Id for recently closed items header or disabled item to which the
+  // accelerator string will be appended.
+  static const int kRecentlyClosedHeaderCommandId;
+  static const int kDisabledRecentlyClosedHeaderCommandId;
+
+  // Command Id for other devices section headers, using the device name.
+  static const int kDeviceNameCommandId;
+
  private:
   struct TabNavigationItem;
   typedef std::vector<TabNavigationItem> TabNavigationItems;

chrome/browser/ui/toolbar/wrench_menu_model.h

@@ -79,16 +79,6 @@ 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.
-  static const int kMinRecentTabsCommandId = 1001;
-  static const int kMaxRecentTabsCommandId = 1200;
-
   // TODO: remove |is_new_menu|.
   WrenchMenuModel(ui::AcceleratorProvider* provider,
                   Browser* browser,

chrome/browser/ui/views/bookmarks/bookmark_menu_controller_views.cc

@@ -34,8 +34,7 @@ BookmarkMenuController::BookmarkMenuController(Browser* browser,
                                                const BookmarkNode* node,
                                                int start_child_index)
     : menu_delegate_(
-        new BookmarkMenuDelegate(browser, page_navigator, parent, 1,
-                                 kint32max)),
+        new BookmarkMenuDelegate(browser, page_navigator, parent, 1)),
       node_(node),
       observer_(NULL),
       for_drop_(false),

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc

@@ -41,8 +41,7 @@ static const int kMaxMenuWidth = 400;
 BookmarkMenuDelegate::BookmarkMenuDelegate(Browser* browser,
                                            PageNavigator* navigator,
                                            views::Widget* parent,
-                                           int first_menu_id,
-                                           int max_menu_id)
+                                           int first_menu_id)
     : browser_(browser),
       profile_(browser->profile()),
       page_navigator_(navigator),
@@ -51,8 +50,6 @@ BookmarkMenuDelegate::BookmarkMenuDelegate(Browser* browser,
       for_drop_(false),
       parent_menu_item_(NULL),
       next_menu_id_(first_menu_id),
-      min_menu_id_(first_menu_id),
-      max_menu_id_(max_menu_id),
       real_delegate_(NULL),
       is_mutating_model_(false),
       location_(BOOKMARK_LAUNCH_LOCATION_NONE) {}
@@ -69,7 +66,6 @@ void BookmarkMenuDelegate::Init(views::MenuDelegate* real_delegate,
                                 BookmarkLaunchLocation location) {
   GetBookmarkModel()->AddObserver(this);
   real_delegate_ = real_delegate;
-  location_ = location;
   if (parent) {
     parent_menu_item_ = parent;
     int initial_count = parent->GetSubmenu() ?
@@ -84,6 +80,8 @@ void BookmarkMenuDelegate::Init(views::MenuDelegate* real_delegate,
   } else {
     menu_ = CreateMenu(node, start_child_index, show_options);
   }
+
+  location_ = location;
 }
 
 void BookmarkMenuDelegate::SetPageNavigator(PageNavigator* navigator) {
@@ -434,17 +432,12 @@ void BookmarkMenuDelegate::BuildMenuForPermanentNode(
   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();
   }
-
+  int id = *next_menu_id;
+  (*next_menu_id)++;
   ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
   gfx::ImageSkia* folder_icon = rb->GetImageSkiaNamed(IDR_BOOKMARK_BAR_FOLDER);
   MenuItemView* submenu = menu->AppendSubMenuWithIcon(
@@ -463,10 +456,6 @@ void BookmarkMenuDelegate::BuildMenu(const BookmarkNode* parent,
   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;
@@ -487,7 +476,3 @@ void BookmarkMenuDelegate::BuildMenu(const BookmarkNode* parent,
     }
   }
 }
-
-bool BookmarkMenuDelegate::IsOutsideMenuIdRange(int menu_id) const {
-  return menu_id < min_menu_id_ || menu_id > max_menu_id_;
-}

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h

@@ -53,8 +53,7 @@ class BookmarkMenuDelegate : public BaseBookmarkModelObserver,
   BookmarkMenuDelegate(Browser* browser,
                        content::PageNavigator* navigator,
                        views::Widget* parent,
-                       int first_menu_id,
-                       int max_menu_id);
+                       int first_menu_id);
   virtual ~BookmarkMenuDelegate();
 
   // Creates the menus from the model.
@@ -130,9 +129,6 @@ class BookmarkMenuDelegate : public BaseBookmarkModelObserver,
   virtual void DidRemoveBookmarks() OVERRIDE;
 
  private:
-  friend class BookmarkMenuDelegateTest;
-  FRIEND_TEST_ALL_PREFIXES(BookmarkMenuDelegateTest, MenuIdRange);
-
   typedef std::map<int, const BookmarkNode*> MenuIDToNodeMap;
   typedef std::map<const BookmarkNode*, views::MenuItemView*> NodeToMenuMap;
 
@@ -162,10 +158,6 @@ class BookmarkMenuDelegate : public BaseBookmarkModelObserver,
                  views::MenuItemView* menu,
                  int* next_menu_id);
 
-  // Returns true if |menu_id_| is outside the range of minimum and maximum menu
-  // ID's allowed.
-  bool IsOutsideMenuIdRange(int menu_id) const;
-
   Browser* browser_;
   Profile* profile_;
 
@@ -198,10 +190,6 @@ class BookmarkMenuDelegate : public 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?

chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc

@@ -13,11 +13,11 @@
 #include "chrome/test/base/browser_with_test_window_test.h"
 #include "chrome/test/base/testing_profile.h"
 #include "ui/views/controls/menu/menu_runner.h"
-#include "ui/views/controls/menu/submenu_view.h"
 
 class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
  public:
-  BookmarkMenuDelegateTest() : model_(NULL) {}
+  BookmarkMenuDelegateTest() : model_(NULL) {
+  }
 
   virtual void SetUp() OVERRIDE {
     BrowserWithTestWindowTest::SetUp();
@@ -28,6 +28,9 @@ class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
     test::WaitForBookmarkModelToLoad(model_);
 
     AddTestData();
+
+    bookmark_menu_delegate_.reset(
+        new BookmarkMenuDelegate(browser(), NULL, NULL, 0));
   }
 
   virtual void TearDown() OVERRIDE {
@@ -39,21 +42,6 @@ class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
   }
 
  protected:
-  void NewDelegate(int min_menu_id, int max_menu_id) {
-    bookmark_menu_delegate_.reset(
-        new BookmarkMenuDelegate(browser(), NULL, NULL,
-                                 min_menu_id, max_menu_id));
-  }
-
-  void NewAndInitDelegateForPermanent(int min_menu_id,
-                                      int max_menu_id) {
-    const BookmarkNode* node = model_->bookmark_bar_node();
-    NewDelegate(min_menu_id, max_menu_id);
-    bookmark_menu_delegate_->Init(&test_delegate_, NULL, node, 0,
-                                  BookmarkMenuDelegate::SHOW_PERMANENT_FOLDERS,
-                                  BOOKMARK_LAUNCH_LOCATION_NONE);
-  }
-
   BookmarkModel* model_;
 
   scoped_ptr<BookmarkMenuDelegate> bookmark_menu_delegate_;
@@ -91,8 +79,6 @@ class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
     model_->AddURL(of1, 0, ASCIIToUTF16("of1a"), GURL(test_base + "of1a"));
   }
 
-  views::MenuDelegate test_delegate_;
-
   DISALLOW_COPY_AND_ASSIGN(BookmarkMenuDelegateTest);
 };
 
@@ -101,7 +87,6 @@ class BookmarkMenuDelegateTest : public BrowserWithTestWindowTest {
 TEST_F(BookmarkMenuDelegateTest, RemoveBookmarks) {
   views::MenuDelegate test_delegate;
   const BookmarkNode* node = model_->bookmark_bar_node()->GetChild(1);
-  NewDelegate(0, kint32max);
   bookmark_menu_delegate_->Init(&test_delegate, NULL, node, 0,
                                 BookmarkMenuDelegate::HIDE_PERMANENT_FOLDERS,
                                 BOOKMARK_LAUNCH_LOCATION_NONE);
@@ -110,111 +95,4 @@ TEST_F(BookmarkMenuDelegateTest, RemoveBookmarks) {
   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.
 }

chrome/browser/ui/views/wrench_menu.h

@@ -59,12 +59,11 @@ class WrenchMenu : public views::MenuDelegate,
   void RemoveObserver(WrenchMenuObserver* observer);
 
   // MenuDelegate overrides:
-  virtual const gfx::Font* GetLabelFont(int command_id) const OVERRIDE;
+  virtual const gfx::Font* GetLabelFont(int index) const OVERRIDE;
   virtual bool GetForegroundColor(int command_id,
                                   bool is_hovered,
                                   SkColor* override_color) const OVERRIDE;
-  virtual string16 GetTooltipText(int command_id,
-                                  const gfx::Point& p) const OVERRIDE;
+  virtual string16 GetTooltipText(int id, const gfx::Point& p) const OVERRIDE;
   virtual bool IsTriggerableEvent(views::MenuItemView* menu,
                                   const ui::Event& e) OVERRIDE;
   virtual bool GetDropFormats(
@@ -81,7 +80,7 @@ class WrenchMenu : public views::MenuDelegate,
                             DropPosition position,
                             const ui::DropTargetEvent& event) OVERRIDE;
   virtual bool ShowContextMenu(views::MenuItemView* source,
-                               int command_id,
+                               int id,
                                const gfx::Point& p,
                                ui::MenuSourceType source_type) OVERRIDE;
   virtual bool CanDrag(views::MenuItemView* menu) OVERRIDE;
@@ -89,11 +88,10 @@ class WrenchMenu : public views::MenuDelegate,
                              ui::OSExchangeData* data) OVERRIDE;
   virtual int GetDragOperations(views::MenuItemView* sender) OVERRIDE;
   virtual int GetMaxWidthForMenu(views::MenuItemView* menu) OVERRIDE;
-  virtual bool IsItemChecked(int command_id) const OVERRIDE;
-  virtual bool IsCommandEnabled(int command_id) const OVERRIDE;
-  virtual void ExecuteCommand(int command_id, int mouse_event_flags) OVERRIDE;
-  virtual bool GetAccelerator(int command_id,
-                              ui::Accelerator* accelerator) OVERRIDE;
+  virtual bool IsItemChecked(int id) const OVERRIDE;
+  virtual bool IsCommandEnabled(int id) const OVERRIDE;
+  virtual void ExecuteCommand(int id, int mouse_event_flags) OVERRIDE;
+  virtual bool GetAccelerator(int id, ui::Accelerator* accelerator) OVERRIDE;
   virtual void WillShowMenu(views::MenuItemView* menu) OVERRIDE;
   virtual void WillHideMenu(views::MenuItemView* menu) OVERRIDE;
 
@@ -111,24 +109,26 @@ class WrenchMenu : public views::MenuDelegate,
   class ZoomView;
 
   typedef std::pair<ui::MenuModel*,int> Entry;
-  typedef std::map<int,Entry> CommandIDToEntry;
+  typedef std::map<int,Entry> IDToEntry;
 
   const ui::NativeTheme* GetNativeTheme() const;
 
   // Populates |parent| with all the child menus in |model|. Recursively invokes
-  // |PopulateMenu| for any submenu.
+  // |PopulateMenu| for any submenu. |next_id| is incremented for every menu
+  // that is created.
   void PopulateMenu(views::MenuItemView* parent,
-                    ui::MenuModel* model);
+                    ui::MenuModel* model,
+                    int* next_id);
 
   // Adds a new menu to |parent| to represent the MenuModel/index pair passed
-  // in.  The returned item's MenuItemView::GetCommand() is the same as that of
-  // |model|->GetCommandIdAt(|index|).
+  // in.
   // Fur button containing menu items a |height| override can be specified with
   // a number bigger then 0.
   views::MenuItemView* AppendMenuItem(views::MenuItemView* parent,
                                       ui::MenuModel* model,
                                       int index,
                                       ui::MenuModel::ItemType menu_type,
+                                      int* next_id,
                                       int height);
 
   // Invoked from the cut/copy/paste menus. Cancels the current active menu and
@@ -139,18 +139,26 @@ class WrenchMenu : public views::MenuDelegate,
   // the bookmark model isn't loaded.
   void CreateBookmarkMenu();
 
-  // Returns the index of the MenuModel/index pair representing the |command_id|
-  // in |command_id_to_entry_|.
-  int ModelIndexFromCommandId(int command_id) const;
+  // Returns true if |id| identifies a bookmark menu item.
+  bool is_bookmark_command(int id) const {
+    return bookmark_menu_delegate_.get() && id >= first_bookmark_command_id_;
+  }
+
+  // Returns true if |id| identifies a recent tabs menu item.
+  bool is_recent_tabs_command(int id) const {
+    return (recent_tabs_menu_model_delegate_.get() &&
+            id >= first_recent_tabs_command_id_ &&
+            id <= last_recent_tabs_command_id_);
+  }
 
   // The views menu. Owned by |menu_runner_|.
   views::MenuItemView* root_;
 
   scoped_ptr<views::MenuRunner> menu_runner_;
 
-  // Maps from the command ID in model to the model/index pair the item came
-  // from.
-  CommandIDToEntry command_id_to_entry_;
+  // Maps from the ID as understood by MenuItemView to the model/index pair the
+  // item came from.
+  IDToEntry id_to_entry_;
 
   // Browser the menu is being shown for.
   Browser* browser_;
@@ -174,6 +182,13 @@ class WrenchMenu : public views::MenuDelegate,
   // Used for managing "Recent tabs" menu items.
   scoped_ptr<RecentTabsMenuModelDelegate> recent_tabs_menu_model_delegate_;
 
+  // First ID to use for the items representing bookmarks in the bookmark menu.
+  int first_bookmark_command_id_;
+
+  // First/last IDs to use for the items of the recent tabs sub-menu.
+  int first_recent_tabs_command_id_;
+  int last_recent_tabs_command_id_;
+
   content::NotificationRegistrar registrar_;
 
   const bool use_new_menu_;