Trim code from sync's ServerConnectionManager

chrome/browser/sync/engine/sync_scheduler.cc

Index: chrome/browser/sync/engine/sync_scheduler.cc
diff --git a/chrome/browser/sync/engine/sync_scheduler.cc b/chrome/browser/sync/engine/sync_scheduler.cc
index dafe685110934deb65a4cc439555877d64ce96e0..e9955a9901e716f22588eb3d8111a13e22d887f5 100644
--- a/chrome/browser/sync/engine/sync_scheduler.cc
+++ b/chrome/browser/sync/engine/sync_scheduler.cc
@@ -194,7 +194,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),
@@ -230,9 +229,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()));
@@ -244,7 +241,6 @@ 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;
 
@@ -256,20 +252,12 @@ void SyncScheduler::UpdateServerConnectionManagerStatus(
   // 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;

[lipalani1, 2012/02/11 00:12:10]

There is some value to this log statement. You can still implement it based on
the connection code alone. if it is difficult(i dont think it is.) then it is
fine to remove it.

[rlarocque, 2012/02/11 01:31:36]

This log statement exists specifically to tell us about status changes.  That
made sense when we actually did something in response to status changes, but we
no longer take any action based on this.  

The logs in the if statement above will tell us what the status is now.  I don't
see why we would need to add logs specifically for tracking cases where that
status changes.  If you *really* need to see the transitions, you can find them
by grepping the logs.

-  }
 }
 
 void SyncScheduler::Start(Mode mode, const base::Closure& callback) {
@@ -385,10 +373,10 @@ SyncScheduler::JobProcessDecision SyncScheduler::DecideOnJob(
     return DROP;
   }
 
-  if (server_connection_ok_)
+  if (!session_context_->connection_manager()->HasInvalidAuthToken())

[rlarocque, 2012/02/10 23:16:06]

You might be thinking that this condition is not the same as the old one.  You
would be right.  

We only reach this if statement when we're not in backoff.  If we were, we would
have hit the DecideWhileInWaitInterval() branch on line 355.  

So this if statement concerns the case where we're not in backoff, but
!server_connection_ok_.  That's a really strange state to be in.  I suppose that
could happen if a non-retryable job failed (ie. a POLL after issue 112954 is
fixed, CONFIGURE jobs).

I think the new logic will do a better job of detecting cases where we don't
want to even try to connect to the server.  Having no valid auth token is the
only scenario I can think of where it makes sense to not even try connecting to
the server, despite not being in backoff.  I'd prefer to err on the side of
CONTINUE rather than SAVE or DROP the job, since we'll usually up going into
backoff if it fails.  

[lipalani1, 2012/02/11 00:12:10]

Yep it was designed to capture cases where this variable was updated outside of
sync. It is fine to sync and figure out whether we have the connection.
On 2012/02/10 23:16:06, rlarocque wrote:

     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;
 }
 
@@ -852,15 +840,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());