Dynamic updating recent menu for tabs from 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 1cd8dbb53938649b46e6fe1d8814cff42fc41596..1928daf828c859bba96bfcf3b52d90e300e78ff5 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.cc
@@ -179,15 +179,18 @@ const int RecentTabsSubMenuModel::kDisabledRecentlyClosedHeaderCommandId = 1121;
 
 RecentTabsSubMenuModel::RecentTabsSubMenuModel(
     ui::AcceleratorProvider* accelerator_provider,
-    Browser* browser,
-    sync_sessions::OpenTabsUIDelegate* open_tabs_delegate)
+    Browser* browser)
     : ui::SimpleMenuModel(this),
       browser_(browser),
-      open_tabs_delegate_(open_tabs_delegate),
+      open_tabs_delegate_(nullptr),
       last_local_model_index_(kHistorySeparatorIndex),
       default_favicon_(
           ui::ResourceBundle::GetSharedInstance().GetNativeImageNamed(
               IDR_DEFAULT_FAVICON)),
+#if !defined(OS_MACOSX)
+      tab_restore_service_observer_(this),
+      sync_observer_(this),
+#endif  // !defined(OS_MACOSX)
       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.
@@ -197,13 +200,21 @@ RecentTabsSubMenuModel::RecentTabsSubMenuModel(
   if (service) {
     service->LoadTabsFromLastSession();
 
-  // TODO(sail): enable this when mac implements the dynamic menu, together with
-  // MenuModelDelegate::MenuStructureChanged().
+// Mac doesn't support the dynamic menu.
 #if !defined(OS_MACOSX)
-    service->AddObserver(this);
+    tab_restore_service_observer_.Add(service);
 #endif
   }
 
+// Mac doesn't support the dynamic menu.
+#if !defined(OS_MACOSX)
+  browser_sync::ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(
+          browser_->profile());
+  if (sync_service && sync_service->IsSyncActive())

[pavely, 2017/03/24 01:20:05]

Is this line executed once per process or every time menu is opened? If former
then it could catch sync_service before it is initialized and therefore not
subscribe for notifications.

[Peter Kasting, 2017/03/24 01:35:21]

Looks like it's called each time the menu is opened.

However, that makes me wonder: can this conditional be false when the menu is
opened, but become true later while the menu is still open, at which point we'd
ideally like to be receiving updates?  If yes, how do we best fix this?  Is it
safe to assume that |sync_service| would always be non-null in such a case, but
IsSyncActive() might not be, yet?  Should we just remove the check for
IsSyncActive() here, or is there something more complex to do?

[pavely, 2017/03/24 04:13:22]

Yeah, we should remove call to IsSyncActive(). If sync is enabled then
sync_service is not nullptr and you can subscribe for notifications regardless
of ISyncActive() state. You should still keep if(sync_service) check though, but
if it is nullptr it is guaranteed to stay nullptr.

+    sync_observer_.Add(sync_service);
+#endif  // !defined(OS_MACOSX)
+
   Build();
 
   // Retrieve accelerator key for IDC_RESTORE_TAB now, because on ASH, it's not
@@ -231,12 +242,7 @@ RecentTabsSubMenuModel::RecentTabsSubMenuModel(
   }
 }
 
-RecentTabsSubMenuModel::~RecentTabsSubMenuModel() {
-  sessions::TabRestoreService* service =
-      TabRestoreServiceFactory::GetForProfile(browser_->profile());
-  if (service)
-    service->RemoveObserver(this);
-}
+RecentTabsSubMenuModel::~RecentTabsSubMenuModel() {}
 
 bool RecentTabsSubMenuModel::IsCommandIdChecked(int command_id) const {
   return false;
@@ -474,8 +480,7 @@ void RecentTabsSubMenuModel::BuildLocalEntries() {
 
 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.
+  // a menu item, because they take always place in the end of menu.
 
   sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate();
   std::vector<const sync_sessions::SyncedSession*> sessions;
@@ -708,6 +713,17 @@ void RecentTabsSubMenuModel::ClearLocalEntries() {
   local_window_items_.clear();
 }
 
+void RecentTabsSubMenuModel::ClearTabsFromOtherDevices() {
+  DCHECK_GE(last_local_model_index_, 0);
+  int count = GetItemCount();
+  for (int index = count - 1; index > last_local_model_index_; --index)
+    RemoveItemAt(index);
+
+  other_devices_tab_cancelable_task_tracker_.TryCancelAll();
+
+  other_devices_tab_navigation_items_.clear();
+}
+
 sync_sessions::OpenTabsUIDelegate*
 RecentTabsSubMenuModel::GetOpenTabsUIDelegate() {
   if (!open_tabs_delegate_) {
@@ -736,3 +752,16 @@ void RecentTabsSubMenuModel::TabRestoreServiceDestroyed(
     sessions::TabRestoreService* service) {
   TabRestoreServiceChanged(service);
 }
+
+void RecentTabsSubMenuModel::OnSyncCycleCompleted(syncer::SyncService* sync) {}
+
+void RecentTabsSubMenuModel::OnForeignSessionUpdated(
+    syncer::SyncService* sync) {
+  ClearTabsFromOtherDevices();
+
+  BuildTabsFromOtherDevices();
+
+  ui::MenuModelDelegate* menu_model_delegate = GetMenuModelDelegate();
+  if (menu_model_delegate)
+    menu_model_delegate->OnMenuStructureChanged();
+}