Wrench menu reorg phase 2

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc

 // Copyright 2012 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.
 
 #include "chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h"
 
 #include "base/bind.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 51 matching lines @@
 const int kFirstLocalTabCommandId = WrenchMenuModel::kMinRecentTabsCommandId;
 const int kFirstLocalWindowCommandId = 1031;
 const int kFirstOtherDevicesTabCommandId = 1051;
 const int kMinDeviceNameCommandId = 1100;
 const int kMaxDeviceNameCommandId = 1110;
 
 // The maximum number of local recently closed entries (tab or window) to be
 // 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,
                            const browser_sync::SyncedSession* s2) {
   return s1->modified_time > s2->modified_time;
 }
 
 // Comparator function for use with std::sort that will sort tabs by
 // descending timestamp (i.e., most recent first).
 bool SortTabsByRecency(const sessions::SessionTab* t1,
@@ skipping 82 matching lines @@
 const int RecentTabsSubMenuModel::kRecentlyClosedHeaderCommandId = 1120;
 const int RecentTabsSubMenuModel::kDisabledRecentlyClosedHeaderCommandId = 1121;
 
 RecentTabsSubMenuModel::RecentTabsSubMenuModel(
     ui::AcceleratorProvider* accelerator_provider,
     Browser* browser,
     browser_sync::OpenTabsUIDelegate* open_tabs_delegate)
     : 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) {
   // Invoke asynchronous call to load tabs from local last session, which does
   // nothing if the tabs have already been loaded or they shouldn't be loaded.
   // TabRestoreServiceChanged() will be called after the tabs are loaded.
   TabRestoreService* service =
       TabRestoreServiceFactory::GetForProfile(browser_->profile());
   if (service) {
     service->LoadTabsFromLastSession();
@@ skipping 16 matching lines @@
     if (accel_data.action == ash::RESTORE_TAB) {
       reopen_closed_tab_accelerator_ = ui::Accelerator(accel_data.keycode,
                                                        accel_data.modifiers);
       break;
     }
   }
 #else
   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() {
   TabRestoreService* service =
       TabRestoreServiceFactory::GetForProfile(browser_->profile());
   if (service)
     service->RemoveObserver(this);
 }
 
 bool RecentTabsSubMenuModel::IsCommandIdChecked(int command_id) const {
@@ skipping 14 matching lines @@
     int command_id, ui::Accelerator* accelerator) {
   // If there are no recently closed items, we show the accelerator beside
   // the header, otherwise, we show it beside the first item underneath it.
   int index_in_menu = GetIndexOfCommandId(command_id);
   int header_index = GetIndexOfCommandId(kRecentlyClosedHeaderCommandId);
   if ((command_id == kDisabledRecentlyClosedHeaderCommandId ||
        (header_index != -1 && index_in_menu == header_index + 1)) &&
       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;
 }
 
 void RecentTabsSubMenuModel::ExecuteCommand(int command_id, int event_flags) {
   if (command_id == IDC_SHOW_HISTORY) {
     UMA_HISTOGRAM_ENUMERATION("WrenchMenu.RecentTabsSubMenu", SHOW_MORE,
                               LIMIT_RECENT_TAB_ACTION);
     // We show all "other devices" on the history page.
     chrome::ExecuteCommandWithDisposition(browser_, IDC_SHOW_HISTORY,
         ui::DispositionFromEventFlags(event_flags));
@@ skipping 101 matching lines @@
     const TabNavigationItem& item = (*tab_items)[tab_items_idx];
     *url = item.url.possibly_invalid_spec();
     *title = item.title;
     return true;
   }
   return false;
 }
 
 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.
     InsertItemWithStringIdAt(++last_local_model_index_,
                              kDisabledRecentlyClosedHeaderCommandId,
                              IDS_RECENTLY_CLOSED);
   } else {
     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));
 
     int added_count = 0;
     TabRestoreService::Entries entries = service->entries();
     for (TabRestoreService::Entries::const_iterator it = entries.begin();
          it != entries.end() && added_count < kMaxLocalEntries; ++it) {
       TabRestoreService::Entry* entry = *it;
       if (entry->type == TabRestoreService::TAB) {
         TabRestoreService::Tab* tab =
             static_cast<TabRestoreService::Tab*>(entry);
         const sessions::SerializedNavigationEntry& current_navigation =
             tab->navigations.at(tab->current_navigation_index);
         BuildLocalTabItem(
             entry->id,
             current_navigation.title(),
             current_navigation.virtual_url(),
             ++last_local_model_index_);
       } else  {
         DCHECK_EQ(entry->type, TabRestoreService::WINDOW);
         BuildLocalWindowItem(
             entry->id,
             static_cast<TabRestoreService::Window*>(entry)->tabs.size(),
             ++last_local_model_index_);
       }
       ++added_count;
     }
   }
-
   DCHECK_GE(last_local_model_index_, 0);
 }
 
 void RecentTabsSubMenuModel::BuildTabsFromOtherDevices() {
   // All other devices' items (device headers or tabs) use AddItem*() to append
   // a menu item, because they are always only built once (i.e. invoked from
   // Constructor()) and don't change after that.
 
   browser_sync::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate();
   std::vector<const browser_sync::SyncedSession*> sessions;
@@ skipping 59 matching lines @@
          k < std::min(tabs_in_session.size(), kMaxTabsPerSessionToShow);
          ++k) {
       BuildOtherDevicesTabItem(session_tag, *tabs_in_session[k]);
     }  // for all tabs in one session
 
     ++num_sessions_added;
   }  // for all sessions
 
   // 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,
                                                const base::string16& title,
                                                const GURL& url,
                                                int curr_model_index) {
   TabNavigationItem item(std::string(), session_id, title, url);
   int command_id = TabVectorIndexToCommandId(
       local_tab_navigation_items_.size(), kFirstLocalTabCommandId);
   // See comments in BuildLocalEntries() about usage of InsertItem*At().
@@ skipping 123 matching lines @@
   if (command_id >= kFirstOtherDevicesTabCommandId) {
     *tab_items = &other_devices_tab_navigation_items_;
     return command_id - kFirstOtherDevicesTabCommandId;
   }
   *tab_items = &local_tab_navigation_items_;
   return command_id - kFirstLocalTabCommandId;
 }
 
 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.
   local_tab_navigation_items_.clear();
 
   // Remove all local window items.
   local_window_items_.clear();
 }
 
 browser_sync::OpenTabsUIDelegate*
@@ skipping 16 matching lines @@
 
   ui::MenuModelDelegate* menu_model_delegate = GetMenuModelDelegate();
   if (menu_model_delegate)
     menu_model_delegate->OnMenuStructureChanged();
 }
 
 void RecentTabsSubMenuModel::TabRestoreServiceDestroyed(
     TabRestoreService* service) {
   TabRestoreServiceChanged(service);
 }