Remove connection_code_ from SyncSchedulerImpl.

Removed connection_code_ from SyncSchedulerImpl since it essentially
mirrors ServerConnectionManager::server_status_.

One behavior changed as result of this change: Previously if
SyncSchedulerImpl::OnConnectionStatusChange was called when
connection_code_ is CONNECTION_UNAVAILABLE it would reset
connection_code_ to SERVER_CONNECTION_OK and consecutive calls to
SyncSchedulerImpl::OnConnectionStatusChange would not post canary job
until current one is finished and updates server_status_. With this
change it is no longer true.

Also moved notification about auth error from
SyncManagerImpl::OnInvalidatorStateChange to common code path. It no
longer makes assumption about error code that connection manager is
going to set in OnInvalidationCredentialsRejected.

Also removed few references to get_time.

BUG=177535


Review URL: https://chromiumcodereview.appspot.com/12390070

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@186092 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 28 insertions, 63 deletions

diff --git a/sync/engine/net/server_connection_manager.cc b/sync/engine/net/server_connection_manager.cc
index c2bb951..7a27229 100644
--- a/sync/engine/net/server_connection_manager.cc
+++ b/sync/engine/net/server_connection_manager.cc
@@ -28,11 +28,6 @@ using std::vector;
 
 static const char kSyncServerSyncPath[] = "/command/";
 
-// At the /time/ path of the sync server, we expect to find a very simple
-// time of day service that we can use to synchronize the local clock with
-// server time.
-static const char kSyncServerGetTimePath[] = "/time";
-
 HttpResponse::HttpResponse()
     : response_code(kUnsetResponseCode),
       content_length(kUnsetContentLength),
@@ -175,11 +170,7 @@ ScopedServerStatusWatcher::ScopedServerStatusWatcher(
 }
 
 ScopedServerStatusWatcher::~ScopedServerStatusWatcher() {
-  if (conn_mgr_->server_status_ != response_->server_status) {
-    conn_mgr_->server_status_ = response_->server_status;
-    conn_mgr_->NotifyStatusChanged();
-    return;
-  }
+  conn_mgr_->SetServerStatus(response_->server_status);
 }
 
 ServerConnectionManager::ServerConnectionManager(
@@ -190,7 +181,6 @@ ServerConnectionManager::ServerConnectionManager(
       sync_server_port_(port),
       use_ssl_(use_ssl),
       proto_sync_path_(kSyncServerSyncPath),
-      get_time_path_(kSyncServerGetTimePath),
       server_status_(HttpResponse::NONE),
       terminated_(false),
       active_connection_(NULL) {
@@ -224,7 +214,7 @@ void ServerConnectionManager::OnConnectionDestroyed(Connection* connection) {
 
 void ServerConnectionManager::OnInvalidationCredentialsRejected() {
   InvalidateAndClearAuthToken();
-  server_status_ = HttpResponse::SYNC_AUTH_ERROR;
+  SetServerStatus(HttpResponse::SYNC_AUTH_ERROR);
 }
 
 void ServerConnectionManager::InvalidateAndClearAuthToken() {
@@ -236,6 +226,14 @@ void ServerConnectionManager::InvalidateAndClearAuthToken() {
   }
 }
 
+void ServerConnectionManager::SetServerStatus(
+    HttpResponse::ServerConnectionCode server_status) {
+  if (server_status_ == server_status)
+    return;
+  server_status_ = server_status;
+  NotifyStatusChanged();
+}
+
 void ServerConnectionManager::NotifyStatusChanged() {
   DCHECK(thread_checker_.CalledOnValidThread());
   FOR_EACH_OBSERVER(ServerConnectionEventListener, listeners_,
diff --git a/sync/engine/net/server_connection_manager.h b/sync/engine/net/server_connection_manager.h
index 0603340..bbb830d 100644
--- a/sync/engine/net/server_connection_manager.h
+++ b/sync/engine/net/server_connection_manager.h
@@ -256,9 +256,8 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager {
     return proto_sync_path_;
   }
 
-  std::string get_time_path() const {
-    return get_time_path_;
-  }
+  // Updates server_status_ and notifies listeners if server_status_ changed
+  void SetServerStatus(HttpResponse::ServerConnectionCode server_status);
 
   // NOTE: Tests rely on this protected function being virtual.
   //
@@ -296,7 +295,6 @@ class SYNC_EXPORT_PRIVATE ServerConnectionManager {
 
   // The paths we post to.
   std::string proto_sync_path_;
-  std::string get_time_path_;
 
   // The auth token to use in authenticated requests.
   std::string auth_token_;
diff --git a/sync/engine/sync_scheduler_impl.cc b/sync/engine/sync_scheduler_impl.cc
index 1345e67..09f28b2 100644
--- a/sync/engine/sync_scheduler_impl.cc
+++ b/sync/engine/sync_scheduler_impl.cc
@@ -171,7 +171,6 @@ SyncSchedulerImpl::SyncSchedulerImpl(const std::string& name,
       mode_(NORMAL_MODE),
       // Start with assuming everything is fine with the connection.
       // At the end of the sync cycle we would have the correct status.
-      connection_code_(HttpResponse::SERVER_CONNECTION_OK),
       pending_nudge_(NULL),
       delay_provider_(delay_provider),
       syncer_(syncer),
@@ -193,17 +192,6 @@ void SyncSchedulerImpl::OnJobDestroyed(SyncSessionJob* job) {
 void SyncSchedulerImpl::OnCredentialsUpdated() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
 
-  // TODO(lipalani): crbug.com/106262. One issue here is that if after
-  // the auth error we happened to do gettime and it succeeded then
-  // the |connection_code_| would be briefly OK however it would revert
-  // back to SYNC_AUTH_ERROR at the end of the sync cycle. The
-  // referenced bug explores the option of removing gettime calls
-  // altogethere
-  // TODO(rogerta): this code no longer checks |connection_code_|.  It uses
-  // ServerConnectionManager::server_status() instead.  This is to resolve a
-  // missing notitification during re-auth.  |connection_code_| is a duplicate
-  // value and should probably be removed, see comment in the function
-  // SyncSchedulerImpl::FinishSyncSessionJob() below.
   if (HttpResponse::SYNC_AUTH_ERROR ==
       session_context_->connection_manager()->server_status()) {
     OnServerConnectionErrorFixed();
@@ -211,7 +199,8 @@ void SyncSchedulerImpl::OnCredentialsUpdated() {
 }
 
 void SyncSchedulerImpl::OnConnectionStatusChange() {
-  if (HttpResponse::CONNECTION_UNAVAILABLE  == connection_code_) {
+  if (HttpResponse::CONNECTION_UNAVAILABLE  ==
+      session_context_->connection_manager()->server_status()) {
     // Optimistically assume that the connection is fixed and try
     // connecting.
     OnServerConnectionErrorFixed();
@@ -219,7 +208,6 @@ void SyncSchedulerImpl::OnConnectionStatusChange() {
 }
 
 void SyncSchedulerImpl::OnServerConnectionErrorFixed() {
-  connection_code_ = HttpResponse::SERVER_CONNECTION_OK;
   // There could be a pending nudge or configuration job in several cases:
   //
   // 1. We're in exponential backoff.
@@ -241,15 +229,6 @@ void SyncSchedulerImpl::OnServerConnectionErrorFixed() {
                       base::Passed(&pending)));
 }
 
-void SyncSchedulerImpl::UpdateServerConnectionManagerStatus(
-    HttpResponse::ServerConnectionCode code) {
-  DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  SDVLOG(2) << "New server connection code: "
-            << HttpResponse::GetServerConnectionCodeString(code);
-
-  connection_code_ = code;
-}
-
 void SyncSchedulerImpl::Start(Mode mode) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   std::string thread_name = MessageLoop::current()->thread_name();
@@ -807,16 +786,6 @@ void SyncSchedulerImpl::UpdateNudgeTimeRecords(const SyncSourceInfo& info) {
 bool SyncSchedulerImpl::FinishSyncSessionJob(scoped_ptr<SyncSessionJob> job,
                                              bool exited_prematurely) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
-  // Now update the status of the connection from SCM. We need this to decide
-  // whether we need to save/run future jobs. The notifications from SCM are
-  // not reliable.
-  //
-  // TODO(rlarocque): crbug.com/110954
-  // We should get rid of the notifications and it is probably not needed to
-  // maintain this status variable in 2 places. We should query it directly
-  // from SCM when needed.
-  ServerConnectionManager* scm = session_context_->connection_manager();
-  UpdateServerConnectionManagerStatus(scm->server_status());
 
   // Let job know that we're through syncing (calling SyncShare) at this point.
   bool succeeded = false;
diff --git a/sync/engine/sync_scheduler_impl.h b/sync/engine/sync_scheduler_impl.h
index f909920..66fd8d5 100644
--- a/sync/engine/sync_scheduler_impl.h
+++ b/sync/engine/sync_scheduler_impl.h
@@ -250,10 +250,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl :
   // Creates a session for a poll and performs the sync.
   void PollTimerCallback();
 
-  // Used to update |connection_code_|, see below.
-  void UpdateServerConnectionManagerStatus(
-      HttpResponse::ServerConnectionCode code);
-
   // Called once the first time thread_ is started to broadcast an initial
   // session snapshot containing data like initial_sync_ended.  Important when
   // the client starts up and does not need to perform an initial sync.
@@ -302,9 +298,6 @@ class SYNC_EXPORT_PRIVATE SyncSchedulerImpl :
   // The mode of operation.
   Mode mode_;
 
-  // The latest connection code we got while trying to connect.
-  HttpResponse::ServerConnectionCode connection_code_;
-
   // Tracks (does not own) in-flight nudges (scheduled or unscheduled),
   // so we can coalesce. NULL if there is no pending nudge.
   SyncSessionJob* pending_nudge_;
diff --git a/sync/engine/sync_scheduler_unittest.cc b/sync/engine/sync_scheduler_unittest.cc
index 3fe4c0d..99b2197 100644
--- a/sync/engine/sync_scheduler_unittest.cc
+++ b/sync/engine/sync_scheduler_unittest.cc
@@ -1192,9 +1192,9 @@ TEST_F(SyncSchedulerTest, StartWhenNotConnected) {
   // Should save the nudge for until after the server is reachable.
   MessageLoop::current()->RunUntilIdle();
 
+  scheduler()->OnConnectionStatusChange();
   connection()->SetServerReachable();
   connection()->UpdateConnectionStatus();
-  scheduler()->OnConnectionStatusChange();
   MessageLoop::current()->RunUntilIdle();
 }
 
@@ -1220,9 +1220,9 @@ TEST_F(SyncSchedulerTest, ServerConnectionChangeDuringBackoff) {
   ASSERT_TRUE(scheduler()->IsBackingOff());
 
   // Before we run the scheduled canary, trigger a server connection change.
+  scheduler()->OnConnectionStatusChange();
   connection()->SetServerReachable();
   connection()->UpdateConnectionStatus();
-  scheduler()->OnConnectionStatusChange();
   MessageLoop::current()->RunUntilIdle();
 }
 
@@ -1248,9 +1248,9 @@ TEST_F(SyncSchedulerTest, ConnectionChangeCanaryPreemptedByNudge) {
   ASSERT_TRUE(scheduler()->IsBackingOff());
 
   // Before we run the scheduled canary, trigger a server connection change.
+  scheduler()->OnConnectionStatusChange();
   connection()->SetServerReachable();
   connection()->UpdateConnectionStatus();
-  scheduler()->OnConnectionStatusChange();
   scheduler()->ScheduleNudgeAsync(
       zero(), NUDGE_SOURCE_LOCAL, ModelTypeSet(BOOKMARKS), FROM_HERE);
   MessageLoop::current()->RunUntilIdle();
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index e9d8d1c..3db980a 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -1228,8 +1228,6 @@ void SyncManagerImpl::OnInvalidatorStateChange(InvalidatorState state) {
     // If the invalidator's credentials were rejected, that means that
     // our sync credentials are also bad, so invalidate those.
     connection_manager_->OnInvalidationCredentialsRejected();
-    FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
-                      OnConnectionStatusChange(CONNECTION_AUTH_ERROR));
   }
 
   if (js_event_handler_.IsInitialized()) {
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index f86b48b..37416c0 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -1343,7 +1343,7 @@ TEST_F(SyncManagerTest, OnInvalidatorStateChangeJsEvents) {
   auth_error_details.SetString("status", "CONNECTION_AUTH_ERROR");
 
   EXPECT_CALL(manager_observer_,
-              OnConnectionStatusChange(CONNECTION_AUTH_ERROR)).Times(3);
+              OnConnectionStatusChange(CONNECTION_AUTH_ERROR));
 
   EXPECT_CALL(
       event_handler,
@@ -1357,12 +1357,21 @@ TEST_F(SyncManagerTest, OnInvalidatorStateChangeJsEvents) {
   EXPECT_CALL(
       event_handler,
       HandleJsEvent("onNotificationStateChange",
-                    HasDetailsAsDictionary(credentials_rejected_details)));
+                    HasDetailsAsDictionary(credentials_rejected_details)))
+      .Times(2);
 
   EXPECT_CALL(event_handler,
               HandleJsEvent("onNotificationStateChange",
                             HasDetailsAsDictionary(transient_error_details)));
 
+  // Test needs to simulate INVALIDATION_CREDENTIALS_REJECTED with event handler
+  // attached because this is the only time when CONNECTION_AUTH_ERROR
+  // notification will be generated, therefore the only chance to verify that
+  // "onConnectionStatusChange" event is delivered
+  SetJsEventHandler(event_handler.AsWeakHandle());
+  SimulateInvalidatorStateChangeForTest(INVALIDATION_CREDENTIALS_REJECTED);
+  SetJsEventHandler(WeakHandle<JsEventHandler>());
+
   SimulateInvalidatorStateChangeForTest(INVALIDATIONS_ENABLED);
   SimulateInvalidatorStateChangeForTest(INVALIDATION_CREDENTIALS_REJECTED);
   SimulateInvalidatorStateChangeForTest(TRANSIENT_INVALIDATION_ERROR);