Dynamic updating recent menu for tabs from other devices.

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h

Index: chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
diff --git a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
index 0c5b74c1196f47f0c2159d9664e3d0889faefdbe..1c089589cccb02e33f467a1c94ac5bbcac89f3ec 100644
--- a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
+++ b/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h
@@ -14,6 +14,7 @@
 #include "components/favicon/core/favicon_service.h"
 #include "components/sessions/core/tab_restore_service.h"
 #include "components/sessions/core/tab_restore_service_observer.h"
+#include "components/sync/driver/sync_service_observer.h"
 #include "components/sync_sessions/synced_session.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/models/simple_menu_model.h"
@@ -36,6 +37,10 @@ namespace sync_sessions {
 class OpenTabsUIDelegate;
 }
 
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
 namespace ui {
 class AcceleratorProvider;
 }
@@ -45,7 +50,8 @@ class AcceleratorProvider;
 // opened tabs of other devices.
 class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
                                public ui::SimpleMenuModel::Delegate,
-                               public sessions::TabRestoreServiceObserver {
+                               public sessions::TabRestoreServiceObserver,
+                               public syncer::SyncServiceObserver {
  public:
   // Command Id for recently closed items header or disabled item to which the
   // accelerator string will be appended.
@@ -57,10 +63,9 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   static int GetFirstRecentTabsCommandId();
 
   // If |open_tabs_delegate| is NULL, the default delegate for |browser|'s
-  // profile will be used. Testing may require a specific |open_tabs_delegate|.
+  // profile will be used.
   RecentTabsSubMenuModel(ui::AcceleratorProvider* accelerator_provider,
-                         Browser* browser,
-                         sync_sessions::OpenTabsUIDelegate* open_tabs_delegate);
+                         Browser* browser);
   ~RecentTabsSubMenuModel() override;
 
   // Overridden from ui::SimpleMenuModel::Delegate:
@@ -122,6 +127,9 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   // Clear all recently closed tabs and windows.
   void ClearLocalEntries();
 
+  // Clear all tabs from other devices.

[Peter Kasting, 2017/03/23 04:48:34]

Nit: Clear -> Clears; see
http://google.github.io/styleguide/cppguide.html#Function_Comments .  (Various
other comments above here also get this wrong, feel free to fix or not)

[Peter Kasting, 2017/03/24 00:06:27]

Looks like you removed this comment instead of updating it.

+  void ClearTabsFromOtherDevices();
+
   // Converts |command_id| of menu item to index in local or other devices'
   // TabNavigationItems, and returns the corresponding local or other devices'
   // TabNavigationItems in |tab_items|.
@@ -139,9 +147,13 @@ class RecentTabsSubMenuModel : public ui::SimpleMenuModel,
   void TabRestoreServiceDestroyed(
       sessions::TabRestoreService* service) override;
 
+  // Overridden from syncer::SyncServiceObserver

[Peter Kasting, 2017/03/23 04:48:34]

Nit: Trailing colon

[Peter Kasting, 2017/03/24 00:06:27]

Looks like you removed the namespace instead of adding this.

+  void OnSyncCycleCompleted(syncer::SyncService* sync) override {}

[Peter Kasting, 2017/03/23 04:48:34]

Nit: I'd weakly prefer defining this in the .cc than the .h, since it's virtual

+  void OnForeignSessionUpdated(syncer::SyncService* sync) override;
+
   Browser* browser_;  // Weak.
 
-  sync_sessions::OpenTabsUIDelegate* open_tabs_delegate_;  // Weak.
+  sync_sessions::OpenTabsUIDelegate* open_tabs_delegate_ = nullptr;  // Weak.

[Peter Kasting, 2017/03/23 04:48:34]

Nit: I'm a fan of initializing at the declaration like this, but only if it's
done for all member variables that need initialization.

So I would say, either do this for all the other members (as applicable), or
else don't do it for this one.

 
   // Accelerator for reopening last closed tab.
   ui::Accelerator reopen_closed_tab_accelerator_;