Revert 226778 "views: change WrenchMenu to use each model's comm..."

that CL added atatic initalizer in recent_tabs_sub_menu_model.cc.

> views: change WrenchMenu to use each model's command ID's when creating MenuItemView's
> 
> 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.
> 
> BUG=256750
> TEST=wrench menu and all its submenus still work.
> 
> Review URL: https://codereview.chromium.org/25811003

TBR=kuan@chromium.org

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

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

7 files changed, 141 insertions, 164 deletions

diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
index 064e600f..0ab3291 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
+++ b/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.
diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
index 691b551..62d9a07 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
+++ b/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;
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.cc b/chrome/browser/ui/toolbar/wrench_menu_model.cc
index c51d0ab..19571ab 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.cc
+++ b/chrome/browser/ui/toolbar/wrench_menu_model.cc
@@ -227,11 +227,6 @@ void ToolsMenuModel::Build(Browser* browser) {
 ////////////////////////////////////////////////////////////////////////////////
 // WrenchMenuModel
 
-const int WrenchMenuModel::kMinBookmarkCommandId = 1;
-const int WrenchMenuModel::kMaxBookmarkCommandId = 1000;
-const int WrenchMenuModel::kMinRecentTabsCommandId = 1001;
-const int WrenchMenuModel::kMaxRecentTabsCommandId = 1200;
-
 WrenchMenuModel::WrenchMenuModel(ui::AcceleratorProvider* provider,
                                  Browser* browser,
                                  bool is_new_menu)
diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.h b/chrome/browser/ui/toolbar/wrench_menu_model.h
index 70f8597..1bf9bb0 100644
--- a/chrome/browser/ui/toolbar/wrench_menu_model.h
+++ b/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;
-  static const int kMaxBookmarkCommandId;
-
-  // 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;
-  static const int kMaxRecentTabsCommandId;
-
   // TODO: remove |is_new_menu|.
   WrenchMenuModel(ui::AcceleratorProvider* provider,
                   Browser* browser,
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
index 1b9434a..908be83 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc
@@ -12,7 +12,6 @@
 #include "chrome/browser/ui/bookmarks/bookmark_drag_drop.h"
 #include "chrome/browser/ui/bookmarks/bookmark_utils.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/toolbar/wrench_menu_model.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.h"
 #include "chrome/browser/ui/views/event_utils.h"
@@ -457,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;
-    if (location_ == BOOKMARK_LAUNCH_LOCATION_WRENCH_MENU &&
-        id > WrenchMenuModel::kMaxBookmarkCommandId)
-      break;
-
     (*next_menu_id)++;
 
     menu_id_to_node_map_[id] = node;
diff --git a/chrome/browser/ui/views/wrench_menu.cc b/chrome/browser/ui/views/wrench_menu.cc
index 44cad3f..d82ec3d 100644
--- a/chrome/browser/ui/views/wrench_menu.cc
+++ b/chrome/browser/ui/views/wrench_menu.cc
@@ -20,7 +20,6 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/browser/ui/toolbar/wrench_menu_model.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.h"
 #include "chrome/browser/ui/views/wrench_menu_observer.h"
 #include "content/public/browser/host_zoom_map.h"
@@ -89,18 +88,6 @@ const int kHorizontalTouchPadding = 15;
 // Menu items which have embedded buttons should have this height in pixel.
 const int kMenuItemContainingButtonsHeight = 43;
 
-// Returns true if |command_id| identifies a bookmark menu item.
-bool IsBookmarkCommand(int command_id) {
-  return command_id >= WrenchMenuModel::kMinBookmarkCommandId &&
-      command_id <= WrenchMenuModel::kMaxBookmarkCommandId;
-}
-
-// Returns true if |command_id| identifies a recent tabs menu item.
-bool IsRecentTabsCommand(int command_id) {
-  return command_id >= WrenchMenuModel::kMinRecentTabsCommandId &&
-      command_id <= WrenchMenuModel::kMaxRecentTabsCommandId;
-}
-
 // Subclass of ImageButton whose preferred size includes the size of the border.
 class FullscreenButton : public ImageButton {
  public:
@@ -424,8 +411,8 @@ class ButtonContainerMenuItemView : public MenuItemView {
  public:
   // Constructor for use with button containing menu items which have a
   // different height then normal items.
-  ButtonContainerMenuItemView(MenuItemView* parent, int command_id, int height)
-      : MenuItemView(parent, command_id, MenuItemView::NORMAL),
+  ButtonContainerMenuItemView(MenuItemView* parent, int id, int height)
+      : MenuItemView(parent, id, MenuItemView::NORMAL),
         height_(height) {
   };
 
@@ -777,7 +764,10 @@ class WrenchMenu::RecentTabsMenuModelDelegate : public ui::MenuModelDelegate {
 
   // ui::MenuModelDelegate implementation:
   virtual void OnIconChanged(int index) OVERRIDE {
-    int command_id = model_->GetCommandIdAt(index);
+    // |index| specifies position in children items of |menu_item_| starting at
+    // 0, its corresponding command id as used in the children menu item views
+    // follows that of the parent menu item view |menu_item_|.
+    int command_id = menu_item_->GetCommand() + 1 + index;
     views::MenuItemView* item = menu_item_->GetMenuItemByID(command_id);
     DCHECK(item);
     gfx::Image icon;
@@ -792,7 +782,9 @@ class WrenchMenu::RecentTabsMenuModelDelegate : public ui::MenuModelDelegate {
     if (!submenu)
       return -1;
     const int kMaxMenuItemWidth = 320;
-    return menu->GetCommand() == menu_item_->GetCommand() ?
+    return menu->GetCommand() >= menu_item_->GetCommand() &&
+        menu->GetCommand() <=
+            menu_item_->GetCommand() + submenu->GetMenuItemCount() ?
         kMaxMenuItemWidth : -1;
   }
 
@@ -800,11 +792,11 @@ class WrenchMenu::RecentTabsMenuModelDelegate : public ui::MenuModelDelegate {
     return model_->GetLabelFontAt(index);
   }
 
-  bool GetForegroundColorAt(int index,
-                            bool is_hovered,
-                            SkColor* override_color) const {
+  bool GetForegroundColor(int command_id,
+                          bool is_hovered,
+                          SkColor* override_color) const {
     // The items for which we get a font, should be shown in black.
-    if (GetLabelFontAt(index)) {
+    if (GetLabelFontAt(command_id)) {
       *override_color = SK_ColorBLACK;
       return true;
     }
@@ -829,6 +821,9 @@ WrenchMenu::WrenchMenu(Browser* browser,
       selected_index_(0),
       bookmark_menu_(NULL),
       feedback_menu_item_(NULL),
+      first_bookmark_command_id_(0),
+      first_recent_tabs_command_id_(-1),
+      last_recent_tabs_command_id_(-1),
       use_new_menu_(use_new_menu),
       supports_new_separators_(supports_new_separators) {
   registrar_.Add(this, chrome::NOTIFICATION_GLOBAL_ERRORS_CHANGED,
@@ -850,15 +845,9 @@ void WrenchMenu::Init(ui::MenuModel* model) {
   root_ = new MenuItemView(this);
   root_->set_has_icons(true);  // We have checks, radios and icons, set this
                                // so we get the taller menu style.
-  PopulateMenu(root_, model);
-
-#if defined(DEBUG)
-  // 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(DEBUG)
-
+  int next_id = 1;
+  PopulateMenu(root_, model, &next_id);
+  first_bookmark_command_id_ = next_id + 1;
   menu_runner_.reset(new views::MenuRunner(root_));
 }
 
@@ -905,10 +894,10 @@ void WrenchMenu::RemoveObserver(WrenchMenuObserver* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
-const gfx::Font* WrenchMenu::GetLabelFont(int command_id) const {
-  if (IsRecentTabsCommand(command_id)) {
+const gfx::Font* WrenchMenu::GetLabelFont(int index) const {
+  if (is_recent_tabs_command(index)) {
     return recent_tabs_menu_model_delegate_->GetLabelFontAt(
-        ModelIndexFromCommandId(command_id));
+        index - first_recent_tabs_command_id_);
   }
   return NULL;
 }
@@ -916,22 +905,24 @@ const gfx::Font* WrenchMenu::GetLabelFont(int command_id) const {
 bool WrenchMenu::GetForegroundColor(int command_id,
                                     bool is_hovered,
                                     SkColor* override_color) const {
-  if (IsRecentTabsCommand(command_id)) {
-    return recent_tabs_menu_model_delegate_->GetForegroundColorAt(
-        ModelIndexFromCommandId(command_id), is_hovered, override_color);
+  if (is_recent_tabs_command(command_id)) {
+    return recent_tabs_menu_model_delegate_->GetForegroundColor(
+        command_id - first_recent_tabs_command_id_,
+        is_hovered,
+        override_color);
   }
   return false;
 }
 
-string16 WrenchMenu::GetTooltipText(int command_id,
+string16 WrenchMenu::GetTooltipText(int id,
                                     const gfx::Point& p) const {
-  return IsBookmarkCommand(command_id) ?
-      bookmark_menu_delegate_->GetTooltipText(command_id, p) : string16();
+  return is_bookmark_command(id) ?
+      bookmark_menu_delegate_->GetTooltipText(id, p) : string16();
 }
 
 bool WrenchMenu::IsTriggerableEvent(views::MenuItemView* menu,
                                     const ui::Event& e) {
-  return IsBookmarkCommand(menu->GetCommand()) ?
+  return is_bookmark_command(menu->GetCommand()) ?
       bookmark_menu_delegate_->IsTriggerableEvent(menu, e) :
       MenuDelegate::IsTriggerableEvent(menu, e);
 }
@@ -962,7 +953,7 @@ int WrenchMenu::GetDropOperation(
     MenuItemView* item,
     const ui::DropTargetEvent& event,
     DropPosition* position) {
-  return IsBookmarkCommand(item->GetCommand()) ?
+  return is_bookmark_command(item->GetCommand()) ?
       bookmark_menu_delegate_->GetDropOperation(item, event, position) :
       ui::DragDropTypes::DRAG_NONE;
 }
@@ -970,7 +961,7 @@ int WrenchMenu::GetDropOperation(
 int WrenchMenu::OnPerformDrop(MenuItemView* menu,
                               DropPosition position,
                               const ui::DropTargetEvent& event) {
-  if (!IsBookmarkCommand(menu->GetCommand()))
+  if (!is_bookmark_command(menu->GetCommand()))
     return ui::DragDropTypes::DRAG_NONE;
 
   int result = bookmark_menu_delegate_->OnPerformDrop(menu, position, event);
@@ -978,34 +969,34 @@ int WrenchMenu::OnPerformDrop(MenuItemView* menu,
 }
 
 bool WrenchMenu::ShowContextMenu(MenuItemView* source,
-                                 int command_id,
+                                 int id,
                                  const gfx::Point& p,
                                  ui::MenuSourceType source_type) {
-  return IsBookmarkCommand(command_id) ?
-      bookmark_menu_delegate_->ShowContextMenu(source, command_id, p,
+  return is_bookmark_command(id) ?
+      bookmark_menu_delegate_->ShowContextMenu(source, id, p,
                                                source_type) :
       false;
 }
 
 bool WrenchMenu::CanDrag(MenuItemView* menu) {
-  return IsBookmarkCommand(menu->GetCommand()) ?
+  return is_bookmark_command(menu->GetCommand()) ?
       bookmark_menu_delegate_->CanDrag(menu) : false;
 }
 
 void WrenchMenu::WriteDragData(MenuItemView* sender,
                                ui::OSExchangeData* data) {
-  DCHECK(IsBookmarkCommand(sender->GetCommand()));
+  DCHECK(is_bookmark_command(sender->GetCommand()));
   return bookmark_menu_delegate_->WriteDragData(sender, data);
 }
 
 int WrenchMenu::GetDragOperations(MenuItemView* sender) {
-  return IsBookmarkCommand(sender->GetCommand()) ?
+  return is_bookmark_command(sender->GetCommand()) ?
       bookmark_menu_delegate_->GetDragOperations(sender) :
       MenuDelegate::GetDragOperations(sender);
 }
 
 int WrenchMenu::GetMaxWidthForMenu(MenuItemView* menu) {
-  if (IsBookmarkCommand(menu->GetCommand()))
+  if (is_bookmark_command(menu->GetCommand()))
     return bookmark_menu_delegate_->GetMaxWidthForMenu(menu);
   int max_width = -1;
   // If recent tabs menu is available, it will decide if |menu| is one of recent
@@ -1018,37 +1009,40 @@ int WrenchMenu::GetMaxWidthForMenu(MenuItemView* menu) {
   return max_width;
 }
 
-bool WrenchMenu::IsItemChecked(int command_id) const {
-  if (IsBookmarkCommand(command_id))
+bool WrenchMenu::IsItemChecked(int id) const {
+  if (is_bookmark_command(id))
     return false;
 
-  const Entry& entry = command_id_to_entry_.find(command_id)->second;
+  const Entry& entry = id_to_entry_.find(id)->second;
   return entry.first->IsItemCheckedAt(entry.second);
 }
 
-bool WrenchMenu::IsCommandEnabled(int command_id) const {
-  if (IsBookmarkCommand(command_id))
+bool WrenchMenu::IsCommandEnabled(int id) const {
+  if (is_bookmark_command(id))
     return true;
 
-  if (command_id == 0)
+  if (id == 0)
     return false;  // The root item.
 
+  const Entry& entry = id_to_entry_.find(id)->second;
+  int command_id = entry.first->GetCommandIdAt(entry.second);
   // The items representing the cut menu (cut/copy/paste) and zoom menu
   // (increment/decrement/reset) are always enabled. The child views of these
   // items enabled state updates appropriately.
-  if (command_id == IDC_CUT || command_id == IDC_ZOOM_MINUS)
-    return true;
-
-  const Entry& entry = command_id_to_entry_.find(command_id)->second;
-  return entry.first->IsEnabledAt(entry.second);
+  return command_id == IDC_CUT || command_id == IDC_ZOOM_MINUS ||
+      entry.first->IsEnabledAt(entry.second);
 }
 
-void WrenchMenu::ExecuteCommand(int command_id, int mouse_event_flags) {
-  if (IsBookmarkCommand(command_id)) {
-    bookmark_menu_delegate_->ExecuteCommand(command_id, mouse_event_flags);
+void WrenchMenu::ExecuteCommand(int id, int mouse_event_flags) {
+  if (is_bookmark_command(id)) {
+    bookmark_menu_delegate_->ExecuteCommand(id, mouse_event_flags);
     return;
   }
 
+  // Not a bookmark
+  const Entry& entry = id_to_entry_.find(id)->second;
+  int command_id = entry.first->GetCommandIdAt(entry.second);
+
   if (command_id == IDC_CUT || command_id == IDC_ZOOM_MINUS) {
     // These items are represented by child views. If ExecuteCommand is invoked
     // it means the user clicked on the area around the buttons and we should
@@ -1056,21 +1050,25 @@ void WrenchMenu::ExecuteCommand(int command_id, int mouse_event_flags) {
     return;
   }
 
-  const Entry& entry = command_id_to_entry_.find(command_id)->second;
   return entry.first->ActivatedAt(entry.second, mouse_event_flags);
 }
 
-bool WrenchMenu::GetAccelerator(int command_id, ui::Accelerator* accelerator) {
-  if (IsBookmarkCommand(command_id))
+bool WrenchMenu::GetAccelerator(int id, ui::Accelerator* accelerator) {
+  if (is_bookmark_command(id))
+    return false;
+  IDToEntry::iterator ix = id_to_entry_.find(id);
+  if (ix == id_to_entry_.end()) {
+    // There is no entry for this id.
     return false;
+  }
 
+  const Entry& entry = ix->second;
+  int command_id = entry.first->GetCommandIdAt(entry.second);
   if (command_id == IDC_CUT || command_id == IDC_ZOOM_MINUS) {
     // These have special child views; don't show the accelerator for them.
     return false;
   }
 
-  CommandIDToEntry::iterator ix = command_id_to_entry_.find(command_id);
-  const Entry& entry = ix->second;
   ui::Accelerator menu_accelerator;
   if (!entry.first->GetAcceleratorAt(entry.second, &menu_accelerator))
     return false;
@@ -1120,7 +1118,8 @@ void WrenchMenu::Observe(int type,
 }
 
 void WrenchMenu::PopulateMenu(MenuItemView* parent,
-                              MenuModel* model) {
+                              MenuModel* model,
+                              int* next_id) {
   for (int i = 0, max = model->GetItemCount(); i < max; ++i) {
     // The button container menu items have a special height which we have to
     // use instead of the normal height.
@@ -1131,10 +1130,17 @@ void WrenchMenu::PopulateMenu(MenuItemView* parent,
       height = kMenuItemContainingButtonsHeight;
 
     MenuItemView* item = AppendMenuItem(
-        parent, model, i, model->GetTypeAt(i), height);
-
-    if (model->GetTypeAt(i) == MenuModel::TYPE_SUBMENU)
-      PopulateMenu(item, model->GetSubmenuModelAt(i));
+        parent, model, i, model->GetTypeAt(i), next_id, height);
+
+    if (model->GetTypeAt(i) == MenuModel::TYPE_SUBMENU) {
+      bool is_recent_tabs_menu =
+          model->GetCommandIdAt(i) == IDC_RECENT_TABS_MENU;
+      if (is_recent_tabs_menu)
+        first_recent_tabs_command_id_ = *next_id;
+      PopulateMenu(item, model->GetSubmenuModelAt(i), next_id);
+      if (is_recent_tabs_menu)
+        last_recent_tabs_command_id_ = *next_id - 1;
+    }
 
     const ui::NativeTheme* native_theme = GetNativeTheme();
 
@@ -1189,34 +1195,23 @@ MenuItemView* WrenchMenu::AppendMenuItem(MenuItemView* parent,
                                          MenuModel* model,
                                          int index,
                                          MenuModel::ItemType menu_type,
+                                         int* next_id,
                                          int height) {
-  int command_id = model->GetCommandIdAt(index);
-  DCHECK(command_id > -1 ||
-         (command_id == -1 &&
-          model->GetTypeAt(index) == MenuModel::TYPE_SEPARATOR));
-
-  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
-    // in both wrench menu and RecentTabs submenu,
-    if (command_id != IDC_SHOW_HISTORY) {
-      DCHECK(command_id_to_entry_.find(command_id) ==
-             command_id_to_entry_.end())
-          << "command ID " << command_id << " already exists!";
-    }
-    command_id_to_entry_[command_id].first = model;
-    command_id_to_entry_[command_id].second = index;
-  }
+  int id = (*next_id)++;
+
+  id_to_entry_[id].first = model;
+  id_to_entry_[id].second = index;
 
   MenuItemView* menu_item = NULL;
   if (height > 0) {
     // For menu items with a special menu height we use our special class to be
     // able to modify the item height.
-    menu_item = new ButtonContainerMenuItemView(parent, command_id, height);
+    menu_item = new ButtonContainerMenuItemView(parent, id, height);
     parent->GetSubmenu()->AddChildView(menu_item);
   } else {
     // For all other cases we use the more generic way to add menu items.
     menu_item = views::MenuModelAdapter::AppendMenuItemFromModel(
-        model, index, parent, command_id);
+        model, index, parent, id);
   }
 
   if (menu_item) {
@@ -1258,7 +1253,7 @@ void WrenchMenu::CreateBookmarkMenu() {
       new BookmarkMenuDelegate(browser_,
                                browser_,
                                parent,
-                               WrenchMenuModel::kMinBookmarkCommandId));
+                               first_bookmark_command_id_));
   bookmark_menu_delegate_->Init(this,
                                 bookmark_menu_,
                                 model->bookmark_bar_node(),
@@ -1266,9 +1261,3 @@ void WrenchMenu::CreateBookmarkMenu() {
                                 BookmarkMenuDelegate::SHOW_PERMANENT_FOLDERS,
                                 BOOKMARK_LAUNCH_LOCATION_WRENCH_MENU);
 }
-
-int WrenchMenu::ModelIndexFromCommandId(int command_id) const {
-  CommandIDToEntry::const_iterator ix = command_id_to_entry_.find(command_id);
-  DCHECK(ix != command_id_to_entry_.end());
-  return ix->second.second;
-}
diff --git a/chrome/browser/ui/views/wrench_menu.h b/chrome/browser/ui/views/wrench_menu.h
index ca0d628..44613be 100644
--- a/chrome/browser/ui/views/wrench_menu.h
+++ b/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_;