Change definition of "syncing" in AllStatus

This affects some stats displayed in about:sync.

Due to a logic error, syncing was defined as 'unsynced_handles > 0 &&
!throttled'.  This seems sort of reasonable, until you notice that the
unsynced_handles value is zero at the beginning of the sync cycle and is
not updated for successful commits that occur during the sync cycle.
And that's just the tip of the iceberg.

This change makes it so that 'syncing' is an edge-triggered value.  When
a sync cycle starts or ends, it will send a SYNC_CYCLE_BEGIN or
SYNC_CYCLE_END event to AllStatus, which will update the flag
accordingly.

This results in a different definition of 'syncing' than the previous
system.  Whereas the old system tried to infer what the SyncScheduler
was doing and remain in the syncing state until it had fully completed
its job, this one will report the 'syncing' state only if a call to
SyncShare() is currently in progress.  One of the implications of this
is that it will now toggle in and out of the syncing state when
committing more than 25 items, because the loop to commit all unsynced
items in batches of 25 currently occurs outside SyncShare.  The new
system will also bounce in an out of the syncing state as we handle the
SYNC_CYCLE_CONTINUATION cycle that follows every cycle that commits an
item.

How does this look in about:sync?  The biggest change is that it will
now be almost impossible to see the status summary of "SYNCING".  This
is because the ProfileSyncService does not receive notification of the
beginning of a sync cycle, only the end.  So it doesn't bother to update
the state of about:sync until the syncing is already completed.  Prior
to this change, about:sync did show the "SYNCING" summary sometimes, but
this was only during the *second* sync cycle, which we enter only because
of the SYNC_CYCLE_CONTINUATION bug (issue 94670).

Also in this commit are changes to remove the infrastructure used to
set the 'syncing' variable in the SyncSessionSnapshot.  This had the
side-effect of removing the mechanism by which we're guaranteed to kick
AllStatus with a STATUS_CHANGED event, so I replaced it with a
SYNC_CYCLE_BEGIN event sent from the SYNCER_BEGIN step.  I also removed
SyncerEndCommand, because the mechanims used to send the
SYNC_CYCLE_BEGIN event could be used to send the SYNC_CYCLE_ENDED
event, too.  

BUG=98346
TEST=


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

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

17 files changed, 44 insertions, 124 deletions

diff --git a/chrome/browser/sync/engine/all_status.cc b/chrome/browser/sync/engine/all_status.cc
index 98f0e00..7769fec 100644
--- a/chrome/browser/sync/engine/all_status.cc
+++ b/chrome/browser/sync/engine/all_status.cc
@@ -32,7 +32,6 @@ sync_api::SyncManager::Status AllStatus::CreateBlankStatus() const {
   // 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;
   status.initial_sync_ended = false;
@@ -53,9 +52,12 @@ sync_api::SyncManager::Status AllStatus::CalcSyncing(
   // But this is only used for status, so it is better to have visibility.
   status.conflicting_count += snapshot->num_conflicting_updates;
 
-  status.syncing |= snapshot->syncer_status.sync_in_progress;
-  status.syncing =
-      snapshot->has_more_to_sync && snapshot->is_silenced;
+  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;
   status.syncer_stuck |= snapshot->syncer_status.syncer_stuck;
 
@@ -105,7 +107,7 @@ void AllStatus::CalcStatusChanges() {
   if (online) {
     if (status_.syncer_stuck)
       status_.summary = sync_api::SyncManager::Status::CONFLICT;
-    else if (unsynced_changes || status_.syncing)
+    else if (status_.syncing)
       status_.summary = sync_api::SyncManager::Status::SYNCING;
     else
       status_.summary = sync_api::SyncManager::Status::READY;
@@ -121,8 +123,9 @@ void AllStatus::CalcStatusChanges() {
 void AllStatus::OnSyncEngineEvent(const SyncEngineEvent& event) {
   ScopedStatusLock lock(this);
   switch (event.what_happened) {
-    case SyncEngineEvent::SYNC_CYCLE_ENDED:
+    case SyncEngineEvent::SYNC_CYCLE_BEGIN:
     case SyncEngineEvent::STATUS_CHANGED:
+    case SyncEngineEvent::SYNC_CYCLE_ENDED:
       status_ = CalcSyncing(event);
       break;
     case SyncEngineEvent::STOP_SYNCING_PERMANENTLY:
diff --git a/chrome/browser/sync/engine/all_status.h b/chrome/browser/sync/engine/all_status.h
index 17f20b5..8b0299c 100644
--- a/chrome/browser/sync/engine/all_status.h
+++ b/chrome/browser/sync/engine/all_status.h
@@ -24,6 +24,17 @@ class ScopedStatusLock;
 struct AuthWatcherEvent;
 struct ServerConnectionEvent;
 
+// TODO(rlarocque):
+// Most of this data ends up on the about:sync page.  But the page is only
+// 'pinged' to update itself at the end of a sync cycle.  A user could refresh
+// manually, but unless their timing is excellent it's unlikely that a user will
+// see any state in mid-sync cycle.  We have no plans to change this.
+//
+// What we do intend to do is improve the UI so that changes following a sync
+// cycle are more visible.  Without such a change, the status summary for a
+// healthy syncer will constantly display as "READY" and never provide any
+// indication of a sync cycle being performed.  See crbug.com/108100.
+
 class AllStatus : public SyncEngineEventListener {
   friend class ScopedStatusLock;
  public:
diff --git a/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc b/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc
index f6ad509..4e02c33 100644
--- a/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc
+++ b/chrome/browser/sync/engine/cleanup_disabled_types_command_unittest.cc
@@ -6,7 +6,6 @@
 
 #include "chrome/browser/sync/engine/cleanup_disabled_types_command.h"
 
-#include "chrome/browser/sync/engine/syncer_end_command.h"
 #include "chrome/browser/sync/sessions/sync_session.h"
 #include "chrome/browser/sync/syncable/model_type_test_util.h"
 #include "chrome/browser/sync/test/engine/syncer_command_test.h"
@@ -30,11 +29,6 @@ class CleanupDisabledTypesCommandTest : public MockDirectorySyncerCommandTest {
     (*mutable_routing_info())[syncable::BOOKMARKS] = GROUP_PASSIVE;
     MockDirectorySyncerCommandTest::SetUp();
   }
-
-  // Overridden to allow SyncerEndCommand Execute to work.
-  virtual bool IsSyncingCurrentlySilenced() {
-    return false;
-  }
 };
 
 // TODO(tim): Add syncer test to verify previous routing info is set.
diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc
index ae97c5d..f6490c5 100644
--- a/chrome/browser/sync/engine/syncer.cc
+++ b/chrome/browser/sync/engine/syncer.cc
@@ -23,7 +23,6 @@
 #include "chrome/browser/sync/engine/process_updates_command.h"
 #include "chrome/browser/sync/engine/resolve_conflicts_command.h"
 #include "chrome/browser/sync/engine/store_timestamps_command.h"
-#include "chrome/browser/sync/engine/syncer_end_command.h"
 #include "chrome/browser/sync/engine/syncer_types.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/engine/verify_updates_command.h"
@@ -82,17 +81,6 @@ const char* SyncerStepToString(const SyncerStep step)
 }
 #undef ENUM_CASE
 
-Syncer::ScopedSyncStartStopTracker::ScopedSyncStartStopTracker(
-    sessions::SyncSession* session) : session_(session) {
-  session_->mutable_status_controller()->
-      SetSyncInProgressAndUpdateStartTime(true);
-}
-
-Syncer::ScopedSyncStartStopTracker::~ScopedSyncStartStopTracker() {
-  session_->mutable_status_controller()->
-      SetSyncInProgressAndUpdateStartTime(false);
-}
-
 Syncer::Syncer()
     : early_exit_requested_(false) {
 }
@@ -121,8 +109,7 @@ void Syncer::SyncShare(sessions::SyncSession* session,
 
   ScopedSessionContextConflictResolver scoped(session->context(),
                                               &resolver_);
-
-  ScopedSyncStartStopTracker start_stop_tracker(session);
+  session->mutable_status_controller()->UpdateStartTime();
   SyncerStep current_step = first_step;
 
   SyncerStep next_step = current_step;
@@ -145,6 +132,8 @@ void Syncer::SyncShare(sessions::SyncSession* session,
         session->context()->extensions_monitor()->GetAndClearRecords(
             session->mutable_extensions_activity());
         session->context()->PruneUnthrottledTypes(base::TimeTicks::Now());
+        session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_BEGIN);
+
         next_step = CLEANUP_DISABLED_TYPES;
         break;
       case CLEANUP_DISABLED_TYPES: {
@@ -295,8 +284,7 @@ void Syncer::SyncShare(sessions::SyncSession* session,
         break;
       }
       case SYNCER_END: {
-        SyncerEndCommand syncer_end_command;
-        syncer_end_command.Execute(session);
+        session->SendEventNotification(SyncEngineEvent::SYNC_CYCLE_ENDED);
         next_step = SYNCER_END;
         break;
       }
diff --git a/chrome/browser/sync/engine/syncer.h b/chrome/browser/sync/engine/syncer.h
index b8b1010..9a17025 100644
--- a/chrome/browser/sync/engine/syncer.h
+++ b/chrome/browser/sync/engine/syncer.h
@@ -74,16 +74,6 @@ class Syncer {
                          SyncerStep first_step,
                          SyncerStep last_step);
 
-  class ScopedSyncStartStopTracker {
-   public:
-    explicit ScopedSyncStartStopTracker(sessions::SyncSession* session);
-    ~ScopedSyncStartStopTracker();
-   private:
-    sessions::SyncSession* session_;
-
-    DISALLOW_COPY_AND_ASSIGN(ScopedSyncStartStopTracker);
-  };
-
  private:
   // Implements the PROCESS_CLIENT_COMMAND syncer step.
   void ProcessClientCommand(sessions::SyncSession* session);
diff --git a/chrome/browser/sync/engine/syncer_end_command.cc b/chrome/browser/sync/engine/syncer_end_command.cc
deleted file mode 100644
index 6e5feb4..0000000
--- a/chrome/browser/sync/engine/syncer_end_command.cc
+++ /dev/null
@@ -1,27 +0,0 @@
-// Copyright (c) 2011 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/syncer_end_command.h"
-
-#include "base/logging.h"
-#include "chrome/browser/sync/engine/syncer_types.h"
-#include "chrome/browser/sync/sessions/sync_session.h"
-#include "chrome/browser/sync/syncable/directory_manager.h"
-
-namespace browser_sync {
-
-SyncerEndCommand::SyncerEndCommand() {}
-SyncerEndCommand::~SyncerEndCommand() {}
-
-void SyncerEndCommand::ExecuteImpl(sessions::SyncSession* session) {
-  // Always send out a cycle ended notification, regardless of end-state.
-  SyncEngineEvent event(SyncEngineEvent::SYNC_CYCLE_ENDED);
-  sessions::SyncSessionSnapshot snapshot(session->TakeSnapshot());
-  DVLOG(1) << "Sending snapshot: " << snapshot.ToString();
-  event.snapshot = &snapshot;
-  session->context()->NotifyListeners(event);
-  DVLOG(1) << this << " sent sync end snapshot";
-}
-
-}  // namespace browser_sync
diff --git a/chrome/browser/sync/engine/syncer_end_command.h b/chrome/browser/sync/engine/syncer_end_command.h
deleted file mode 100644
index 01e99f3..0000000
--- a/chrome/browser/sync/engine/syncer_end_command.h
+++ /dev/null
@@ -1,34 +0,0 @@
-// Copyright (c) 2011 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.
-
-#ifndef CHROME_BROWSER_SYNC_ENGINE_SYNCER_END_COMMAND_H_
-#define CHROME_BROWSER_SYNC_ENGINE_SYNCER_END_COMMAND_H_
-#pragma once
-
-#include "base/basictypes.h"
-#include "base/compiler_specific.h"
-#include "chrome/browser/sync/engine/syncer_command.h"
-
-namespace browser_sync {
-
-// A syncer command for wrapping up a sync cycle.
-//
-// Preconditions - syncing is complete.
-//
-// Postconditions - The UI has been told that we're done syncing.
-
-class SyncerEndCommand : public SyncerCommand {
- public:
-  SyncerEndCommand();
-  virtual ~SyncerEndCommand();
-
-  // SyncerCommand implementation.
-  virtual void ExecuteImpl(sessions::SyncSession* session) OVERRIDE;
- private:
-  DISALLOW_COPY_AND_ASSIGN(SyncerEndCommand);
-};
-
-}  // namespace browser_sync
-
-#endif  // CHROME_BROWSER_SYNC_ENGINE_SYNCER_END_COMMAND_H_
diff --git a/chrome/browser/sync/engine/syncer_types.h b/chrome/browser/sync/engine/syncer_types.h
index f7b021e..e4fde71 100644
--- a/chrome/browser/sync/engine/syncer_types.h
+++ b/chrome/browser/sync/engine/syncer_types.h
@@ -80,12 +80,13 @@ enum VerifyCommitResult {
 struct SyncEngineEvent {
   enum EventCause {
     ////////////////////////////////////////////////////////////////
+    // Sent on entry of Syncer state machine
+    SYNC_CYCLE_BEGIN,
+
     // SyncerCommand generated events.
     STATUS_CHANGED,
 
     // We have reached the SYNCER_END state in the main sync loop.
-    // Check the SyncerSession for information like whether we need to continue
-    // syncing (SyncerSession::HasMoreToSync).
     SYNC_CYCLE_ENDED,
 
     ////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/sync/engine/syncer_unittest.cc b/chrome/browser/sync/engine/syncer_unittest.cc
index 1bf9cc5..fde8bb7 100644
--- a/chrome/browser/sync/engine/syncer_unittest.cc
+++ b/chrome/browser/sync/engine/syncer_unittest.cc
@@ -155,8 +155,8 @@ class SyncerTest : public testing::Test,
     DVLOG(1) << "HandleSyncEngineEvent in unittest " << event.what_happened;
     // we only test for entry-specific events, not status changed ones.
     switch (event.what_happened) {
+      case SyncEngineEvent::SYNC_CYCLE_BEGIN: // Fall through.
       case SyncEngineEvent::STATUS_CHANGED:
-        // fall through
       case SyncEngineEvent::SYNC_CYCLE_ENDED:
         return;
       default:
diff --git a/chrome/browser/sync/sessions/session_state.cc b/chrome/browser/sync/sessions/session_state.cc
index 3dabec9..19f880c 100644
--- a/chrome/browser/sync/sessions/session_state.cc
+++ b/chrome/browser/sync/sessions/session_state.cc
@@ -47,7 +47,6 @@ DictionaryValue* SyncSourceInfo::ToValue() const {
 SyncerStatus::SyncerStatus()
     : invalid_store(false),
       syncer_stuck(false),
-      sync_in_progress(false),
       num_successful_commits(0),
       num_successful_bookmark_commits(0),
       num_updates_downloaded_total(0),
@@ -63,7 +62,6 @@ DictionaryValue* SyncerStatus::ToValue() const {
   DictionaryValue* value = new DictionaryValue();
   value->SetBoolean("invalidStore", invalid_store);
   value->SetBoolean("syncerStuck", syncer_stuck);
-  value->SetBoolean("syncInProgress", sync_in_progress);
   value->SetInteger("numSuccessfulCommits", num_successful_commits);
   value->SetInteger("numSuccessfulBookmarkCommits",
                 num_successful_bookmark_commits);
diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h
index aa5f6b6..448ecc1 100644
--- a/chrome/browser/sync/sessions/session_state.h
+++ b/chrome/browser/sync/sessions/session_state.h
@@ -67,7 +67,6 @@ struct SyncerStatus {
   bool invalid_store;
   // True iff we're stuck.
   bool syncer_stuck;
-  bool sync_in_progress;
   int num_successful_commits;
   // This is needed for monitoring extensions activity.
   int num_successful_bookmark_commits;
diff --git a/chrome/browser/sync/sessions/session_state_unittest.cc b/chrome/browser/sync/sessions/session_state_unittest.cc
index f7132da..cb19304 100644
--- a/chrome/browser/sync/sessions/session_state_unittest.cc
+++ b/chrome/browser/sync/sessions/session_state_unittest.cc
@@ -46,7 +46,6 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
   SyncerStatus status;
   status.invalid_store = true;
   status.syncer_stuck = false;
-  status.sync_in_progress = true;
   status.num_successful_commits = 5;
   status.num_successful_bookmark_commits = 10;
   status.num_updates_downloaded_total = 100;
@@ -55,10 +54,9 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
   status.num_server_overwrites = 18;
 
   scoped_ptr<DictionaryValue> value(status.ToValue());
-  EXPECT_EQ(9u, value->size());
+  EXPECT_EQ(8u, value->size());
   ExpectDictBooleanValue(status.invalid_store, *value, "invalidStore");
   ExpectDictBooleanValue(status.syncer_stuck, *value, "syncerStuck");
-  ExpectDictBooleanValue(status.sync_in_progress, *value, "syncInProgress");
   ExpectDictIntegerValue(status.num_successful_commits,
                          *value, "numSuccessfulCommits");
   ExpectDictIntegerValue(status.num_successful_bookmark_commits,
diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc
index 49955f3..bf9416d 100644
--- a/chrome/browser/sync/sessions/status_controller.cc
+++ b/chrome/browser/sync/sessions/status_controller.cc
@@ -163,12 +163,8 @@ void StatusController::set_syncer_stuck(bool syncer_stuck) {
     shared_.syncer_status.mutate()->syncer_stuck = syncer_stuck;
 }
 
-void StatusController::SetSyncInProgressAndUpdateStartTime(
-    bool sync_in_progress) {
-  if (shared_.syncer_status.value().sync_in_progress != sync_in_progress) {
-    shared_.syncer_status.mutate()->sync_in_progress = sync_in_progress;
-    sync_start_time_ = base::Time::Now();
-  }
+void StatusController::UpdateStartTime() {
+  sync_start_time_ = base::Time::Now();
 }
 
 void StatusController::set_num_successful_bookmark_commits(int value) {
diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h
index 0140e14..2509894 100644
--- a/chrome/browser/sync/sessions/status_controller.h
+++ b/chrome/browser/sync/sessions/status_controller.h
@@ -235,7 +235,7 @@ class StatusController {
   void reset_conflicts_resolved();
   void set_items_committed();
 
-  void SetSyncInProgressAndUpdateStartTime(bool sync_in_progress);
+  void UpdateStartTime();
 
   void set_debug_info_sent();
 
diff --git a/chrome/browser/sync/sessions/status_controller_unittest.cc b/chrome/browser/sync/sessions/status_controller_unittest.cc
index 7b850e8..90e9860 100644
--- a/chrome/browser/sync/sessions/status_controller_unittest.cc
+++ b/chrome/browser/sync/sessions/status_controller_unittest.cc
@@ -60,11 +60,6 @@ TEST_F(StatusControllerTest, GetsDirty) {
   status.set_syncer_stuck(false);
   EXPECT_TRUE(status.TestAndClearIsDirty());
 
-  status.SetSyncInProgressAndUpdateStartTime(true);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-  status.SetSyncInProgressAndUpdateStartTime(false);
-  EXPECT_TRUE(status.TestAndClearIsDirty());
-
   status.increment_num_successful_commits();
   EXPECT_TRUE(status.TestAndClearIsDirty());
   status.increment_num_successful_commits();
@@ -135,10 +130,6 @@ TEST_F(StatusControllerTest, ReadYourWrites) {
   status.set_syncer_stuck(true);
   EXPECT_TRUE(status.syncer_status().syncer_stuck);
 
-  EXPECT_FALSE(status.syncer_status().sync_in_progress);
-  status.SetSyncInProgressAndUpdateStartTime(true);
-  EXPECT_TRUE(status.syncer_status().sync_in_progress);
-
   for (int i = 0; i < 14; i++)
     status.increment_num_successful_commits();
   EXPECT_EQ(14, status.syncer_status().num_successful_commits);
diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc
index e8c4da1..064fef6 100644
--- a/chrome/browser/sync/sessions/sync_session.cc
+++ b/chrome/browser/sync/sessions/sync_session.cc
@@ -168,6 +168,15 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
       status_controller_->sync_start_time());
 }
 
+void SyncSession::SendEventNotification(SyncEngineEvent::EventCause cause) {
+  SyncEngineEvent event(cause);
+  const SyncSessionSnapshot& snapshot = TakeSnapshot();
+  event.snapshot = &snapshot;
+
+  DVLOG(1) << "Sending event with snapshot: " << snapshot.ToString();
+  context()->NotifyListeners(event);
+}
+
 SyncSourceInfo SyncSession::TestAndSetSource() {
   SyncSourceInfo old_source = source_;
   source_ = SyncSourceInfo(
diff --git a/chrome/browser/sync/sessions/sync_session.h b/chrome/browser/sync/sessions/sync_session.h
index 135e6d7..abed10e 100644
--- a/chrome/browser/sync/sessions/sync_session.h
+++ b/chrome/browser/sync/sessions/sync_session.h
@@ -105,6 +105,9 @@ class SyncSession {
   // Builds a thread-safe and read-only copy of the current session state.
   SyncSessionSnapshot TakeSnapshot() const;
 
+  // Builds and sends a snapshot to the session context's listeners.
+  void SendEventNotification(SyncEngineEvent::EventCause cause);
+
   // Returns true if this session contains data that should go through the sync
   // engine again.
   bool HasMoreToSync() const;