Make back forward menu model a MenuModel....

chrome/browser/back_forward_menu_model.cc

Index: chrome/browser/back_forward_menu_model.cc
===================================================================
--- chrome/browser/back_forward_menu_model.cc	(revision 35347)
+++ chrome/browser/back_forward_menu_model.cc	(working copy)
@@ -28,6 +28,136 @@
       model_type_(model_type) {
 }
 
+bool BackForwardMenuModel::HasIcons() const {
+  return true;
+}
+
+int BackForwardMenuModel::GetItemCount() const {
+  return GetTotalItemCount();
+}
+
+menus::MenuModel::ItemType BackForwardMenuModel::GetTypeAt(int index) const {
+  return IsSeparator(index) ? TYPE_SEPARATOR : TYPE_COMMAND;
+}
+
+int BackForwardMenuModel::GetCommandIdAt(int index) const {
+  return index;
+}
+
+string16 BackForwardMenuModel::GetLabelAt(int index) const {
+  // Return label "Show Full History" for the last item of the menu.
+  if (index == GetTotalItemCount() - 1)
+    return l10n_util::GetStringUTF16(IDS_SHOWFULLHISTORY_LINK);
+
+  // Return an empty string for a separator.
+  if (IsSeparator(index))
+    return string16();
+
+  NavigationEntry* entry = GetNavigationEntry(index);
+  return entry->GetTitleForDisplay(&GetTabContents()->controller());
+}
+
+bool BackForwardMenuModel::IsLabelDynamicAt(int index) const {
+  // This object is only used for a single showing of a menu.
+  return false;
+}
+
+bool BackForwardMenuModel::GetAcceleratorAt(
+    int index,
+    menus::Accelerator* accelerator) const {
+  return false;
+}
+
+bool BackForwardMenuModel::IsItemCheckedAt(int index) const {
+  NOTREACHED();
+  return false;
+}
+
+int BackForwardMenuModel::GetGroupIdAt(int index) const {
+  NOTREACHED();
+  return false;
+}
+
+bool BackForwardMenuModel::GetIconAt(int index, SkBitmap* icon) const {
+  if (!ItemHasIcon(index))
+    return false;
+
+  if (index == GetTotalItemCount() - 1) {
+    *icon = *ResourceBundle::GetSharedInstance().GetBitmapNamed(
+        IDR_HISTORY_FAVICON);
+  } else {
+    NavigationEntry* entry = GetNavigationEntry(index);
+    *icon = entry->favicon().bitmap();
+  }
+
+  return true;
+}
+
+bool BackForwardMenuModel::IsEnabledAt(int index) const {
+  return index < GetTotalItemCount() && !IsSeparator(index);

[viettrungluu, 2009/12/30 00:54:26]

It seems to me that |index < GetTotalItemCount()| should always be true, in
which case maybe this condition should just be DCHECK()ed? (If we're paranoid,
we might want DCHECK()s on other instances of |index| as well.)

[Evan Stade, 2009/12/30 01:12:10]

just copying the existing code.

+}
+
+menus::MenuModel* BackForwardMenuModel::GetSubmenuModelAt(int index) const {
+  return NULL;

[viettrungluu, 2009/12/30 00:54:26]

NOTREACHED()?

[Evan Stade, 2009/12/30 01:12:10]

this is not new code; it is taken from BackForwardMenuModelViews

also it appears to be the standard to leave it without a NOTREACHED()---perhaps
some code somewhere calls it without checking the type of the menu item.

[viettrungluu, 2009/12/30 01:20:34]

To be clear, I'm not blaming you for the code. I just like shooting messengers,
so to speak. ;-)
Fair enough, though some methods get a NOTREACHED() (e.g., |IsItemCheckedAt()|),
hence my confusion.

+}
+
+void BackForwardMenuModel::HighlightChangedTo(int index) {
+}
+
+void BackForwardMenuModel::ActivatedAt(int index) {
+  NavigationController& controller = GetTabContents()->controller();
+
+  DCHECK(!IsSeparator(index));
+
+  // Execute the command for the last item: "Show Full History".
+  if (index == GetTotalItemCount() - 1) {
+    UserMetrics::RecordComputedAction(BuildActionName("ShowFullHistory", -1),
+                                      controller.profile());
+    browser_->ShowSingleDOMUITab(GURL(chrome::kChromeUIHistoryURL));
+    return;
+  }
+
+  // Log whether it was a history or chapter click.
+  if (index < GetHistoryItemCount()) {
+    UserMetrics::RecordComputedAction(
+        BuildActionName("HistoryClick", index), controller.profile());
+  } else {
+    UserMetrics::RecordComputedAction(
+        BuildActionName("ChapterClick", index - GetHistoryItemCount() - 1),
+        controller.profile());
+  }
+
+  int controller_index = MenuIdToNavEntryIndex(index);
+  if (controller_index >= 0 && controller_index < controller.entry_count())
+    controller.GoToIndex(controller_index);
+  else
+    NOTREACHED();
+}
+
+void BackForwardMenuModel::MenuWillShow() {
+  UserMetrics::RecordComputedAction(BuildActionName("Popup", -1),

[viettrungluu, 2009/12/30 00:54:26]

Really? (I mean this in the sense that "Popup" doesn't seem very descriptive,
and also that I'm not sure this is something we want to record.)

[Evan Stade, 2009/12/30 01:12:10]

this is not new code; it is taken from BackForwardMenuModelViews

+                                    browser_->profile());
+}
+
+bool BackForwardMenuModel::IsSeparator(int index) const {
+  int history_items = GetHistoryItemCount();
+  // If the index is past the number of history items + separator,
+  // we then consider if it is a chapter-stop entry.
+  if (index > history_items) {
+    // We either are in ChapterStop area, or at the end of the list (the "Show
+    // Full History" link).
+    int chapter_stops = GetChapterStopCount(history_items);
+    if (chapter_stops == 0)
+      return false;  // We must have reached the "Show Full History" link.
+    // Otherwise, look to see if we have reached the separator for the
+    // chapter-stops. If not, this is a chapter stop.
+    return (index == history_items + 1 + chapter_stops);
+  }
+
+  // Look to see if we have reached the separator for the history items.
+  return index == history_items;
+}
+
 int BackForwardMenuModel::GetHistoryItemCount() const {
   TabContents* contents = GetTabContents();
   int items = 0;
@@ -156,88 +286,14 @@
   return entry;
 }
 
-void BackForwardMenuModel::ExecuteCommandById(int menu_id) {
-  TabContents* contents = GetTabContents();
-  NavigationController& controller = contents->controller();
-
-  DCHECK(!IsSeparator(menu_id));
-
-  // Execute the command for the last item: "Show Full History".
-  if (menu_id == GetTotalItemCount()) {
-    UserMetrics::RecordComputedAction(BuildActionName("ShowFullHistory", -1),
-                                      controller.profile());
-    browser_->ShowSingleDOMUITab(GURL(chrome::kChromeUIHistoryURL));
-    return;
-  }
-
-  // Log whether it was a history or chapter click.
-  if (menu_id <= GetHistoryItemCount()) {
-    UserMetrics::RecordComputedAction(
-        BuildActionName("HistoryClick", menu_id), controller.profile());
-  } else {
-    UserMetrics::RecordComputedAction(
-        BuildActionName("ChapterClick", menu_id - GetHistoryItemCount() - 1),
-        controller.profile());
-  }
-
-  int index = MenuIdToNavEntryIndex(menu_id);
-  if (index >= 0 && index < controller.entry_count())
-    controller.GoToIndex(index);
+bool BackForwardMenuModel::ItemHasCommand(int index) const {
+  return !IsSeparator(index) && index < GetTotalItemCount();

[viettrungluu, 2009/12/30 00:54:26]

What I previously said about |index < GetTotalItemCount()|....

[Evan Stade, 2009/12/30 01:12:10]

ditto

 }
 
-bool BackForwardMenuModel::IsSeparator(int menu_id) const {
-  int history_items = GetHistoryItemCount();
-  // If the menu_id is higher than the number of history items + separator,
-  // we then consider if it is a chapter-stop entry.
-  if (menu_id > history_items + 1) {
-    // We either are in ChapterStop area, or at the end of the list (the "Show
-    // Full History" link).
-    int chapter_stops = GetChapterStopCount(history_items);
-    if (chapter_stops == 0)
-      return false;  // We must have reached the "Show Full History" link.
-    // Otherwise, look to see if we have reached the separator for the
-    // chapter-stops. If not, this is a chapter stop.
-    return (menu_id == history_items + 1 +
-                       chapter_stops + 1);
-  }
-
-  // Look to see if we have reached the separator for the history items.
-  return menu_id == history_items + 1;
+bool BackForwardMenuModel::ItemHasIcon(int index) const {
+  return index < GetTotalItemCount() && !IsSeparator(index);

[viettrungluu, 2009/12/30 00:54:26]

Ditto (and if you keep it this way, could you make the order of the checks
consistent?).

[Evan Stade, 2009/12/30 01:12:10]

Done.

 }
 
-string16 BackForwardMenuModel::GetItemLabel(int menu_id) const {
-  // Return label "Show Full History" for the last item of the menu.
-  if (menu_id == GetTotalItemCount())
-    return l10n_util::GetStringUTF16(IDS_SHOWFULLHISTORY_LINK);
-
-  // Return an empty string for a separator.
-  if (IsSeparator(menu_id))
-    return string16();
-
-  NavigationEntry* entry = GetNavigationEntry(menu_id);
-  return entry->GetTitleForDisplay(&GetTabContents()->controller());
-}
-
-const SkBitmap& BackForwardMenuModel::GetItemIcon(int menu_id) const {
-  DCHECK(ItemHasIcon(menu_id));
-
-  if (menu_id == GetTotalItemCount()) {
-    return *ResourceBundle::GetSharedInstance().GetBitmapNamed(
-        IDR_HISTORY_FAVICON);
-  }
-
-  NavigationEntry* entry = GetNavigationEntry(menu_id);
-  return entry->favicon().bitmap();
-}
-
-bool BackForwardMenuModel::ItemHasIcon(int menu_id) const {
-  return menu_id - 1 < GetTotalItemCount() && !IsSeparator(menu_id);
-}
-
-bool BackForwardMenuModel::ItemHasCommand(int menu_id) const {
-  return menu_id - 1 < GetTotalItemCount() && !IsSeparator(menu_id);
-}
-
 string16 BackForwardMenuModel::GetShowFullHistoryLabel() const {
   return l10n_util::GetStringUTF16(IDS_SHOWFULLHISTORY_LINK);
 }
@@ -248,40 +304,44 @@
                               browser_->GetSelectedTabContents();
 }
 
-int BackForwardMenuModel::MenuIdToNavEntryIndex(int menu_id) const {
+int BackForwardMenuModel::MenuIdToNavEntryIndex(int index) const {

[Evan Martin, 2009/12/30 19:29:30]

This is a bit weird, that the function name says it takes an id and you renamed
it to "index".

[Evan Stade, 2009/12/30 20:32:30]

will rename the function

   TabContents* contents = GetTabContents();
   int history_items = GetHistoryItemCount();
 
-  DCHECK(menu_id > 0);
+  DCHECK_GE(index, 0);
 
   // Convert anything above the History items separator.
-  if (menu_id <= history_items) {
+  if (index < history_items) {
     if (model_type_ == FORWARD_MENU) {
-      // The |menu_id| is relative to our current position, so we need to add.
-      menu_id += contents->controller().GetCurrentEntryIndex();
+      index += contents->controller().GetCurrentEntryIndex() + 1;
     } else {
       // Back menu is reverse.
-      menu_id = contents->controller().GetCurrentEntryIndex() - menu_id;
+      index = contents->controller().GetCurrentEntryIndex() - (index + 1);
     }
-    return menu_id;
+    return index;
   }
-  if (menu_id == history_items + 1)
-    return -1;           // Don't translate the separator for history items.
+  if (index == history_items)
+    return -1;  // Don't translate the separator for history items.
 
-  if (menu_id >= history_items + 1 + GetChapterStopCount(history_items) + 1)
-    return -1;           // This is beyond the last chapter stop so we abort.
+  if (index >= history_items + 1 + GetChapterStopCount(history_items))
+    return -1;  // This is beyond the last chapter stop so we abort.
 
   // This menu item is a chapter stop located between the two separators.
-  menu_id = FindChapterStop(history_items,
-                            model_type_ == FORWARD_MENU,
-                            menu_id - history_items - 1 - 1);
+  index = FindChapterStop(history_items,
+                          model_type_ == FORWARD_MENU,
+                          index - history_items - 1);
 
-  return menu_id;
+  return index;
 }
 
-NavigationEntry* BackForwardMenuModel::GetNavigationEntry(int menu_id) const {
-  int index = MenuIdToNavEntryIndex(menu_id);
-  return GetTabContents()->controller().GetEntryAtIndex(index);
+NavigationEntry* BackForwardMenuModel::GetNavigationEntry(int index) const {
+  int controller_index = MenuIdToNavEntryIndex(index);
+  NavigationController& controller = GetTabContents()->controller();
+  if (controller_index >= 0 && controller_index < controller.entry_count())
+    return controller.GetEntryAtIndex(controller_index);
+
+  NOTREACHED();
+  return NULL;
 }
 
 std::string BackForwardMenuModel::BuildActionName(
@@ -294,7 +354,9 @@
   else
     metric_string += "BackMenu_";
   metric_string += action;
-  if (index != -1)
-    metric_string += IntToString(index);
+  if (index != -1) {
+    // +1 is for historical reasons (indices used to start at 1).
+    metric_string += IntToString(index + 1);
+  }
   return metric_string;
 }