[Sync] - In about:sync page display if the first sync after restart has taken place or not.

chrome/browser/sync/engine/all_status.cc

Index: chrome/browser/sync/engine/all_status.cc
diff --git a/chrome/browser/sync/engine/all_status.cc b/chrome/browser/sync/engine/all_status.cc
index 374c0d35a42050e0c8944af7675a2aae7e400956..8a8ed40b9e6dcf55cfb42a15f127edd7ef1dad60 100644
--- a/chrome/browser/sync/engine/all_status.cc
+++ b/chrome/browser/sync/engine/all_status.cc
@@ -17,8 +17,7 @@
 namespace browser_sync {
 
 AllStatus::AllStatus() {
-  status_.summary = sync_api::SyncManager::Status::OFFLINE;
-  status_.initial_sync_ended = true;
+  status_.summary = sync_api::SyncManager::Status::UNININITIALIZED;
   status_.notifications_enabled = false;
   status_.cryptographer_ready = false;
   status_.crypto_has_pending_keys = false;
@@ -34,7 +33,6 @@ sync_api::SyncManager::Status AllStatus::CreateBlankStatus() const {
   sync_api::SyncManager::Status status = status_;
   status.unsynced_count = 0;
   status.conflicting_count = 0;
-  status.initial_sync_ended = false;

[rlarocque, 2012/02/02 22:44:13]

How is this change related to the intentions described in the commit message?

[lipalani1, 2012/02/03 01:55:39]

I thought about it some more. I had few reasons for removing this but have to
agree with you that may be it should be a patch by itself.
On 2012/02/02 22:44:13, rlarocque wrote:

   status.max_consecutive_errors = 0;
   status.updates_available = 0;
   return status;
@@ -56,8 +54,6 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
     status.syncing = false;
   }
 
-  status.initial_sync_ended |= snapshot->is_share_usable;
-
   const sessions::ErrorCounters& errors(snapshot->errors);
   if (errors.consecutive_errors > status.max_consecutive_errors)
     status.max_consecutive_errors = errors.consecutive_errors;
@@ -69,6 +65,7 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
   // TODO(ncarter): Make this realtime by having the syncer_status
   // counter preserve its value across sessions.  http://crbug.com/26339
   if (event.what_happened == SyncEngineEvent::SYNC_CYCLE_ENDED) {
+    ++status.sync_count;
     status.updates_received +=
         snapshot->syncer_status.num_updates_downloaded_total;
     status.tombstone_updates_received +=
@@ -94,19 +91,17 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
 
 void AllStatus::CalcStatusChanges() {
   const bool unsynced_changes = status_.unsynced_count > 0;
-  const bool online = status_.authenticated &&
+  const bool online = status_.sync_count > 0 && status_.authenticated &&
     status_.server_reachable && status_.server_up;
   if (online) {
     if (status_.syncing)
       status_.summary = sync_api::SyncManager::Status::SYNCING;
     else
       status_.summary = sync_api::SyncManager::Status::READY;
-  } else if (!status_.initial_sync_ended) {
-    status_.summary = sync_api::SyncManager::Status::OFFLINE_UNUSABLE;
   } else if (unsynced_changes) {
     status_.summary = sync_api::SyncManager::Status::OFFLINE_UNSYNCED;
-  } else {
-    status_.summary = sync_api::SyncManager::Status::OFFLINE;
+  } else if (status_.sync_count > 0) {
+    status_.summary = sync_api::SyncManager::Status::OFFLINE_UNUSABLE;

[rlarocque, 2012/02/02 22:44:13]

Why?  This would make the OFFLINE_UNSYNCED branch a higher priority than the
OFFLINE_UNUSABLE branch, meaning that in many cases where we used to display
OFFLINE_UNUSABLE, we will now display OFFLINE_UNSYNCED.

I'm not necessarily against the change, but I'd like to know why it's being
made.

[lipalani1, 2012/02/03 01:55:39]

If you look at the comments for this enum OFFLINE UNSYNCED means there are local
changes and sync failed. OFFLINE UNUSABLE means there are no local changes and
sync failed. Poor choice of names and probably a better way of expression them
would be as 2 seperate variables. one just saying offline and other saying
unsynced is true/false. but doing that would be beyond the scope of this CL. I
will add a todo.
On 2012/02/02 22:44:13, rlarocque wrote:

[rlarocque, 2012/02/03 18:55:03]

OK, but this is still a big change to the definition of OFFLINE_UNUSABLE. 
Whereas OFFLINE_UNUSABLE used to be based on sync setup having failed or not yet
completed (!status_.initial_sync_ended), this bases it off of the number of
syncs since last restart.  Could you make defer this change to that separate CL
with the other initial_sync_ended changes?  

btw, I've seen the most recent patch set (#3).  I don't think removing the state
is the right way to go, either.

   }

[rlarocque, 2012/02/02 22:44:13]

For completeness, I'd like to see an else branch that handles the
status_.sync_count == 0 case.  That should make it more clear to readers that
there is a specific summary value intended to be used when we haven't attempted
to sync yet.  Otherwise, this behaviour might look like an accident.

[lipalani1, 2012/02/03 01:55:39]

Done.

 }