sync: hook up ServerConnectionManager <> SyncerThread2 and tie up more loose ends

chrome/browser/sync/engine/syncer_thread2.cc

Index: chrome/browser/sync/engine/syncer_thread2.cc
diff --git a/chrome/browser/sync/engine/syncer_thread2.cc b/chrome/browser/sync/engine/syncer_thread2.cc
index 220f5d41700709d6727bdcef0b19de549d2b31cf..3fef27ca12fe6dd22db55c85cd41efbe89050fae 100644
--- a/chrome/browser/sync/engine/syncer_thread2.cc
+++ b/chrome/browser/sync/engine/syncer_thread2.cc
@@ -78,22 +78,64 @@ SyncerThread::~SyncerThread() {
   DCHECK(!thread_.IsRunning());
 }
 
-void SyncerThread::Start(Mode mode) {
-  if (!thread_.IsRunning() && !thread_.Start()) {
-    NOTREACHED() << "Unable to start SyncerThread.";
-    return;
+void SyncerThread::CheckServerConnectionManagerStatus(
+    HttpResponse::ServerConnectionCode code) {
+  // Note, be careful when adding cases here because if the SyncerThread
+  // 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;
+  } else if (HttpResponse::SERVER_CONNECTION_OK == code) {
+    server_connection_ok_ = true;

[lipalani1, 2011/03/25 23:18:30]

This seems to be a little odd. There is no catch all else condition probably to
leave the variable server_connection_ok_ untouched in that case.

A better approach might be to use the server_reachable_ value that we get as
part of the event listener notification.

[tim (not reviewing), 2011/03/27 19:23:19]

We surface both 'reachable' and 'server up' and 'server broken' in about:sync
and they mean 3 different things.  Not sure what you're proposing? Note,
this was copy/pasted from the existing code in syncer_thread.cc and i'm trying
to keep parity where possible.

+  }
+}
+
+void SyncerThread::Start(Mode mode, ModeChangeCallback* callback) {

[lipalani1, 2011/03/25 23:18:30]

I am assuming start can be called from only one thread(sync_core?) it might
makes sense to add a DCHECK here on the thread that is calling it.
I assuming,based on the lack of locks, it is not designed to be called from
multiple threads.

[tim (not reviewing), 2011/03/27 19:23:19]

Right.  We can't easily add a DCHECK though since the core thread is not a "well
known" browser thread.  I could build something I suppose to stash the
created-on-message loop and dcheck, but I'm not sure that's what you wanted...
One day soon I hope to eliminate core_thread altogether by exposing a
MessageLoopProxy from the syncer thread interface.

+  if (!thread_.IsRunning()) {
+    if (!thread_.Start()) {
+      NOTREACHED() << "Unable to start SyncerThread.";
+      return;
+    }
+    thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
+        this, &SyncerThread::WatchConnectionManager));
+    thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
+        this, &SyncerThread::SendInitialSnapshot));
   }
 
   thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
-      this, &SyncerThread::StartImpl, mode));
+      this, &SyncerThread::StartImpl, mode, make_linked_ptr(callback)));
 }
 
-void SyncerThread::StartImpl(Mode mode) {
+void SyncerThread::SendInitialSnapshot() {
+  DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
+  scoped_ptr<SyncSession> dummy(new SyncSession(session_context_.get(), this,
+      SyncSourceInfo(), ModelSafeRoutingInfo(),
+      std::vector<ModelSafeWorker*>()));
+  SyncEngineEvent event(SyncEngineEvent::STATUS_CHANGED);
+  sessions::SyncSessionSnapshot snapshot(dummy->TakeSnapshot());
+  event.snapshot = &snapshot;
+  session_context_->NotifyListeners(event);
+}
+
+void SyncerThread::WatchConnectionManager() {
+  ServerConnectionManager* scm = session_context_->connection_manager();
+  CheckServerConnectionManagerStatus(scm->server_status());

[lipalani1, 2011/03/25 23:18:30]

The name(CheckServerConnectionManagerStatus) seems a little confusing here. This
actually sets the server_connect_ok_ right? what does it check?

[tim (not reviewing), 2011/03/27 19:23:19]

It checks the passed-in value to figure out what to set the value to.  Agree it
could have a better name (again this was copied from syncer_thread). 
UpdateServerConnectionStatus?  That's a bit ambiguous too but maybe better.

+  scm->AddListener(this);
+}
+
+void SyncerThread::StartImpl(Mode mode,
+                             linked_ptr<ModeChangeCallback> callback) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
   DCHECK(!session_context_->account_name().empty());
   DCHECK(syncer_.get());
   mode_ = mode;
   AdjustPolling(NULL);  // Will kick start poll timer if needed.
+  if (callback.get())
+    callback->Run();

[lipalani1, 2011/03/25 23:18:30]

This callback seems to be never used? what is the purpose of this? Future use?

[tim (not reviewing), 2011/03/27 19:23:19]

See StartConfigurationMode in syncapi -- we use this when the user changes the
set of active data types to alert the DTM when it's safe to proceed with
associations.

 }
 
 bool SyncerThread::ShouldRunJob(SyncSessionJobPurpose purpose,
@@ -221,6 +263,11 @@ void SyncerThread::ScheduleNudgeImpl(const TimeDelta& delay,
     NudgeSource source, const ModelTypePayloadMap& types_with_payloads) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
   TimeTicks rough_start = TimeTicks::Now() + delay;
+  if (!ShouldRunJob(NUDGE, rough_start)) {
+    LOG(WARNING) << "Dropping nudge at scheduling time, source = "
+                 << source;
+    return;
+  }

[lipalani1, 2011/03/25 23:18:30]

Is this needed here? When we run the job we check shouldrunjob once more and in
fact at that time we may be able to run this job...

[tim (not reviewing), 2011/03/27 19:23:19]

I originally thought the same, but it turns out it doesn't  make sense to
construct a session when we're in CONFIGURATION mode because we're configuring
things like the model safe routing info.

As a result, we drop some nudges during config.  So this was what my commet in
the CL email was about.  We need to track if we dropped a nudge during config. 
We could either just run it, or optimize by checking if there are unsynced items
or a pending notification.

 
   // Note we currently nudge for all types regardless of the ones incurring
   // the nudge.  Doing different would throw off some syncer commands like
@@ -349,6 +396,8 @@ void SyncerThread::SetSyncerStepsForPurpose(SyncSessionJobPurpose purpose,
 
 void SyncerThread::DoSyncSessionJob(const SyncSessionJob& job) {
   DCHECK_EQ(MessageLoop::current(), thread_.message_loop());
+  if (!ShouldRunJob(job.purpose, job.scheduled_start))
+    return;

[lipalani1, 2011/03/25 23:18:30]

We need some kind of logging here.
I am assuming the following answers are no. But can you confirm?
1. when clear user data command is issued would we ever drop it?

[tim (not reviewing), 2011/03/27 19:23:19]

good question -- see bug 71616.  We currently do drop it but mainly for
consistency with other impl and simplicity for now, need to fix in future.

I can add a LOG WARNING here too.

 
   if (job.purpose == NUDGE) {
     DCHECK(pending_nudge_.get());
@@ -362,10 +411,8 @@ void SyncerThread::DoSyncSessionJob(const SyncSessionJob& job) {
   SetSyncerStepsForPurpose(job.purpose, &begin, &end);
 
   bool has_more_to_sync = true;
-  bool did_job = false;
   while (ShouldRunJob(job.purpose, job.scheduled_start) && has_more_to_sync) {
     VLOG(1) << "SyncerThread: Calling SyncShare.";
-    did_job = true;
     // Synchronously perform the sync session from this thread.
     syncer_->SyncShare(job.session.get(), begin, end);
     has_more_to_sync = job.session->HasMoreToSync();
@@ -373,8 +420,26 @@ void SyncerThread::DoSyncSessionJob(const SyncSessionJob& job) {
       job.session->ResetTransientState();
   }
   VLOG(1) << "SyncerThread: Done SyncShare looping.";
-  if (did_job)
-    FinishSyncSessionJob(job);
+  FinishSyncSessionJob(job);
+}
+
+void SyncerThread::UpdateCarryoverSessionState(const SyncSessionJob& old_job) {
+  if (old_job.purpose == CONFIGURATION) {
+    // Whatever types were part of a configuration task will have had updates
+    // downloaded.  For that reason, we make sure they get recorded in the
+    // event that they get disabled at a later time.
+    ModelSafeRoutingInfo r(session_context_->previous_session_routing_info());
+    if (!r.empty()) {
+      ModelSafeRoutingInfo temp_r;
+      ModelSafeRoutingInfo old_info(old_job.session->routing_info());
+      std::set_union(r.begin(), r.end(), old_info.begin(), old_info.end(),
+          std::insert_iterator<ModelSafeRoutingInfo>(temp_r, temp_r.begin()));
+      session_context_->set_previous_session_routing_info(temp_r);
+    }
+  } else {
+    session_context_->set_previous_session_routing_info(
+        old_job.session->routing_info());
+  }
 }
 
 void SyncerThread::FinishSyncSessionJob(const SyncSessionJob& job) {
@@ -391,6 +456,7 @@ void SyncerThread::FinishSyncSessionJob(const SyncSessionJob& job) {
     }
   }
   last_sync_session_end_time_ = now;
+  UpdateCarryoverSessionState(job);
   if (IsSyncingCurrentlySilenced())
     return;  // Nothing to do.
 
@@ -506,8 +572,8 @@ TimeDelta SyncerThread::GetRecommendedDelay(const TimeDelta& last_delay) {
 
 void SyncerThread::Stop() {
   syncer_->RequestEarlyExit();  // Safe to call from any thread.
+  session_context_->connection_manager()->RemoveListener(this);
   thread_.Stop();
-  Notify(SyncEngineEvent::SYNCER_THREAD_EXITING);
 }
 
 void SyncerThread::DoCanaryJob() {
@@ -577,8 +643,8 @@ void SyncerThread::OnShouldStopSyncingPermanently() {
 }
 
 void SyncerThread::OnServerConnectionEvent(
-    const ServerConnectionEvent& event) {
-  NOTIMPLEMENTED();
+    const ServerConnectionEvent2& event) {
+  CheckServerConnectionManagerStatus(event.connection_code);
 }
 
 void SyncerThread::set_notifications_enabled(bool notifications_enabled) {