sync: use progress markers instead of timestamps during GetUpdates

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 5beaa80c060ed80793d0a8d255927f5beec022a2..2790009486cbb925c46780782b9629a2f83e3207 100644
--- a/chrome/browser/sync/engine/all_status.cc
+++ b/chrome/browser/sync/engine/all_status.cc
@@ -15,17 +15,22 @@
 
 namespace browser_sync {
 
-const char* AllStatus::GetSyncStatusString(SyncStatus icon) {
-  const char* strings[] = {"OFFLINE", "OFFLINE_UNSYNCED", "SYNCING", "READY",
-      "CONFLICT", "OFFLINE_UNUSABLE"};
-  COMPILE_ASSERT(arraysize(strings) == ICON_STATUS_COUNT, enum_indexed_array);
-  if (icon < 0 || icon >= static_cast<SyncStatus>(arraysize(strings)))
-    LOG(FATAL) << "Illegal Icon State:" << icon;
-  return strings[icon];
+const char* AllStatus::GetSyncStatusString(
+    sync_api::SyncManager::Status::Summary summary) {

[tim (not reviewing), 2011/01/11 19:14:23]

Did you consider moving this to syncapi too? We should really get rid of
'BuildSyncStatusSummaryText' in profile_sync_service.cc, it does the exact same
thing.

[ncarter (slow), 2011/01/13 00:06:13]

Moved this body to PSS.  This function was never actually called.

+  const char* strings[] = {"INVALID", "OFFLINE", "OFFLINE_UNSYNCED", "SYNCING",
+      "READY", "CONFLICT", "OFFLINE_UNUSABLE"};
+  COMPILE_ASSERT(arraysize(strings) ==
+                 sync_api::SyncManager::Status::SUMMARY_STATUS_COUNT,
+                 enum_indexed_array);
+  if (summary < 0 ||
+      summary >= sync_api::SyncManager::Status::SUMMARY_STATUS_COUNT) {
+    LOG(FATAL) << "Illegal Summary Value: " << summary;
+  }
+  return strings[summary];
 }
 
-static const AllStatus::Status init_status =
-  { AllStatus::OFFLINE };
+static const sync_api::SyncManager::Status init_status =
+  { sync_api::SyncManager::Status::OFFLINE };
 
 AllStatus::AllStatus() : status_(init_status) {
   status_.initial_sync_ended = true;
@@ -35,8 +40,11 @@ AllStatus::AllStatus() : status_(init_status) {
 AllStatus::~AllStatus() {
 }
 
-AllStatus::Status AllStatus::CreateBlankStatus() const {
-  Status status = status_;
+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.syncing = true;
   status.unsynced_count = 0;
   status.conflicting_count = 0;
@@ -45,12 +53,12 @@ AllStatus::Status AllStatus::CreateBlankStatus() const {
   status.max_consecutive_errors = 0;
   status.server_broken = false;
   status.updates_available = 0;
-  status.updates_received = 0;
   return status;
 }
 
-AllStatus::Status AllStatus::CalcSyncing(const SyncEngineEvent &event) const {
-  Status status = CreateBlankStatus();
+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;
@@ -72,7 +80,15 @@ AllStatus::Status AllStatus::CalcSyncing(const SyncEngineEvent &event) const {
     status.server_broken = true;
 
   status.updates_available += snapshot->num_server_changes_remaining;
-  status.updates_received += snapshot->max_local_timestamp;
+
+  // 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.

[tim (not reviewing), 2011/01/11 19:14:23]

Referencing bug 26339 here would be better than nothing.  This'll be handled as
a result of that refactoring.

[ncarter (slow), 2011/01/13 00:06:13]

Done.

+  if (event.what_happened == SyncEngineEvent::SYNC_CYCLE_ENDED) {
+    status.updates_received += snapshot->syncer_status.num_updates_downloaded;
+    status.tombstone_updates_received +=
+        snapshot->syncer_status.num_tombstone_updates_downloaded;
+  }
   return status;
 }
 
@@ -82,17 +98,17 @@ void AllStatus::CalcStatusChanges() {
     status_.server_reachable && status_.server_up && !status_.server_broken;
   if (online) {
     if (status_.syncer_stuck)
-      status_.icon = CONFLICT;
+      status_.summary = sync_api::SyncManager::Status::CONFLICT;
     else if (unsynced_changes || status_.syncing)
-      status_.icon = SYNCING;
+      status_.summary = sync_api::SyncManager::Status::SYNCING;
     else
-      status_.icon = READY;
+      status_.summary = sync_api::SyncManager::Status::READY;
   } else if (!status_.initial_sync_ended) {
-    status_.icon = OFFLINE_UNUSABLE;
+    status_.summary = sync_api::SyncManager::Status::OFFLINE_UNUSABLE;
   } else if (unsynced_changes) {
-    status_.icon = OFFLINE_UNSYNCED;
+    status_.summary = sync_api::SyncManager::Status::OFFLINE_UNSYNCED;
   } else {
-    status_.icon = OFFLINE;
+    status_.summary = sync_api::SyncManager::Status::OFFLINE;
   }
 }
 
@@ -134,7 +150,7 @@ void AllStatus::HandleServerConnectionEvent(
   }
 }
 
-AllStatus::Status AllStatus::status() const {
+sync_api::SyncManager::Status AllStatus::status() const {
   AutoLock lock(mutex_);
   return status_;
 }