Improve the integration test harness by using the max_local_timestamp

from the sync engine rather than waiting for "a couple syncs" to happen
before declaring sync "done".

BUG=37351

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

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

8 files changed, 112 insertions, 101 deletions

diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index 433475c..e807d3d 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -1739,7 +1739,7 @@ void SyncManager::SyncInternal::HandleSyncerEvent(const SyncerEvent& event) {
   // whether we should sync again.
   if (event.what_happened == SyncerEvent::SYNC_CYCLE_ENDED) {
     if (!event.snapshot->has_more_to_sync) {
-      observer_->OnSyncCycleCompleted();
+      observer_->OnSyncCycleCompleted(event.snapshot);
     }
 
     // TODO(chron): Consider changing this back to track has_more_to_sync
diff --git a/chrome/browser/sync/engine/syncapi.h b/chrome/browser/sync/engine/syncapi.h
index ca1cc01..98aeeb9 100644
--- a/chrome/browser/sync/engine/syncapi.h
+++ b/chrome/browser/sync/engine/syncapi.h
@@ -53,6 +53,10 @@
 
 namespace browser_sync {
 class ModelSafeWorkerRegistrar;
+
+namespace sessions {
+struct SyncSessionSnapshot;
+}
 }
 
 // Forward declarations of internal class types so that sync API objects
@@ -564,10 +568,9 @@ class SyncManager {
                                   int change_count) = 0;
 
     // A round-trip sync-cycle took place and the syncer has resolved any
-    // conflicts that may have arisen.  This is kept separate from
-    // OnStatusChanged as there isn't really any state update; it is plainly
-    // a notification of a state transition.
-    virtual void OnSyncCycleCompleted() = 0;
+    // conflicts that may have arisen.
+    virtual void OnSyncCycleCompleted(
+        const browser_sync::sessions::SyncSessionSnapshot* snapshot) = 0;
 
     // Called when user interaction may be required due to an auth problem.
     virtual void OnAuthError(const GoogleServiceAuthError& auth_error) = 0;
diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc
index 51a8f86..ae3519d 100644
--- a/chrome/browser/sync/glue/sync_backend_host.cc
+++ b/chrome/browser/sync/glue/sync_backend_host.cc
@@ -13,6 +13,7 @@
 #include "chrome/browser/sync/glue/history_model_worker.h"
 #include "chrome/browser/sync/glue/sync_backend_host.h"
 #include "chrome/browser/sync/glue/http_bridge.h"
+#include "chrome/browser/sync/sessions/session_state.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/notification_type.h"
 #include "webkit/glue/webkit_glue.h"
@@ -29,6 +30,8 @@ typedef GoogleServiceAuthError AuthError;
 
 namespace browser_sync {
 
+using sessions::SyncSessionSnapshot;
+
 SyncBackendHost::SyncBackendHost(
     SyncFrontend* frontend,
     Profile* profile,
@@ -242,6 +245,10 @@ const GoogleServiceAuthError& SyncBackendHost::GetAuthError() const {
   return last_auth_error_;
 }
 
+const SyncSessionSnapshot* SyncBackendHost::GetLastSessionSnapshot() const {
+  return last_snapshot_.get();
+}
+
 void SyncBackendHost::GetWorkers(std::vector<ModelSafeWorker*>* out) {
   AutoLock lock(registrar_lock_);
   out->clear();
@@ -387,9 +394,21 @@ void SyncBackendHost::Core::OnChangesApplied(
   processor->ApplyChangesFromSyncModel(trans, changes, change_count);
 }
 
-void SyncBackendHost::Core::OnSyncCycleCompleted() {
+void SyncBackendHost::Core::OnSyncCycleCompleted(
+    const SyncSessionSnapshot* snapshot) {
   host_->frontend_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
-      &Core::NotifyFrontend, SYNC_CYCLE_COMPLETED));
+      &Core::HandleSyncCycleCompletedOnFrontendLoop,
+      new SyncSessionSnapshot(*snapshot)));
+}
+
+void SyncBackendHost::Core::HandleSyncCycleCompletedOnFrontendLoop(
+    SyncSessionSnapshot* snapshot) {
+  if (!host_ || !host_->frontend_)
+    return;
+  DCHECK_EQ(MessageLoop::current(), host_->frontend_loop_);
+
+  host_->last_snapshot_.reset(snapshot);
+  host_->frontend_->OnSyncCycleCompleted();
 }
 
 void SyncBackendHost::Core::OnInitializationComplete() {
diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h
index 8edc7ba..e362d09 100644
--- a/chrome/browser/sync/glue/sync_backend_host.h
+++ b/chrome/browser/sync/glue/sync_backend_host.h
@@ -29,6 +29,10 @@ class Profile;
 
 namespace browser_sync {
 
+namespace sessions {
+struct SyncSessionSnapshot;
+}
+
 class ChangeProcessor;
 class DataTypeController;
 
@@ -135,6 +139,7 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar {
   Status GetDetailedStatus();
   StatusSummary GetStatusSummary();
   const GoogleServiceAuthError& GetAuthError() const;
+  const sessions::SyncSessionSnapshot* GetLastSessionSnapshot() const;
 
   const FilePath& sync_data_folder_path() const {
     return sync_data_folder_path_;
@@ -191,7 +196,8 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar {
         const sync_api::BaseTransaction* trans,
         const sync_api::SyncManager::ChangeRecord* changes,
         int change_count);
-    virtual void OnSyncCycleCompleted();
+    virtual void OnSyncCycleCompleted(
+        const sessions::SyncSessionSnapshot* snapshot);
     virtual void OnInitializationComplete();
     virtual void OnAuthError(const GoogleServiceAuthError& auth_error);
     virtual void OnPaused();
@@ -334,6 +340,11 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar {
     void HandleAuthErrorEventOnFrontendLoop(
         const GoogleServiceAuthError& new_auth_error);
 
+    // Called from Core::OnSyncCycleCompleted to handle updating frontend
+    // thread components.
+    void HandleSyncCycleCompletedOnFrontendLoop(
+        sessions::SyncSessionSnapshot* snapshot);
+
     // Our parent SyncBackendHost
     SyncBackendHost* host_;
 
@@ -403,6 +414,9 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar {
   // UI-thread cache of the last AuthErrorState received from syncapi.
   GoogleServiceAuthError last_auth_error_;
 
+  // UI-thread cache of the last SyncSessionSnapshot received from syncapi.
+  scoped_ptr<sessions::SyncSessionSnapshot> last_snapshot_;
+
   DISALLOW_COPY_AND_ASSIGN(SyncBackendHost);
 };
 
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index 25e7e7c..b00ea71 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1692,6 +1692,7 @@
         'chrome_resources',
         'chrome_strings',
         'syncapi',
+        'sync_proto',
         'test_support_unit',
         '../net/net.gyp:net_test_support',
         '../printing/printing.gyp:printing',
@@ -1706,6 +1707,7 @@
       'include_dirs': [
         '..',
         '<(INTERMEDIATE_DIR)',
+        '<(protoc_out_dir)',
       ],
       # TODO(phajdan.jr): Only temporary, to make transition easier.
       'defines': [ 'ALLOW_IN_PROC_BROWSER_TEST' ],
diff --git a/chrome/test/live_sync/profile_sync_service_test_harness.cc b/chrome/test/live_sync/profile_sync_service_test_harness.cc
index 80a90b2..7c4ce65 100644
--- a/chrome/test/live_sync/profile_sync_service_test_harness.cc
+++ b/chrome/test/live_sync/profile_sync_service_test_harness.cc
@@ -6,12 +6,16 @@
 #include "chrome/browser/browser.h"
 #include "chrome/browser/pref_service.h"
 #include "chrome/browser/profile.h"
+#include "chrome/browser/sync/glue/sync_backend_host.h"
+#include "chrome/browser/sync/sessions/session_state.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/live_sync/profile_sync_service_test_harness.h"
 #include "chrome/test/ui_test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using browser_sync::sessions::SyncSessionSnapshot;
+
 // The default value for min_updates_needed_ when we're not in a call to
 // WaitForUpdatesRecievedAtLeast.
 static const int kMinUpdatesNeededNone = -1;
@@ -78,39 +82,12 @@ bool StateChangeTimeoutEvent::Abort() {
   return !did_timeout_;
 }
 
-class ConflictTimeoutEvent
-    : public base::RefCountedThreadSafe<ConflictTimeoutEvent> {
- public:
-  explicit ConflictTimeoutEvent(ProfileSyncServiceTestHarness* caller)
-      : did_run_(false), caller_(caller)  {
-  }
-
-  // The entry point to the class from PostDelayedTask.
-  void Callback();
-  bool did_run_;
-
- private:
-  friend class base::RefCountedThreadSafe<ConflictTimeoutEvent>;
-
-  ~ConflictTimeoutEvent() { }
-
-  // Due to synchronization of the IO loop, the caller will always be alive
-  // if the class is not aborted.
-  ProfileSyncServiceTestHarness* caller_;
-
-  DISALLOW_COPY_AND_ASSIGN(ConflictTimeoutEvent);
-};
-
-void ConflictTimeoutEvent::Callback() {
-  caller_->SignalStateComplete();
-  did_run_ = true;
-}
-
-
 ProfileSyncServiceTestHarness::ProfileSyncServiceTestHarness(
     Profile* p, const std::string& username, const std::string& password)
     : wait_state_(WAITING_FOR_INITIAL_CALLBACK), profile_(p), service_(NULL),
-      last_status_(kInvalidStatus), min_updates_needed_(kMinUpdatesNeededNone),
+      last_status_(kInvalidStatus),
+      last_timestamp_(0),
+      min_timestamp_needed_(kMinUpdatesNeededNone),
       username_(username), password_(password) {
   // Ensure the profile has enough prefs registered for use by sync.
   if (!p->GetPrefs()->FindPreference(prefs::kAcceptLanguages))
@@ -151,22 +128,29 @@ bool ProfileSyncServiceTestHarness::RunStateChangeMachine() {
         SignalStateCompleteWithNextState(WAITING_FOR_NOTHING);
       }
       break;
-    case WAITING_FOR_TIMESTAMP_UPDATE: {
-      const base::Time current_timestamp = service_->last_synced_time();
-      if (current_timestamp == last_timestamp_) {
+    case WAITING_FOR_SYNC_TO_FINISH: {
+      const SyncSessionSnapshot* snap =
+          service_->backend()->GetLastSessionSnapshot();
+      DCHECK(snap) << "Should have been at least one sync session by now";
+      if (snap->has_more_to_sync)
         break;
-      }
-      EXPECT_TRUE(last_timestamp_ < current_timestamp);
-      last_timestamp_ = current_timestamp;
+
+      EXPECT_LE(last_timestamp_, snap->max_local_timestamp);
+      last_timestamp_ = snap->max_local_timestamp;
+
       SignalStateCompleteWithNextState(WAITING_FOR_NOTHING);
       break;
     }
-    case WAITING_FOR_UPDATES:
-      if (status.updates_received < min_updates_needed_) {
+    case WAITING_FOR_UPDATES: {
+      const SyncSessionSnapshot* snap =
+          service_->backend()->GetLastSessionSnapshot();
+      DCHECK(snap) << "Should have been at least one sync session by now";
+      if (snap->max_local_timestamp < min_timestamp_needed_)
         break;
-      }
+
       SignalStateCompleteWithNextState(WAITING_FOR_NOTHING);
       break;
+    }
     case WAITING_FOR_NOTHING:
     default:
       // Invalid state during observer callback which may be triggered by other
@@ -183,15 +167,25 @@ void ProfileSyncServiceTestHarness::OnStateChanged() {
 
 bool ProfileSyncServiceTestHarness::AwaitSyncCycleCompletion(
     const std::string& reason) {
-  wait_state_ = WAITING_FOR_TIMESTAMP_UPDATE;
+  wait_state_ = WAITING_FOR_SYNC_TO_FINISH;
   return AwaitStatusChangeWithTimeout(60, reason);
 }
 
 bool ProfileSyncServiceTestHarness::AwaitMutualSyncCycleCompletion(
     ProfileSyncServiceTestHarness* partner) {
-  return AwaitSyncCycleCompletion("Sync cycle completion on active client.") &&
-         partner->AwaitSyncCycleCompletion(
-            "Sync cycle completion on passive client.");
+  bool success = AwaitSyncCycleCompletion(
+      "Sync cycle completion on active client.");
+  if (!success)
+    return false;
+  return partner->WaitUntilTimestampIsAtLeast(last_timestamp_,
+      "Sync cycle completion on passive client.");
+}
+
+bool ProfileSyncServiceTestHarness::WaitUntilTimestampIsAtLeast(
+    int64 timestamp, const std::string& reason) {
+  wait_state_ = WAITING_FOR_UPDATES;
+  min_timestamp_needed_ = timestamp;
+  return AwaitStatusChangeWithTimeout(60, reason);
 }
 
 bool ProfileSyncServiceTestHarness::AwaitStatusChangeWithTimeout(
@@ -209,33 +203,6 @@ bool ProfileSyncServiceTestHarness::AwaitStatusChangeWithTimeout(
   return timeout_signal->Abort();
 }
 
-bool ProfileSyncServiceTestHarness::AwaitMutualSyncCycleCompletionWithConflict(
-    ProfileSyncServiceTestHarness* partner) {
-
-  if (!AwaitMutualSyncCycleCompletion(partner)) {
-    return false;
-  }
-  if (!partner->AwaitMutualSyncCycleCompletion(this)) {
-    return false;
-  }
-
-  scoped_refptr<ConflictTimeoutEvent> timeout_signal(
-      new ConflictTimeoutEvent(this));
-
-  // Now we want to wait an extra 20 seconds to ensure any rebounding updates
-  // due to a conflict are processed and observed by each client.
-  MessageLoopForUI* loop = MessageLoopForUI::current();
-  loop->PostDelayedTask(FROM_HERE, NewRunnableMethod(timeout_signal.get(),
-      &ConflictTimeoutEvent::Callback), 1000 * 20);
-  // It is possible that timeout has not run yet and loop exited due to
-  // OnStateChanged event. So to avoid pre-mature termination of loop,
-  // we are re-running the loop until did_run_ becomes true.
-  while (!timeout_signal->did_run_) {
-     ui_test_utils::RunMessageLoop();
-  }
-  return true;
-}
-
 bool ProfileSyncServiceTestHarness::WaitForServiceInit() {
   // Wait for the initial (auth needed) callback.
   EXPECT_EQ(wait_state_, WAITING_FOR_INITIAL_CALLBACK);
diff --git a/chrome/test/live_sync/profile_sync_service_test_harness.h b/chrome/test/live_sync/profile_sync_service_test_harness.h
index 8877c70..7ebf477 100644
--- a/chrome/test/live_sync/profile_sync_service_test_harness.h
+++ b/chrome/test/live_sync/profile_sync_service_test_harness.h
@@ -34,10 +34,9 @@ class ProfileSyncServiceTestHarness : public ProfileSyncServiceObserver {
   // since the previous one.  Returns true if a sync cycle has completed.
   bool AwaitSyncCycleCompletion(const std::string& reason);
 
-  // Wait an extra 20 seconds to ensure any rebounding updates
-  // due to a conflict are processed and observed by each client.
-  bool AwaitMutualSyncCycleCompletionWithConflict(
-      ProfileSyncServiceTestHarness* partner);
+  // Blocks the caller until this harness has observed that the sync engine
+  // has "synced" up to at least the specified local timestamp.
+  bool WaitUntilTimestampIsAtLeast(int64 timestamp, const std::string& reason);
 
   // Calling this acts as a barrier and blocks the caller until |this| and
   // |partner| have both completed a sync cycle.  When calling this method,
@@ -53,12 +52,11 @@ class ProfileSyncServiceTestHarness : public ProfileSyncServiceObserver {
 
  private:
   friend class StateChangeTimeoutEvent;
-  friend class ConflictTimeoutEvent;
 
   enum WaitState {
     WAITING_FOR_INITIAL_CALLBACK = 0,
     WAITING_FOR_READY_TO_PROCESS_CHANGES,
-    WAITING_FOR_TIMESTAMP_UPDATE,
+    WAITING_FOR_SYNC_TO_FINISH,
     WAITING_FOR_UPDATES,
     WAITING_FOR_NOTHING,
     NUMBER_OF_STATES
@@ -87,16 +85,15 @@ class ProfileSyncServiceTestHarness : public ProfileSyncServiceObserver {
   // State tracking.  Used for debugging and tracking of state.
   ProfileSyncService::Status last_status_;
 
-  // When awaiting quiescence, we are waiting for a change to this value.  It
-  // is set to the current (ui-threadsafe) last synced timestamp returned by
-  // the sync service when AwaitQuiescence is called, and then we wait for an
-  // OnStatusChanged event to observe a new, more recent last synced timestamp.
-  base::Time last_timestamp_;
+  // This value tracks the max sync timestamp (e.g. synced-to revision) inside
+  // the sync engine.  It gets updated when a sync cycle ends and the session
+  // snapshot implies syncing is "done".
+  int64 last_timestamp_;
 
-  // The minimum value of the 'updates_received' member of SyncManager Status
-  // we need to wait to observe in OnStateChanged when told to
-  // WaitForUpdatesReceivedAtLeast.
-  int64 min_updates_needed_;
+  // The minimum value of the 'max_local_timestamp' member of a
+  // SyncSessionSnapshot we need to wait to observe in OnStateChanged when told
+  // to WaitUntilTimestampIsAtLeast(...).
+  int64 min_timestamp_needed_;
 
   // Credentials used for GAIA authentication.
   std::string username_;
diff --git a/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc b/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
index e7b5904..bfae3c7 100644
--- a/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
+++ b/chrome/test/live_sync/two_client_live_bookmarks_sync_test.cc
@@ -353,11 +353,11 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest, Sanity) {
     model_one->SetTitle(google_one, L"Google++");
     model_two->SetTitle(google_two, L"Google--");
   }
-  // The extra wait here is because both clients generated changes, and the
-  // first client reaches a happy state before the second client gets a chance
-  // to push, so we explicitly double check.  This shouldn't be necessary once
-  // we have an easy way to verify the head version on each client.
-  ASSERT_TRUE(client1()->AwaitMutualSyncCycleCompletionWithConflict(client2()));
+
+  ASSERT_TRUE(client1()->AwaitMutualSyncCycleCompletion(client2()));
+  // Make sure that client2 has pushed all of it's changes as well.
+  ASSERT_TRUE(client2()->AwaitMutualSyncCycleCompletion(client1()));
+
   BookmarkModelVerifier::ExpectModelsMatch(model_one, model_two);
 
   Cleanup();
@@ -399,7 +399,10 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     bookmark_utils::ApplyEditsWithNoGroupChange(model_two, bbn_two,
         BookmarkEditor::EditDetails(google_two), title, third_url, NULL);
   }
-  ASSERT_TRUE(client1()->AwaitMutualSyncCycleCompletionWithConflict(client2()));
+
+  ASSERT_TRUE(client1()->AwaitMutualSyncCycleCompletion(client2()));
+  // Make sure that client2 has pushed all of it's changes as well.
+  ASSERT_TRUE(client2()->AwaitMutualSyncCycleCompletion(client1()));
   BookmarkModelVerifier::ExpectModelsMatch(model_one, model_two);
 
   {
@@ -1029,7 +1032,10 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     // Delete this newly created bookmark
     verifier->Remove(model_one, bbn_one, 0);
   }
-  client1()->AwaitMutualSyncCycleCompletionWithConflict(client2());
+
+  ASSERT_TRUE(client1()->AwaitMutualSyncCycleCompletion(client2()));
+  // Make sure that client2 has pushed all of it's changes as well.
+  ASSERT_TRUE(client2()->AwaitMutualSyncCycleCompletion(client1()));
   verifier->ExpectMatch(model_one);
   verifier->ExpectMatch(model_two);
 
@@ -2310,7 +2316,10 @@ IN_PROC_BROWSER_TEST_F(TwoClientLiveBookmarksSyncTest,
     ASSERT_TRUE(bm_foo4 != NULL);
   }
 
-  ASSERT_TRUE(client1()->AwaitMutualSyncCycleCompletionWithConflict(client2()));
+
+  ASSERT_TRUE(client1()->AwaitMutualSyncCycleCompletion(client2()));
+  // Make sure that client2 has pushed all of it's changes as well.
+  ASSERT_TRUE(client2()->AwaitMutualSyncCycleCompletion(client1()));
   BookmarkModelVerifier::ExpectModelsMatch(model_one, model_two);
   Cleanup();
 }