sync: Route local sessions events to SessionsSyncManager

chrome/browser/sync/sessions2/sessions_sync_manager.h

 // Copyright 2013 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_SYNC_SESSIONS2_SESSIONS_SYNC_MANAGER_H_
 #define CHROME_BROWSER_SYNC_SESSIONS2_SESSIONS_SYNC_MANAGER_H_
 
 #include <map>
 #include <set>
 #include <string>
 #include <vector>
 
 #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"
 #include "chrome/browser/sync/glue/device_info.h"
 #include "chrome/browser/sync/glue/favicon_cache.h"
 #include "chrome/browser/sync/glue/synced_session.h"
 #include "chrome/browser/sync/glue/synced_session_tracker.h"
 #include "chrome/browser/sync/open_tabs_ui_delegate.h"
 #include "chrome/browser/sync/sessions2/tab_node_pool2.h"
 #include "chrome/browser/sync/sync_prefs.h"
@@ skipping 12 matching lines @@
 class SessionWindow;
 class TabNavigation;
 }  // namespace sync_pb
 
 namespace browser_sync {
 
 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;
 
     // Used for creation of the machine tag for this local session.
     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 {

[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() {}
+
+    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,
       const syncer::SyncDataList& initial_sync_data,
       scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
       scoped_ptr<syncer::SyncErrorFactory> error_handler) OVERRIDE;
   virtual void StopSyncing(syncer::ModelType type) OVERRIDE;
   virtual syncer::SyncDataList GetAllSyncData(
       syncer::ModelType type) const OVERRIDE;
   virtual syncer::SyncError ProcessSyncChanges(
       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,
       scoped_refptr<base::RefCountedMemory>* favicon_png) const OVERRIDE;
   virtual bool GetAllForeignSessions(
       std::vector<const SyncedSession*>* sessions) OVERRIDE;
   virtual bool GetForeignSession(
       const std::string& tag,
       std::vector<const SessionWindow*>* windows) OVERRIDE;
   virtual bool GetForeignTab(const std::string& tag,
                              const SessionID::id_type tab_id,
                              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
   // all other fields are.
   class TabLink {
    public:
     TabLink(int tab_node_id, const SyncedTabDelegate* tab)
       : tab_node_id_(tab_node_id),
         tab_(tab) {}
 
@@ skipping 164 matching lines @@
   // Unique client tag.
   std::string current_machine_tag_;
 
   // User-visible machine name.
   std::string current_session_name_;
 
   // SyncID for the sync node containing all the window information for this
   // client.
   int local_session_header_node_id_;
 
+  scoped_ptr<LocalEventRouter> local_event_router_;
+
   DISALLOW_COPY_AND_ASSIGN(SessionsSyncManager);
 };
 
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_SESSIONS2_SESSIONS_SYNC_MANAGER_H_