sync: Route local sessions events to SessionsSyncManager

chrome/browser/sync/sessions2/sessions_sync_manager.h

Index: chrome/browser/sync/sessions2/sessions_sync_manager.h
diff --git a/chrome/browser/sync/sessions2/sessions_sync_manager.h b/chrome/browser/sync/sessions2/sessions_sync_manager.h
index 88bb884ffc1b28f4dde308c632d45d59c2581ae0..3762c241633e1727df81c94f25839360ed892e04 100644
--- a/chrome/browser/sync/sessions2/sessions_sync_manager.h
+++ b/chrome/browser/sync/sessions2/sessions_sync_manager.h
@@ -13,6 +13,7 @@
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_vector.h"
+#include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/sessions/session_types.h"
@@ -45,14 +46,35 @@ class DataTypeErrorHandler;
 class SyncedTabDelegate;
 class SyncedWindowDelegate;
 
+// An interface defining the ways in which local open tab events can interact
+// with session sync.  All local tab events flow to sync via this interface.
+// In that way it is analogous to sync changes flowing to the local model
+// via ProcessSyncChanges, just with a more granular breakdown.
+class LocalSessionEventHandler {
+ public:
+  // A local navigation event took place that affects the synced session
+  // for this instance of Chrome.
+  virtual void OnLocalTabModified(SyncedTabDelegate* modified_tab) = 0;
+
+  // A local navigation occurred that triggered updates to favicon data for
+  // each URL in |updated_page_urls|.  This is routed through Sessions Sync so
+  // that we can filter (exclude) favicon updates for pages that aren't
+  // currently part of the set of local open tabs, and pass relevant updates
+  // on to FaviconCache for out-of-band favicon syncing.
+  virtual void OnFaviconPageUrlsUpdated(
+      const std::set<GURL>& updated_page_urls) = 0;
+};
+
 // Contains all logic for associating the Chrome sessions model and
 // the sync sessions model.
 class SessionsSyncManager : public syncer::SyncableService,
-                            public OpenTabsUIDelegate {
+                            public OpenTabsUIDelegate,
+                            public LocalSessionEventHandler {
  public:
   // Isolates SessionsSyncManager from having to depend on sync internals.
   class SyncInternalApiDelegate {
    public:
+    virtual ~SyncInternalApiDelegate() {}
     // Returns sync's representation of the local device info.
     // Return value is an empty scoped_ptr if the device info is unavailable.
     virtual scoped_ptr<DeviceInfo> GetLocalDeviceInfo() const = 0;
@@ -61,33 +83,21 @@ class SessionsSyncManager : public syncer::SyncableService,
     virtual std::string GetLocalSyncCacheGUID() const = 0;
   };
 
-  SessionsSyncManager(Profile* profile,
-                      SyncInternalApiDelegate* delegate);
-  virtual ~SessionsSyncManager();
-
-  // A local navigation event took place that affects the synced session
-  // for this instance of Chrome.
-  void OnLocalTabModified(const SyncedTabDelegate& modified_tab,
-                          syncer::SyncError* error);
-
-  // When a Browser window is opened, we want to know so we can make sure our
-  // bookkeeping of open windows / sessions on this device is up-to-date.
-  void OnBrowserOpened();
+  // The LocalEventRouter is responsible for hooking itself up to various
+  // notification sources in the browser process and forwarding relevant
+  // events to a handler as defined in the LocalSessionEventHandler contract.
+  class LocalEventRouter {

[rlarocque, 2013/11/27 22:47:51]

Unless there's some C++ trivia regarding inner classes that I don't know about,
I think this is in many ways worse than a top level class definition and in no
way better.

It makes forward declaration impossible.

It makes it impossible to define a class that uses this interface without
including sync_sessions_manager.h.  Despite your work to reduce dependencies,
profile_sync_service.cc will still end up including this file through
notification_service_sessions_router.h.

It's more verbose than even a top-level "class
SessionsSyncManager_LocalEventRouter" would have been (:: vs _).  That's just a
silly example to emphasize the difference between the two options; I'm sure you
could come up with a better and shorter name than that.

In summary, I oppose all public inner classes in C++, including this one.

[tim (not reviewing), 2013/12/02 18:29:49]

OK. Made it a top level interface so that it's forward declarable and less
verbose. My goal here actually wasn't about reducing dependencies fwiw,
ProfileSyncService still needs to include SessionsSyncManager by design. The
Handler+Router are basically how SessionsSyncManager declares what it needs to
work properly. They aren't intended to be used separately and changes to one
likely means changes to the other, so I'd prefer to keep them close.

+   public:
+    virtual ~LocalEventRouter() {}
 
-  // A local navigation occurred that triggered updates to favicon data for
-  // each URL in |updated_page_urls|.  This is routed through Sessions Sync so
-  // that we can filter (exclude) favicon updates for pages that aren't
-  // currently part of the set of local open tabs, and pass relevant updates
-  // on to FaviconCache for out-of-band favicon syncing.
-  void ForwardRelevantFaviconUpdatesToFaviconCache(
-      const std::set<GURL>& updated_favicon_page_urls);
+    virtual void StartRoutingTo(LocalSessionEventHandler* handler) = 0;
+    virtual void Stop() = 0;
+  };
 
-  // Returns the tag used to uniquely identify this machine's session in the
-  // sync model.
-  const std::string& current_machine_tag() const {
-    DCHECK(!current_machine_tag_.empty());
-    return current_machine_tag_;
-  }
+  SessionsSyncManager(Profile* profile,
+                      SyncInternalApiDelegate* delegate,
+                      scoped_ptr<LocalEventRouter> router);
+  virtual ~SessionsSyncManager();
 
   // syncer::SyncableService implementation.
   virtual syncer::SyncMergeResult MergeDataAndStartSyncing(
@@ -102,14 +112,6 @@ class SessionsSyncManager : public syncer::SyncableService,
       const tracked_objects::Location& from_here,
       const syncer::SyncChangeList& change_list) OVERRIDE;
 
-  // Return the virtual URL of the current tab, even if it's pending.
-  static GURL GetCurrentVirtualURL(const SyncedTabDelegate& tab_delegate);
-
-  // Return the favicon url of the current tab, even if it's pending.
-  static GURL GetCurrentFaviconURL(const SyncedTabDelegate& tab_delegate);
-
-  FaviconCache* GetFaviconCache();
-
   // OpenTabsUIDelegate implementation.
   virtual bool GetSyncedFaviconForPageURL(
       const std::string& pageurl,
@@ -124,6 +126,26 @@ class SessionsSyncManager : public syncer::SyncableService,
                              const SessionTab** tab) OVERRIDE;
   virtual void DeleteForeignSession(const std::string& tag) OVERRIDE;
 
+  // LocalSessionEventHandler implementation.
+  virtual void OnLocalTabModified(SyncedTabDelegate* modified_tab) OVERRIDE;
+  virtual void OnFaviconPageUrlsUpdated(
+      const std::set<GURL>& updated_favicon_page_urls) OVERRIDE;
+
+  // Returns the tag used to uniquely identify this machine's session in the
+  // sync model.
+  const std::string& current_machine_tag() const {
+    DCHECK(!current_machine_tag_.empty());
+    return current_machine_tag_;
+  }
+
+  // Return the virtual URL of the current tab, even if it's pending.
+  static GURL GetCurrentVirtualURL(const SyncedTabDelegate& tab_delegate);
+
+  // Return the favicon url of the current tab, even if it's pending.
+  static GURL GetCurrentFaviconURL(const SyncedTabDelegate& tab_delegate);
+
+  FaviconCache* GetFaviconCache();
+
  private:
   // Keep all the links to local tab data in one place. A tab_node_id and tab
   // must be passed at creation. The tab_node_id is not mutable, although
@@ -308,6 +330,8 @@ class SessionsSyncManager : public syncer::SyncableService,
   // client.
   int local_session_header_node_id_;
 
+  scoped_ptr<LocalEventRouter> local_event_router_;
+
   DISALLOW_COPY_AND_ASSIGN(SessionsSyncManager);
 };