Wrench menu reorg phase 2

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc

Index: chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
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 32df7dca80dff70f969368db9f6fdc30a107137a..5601972f7c07c62f0c914d5c55365414611cddbc 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
@@ -69,6 +69,10 @@ const int kMaxDeviceNameCommandId = 1110;
 // shown in the menu.
 const int kMaxLocalEntries = 8;
 
+// Starting index for local tab entries. Takes into account the history menu
+// item and separator.
+const int kLocalEntriesStartIndex = 1;

[Peter Kasting, 2015/06/17 22:59:42]

This seems like a technically-inaccurate name given that these entries actually
start at 2, not 1.

[edwardjung, 2015/06/18 16:52:05]

Renamed.

+
 // Comparator function for use with std::sort that will sort sessions by
 // descending modified_time (i.e., most recent first).
 bool SortSessionsByRecency(const browser_sync::SyncedSession* s1,
@@ -171,7 +175,7 @@ RecentTabsSubMenuModel::RecentTabsSubMenuModel(
     : ui::SimpleMenuModel(this),
       browser_(browser),
       open_tabs_delegate_(open_tabs_delegate),
-      last_local_model_index_(-1),
+      last_local_model_index_(kLocalEntriesStartIndex),
       default_favicon_(ui::ResourceBundle::GetSharedInstance().
                        GetNativeImageNamed(IDR_DEFAULT_FAVICON)),
       weak_ptr_factory_(this) {
@@ -208,8 +212,15 @@ RecentTabsSubMenuModel::RecentTabsSubMenuModel(
   if (accelerator_provider) {
     accelerator_provider->GetAcceleratorForCommandId(
         IDC_RESTORE_TAB, &reopen_closed_tab_accelerator_);
+    accelerator_provider->GetAcceleratorForCommandId(
+        IDC_SHOW_HISTORY, &show_history_accelerator_);

[Peter Kasting, 2015/06/17 22:59:42]

This block duplicates the one just below

[edwardjung, 2015/06/18 16:52:05]

Removed.

   }
 #endif  // defined(USE_ASH)
+
+  if (accelerator_provider) {
+    accelerator_provider->GetAcceleratorForCommandId(
+        IDC_SHOW_HISTORY, &show_history_accelerator_);
+  }
 }
 
 RecentTabsSubMenuModel::~RecentTabsSubMenuModel() {
@@ -244,7 +255,11 @@ bool RecentTabsSubMenuModel::GetAcceleratorForCommandId(
       reopen_closed_tab_accelerator_.key_code() != ui::VKEY_UNKNOWN) {
     *accelerator = reopen_closed_tab_accelerator_;
     return true;
+  } else if (command_id == IDC_SHOW_HISTORY) {

[Peter Kasting, 2015/06/17 22:59:42]

Nit: No else after return

[edwardjung, 2015/06/18 16:52:05]

Done.

+    *accelerator = show_history_accelerator_;
+    return true;
   }
+
   return false;
 }
 
@@ -366,30 +381,38 @@ bool RecentTabsSubMenuModel::GetURLAndTitleForItemAtIndex(
 
 void RecentTabsSubMenuModel::Build() {
   // The menu contains:
+  // - History to open the full history tab.
+  // - Separator
   // - Recently closed header, then list of local recently closed tabs/windows,
   //   then separator
   // - device 1 section header, then list of tabs from device, then separator
   // - device 2 section header, then list of tabs from device, then separator
   // - device 3 section header, then list of tabs from device, then separator
-  // - More... to open the history tab to get more other devices.
   // |local_tab_navigation_items_| and |other_devices_tab_navigation_items_|
   // only contain navigatable (and hence executable) tab items for local
   // recently closed tabs and tabs from other devices respectively.
   // |local_window_items_| contains the local recently closed windows.
+  // Add the History item to top of the menu followed by a separator.

[Peter Kasting, 2015/06/17 22:59:42]

Nit: This sentence is redundant with the comments above + code below, I'd remove
it

[edwardjung, 2015/06/18 16:52:05]

Done.

+  InsertItemWithStringIdAt(0, IDC_SHOW_HISTORY, IDS_SHOW_HISTORY);
+  InsertSeparatorAt(1, ui::NORMAL_SEPARATOR);
+

[Peter Kasting, 2015/06/17 22:59:42]

Nit: I'd also remove this blank line

[edwardjung, 2015/06/18 16:52:05]

Done.

   BuildLocalEntries();
   BuildTabsFromOtherDevices();
 }
 
 void RecentTabsSubMenuModel::BuildLocalEntries() {
+  // Assigning the correct index for inserting items.
+  // Always starts at the 2 second item in the list.

[Peter Kasting, 2015/06/17 22:59:42]

These comments don't make much sense, plus they seem likely to get out of date. 
The whole reason you defined a constant for this was so you wouldn't have to
give exact values elsewhere.

[edwardjung, 2015/06/18 16:52:05]

Removed.

+  last_local_model_index_ = kLocalEntriesStartIndex;

[Peter Kasting, 2015/06/17 22:59:42]

Nit: Do this right above the DCHECK

[edwardjung, 2015/06/18 16:52:05]

Done.

+
   // All local items use InsertItem*At() to append or insert a menu item.
   // We're appending if building the entries for the first time i.e. invoked
   // from Constructor(), inserting when local entries change subsequently i.e.
   // invoked from TabRestoreServiceChanged().
-
-  DCHECK_EQ(last_local_model_index_, -1);
-
   TabRestoreService* service =
       TabRestoreServiceFactory::GetForProfile(browser_->profile());
+
+  DCHECK_EQ(last_local_model_index_, 1);

[Peter Kasting, 2015/06/17 22:59:42]

Nit: (expected, actual)

Though, I don't see why you're DCHECKing this value.  This defeats the purpose
of using a separate constant for this.

[edwardjung, 2015/06/18 16:52:05]

Good point, removed. 

   if (!service || service->entries().size() == 0) {
     // This is to show a disabled restore tab entry with the accelerator to
     // teach users about this command.
@@ -400,6 +423,7 @@ void RecentTabsSubMenuModel::BuildLocalEntries() {
     InsertItemWithStringIdAt(++last_local_model_index_,
                              kRecentlyClosedHeaderCommandId,
                              IDS_RECENTLY_CLOSED);
+

[Peter Kasting, 2015/06/17 22:59:42]

Nit: Why did you add this?

[edwardjung, 2015/06/18 16:52:05]

Removed.

     ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
     SetIcon(last_local_model_index_,
             rb.GetNativeImageNamed(IDR_RECENTLY_CLOSED_WINDOW));
@@ -429,7 +453,6 @@ void RecentTabsSubMenuModel::BuildLocalEntries() {
       ++added_count;
     }
   }
-
   DCHECK_GE(last_local_model_index_, 0);
 }
 
@@ -509,8 +532,6 @@ void RecentTabsSubMenuModel::BuildTabsFromOtherDevices() {
 
   // We are not supposed to get here unless at least some items were added.
   DCHECK_GT(GetItemCount(), 0);
-  AddSeparator(ui::NORMAL_SEPARATOR);
-  AddItemWithStringId(IDC_SHOW_HISTORY, IDS_RECENT_TABS_MORE);
 }
 
 void RecentTabsSubMenuModel::BuildLocalTabItem(int session_id,
@@ -654,12 +675,11 @@ int RecentTabsSubMenuModel::CommandIdToTabVectorIndex(
 
 void RecentTabsSubMenuModel::ClearLocalEntries() {
   // Remove local items (recently closed tabs and windows) from menumodel.
-  while (last_local_model_index_ >= 0)
+  while (last_local_model_index_ > kLocalEntriesStartIndex)
     RemoveItemAt(last_local_model_index_--);
 
   // Cancel asynchronous FaviconService::GetFaviconImageForPageURL() tasks of
-  // all
-  // local tabs.
+  // all local tabs.
   local_tab_cancelable_task_tracker_.TryCancelAll();
 
   // Remove all local tab navigation items.