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..6b26353845abb53852f796526e84683e4fd6c8c0 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,39 @@ 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;
+
+  // 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.
+  virtual void OnBrowserOpened() = 0;

[Nicolas Zea, 2013/11/21 02:34:52]

Is this necessary? We always reassociate windows anyways; providing this kind of
implies that OnLocalTabModified isn't itself looking at windows.

Perhaps just have OnLocalTabModified support a NULL tab delegate (and comment
that that only looks at the windows)?

[tim (not reviewing), 2013/11/21 21:38:59]

I could maybe change it to OnBrowserModified(SyncedTabDelegate* modified_tab).
On the other hand, I would argue that this interface is meant to only define
"what sorts of events are interesting to tab sync" versus what it does in
response to different events. 

There are some similarities between the two implementations but OLTM does
additional stuff and it's kind of nice to not have an extra state to handle. I
also think the current name is nice because it makes it clear we only handle one
tab at a time and the browser opened event is separate (null |modified_tab|
doesn't imply a "multiple tab" browser opened event). A comment something like
"A Browser was opened [x]or a single tab was modified" might be what we'd want.

If we forsee more events in the future we want to react to where the
implementation will always be identical and we won't be tempted to add
yet-another state (like handling the NULL parameter), I could be swayed. If we
reduced the 3 to one single callback I might agree more, but we're likely going
to have separate functions with different arguments anyway (favicons).

 

[Nicolas Zea, 2013/11/22 23:34:51]

How about two methods, with the second method being named OnBrowserModified, and
a comment that only one of these methods should be called for a single event.

[tim (not reviewing), 2013/11/25 18:12:08]

As discussed, went ahead and removed OnBrowserOpened. 

+
+  // 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() {}

[Nicolas Zea, 2013/11/21 02:34:52]

nit: newline after

[tim (not reviewing), 2013/11/21 21:38:59]

Done.

     // 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,34 +87,21 @@ class SessionsSyncManager : public syncer::SyncableService,
     virtual std::string GetLocalSyncCacheGUID() const = 0;
   };
 
+  // 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 {
+   public:
+    virtual ~LocalEventRouter() {}
+    virtual void StartRoutingTo(LocalSessionEventHandler* handler) = 0;
+    virtual void Stop() = 0;
+  };
+
   SessionsSyncManager(Profile* profile,
-                      SyncInternalApiDelegate* delegate);
+                      SyncInternalApiDelegate* delegate,
+                      scoped_ptr<LocalEventRouter> router);
   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();
-
-  // 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);
-
-  // 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_;
-  }
-
   // syncer::SyncableService implementation.
   virtual syncer::SyncMergeResult MergeDataAndStartSyncing(
       syncer::ModelType type,
@@ -102,14 +115,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 +129,27 @@ 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 OnBrowserOpened() 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 +334,8 @@ class SessionsSyncManager : public syncer::SyncableService,
   // client.
   int local_session_header_node_id_;
 
+  scoped_ptr<LocalEventRouter> local_event_router_;
+
   DISALLOW_COPY_AND_ASSIGN(SessionsSyncManager);
 };