Add new option to open history page to show more other devices

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 d8d5204575afe9a1a9644f67fa4f559b63d30c57..80ebc38e66dfbeefad25f0b876fc670f80ae12a6 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
@@ -150,6 +150,9 @@ bool RecentTabsSubMenuModel::IsCommandIdEnabled(int command_id) const {
       command_id == IDC_RECENT_TABS_NO_DEVICE_TABS) {
     return false;
   }
+  if (command_id == IDC_SHOW_HISTORY) {
+    return true;
+  }
   int model_index = CommandIdToModelIndex(command_id);
   return model_index >= 0 && model_index < static_cast<int>(model_.size());
 }
@@ -189,6 +192,12 @@ void RecentTabsSubMenuModel::ExecuteCommand(int command_id, int event_flags) {
         ui::DispositionFromEventFlags(event_flags));
     return;
   }
+  if (command_id == IDC_SHOW_HISTORY) {
+    // We show all "other devices" on the history page.
+    chrome::ExecuteCommandWithDisposition(browser_, IDC_SHOW_HISTORY,
+        ui::DispositionFromEventFlags(event_flags));
+    return;
+  }
 
   DCHECK_NE(kDisabledCommandId, command_id);
   DCHECK_NE(IDC_RECENT_TABS_NO_DEVICE_TABS, command_id);
@@ -270,8 +279,7 @@ void RecentTabsSubMenuModel::BuildDevices() {
   const size_t kMaxSessionsToShow = 3;
   size_t num_sessions_added = 0;
   for (size_t i = 0;
-       i < sessions.size() && num_sessions_added < kMaxSessionsToShow;
-       ++i) {
+       i < sessions.size() && num_sessions_added < kMaxSessionsToShow; ++i) {
     const browser_sync::SyncedSession* session = sessions[i];
     const std::string& session_tag = session->session_tag;
 
@@ -322,6 +330,11 @@ void RecentTabsSubMenuModel::BuildDevices() {
 
     ++num_sessions_added;
   }  // for all sessions
+
+  // We are not supposed to get here unless at least some items were added.
+  DCHECK(GetItemCount() > 0);

[Alexei Svitkine (slow), 2013/03/22 19:20:20]

I don't think this is correct, since items are also added outside of
BuildDevices().

For example, RecentTabsSubMenuModel::Build() calls AddItem() on the first
message item, before calling BuildDevices().

Also, can you use DCHECK_GT()?

[MAD, 2013/03/22 19:26:53]

Changed to _GT, but I don't understand why it's not correct. The comment was
about the fact that we would add a separator when there are no other items
before in the menu... This DCHECK makes sure it won't happen... Right?

That's why I didn't use the term devices/tabs in the comment, but items. Because
it's still possible (though I couldn't find ways to reproduce it) that none of
the devices returned by GetAllForeignSessions would generate at least one tab. I
have tried having only devices with NTP, and they are actually don't show up,
seems like there is already filtering from within GetAllForeignSessions, so
maybe the filtering we are doing here is not necessary????

[Alexei Svitkine (slow), 2013/03/22 19:39:40]

Ah, I didn't realise that was the intention of the check. I thought it was there
to check if any tabs were added.

+  AddSeparator(ui::NORMAL_SEPARATOR);
+  AddItemWithStringId(IDC_SHOW_HISTORY, IDS_RECENT_TABS_MORE);
 }
 
 void RecentTabsSubMenuModel::BuildForeignTabItem(