Many small sync changes centered around AllStatus

* Removed num_consecutive_errors and consecutive_transient_error
counters.  These aren't very useful because they tell us nothing about
the cause of the errors.  They lead to confusing bugs, with titles like
"Need a graceful way to recover from a non-zero 'Max consecutive error
count'" (most users commenting on this bug were being throttled).

Further adding to the confusion is a bug in AllStatus that caused this
counter to be reset after every sync cycle, so the value displayed was
the maximum number of consecutive errors *for the most recent sync
cycle*.

* Removed SyncInternal::RaiseAuthNeededEvent().  This method was
undefined.

* Removed notifiable commits counter.  We no longer use P2P
notifications, so the client no longer has any say about whether or not
a commit is "notifiable".  The terminology is out of date and the code
to maintain the count seemed out of place in SyncManager.

* Added some counters to report on the number of commit cycles.  These
counters should provide all the information that used to be made
available through the notifiable commits counter, and more.

BUG=98346
TEST=


Review URL: http://codereview.chromium.org/9269025

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@120376 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 28 insertions, 146 deletions

diff --git a/chrome/browser/sync/engine/all_status.cc b/chrome/browser/sync/engine/all_status.cc
index 374c0d3..455679a 100644
--- a/chrome/browser/sync/engine/all_status.cc
+++ b/chrome/browser/sync/engine/all_status.cc
@@ -34,8 +34,8 @@ sync_api::SyncManager::Status AllStatus::CreateBlankStatus() const {
   sync_api::SyncManager::Status status = status_;
   status.unsynced_count = 0;
   status.conflicting_count = 0;
+  status.committed_count = 0;
   status.initial_sync_ended = false;
-  status.max_consecutive_errors = 0;
   status.updates_available = 0;
   return status;
 }
@@ -46,6 +46,7 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
   const sessions::SyncSessionSnapshot* snapshot = event.snapshot;
   status.unsynced_count += static_cast<int>(snapshot->unsynced_count);
   status.conflicting_count += snapshot->errors.num_conflicting_commits;
+  status.committed_count += snapshot->syncer_status.num_successful_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;
@@ -58,10 +59,6 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
 
   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;
 
@@ -82,6 +79,11 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
     } else {
       ++status.nonempty_get_updates;
     }
+    if (snapshot->syncer_status.num_successful_commits == 0) {
+      ++status.sync_cycles_without_commits;
+    } else {
+      ++status.sync_cycles_with_commits;
+    }
     if (snapshot->syncer_status.num_successful_commits == 0 &&
         snapshot->syncer_status.num_updates_downloaded_total == 0) {
       ++status.useless_sync_cycles;
@@ -156,11 +158,6 @@ void AllStatus::SetNotificationsEnabled(bool notifications_enabled) {
   status_.notifications_enabled = notifications_enabled;
 }
 
-void AllStatus::IncrementNotifiableCommits() {
-  ScopedStatusLock lock(this);
-  ++status_.notifiable_commits;
-}
-
 void AllStatus::IncrementNotificationsReceived() {
   ScopedStatusLock lock(this);
   ++status_.notifications_received;
diff --git a/chrome/browser/sync/engine/download_updates_command.cc b/chrome/browser/sync/engine/download_updates_command.cc
index ee90cbe..77a0080 100644
--- a/chrome/browser/sync/engine/download_updates_command.cc
+++ b/chrome/browser/sync/engine/download_updates_command.cc
@@ -102,7 +102,6 @@ SyncerError DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) {
   StatusController* status = session->mutable_status_controller();
   status->set_updates_request_types(enabled_types);
   if (result != SYNCER_OK) {
-    status->increment_num_consecutive_errors();
     status->mutable_updates_response()->Clear();
     LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates";
     return result;
diff --git a/chrome/browser/sync/engine/post_commit_message_command.cc b/chrome/browser/sync/engine/post_commit_message_command.cc
index 4379a80..a2b38c1 100644
--- a/chrome/browser/sync/engine/post_commit_message_command.cc
+++ b/chrome/browser/sync/engine/post_commit_message_command.cc
@@ -35,7 +35,6 @@ SyncerError PostCommitMessageCommand::ExecuteImpl(
     // set to true during BuildCommitCommand, and which may still be true.
     // Not to be confused with IS_UNSYNCED, this bit is used to detect local
     // changes to items that happen during the server Commit operation.
-    status->increment_num_consecutive_errors();
     syncable::WriteTransaction trans(FROM_HERE, syncable::SYNCER, dir);
     const vector<syncable::Id>& commit_ids = status->commit_ids();
     for (size_t i = 0; i < commit_ids.size(); i++) {
diff --git a/chrome/browser/sync/engine/process_commit_response_command.cc b/chrome/browser/sync/engine/process_commit_response_command.cc
index 43ec2ca..536db0e 100644
--- a/chrome/browser/sync/engine/process_commit_response_command.cc
+++ b/chrome/browser/sync/engine/process_commit_response_command.cc
@@ -50,13 +50,6 @@ using sessions::StatusController;
 using sessions::SyncSession;
 using sessions::ConflictProgress;
 
-void IncrementErrorCounters(StatusController* status) {
-  status->increment_num_consecutive_errors();
-}
-void ResetErrorCounters(StatusController* status) {
-  status->set_num_consecutive_errors(0);
-}
-
 ProcessCommitResponseCommand::ProcessCommitResponseCommand() {}
 ProcessCommitResponseCommand::~ProcessCommitResponseCommand() {}
 
@@ -97,7 +90,6 @@ SyncerError ProcessCommitResponseCommand::ModelNeutralExecuteImpl(
   if (!response.has_commit()) {
     // TODO(sync): What if we didn't try to commit anything?
     LOG(WARNING) << "Commit response has no commit body!";
-    IncrementErrorCounters(session->mutable_status_controller());
     return SERVER_RESPONSE_VALIDATION_FAILED;
   }
 
@@ -112,7 +104,6 @@ SyncerError ProcessCommitResponseCommand::ModelNeutralExecuteImpl(
       if (cr.entryresponse(i).has_error_message())
         LOG(ERROR) << "  " << cr.entryresponse(i).error_message();
     }
-    IncrementErrorCounters(session->mutable_status_controller());
     return SERVER_RESPONSE_VALIDATION_FAILED;
   }
   return SYNCER_OK;
@@ -198,21 +189,9 @@ SyncerError ProcessCommitResponseCommand::ProcessCommitResponse(
 
   // TODO(sync): move status reporting elsewhere.
   status->increment_num_conflicting_commits_by(conflicting_commits);
-  if (0 == successes) {
-    status->increment_num_consecutive_transient_error_commits_by(
-        transient_error_commits);
-    status->increment_num_consecutive_errors_by(transient_error_commits);
-  } else {
-    status->set_num_consecutive_transient_error_commits(0);
-    status->set_num_consecutive_errors(0);
-  }
-  int commit_count = static_cast<int>(proj.size());
-  if (commit_count != (conflicting_commits + error_commits +
-                       transient_error_commits)) {
-    ResetErrorCounters(status);
-  }
   SyncerUtil::MarkDeletedChildrenSynced(dir, &deleted_folders);
 
+  int commit_count = static_cast<int>(proj.size());
   if (commit_count == (successes + conflicting_commits)) {
     // We consider conflicting commits as a success because things will work out
     // on their own when we receive them.  Flags will be set so that
diff --git a/chrome/browser/sync/engine/process_updates_command.cc b/chrome/browser/sync/engine/process_updates_command.cc
index d9d397e..5d7ccd3 100644
--- a/chrome/browser/sync/engine/process_updates_command.cc
+++ b/chrome/browser/sync/engine/process_updates_command.cc
@@ -69,7 +69,6 @@ SyncerError ProcessUpdatesCommand::ModelChangingExecuteImpl(
   }
 
   StatusController* status = session->mutable_status_controller();
-  status->set_num_consecutive_errors(0);
   status->mutable_update_progress()->ClearVerifiedUpdates();
   return SYNCER_OK;
 }
diff --git a/chrome/browser/sync/internal_api/sync_manager.cc b/chrome/browser/sync/internal_api/sync_manager.cc
index a93e256..ff6ad07 100644
--- a/chrome/browser/sync/internal_api/sync_manager.cc
+++ b/chrome/browser/sync/internal_api/sync_manager.cc
@@ -394,10 +394,6 @@ class SyncManager::SyncInternal
   typedef base::Callback<JsArgList(const JsArgList&)> JsMessageHandler;
   typedef std::map<std::string, JsMessageHandler> JsMessageHandlerMap;
 
-  // Helper to call OnAuthError when no authentication credentials are
-  // available.
-  void RaiseAuthNeededEvent();
-
   // Helpers for SetPassphrase. TODO(rsimha): make these the public methods
   // eventually and have them replace SetPassphrase(..).
   // These correspond to setting a passphrase for decryption (when we have
@@ -690,10 +686,9 @@ SyncManager::Status::Status()
       server_reachable(false),
       notifications_enabled(false),
       notifications_received(0),
-      notifiable_commits(0),
-      max_consecutive_errors(0),
       unsynced_count(0),
       conflicting_count(0),
+      committed_count(0),
       syncing(false),
       initial_sync_ended(false),
       updates_available(0),
@@ -703,6 +698,8 @@ SyncManager::Status::Status()
       num_server_overwrites_total(0),
       nonempty_get_updates(0),
       empty_get_updates(0),
+      sync_cycles_with_commits(0),
+      sync_cycles_without_commits(0),
       useless_sync_cycles(0),
       useful_sync_cycles(0),
       cryptographer_ready(false),
@@ -1194,11 +1191,6 @@ void SyncManager::SyncInternal::MaybeSetSyncTabsInNigoriNode(
   }
 }
 
-void SyncManager::SyncInternal::RaiseAuthNeededEvent() {
-  FOR_EACH_OBSERVER(SyncManager::Observer, observers_,
-      OnAuthError(AuthError(AuthError::INVALID_GAIA_CREDENTIALS)));
-}
-
 void SyncManager::SyncInternal::SetPassphrase(
     const std::string& passphrase, bool is_explicit, bool user_provided) {
   DCHECK(user_provided || !is_explicit);
@@ -2029,7 +2021,6 @@ void SyncManager::SyncInternal::OnSyncEngineEvent(
     bool is_notifiable_commit =
         (event.snapshot->syncer_status.num_successful_commits > 0);
     if (is_notifiable_commit) {
-      allstatus_.IncrementNotifiableCommits();
       if (sync_notifier_.get()) {
         const ModelTypeSet changed_types =
             syncable::ModelTypePayloadMapToEnumSet(
diff --git a/chrome/browser/sync/internal_api/sync_manager.h b/chrome/browser/sync/internal_api/sync_manager.h
index cfb466c..afc090f 100644
--- a/chrome/browser/sync/internal_api/sync_manager.h
+++ b/chrome/browser/sync/internal_api/sync_manager.h
@@ -123,10 +123,6 @@ class SyncManager {
 
     // Notifications counters updated by the actions in synapi.
     int notifications_received;
-    int notifiable_commits;
-
-    // The max number of consecutive errors from any component.
-    int max_consecutive_errors;
 
     browser_sync::SyncProtocolError sync_protocol_error;
 
@@ -136,6 +132,9 @@ class SyncManager {
     // Number of conflicting items counted during most recent sync cycle.
     int conflicting_count;
 
+    // Number of items successfully committed during most recent sync cycle.
+    int committed_count;
+
     bool syncing;
     // True after a client has done a first sync.
     bool initial_sync_ended;
@@ -156,6 +155,10 @@ class SyncManager {
     int nonempty_get_updates;
     int empty_get_updates;
 
+    // Count of sync cycles that successfully committed items;
+    int sync_cycles_with_commits;
+    int sync_cycles_without_commits;
+
     // Count of useless and useful syncs we perform.
     int useless_sync_cycles;
     int useful_sync_cycles;
diff --git a/chrome/browser/sync/sessions/session_state.cc b/chrome/browser/sync/sessions/session_state.cc
index d9392f2..18f4e59 100644
--- a/chrome/browser/sync/sessions/session_state.cc
+++ b/chrome/browser/sync/sessions/session_state.cc
@@ -92,8 +92,6 @@ DictionaryValue* DownloadProgressMarkersToValue(
 
 ErrorCounters::ErrorCounters()
     : num_conflicting_commits(0),
-      consecutive_transient_error_commits(0),
-      consecutive_errors(0),
       last_download_updates_result(UNSET),
       last_post_commit_result(UNSET),
       last_process_commit_response_result(UNSET) {
@@ -102,9 +100,6 @@ ErrorCounters::ErrorCounters()
 DictionaryValue* ErrorCounters::ToValue() const {
   DictionaryValue* value = new DictionaryValue();
   value->SetInteger("numConflictingCommits", num_conflicting_commits);
-  value->SetInteger("consecutiveTransientErrorCommits",
-                consecutive_transient_error_commits);
-  value->SetInteger("consecutiveErrors", consecutive_errors);
   return value;
 }
 
diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h
index 72bcb00..7b96ec8 100644
--- a/chrome/browser/sync/sessions/session_state.h
+++ b/chrome/browser/sync/sessions/session_state.h
@@ -92,15 +92,6 @@ struct ErrorCounters {
 
   int num_conflicting_commits;
 
-  // Number of commits hitting transient errors since the last successful
-  // commit.
-  int consecutive_transient_error_commits;
-
-  // Incremented when get_updates fails, commit fails, and when hitting
-  // transient errors. When any of these succeed, this counter is reset.
-  // TODO(chron): Reduce number of weird counters we use.
-  int consecutive_errors;
-
   // Any protocol errors that we received during this sync session.
   SyncProtocolError sync_protocol_error;
 
diff --git a/chrome/browser/sync/sessions/session_state_unittest.cc b/chrome/browser/sync/sessions/session_state_unittest.cc
index 3220dd1..244ece8e 100644
--- a/chrome/browser/sync/sessions/session_state_unittest.cc
+++ b/chrome/browser/sync/sessions/session_state_unittest.cc
@@ -72,17 +72,11 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
 TEST_F(SessionStateTest, ErrorCountersToValue) {
   ErrorCounters counters;
   counters.num_conflicting_commits = 1;
-  counters.consecutive_transient_error_commits = 5;
-  counters.consecutive_errors = 3;
 
   scoped_ptr<DictionaryValue> value(counters.ToValue());
-  EXPECT_EQ(3u, value->size());
+  EXPECT_EQ(1u, value->size());
   ExpectDictIntegerValue(counters.num_conflicting_commits,
                          *value, "numConflictingCommits");
-  ExpectDictIntegerValue(counters.consecutive_transient_error_commits,
-                         *value, "consecutiveTransientErrorCommits");
-  ExpectDictIntegerValue(counters.consecutive_errors,
-                         *value, "consecutiveErrors");
 }
 
 TEST_F(SessionStateTest, DownloadProgressMarkersToValue) {
diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc
index 95c8c4f..ca9903d 100644
--- a/chrome/browser/sync/sessions/status_controller.cc
+++ b/chrome/browser/sync/sessions/status_controller.cc
@@ -127,26 +127,6 @@ void StatusController::reset_num_conflicting_commits() {
     shared_.error.mutate()->num_conflicting_commits = 0;
 }
 
-void StatusController::set_num_consecutive_transient_error_commits(int value) {
-  if (shared_.error.value().consecutive_transient_error_commits !=
-      value) {
-    shared_.error.mutate()->consecutive_transient_error_commits =
-        value;
-  }
-}
-
-void StatusController::increment_num_consecutive_transient_error_commits_by(
-    int value) {
-  set_num_consecutive_transient_error_commits(
-      shared_.error.value().consecutive_transient_error_commits +
-      value);
-}
-
-void StatusController::set_num_consecutive_errors(int value) {
-  if (shared_.error.value().consecutive_errors != value)
-    shared_.error.mutate()->consecutive_errors = value;
-}
-
 void StatusController::set_num_server_changes_remaining(
     int64 changes_remaining) {
   if (shared_.num_server_changes_remaining.value() != changes_remaining)
@@ -174,16 +154,6 @@ void StatusController::set_unsynced_handles(
   }
 }
 
-void StatusController::increment_num_consecutive_errors() {
-  set_num_consecutive_errors(
-      shared_.error.value().consecutive_errors + 1);
-}
-
-void StatusController::increment_num_consecutive_errors_by(int value) {
-  set_num_consecutive_errors(
-      shared_.error.value().consecutive_errors + value);
-}
-
 void StatusController::increment_num_successful_bookmark_commits() {
   set_num_successful_bookmark_commits(
       shared_.syncer_status.value().num_successful_bookmark_commits + 1);
diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h
index b072577..651e90c6 100644
--- a/chrome/browser/sync/sessions/status_controller.h
+++ b/chrome/browser/sync/sessions/status_controller.h
@@ -207,11 +207,6 @@ class StatusController {
   // A toolbelt full of methods for updating counters and flags.
   void increment_num_conflicting_commits_by(int value);
   void reset_num_conflicting_commits();
-  void set_num_consecutive_transient_error_commits(int value);
-  void increment_num_consecutive_transient_error_commits_by(int value);
-  void set_num_consecutive_errors(int value);
-  void increment_num_consecutive_errors();
-  void increment_num_consecutive_errors_by(int value);
   void set_num_server_changes_remaining(int64 changes_remaining);
   void set_invalid_store(bool invalid_store);
   void set_num_successful_bookmark_commits(int value);
diff --git a/chrome/browser/sync/sessions/status_controller_unittest.cc b/chrome/browser/sync/sessions/status_controller_unittest.cc
index 1b3c17f..a207893 100644
--- a/chrome/browser/sync/sessions/status_controller_unittest.cc
+++ b/chrome/browser/sync/sessions/status_controller_unittest.cc
@@ -28,25 +28,6 @@ TEST_F(StatusControllerTest, GetsDirty) {
   status.increment_num_conflicting_commits_by(1);
   EXPECT_TRUE(status.TestAndClearIsDirty());
 
-  status.set_num_consecutive_transient_error_commits(1);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-
-  status.increment_num_consecutive_transient_error_commits_by(1);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  status.increment_num_consecutive_transient_error_commits_by(0);
-  EXPECT_FALSE(status.TestAndClearIsDirty());
-
-  status.set_num_consecutive_errors(10);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  status.set_num_consecutive_errors(10);
-  EXPECT_FALSE(status.TestAndClearIsDirty());  // Only dirty if value changed.
-  status.increment_num_consecutive_errors();
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  status.increment_num_consecutive_errors_by(1);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  status.increment_num_consecutive_errors_by(0);
-  EXPECT_FALSE(status.TestAndClearIsDirty());
-
   status.set_num_server_changes_remaining(30);
   EXPECT_TRUE(status.TestAndClearIsDirty());
 
@@ -98,20 +79,6 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   status.increment_num_conflicting_commits_by(1);
   EXPECT_EQ(1, status.error().num_conflicting_commits);
 
-  status.set_num_consecutive_transient_error_commits(6);
-  EXPECT_EQ(6, status.error().consecutive_transient_error_commits);
-  status.increment_num_consecutive_transient_error_commits_by(1);
-  EXPECT_EQ(7, status.error().consecutive_transient_error_commits);
-  status.increment_num_consecutive_transient_error_commits_by(0);
-  EXPECT_EQ(7, status.error().consecutive_transient_error_commits);
-
-  status.set_num_consecutive_errors(8);
-  EXPECT_EQ(8, status.error().consecutive_errors);
-  status.increment_num_consecutive_errors();
-  EXPECT_EQ(9, status.error().consecutive_errors);
-  status.increment_num_consecutive_errors_by(2);
-  EXPECT_EQ(11, status.error().consecutive_errors);
-
   status.set_num_server_changes_remaining(13);
   EXPECT_EQ(13, status.num_server_changes_remaining());
 
diff --git a/chrome/browser/sync/sync_ui_util.cc b/chrome/browser/sync/sync_ui_util.cc
index 8cf7b78..3dd7571 100644
--- a/chrome/browser/sync/sync_ui_util.cc
+++ b/chrome/browser/sync/sync_ui_util.cc
@@ -543,15 +543,15 @@ void ConstructAboutInformation(ProfileSyncService* service,
                                    "Notifications Received",
                                    full_status.notifications_received);
     sync_ui_util::AddIntSyncDetail(details,
-                                   "Notifiable Commits",
-                                   full_status.notifiable_commits);
-    sync_ui_util::AddIntSyncDetail(details,
                                    "Unsynced Count",
                                    full_status.unsynced_count);
     sync_ui_util::AddIntSyncDetail(details,
                                    "Conflicting Count",
                                    full_status.conflicting_count);
     sync_ui_util::AddIntSyncDetail(details,
+                                   "Committed Items (this session)",
+                                   full_status.committed_count);
+    sync_ui_util::AddIntSyncDetail(details,
                                    "Local Overwrites",
                                    full_status.num_local_overwrites_total);
     sync_ui_util::AddIntSyncDetail(details,
@@ -571,15 +571,18 @@ void ConstructAboutInformation(ProfileSyncService* service,
                                    "Updates Downloaded (Tombstones)",
                                    full_status.tombstone_updates_received);
     sync_ui_util::AddIntSyncDetail(details,
-                                   "Max Consecutive Errors",
-                                   full_status.max_consecutive_errors);
-    sync_ui_util::AddIntSyncDetail(details,
                                    "Empty GetUpdates",
                                    full_status.empty_get_updates);
     sync_ui_util::AddIntSyncDetail(details,
                                    "Nonempty GetUpdates",
                                    full_status.nonempty_get_updates);
     sync_ui_util::AddIntSyncDetail(details,
+                                   "Sync Cycles with Successful Commits",
+                                   full_status.sync_cycles_with_commits);
+    sync_ui_util::AddIntSyncDetail(details,
+                                   "Sync Cycles without Successful Commits",
+                                   full_status.sync_cycles_without_commits);
+    sync_ui_util::AddIntSyncDetail(details,
                                    "Useless Sync Cycles",
                                    full_status.useless_sync_cycles);
     sync_ui_util::AddIntSyncDetail(details,