Trim code form sync's ServerConnectionManager

- Remove 'authenticated' from SyncManager::Status.  This variable was
set to true iff the last attempt to contact the sync server resulted in
a 200 OK response.  There are lots of problems with this approach.
See also: crbug.com/117611.

The about:sync page also displays auth errors with
MakeSyncAuthErrorText(), which is much more likely to be accurate

- Remove server_up from SyncManager::Status.  It used to be set to true
iff the last server response was SERVER_CONNECTION_OK.  That doesn't
make a lot of sense and isn't very useful.

- Remove server_reachable from SyncManager::Status.  This was toggled
only when ServerConnectionManager::CheckServerReachable() failed, which
didn't happen very often.  CheckServerReachable() has since been
removed, so I think it now gets set only at startup and on the first
successful contact with the server.

- Remove server_reachable from ServerConnectionEvent and
ServerConnectionManager, for similar reasons.

- Remove the methods CheckTime(), IsServerReachable(),
IsUserAuthenticated(), and CheckServerReachable(),
SetServerParameters(), server_reachable() from ServerConnectionManager.
These methods were not used anywhere.  Also removed
IsGoodReplyFromServer(), whose last remaining callsite has been removed
in this commit.

- Remove the server_connection_ok_ flag from SyncScheduler.

- Hard-code AllStatus' online status to true.  See TODO comment.

- Remove OnCredentialsUpdated() and OnConnectionStatusChange() from
SyncScheduler.  Their functionality has been moved to the SyncManager.

- Rearrange some code in MockConnectionManager.  Apparently there used
to be such a thing as an "AUTHENTICATE" request, which we had unit tests
for.  That no longer exists, so I've rewritten some parts of this mock
class to better relfect ServerConnectionManager's current authentication
mechanisms.

- Update various unit tests in response to these changes.

- Update some integration tests to verify backoff behaviour when the
sync server is unreachable.  This replaces the old behaviour of
verifying that certain flags were set in the sync snapshot.

- Reduce the number of retries required to verify backoff.  This is to
help ensure that our integration tests fall within the 45s time limit.

- Disable notifications when disabling network connectivity in
integration tests.  This improves the reliability of exponential backoff
verification because the notifications would sometimes interfere with
backoff logic.


BUG=110954,105739,98346,106262
TEST=


Review URL: http://codereview.chromium.org/9348036

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

1 files changed, 9 insertions, 37 deletions

diff --git a/sync/engine/sync_scheduler.cc b/sync/engine/sync_scheduler.cc
index f03b727..87304ea 100644
--- a/sync/engine/sync_scheduler.cc
+++ b/sync/engine/sync_scheduler.cc
@@ -195,7 +195,6 @@ SyncScheduler::SyncScheduler(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.
-      server_connection_ok_(true),
       connection_code_(HttpResponse::SERVER_CONNECTION_OK),
       delay_provider_(new DelayProvider()),
       syncer_(syncer),
@@ -231,9 +230,7 @@ void SyncScheduler::OnConnectionStatusChange() {
 }
 
 void SyncScheduler::OnServerConnectionErrorFixed() {
-  DCHECK(!server_connection_ok_);
   connection_code_ = HttpResponse::SERVER_CONNECTION_OK;
-  server_connection_ok_ = true;
   PostTask(FROM_HERE, "DoCanaryJob",
            base::Bind(&SyncScheduler::DoCanaryJob,
                       weak_ptr_factory_.GetWeakPtr()));
@@ -245,32 +242,8 @@ void SyncScheduler::UpdateServerConnectionManagerStatus(
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SDVLOG(2) << "New server connection code: "
             << HttpResponse::GetServerConnectionCodeString(code);
-  bool old_server_connection_ok = server_connection_ok_;
 
   connection_code_ = code;
-
-  // Note, be careful when adding cases here because if the SyncScheduler
-  // thinks there is no valid connection as determined by this method, it
-  // will drop out of *all* forward progress sync loops (it won't poll and it
-  // will queue up Talk notifications but not actually call SyncShare) until
-  // some external action causes a ServerConnectionManager to broadcast that
-  // a valid connection has been re-established
-  if (HttpResponse::CONNECTION_UNAVAILABLE == code ||
-      HttpResponse::SYNC_AUTH_ERROR == code) {
-    server_connection_ok_ = false;
-    SDVLOG(2) << "Sync auth error or unavailable connection: "
-              << "server connection is down";
-  } else if (HttpResponse::SERVER_CONNECTION_OK == code) {
-    server_connection_ok_ = true;
-    SDVLOG(2) << "Sync server connection is ok: "
-              << "server connection is up, doing canary job";
-  }
-
-  if (old_server_connection_ok != server_connection_ok_) {
-    const char* transition =
-        server_connection_ok_ ? "down -> up" : "up -> down";
-    SDVLOG(2) << "Server connection changed: " << transition;
-  }
 }
 
 void SyncScheduler::Start(Mode mode, const base::Closure& callback) {
@@ -403,10 +376,10 @@ SyncScheduler::JobProcessDecision SyncScheduler::DecideOnJob(
     return DROP;
   }
 
-  if (server_connection_ok_)
+  if (!session_context_->connection_manager()->HasInvalidAuthToken())
     return CONTINUE;
 
-  SDVLOG(2) << "Bad server connection. Using that to decide on job.";
+  SDVLOG(2) << "No valid auth token. Using that to decide on job.";
   return job.purpose == SyncSessionJob::NUDGE ? SAVE : DROP;
 }
 
@@ -873,15 +846,14 @@ void SyncScheduler::FinishSyncSessionJob(const SyncSessionJob& job) {
   }
   last_sync_session_end_time_ = now;
 
-  // 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.
+  // 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.
-  // But that would need little more refactoring(including a method to
-  // query if the auth token is invalid) from SCM side.
+  // 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());