sync: Route local sessions events to SessionsSyncManager

chrome/browser/sync/profile_sync_service.cc

Index: chrome/browser/sync/profile_sync_service.cc
diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc
index 5173ed7c0d765c02a5209aa955e2c9a8cf9a43cc..a090075a215c939d7009ce103fb62c35824d85d9 100644
--- a/chrome/browser/sync/profile_sync_service.cc
+++ b/chrome/browser/sync/profile_sync_service.cc
@@ -48,6 +48,7 @@
 #include "chrome/browser/sync/glue/synced_device_tracker.h"
 #include "chrome/browser/sync/glue/typed_url_data_type_controller.h"
 #include "chrome/browser/sync/profile_sync_components_factory_impl.h"
+#include "chrome/browser/sync/sessions2/notification_service_sessions_router.h"
 #include "chrome/browser/sync/sessions2/sessions_sync_manager.h"
 #include "chrome/browser/sync/sync_global_error.h"
 #include "chrome/browser/sync/user_selectable_sync_type.h"
@@ -91,6 +92,7 @@ using browser_sync::ChangeProcessor;
 using browser_sync::DataTypeController;
 using browser_sync::DataTypeManager;
 using browser_sync::FailedDataTypesHandler;
+using browser_sync::NotificationServiceSessionsRouter;
 using browser_sync::SyncBackendHost;
 using syncer::ModelType;
 using syncer::ModelTypeSet;
@@ -196,7 +198,10 @@ ProfileSyncService::ProfileSyncService(
 
   if (CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableSyncSessionsV2)) {
-    sessions_sync_manager_.reset(new SessionsSyncManager(profile, this));
+    scoped_ptr<SessionsSyncManager::LocalEventRouter> router(

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

It looks like there's a circular dependency here.  The
NotificationServiceSessionsRouter needs a SessionsSyncManager, and the
SessionsSyncManager needs a LocalEventRouter.  You've resolved it by writing the
NotificationServiceSessionsRouter so that it doesn't require a LocalEventHandler
(SessionsSyncManager) right away, and is prepared to operate without one.

If this is the only reason why you've made NotificationServiceSessionsRouter
support a mode of operation where its handler_ == NULL, I suggest you
reconsider.  There are other solutions.  I would prefer to not see the design of
the NotificationServiceSessionsRouter class compromised for this reason alone.

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

That's not quite what's happening here.  The router always a) exists (with
ProfileSyncService) and b) observes NotificationService. It won't route things
to SessionsSyncManager until SessionsSyncManager is told to
MergeDataAndStartSyncing. At that point SSM tells the router to start routing to
it.  In the interim, if there is no handler, the router may decide to instead
send up a sync flare to start syncing becuase local sessions activity is
happening.  

Once we create a TabStripModel based LocalEventRouter, things will keep working
the same way from SSM's perspective and it shouldn't need to change. What will
change, however, is what the different router implementation relies on to
trigger a flare.

[rlarocque, 2013/12/02 18:53:53]

OK, I think I understand now.

Would it be a viable alternative to have the SessionsSyncManager always be
registered, but keep a boolean 'is_watching_changes_' to ignore events that
occur prior to the MergeDataAndStartSyncing() class?

(That's not necessarily a recommendation.  I'm just trying to make sure that I
understand how it works.)

[tim (not reviewing), 2013/12/02 19:50:56]

Yes, except that the goal with the Router approach was to de-couple
SessionsSyncManager from the observer mechanism and make it easy to swap in a
different module for that.  The current one is based on NotificationService, but
as bug 98892 outlines we'd like to try moving to TabStripModelObserver in the
future.  With this patch, SessionsSyncManager won't need to change because the
LocalSessionsEventHandler contract remains the same.

It also allows for more targeted tests in the future, since we just have to
satisfy LocalSessionEventHandler vs the full navigation stack.

+        new NotificationServiceSessionsRouter(profile));
+    sessions_sync_manager_.reset(
+        new SessionsSyncManager(profile, this, router.Pass()));
   }
 }