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

chrome/browser/sync/engine/all_status.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/engine/all_status.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/port.h"
 #include "chrome/browser/sync/engine/net/server_connection_manager.h"
 #include "chrome/browser/sync/protocol/service_constants.h"
 #include "chrome/browser/sync/sessions/session_state.h"
 #include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/model_type.h"
 
 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;
 }
 
 AllStatus::~AllStatus() {
 }
 
 sync_api::SyncManager::Status AllStatus::CreateBlankStatus() const {
   // Status is initialized with the previous status value.  Variables
   // whose values accumulate (e.g. lifetime counters like updates_received)
   // are not to be cleared here.
   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;
 }
 
 sync_api::SyncManager::Status AllStatus::CalcSyncing(
     const SyncEngineEvent &event) const {
   sync_api::SyncManager::Status status = CreateBlankStatus();
   const sessions::SyncSessionSnapshot* snapshot = event.snapshot;
   status.unsynced_count += static_cast<int>(snapshot->unsynced_count);
   status.conflicting_count += snapshot->errors.num_conflicting_commits;
   // The syncer may not be done yet, which could cause conflicting updates.
   // But this is only used for status, so it is better to have visibility.
   status.conflicting_count += snapshot->num_conflicting_updates;
 
   if (event.what_happened == SyncEngineEvent::SYNC_CYCLE_BEGIN) {
     status.syncing = true;
   } else if (event.what_happened == SyncEngineEvent::SYNC_CYCLE_ENDED) {
     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;
 
   status.updates_available += snapshot->num_server_changes_remaining;
   status.sync_protocol_error = snapshot->errors.sync_protocol_error;
 
   // Accumulate update count only once per session to avoid double-counting.
   // 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 +=
         snapshot->syncer_status.num_tombstone_updates_downloaded_total;
     status.num_local_overwrites_total +=
         snapshot->syncer_status.num_local_overwrites;
     status.num_server_overwrites_total +=
         snapshot->syncer_status.num_server_overwrites;
     if (snapshot->syncer_status.num_updates_downloaded_total == 0) {
       ++status.empty_get_updates;
     } else {
       ++status.nonempty_get_updates;
     }
     if (snapshot->syncer_status.num_successful_commits == 0 &&
         snapshot->syncer_status.num_updates_downloaded_total == 0) {
       ++status.useless_sync_cycles;
     } else {
       ++status.useful_sync_cycles;
     }
   }
   return status;
 }
 
 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.

 }
 
 void AllStatus::OnSyncEngineEvent(const SyncEngineEvent& event) {
   ScopedStatusLock lock(this);
   switch (event.what_happened) {
     case SyncEngineEvent::SYNC_CYCLE_BEGIN:
     case SyncEngineEvent::STATUS_CHANGED:
     case SyncEngineEvent::SYNC_CYCLE_ENDED:
       status_ = CalcSyncing(event);
       break;
@@ skipping 69 matching lines @@
     : allstatus_(allstatus) {
   allstatus->mutex_.Acquire();
 }
 
 ScopedStatusLock::~ScopedStatusLock() {
   allstatus_->CalcStatusChanges();
   allstatus_->mutex_.Release();
 }
 
 }  // namespace browser_sync