Dynamic updating recent menu for tabs from other devices.

chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h

 // 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.
 
 #ifndef CHROME_BROWSER_UI_TOOLBAR_RECENT_TABS_SUB_MENU_MODEL_H_
 #define CHROME_BROWSER_UI_TOOLBAR_RECENT_TABS_SUB_MENU_MODEL_H_
 
 #include <set>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/timer/elapsed_timer.h"
 #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"
 
 class Browser;
 
 namespace favicon_base {
 struct FaviconImageResult;
 }
 
 namespace gfx {
 class Image;
 }
 
 namespace sessions {
 struct SessionTab;
 }
 
 namespace sync_sessions {
 class OpenTabsUIDelegate;
 }
 
+namespace syncer {
+class SyncService;
+}  // namespace syncer
+
 namespace ui {
 class AcceleratorProvider;
 }
 
 // A menu model that builds the contents of "Recent tabs" submenu, which include
 // the recently closed tabs/windows of current device i.e. local entries, and
 // 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.
   static const int kRecentlyClosedHeaderCommandId;
   static const int kDisabledRecentlyClosedHeaderCommandId;
 
   // Exposed for tests only: return the Command Id for the first entry in the
   // recently closed window items list.
   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:
   bool IsCommandIdChecked(int command_id) const override;
   bool IsCommandIdEnabled(int command_id) const override;
   bool GetAcceleratorForCommandId(int command_id,
                                   ui::Accelerator* accelerator) const override;
   void ExecuteCommand(int command_id, int event_flags) override;
   const gfx::FontList* GetLabelFontListAt(int index) const override;
 
@@ skipping 41 matching lines @@
   // Add the favicon for a local or other devices' tab asynchronously,
   // OnFaviconDataAvailable() will be invoked when the favicon is ready.
   void AddTabFavicon(int command_id, const GURL& url);
   void OnFaviconDataAvailable(
       int command_id,
       const favicon_base::FaviconImageResult& image_result);
 
   // 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|.
   int CommandIdToTabVectorIndex(int command_id, TabNavigationItems** tab_items);
 
   // Used to access (and lazily initialize) open_tabs_delegate_.
   // TODO(tim): This lazy-init for member variables is error prone because you
   // can always skip going through the function and access the field directly.
   // Consider instead having code just deal with potentially NULL open_tabs_
   // and have it initialized by an event / callback.
   sync_sessions::OpenTabsUIDelegate* GetOpenTabsUIDelegate();
 
   // Overridden from TabRestoreServiceObserver:
   void TabRestoreServiceChanged(sessions::TabRestoreService* service) override;
   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_;
 
   // Accelerator for showing history.
   ui::Accelerator show_history_accelerator_;
 
   // Navigation items for local recently closed tabs.  The |command_id| for
   // these is set to |kFirstLocalTabCommandId| plus the index into the vector.
   // Upon invocation of the menu, the navigation information is retrieved from
@@ skipping 22 matching lines @@
 
   // Time the menu is open for until a recent tab is selected.
   base::ElapsedTimer menu_opened_timer_;
 
   base::WeakPtrFactory<RecentTabsSubMenuModel> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(RecentTabsSubMenuModel);
 };
 
 #endif  // CHROME_BROWSER_UI_TOOLBAR_RECENT_TABS_SUB_MENU_MODEL_H_