sync: Route local sessions events to SessionsSyncManager

chrome/browser/sync/profile_sync_service.cc

 // Copyright (c) 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.
 
 #include "chrome/browser/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 30 matching lines @@
 #include "chrome/browser/sync/glue/data_type_controller.h"
 #include "chrome/browser/sync/glue/device_info.h"
 #include "chrome/browser/sync/glue/favicon_cache.h"
 #include "chrome/browser/sync/glue/session_data_type_controller.h"
 #include "chrome/browser/sync/glue/session_model_associator.h"
 #include "chrome/browser/sync/glue/sync_backend_host.h"
 #include "chrome/browser/sync/glue/sync_backend_host_impl.h"
 #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"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/global_error/global_error_service.h"
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/chrome_version_info.h"
@@ skipping 23 matching lines @@
 #endif
 
 #if defined(OS_ANDROID)
 #include "sync/internal_api/public/read_transaction.h"
 #endif
 
 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;
 using syncer::JsBackend;
 using syncer::JsController;
 using syncer::JsEventDetails;
 using syncer::JsEventHandler;
 using syncer::ModelSafeRoutingInfo;
 using syncer::SyncCredentials;
 using syncer::SyncProtocolError;
@@ skipping 85 matching lines @@
   // GetChannel hits the registry on Windows. See http://crbug.com/70380.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
   chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
   if (channel == chrome::VersionInfo::CHANNEL_STABLE ||
       channel == chrome::VersionInfo::CHANNEL_BETA) {
     sync_service_url_ = GURL(kSyncServerUrl);
   }
 
   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()));
   }
 }
 
 ProfileSyncService::~ProfileSyncService() {
   sync_prefs_.RemoveSyncPrefObserver(this);
   // Shutdown() should have been called before destruction.
   CHECK(!backend_initialized_);
 }
 
 bool ProfileSyncService::IsSyncEnabledAndLoggedIn() {
@@ skipping 2042 matching lines @@
   SyncTokenStatus status;
   status.connection_status_update_time = connection_status_update_time_;
   status.connection_status = connection_status_;
   status.token_request_time = token_request_time_;
   status.token_receive_time = token_receive_time_;
   status.last_get_token_error = last_get_token_error_;
   if (request_access_token_retry_timer_.IsRunning())
     status.next_token_request_time = next_token_request_time_;
   return status;
 }