Make back forward menu model a MenuModel....

chrome/browser/back_forward_menu_model.h

-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_BACK_FORWARD_MENU_MODEL_H_
 #define CHROME_BROWSER_BACK_FORWARD_MENU_MODEL_H_
 
 #include <string>
 
+#include "app/menus/menu_model.h"
 #include "base/basictypes.h"
 #include "base/string16.h"
+#include "testing/gtest/include/gtest/gtest_prod.h"  // For FRIEND_TEST
 
 class Browser;
 class SkBitmap;
 class TabContents;
 class NavigationEntry;
 
 ///////////////////////////////////////////////////////////////////////////////
 //
 // BackForwardMenuModel
 //
 // Interface for the showing of the dropdown menu for the Back/Forward buttons.
 // Actual implementations are platform-specific.
 ///////////////////////////////////////////////////////////////////////////////
-class BackForwardMenuModel {
+class BackForwardMenuModel : public menus::MenuModel {
  public:
   // These are IDs used to identify individual UI elements within the
   // browser window using View::GetViewByID.
   enum ModelType {
     FORWARD_MENU = 1,
     BACKWARD_MENU = 2
   };
 
   BackForwardMenuModel(Browser* browser, ModelType model_type);
   virtual ~BackForwardMenuModel() { }
 
+  // MenuModel implementation.
+  virtual bool HasIcons() const;
+  virtual int GetItemCount() const;
+  virtual ItemType GetTypeAt(int index) const;
+  virtual int GetCommandIdAt(int index) const;
+  virtual string16 GetLabelAt(int index) const;
+  virtual bool IsLabelDynamicAt(int index) const;
+  virtual bool GetAcceleratorAt(int index,
+                                menus::Accelerator* accelerator) const;
+  virtual bool IsItemCheckedAt(int index) const;
+  virtual int GetGroupIdAt(int index) const;
+  virtual bool GetIconAt(int index, SkBitmap* icon) const;
+  virtual bool IsEnabledAt(int index) const;
+  virtual MenuModel* GetSubmenuModelAt(int index) const;
+  virtual void HighlightChangedTo(int index);
+  virtual void ActivatedAt(int index);
+  virtual void MenuWillShow();
+
+  // Is the item at |index| a separator?
+  bool IsSeparator(int index) const;
+
+ private:
+  // Allows the unit test to use its own dummy tab contents.
+  void set_test_tab_contents(TabContents* test_tab_contents) {
+    test_tab_contents_ = test_tab_contents;
+  }
+
   // Returns how many history items the menu should show. For example, if the
   // navigation controller of the current tab has a current entry index of 5 and
   // forward_direction_ is false (we are the back button delegate) then this
   // function will return 5 (representing 0-4). If forward_direction_ is
   // true (we are the forward button delegate), then this function will return
   // the number of entries after 5. Note, though, that in either case it will
   // not report more than kMaxHistoryItems. The number returned also does not
   // include the separator line after the history items (nor the separator for
   // the "Show Full History" link).
   int GetHistoryItemCount() const;
 
   // Returns how many chapter-stop items the menu should show. For the
   // definition of a chapter-stop, see GetIndexOfNextChapterStop(). The number
   // returned does not include the separator lines before and after the
   // chapter-stops.
   int GetChapterStopCount(int history_items) const;
 
   // Returns how many items the menu should show, including history items,
   // chapter-stops, separators and the Show Full History link. This function
   // uses GetHistoryItemCount() and GetChapterStopCount() internally to figure
   // out the total number of items to show.
   int GetTotalItemCount() const;

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

Is there a reason why this shouldn't just be replaced with |GetItemCount()|
(whose implementation just calls this)?

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

yes.

1. I was trying to reduce the number of lines changed in this changelist, and
between the unit test and the implementation renaming this function required
changing about 20 lines of code where the logic didn't change at all
2. The comment explaining its implementation makes a lot more sense here, among
related functions, than it does above, among unrelated functions.

but I see that it could cause confusion, so I went ahead and made the change

 
   // Finds the next chapter-stop in the NavigationEntryList starting from
   // the index specified in |start_from| and continuing in the direction
   // specified (|forward|) until either a chapter-stop is found or we reach the
   // end, in which case -1 is returned. If |start_from| is out of bounds, -1
   // will also be returned. A chapter-stop is defined as the last page the user
   // browsed to within the same domain. For example, if the user's homepage is
   // Google and she navigates to Google pages G1, G2 and G3 before heading over
   // to WikiPedia for pages W1 and W2 and then back to Google for pages G4 and
   // G5 then G3, W2 and G5 are considered chapter-stops. The return value from
   // this function is an index into the NavigationEntryList vector.
   int GetIndexOfNextChapterStop(int start_from, bool forward) const;
 
   // Finds a given chapter-stop starting at the currently active entry in the
   // NavigationEntryList vector advancing first forward or backward by |offset|
   // (depending on the direction specified in parameter |forward|). It also
   // allows you to skip chapter-stops by specifying a positive value for |skip|.
   // Example: FindChapterStop(5, false, 3) starts with the currently active
   // index, subtracts 5 from it and then finds the fourth chapter-stop before
   // that index (skipping the first 3 it finds).
   // Example: FindChapterStop(0, true, 0) is functionally equivalent to
   // calling GetIndexOfNextChapterStop(GetCurrentEntryIndex(), true).
   //
   // NOTE: Both |offset| and |skip| must be non-negative. The return value from
   // this function is an index into the NavigationEntryList vector. If |offset|
   // is out of bounds or if we skip too far (run out of chapter-stops) this
   // function returns -1.
   int FindChapterStop(int offset, bool forward, int skip) const;
 
-  // Execute the command associated with |menu_id|.
-  void ExecuteCommandById(int menu_id);
-
-  // Is the item at |menu_id| a separator?
-  bool IsSeparator(int menu_id) const;
-
-  // Get the display text for the item. This should not be called on a
-  // separator.
-  string16 GetItemLabel(int menu_id) const;
-
-  // Get the display icon for the item. This should not be called on a
-  // separator or an item that does not have an icon.
-  const SkBitmap& GetItemIcon(int menu_id) const;
-
-  // Returns true if there is an icon for this menu item.
-  bool ItemHasIcon(int menu_id) const;
-
-  // Does the item does something when you click on it?
-  bool ItemHasCommand(int menu_id) const;
-
-#ifdef UNIT_TEST
-  // Allows the unit test to use its own dummy tab contents.
-  void set_test_tab_contents(TabContents* test_tab_contents) {
-    test_tab_contents_ = test_tab_contents;
-  }
-#endif
-
-  // Allow the unit test to use the "Show Full History" label.
-  string16 GetShowFullHistoryLabel() const;
-
-  // Retrieves the TabContents pointer to use, which is either the one that
-  // the unit test sets (using SetTabContentsForUnitTest) or the one from
-  // the browser window.
-  TabContents* GetTabContents() const;
-
   // How many items (max) to show in the back/forward history menu dropdown.
   static const int kMaxHistoryItems;
 
   // How many chapter-stops (max) to show in the back/forward dropdown list.
   static const int kMaxChapterStops;
 
- protected:
   // Converts a menu item id, as passed in through one of the menu delegate
   // functions and converts it into an absolute index into the
-  // NavigationEntryList vector. |menu_id| can point to a separator, or the
+  // NavigationEntryList vector. |index| can point to a separator, or the
   // "Show Full History" link in which case this function returns -1.
-  int MenuIdToNavEntryIndex(int menu_id) const;
+  int MenuIdToNavEntryIndex(int index) const;
+
+  // Does the item have a command associated with it?
+  bool ItemHasCommand(int index) const;
+
+  // Returns true if there is an icon for this menu item.
+  bool ItemHasIcon(int index) const;
+
+  // Allow the unit test to use the "Show Full History" label.
+  string16 GetShowFullHistoryLabel() const;
 
   // Looks up a NavigationEntry by menu id.
-  NavigationEntry* GetNavigationEntry(int menu_id) const;
+  NavigationEntry* GetNavigationEntry(int index) const;
+
+  // Retrieves the TabContents pointer to use, which is either the one that
+  // the unit test sets (using SetTabContentsForUnitTest) or the one from
+  // the browser window.
+  TabContents* GetTabContents() const;
 
   // Build a string version of a user action on this menu, used as an
   // identifier for logging user behavior.
   // E.g. BuildActionName("Click", 2) returns "BackMenu_Click2".
   // An index of -1 means no index.
   std::string BuildActionName(const std::string& name, int index) const;
 
   Browser* browser_;
 
   // The unit tests will provide their own TabContents to use.
   TabContents* test_tab_contents_;
 
   // Represents whether this is the delegate for the forward button or the
   // back button.
   ModelType model_type_;
 
- private:
+  friend class BackFwdMenuModelTest;
+  FRIEND_TEST(BackFwdMenuModelTest, BasicCase);
+  FRIEND_TEST(BackFwdMenuModelTest, MaxItemsTest);
+  FRIEND_TEST(BackFwdMenuModelTest, ChapterStops);
+
   DISALLOW_COPY_AND_ASSIGN(BackForwardMenuModel);
 };
 
 #endif  // CHROME_BROWSER_BACK_FORWARD_MENU_MODEL_H_