Initial clear server data impl

chrome/browser/sync/test/integration/profile_sync_service_harness.cc

Index: chrome/browser/sync/test/integration/profile_sync_service_harness.cc
diff --git a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
index 97f45645a54a9bc05db265383cc7f7fa1849b5c1..54a437562acdcf20e3d6a49b4b4c261747a03a2d 100644
--- a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
+++ b/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
@@ -219,6 +219,110 @@ bool ProfileSyncServiceHarness::SetupSync(
   return true;
 }
 
+bool ProfileSyncServiceHarness::SetupSyncForClear() {
+  bool result = SetupSyncForClear(syncer::UserSelectableTypes());
+  if (result == false) {
+    std::string status = GetServiceStatus();
+    LOG(ERROR) << profile_debug_name_
+               << ": SetupSyncForClear failed. Syncer status:\n"
+               << status;
+  } else {
+    DVLOG(1) << profile_debug_name_ << ": SetupSyncForClear successful.";
+  }
+  return result;
+}
+
+bool ProfileSyncServiceHarness::SetupSyncForClear(

[pavely, 2017/03/07 07:26:15]

This function is awfully similar to SetupSync(). Imagine someone else decides to
change one of them, will they remember to change the other as well? Is it
possible to refactor functions to reduce code duplication?

[skym, 2017/03/07 18:38:22]

+1

[wylieb, 2017/03/07 19:52:22]

Well that depends. If they want to change the the behavior of setting up sync
for an account to clear the server data, then yes I would expect them to. But if
someone decides to change SetupSync without understanding the context from which
it's called, then I'd say it's prone to what you're pointing out here. I
generally favor _some_ code duplication over a function doing more than one
thing. You have a good point, though. 

LMK about the change I made here: I passed a bool forClear, and keyed off of
that for tasks that were problematic within the function. It satisfies the your
concern, as the only duplicate method that there is now is one that calls into
SetupSync (ie another parameter-less function like the one that exists for
SetupSync already.

+    syncer::ModelTypeSet synced_datatypes) {
+  DCHECK(!profile_->IsLegacySupervised())
+      << "SetupSync should not be used for legacy supervised users.";
+
+  // Initialize the sync client's profile sync service object.
+  if (service() == nullptr) {
+    LOG(ERROR) << "SetupSync(): service() is null.";
+    return false;
+  }
+
+  // Tell the sync service that setup is in progress so we don't start syncing
+  // until we've finished configuration.
+  sync_blocker_ = service()->GetSetupInProgressHandle();
+
+  DCHECK(!username_.empty());
+  if (signin_type_ == SigninType::UI_SIGNIN) {
+    Browser* browser = chrome::FindBrowserWithProfile(profile_);
+    DCHECK(browser);
+    if (!login_ui_test_utils::SignInWithUI(browser, username_, password_)) {
+      LOG(ERROR) << "Could not sign in to GAIA servers.";
+      return false;
+    }
+  } else if (signin_type_ == SigninType::FAKE_SIGNIN) {
+    // Authenticate sync client using GAIA credentials.
+    std::string gaia_id = GetGaiaIdForUsername(username_);
+    service()->signin()->SetAuthenticatedAccountInfo(gaia_id, username_);
+    std::string account_id = service()->signin()->GetAuthenticatedAccountId();
+    service()->GoogleSigninSucceeded(account_id, username_, password_);
+    ProfileOAuth2TokenServiceFactory::GetForProfile(profile_)
+        ->UpdateCredentials(account_id, GenerateFakeOAuth2RefreshTokenString());
+  } else {
+    LOG(ERROR) << "Unsupported profile signin type.";
+  }
+
+  // Now that auth is completed, request that sync actually start.
+  service()->RequestStart();
+
+  if (!AwaitEngineInitializationForClear()) {
+    return false;
+  }
+
+  // Choose the datatypes to be synced. If all datatypes are to be synced,
+  // set sync_everything to true; otherwise, set it to false.
+  bool sync_everything = (synced_datatypes == syncer::UserSelectableTypes());
+  service()->OnUserChoseDatatypes(sync_everything, synced_datatypes);
+
+  // Notify ProfileSyncService that we are done with configuration.
+  FinishSyncSetup();
+
+  if ((signin_type_ == SigninType::UI_SIGNIN) &&
+      !login_ui_test_utils::DismissSyncConfirmationDialog(
+          chrome::FindBrowserWithProfile(profile_),
+          base::TimeDelta::FromSeconds(30))) {
+    LOG(ERROR) << "Failed to dismiss sync confirmation dialog.";
+    return false;
+  }
+
+  // Wait for initial sync cycle to be completed.
+  if (!AwaitSyncSetupCompletionForClear()) {
+    LOG(ERROR) << "Initial sync cycle timed out.";
+    return false;
+  }
+
+  return true;
+}
+
+bool ProfileSyncServiceHarness::RestartSyncService() {
+  std::unique_ptr<syncer::SyncSetupInProgressHandle> blocker =

[skym, 2017/03/07 18:38:22]

Is there a reason that |sync_blocker_| isn't used?

[wylieb, 2017/03/07 19:52:22]

Done.

+      service()->GetSetupInProgressHandle();
+

[skym, 2017/03/07 18:38:22]

So

[wylieb, 2017/03/07 19:52:22]

Ok

+  DVLOG(1) << "Requesting stop for service.";
+

[skym, 2017/03/07 18:38:22]

many

[wylieb, 2017/03/07 19:52:22]

I

+  service()->RequestStop(ProfileSyncService::CLEAR_DATA);
+

[skym, 2017/03/07 18:38:22]

new

[wylieb, 2017/03/07 19:52:22]

Fixed

+  DVLOG(1) << "Requesting start for service";
+

[skym, 2017/03/07 18:38:22]

lines

[wylieb, 2017/03/07 19:52:22]

It

+  service()->RequestStart();
+  if (!AwaitEngineInitialization()) {

[skym, 2017/03/07 18:38:22]

We should also always require a passphrase, right? Should we verify as such?

[wylieb, 2017/03/07 19:52:22]

Done.

+    LOG(ERROR) << "AwaitEngineInitialization failed.";
+    return false;
+  }
+
+  DVLOG(1) << "Releasing blocker and setting first setup complete";
+
+  blocker.reset();
+  service()->SetFirstSetupComplete();
+
+  return true;
+}
+
 bool ProfileSyncServiceHarness::AwaitMutualSyncCycleCompletion(
     ProfileSyncServiceHarness* partner) {
   std::vector<ProfileSyncServiceHarness*> harnesses;
@@ -272,6 +376,25 @@ bool ProfileSyncServiceHarness::AwaitEngineInitialization() {
   return true;
 }
 
+bool ProfileSyncServiceHarness::AwaitEngineInitializationForClear() {
+  if (!EngineInitializeChecker(service()).Wait()) {
+    LOG(ERROR) << "EngineInitializeChecker timed out.";
+    return false;
+  }
+
+  if (!service()->IsEngineInitialized()) {
+    LOG(ERROR) << "Service engine not initialized.";
+    return false;
+  }
+
+  if (HasAuthError(service())) {
+    LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
+    return false;
+  }
+
+  return true;
+}
+
 bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() {

[skym, 2017/03/07 18:38:22]

Do you understand why this method even exists? As far as I can tell, it's
AwaitEngineInitialization() without the engine check. Looking at all of its call
sites, it seems like replacing it with AwaitEngineInitialization might even just
work...

Would love it if we could avoid the other two flavors of this function being
added in this CL as well.

[skym, 2017/03/07 18:47:44]

Oooh, I missed that the first check was significantly different. Hmm, never mind
I guess.

[wylieb, 2017/03/07 19:52:22]

Done.

[wylieb, 2017/03/07 19:52:22]

Done.

   if (!SyncSetupChecker(service()).Wait()) {
     LOG(ERROR) << "SyncSetupChecker timed out.";
@@ -293,6 +416,20 @@ bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() {
   return true;
 }
 
+bool ProfileSyncServiceHarness::AwaitSyncSetupCompletionForClear() {
+  if (!SyncSetupChecker(service()).Wait()) {
+    LOG(ERROR) << "SyncSetupChecker timed out.";
+    return false;
+  }
+
+  if (HasAuthError(service())) {
+    LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
+    return false;
+  }
+
+  return true;
+}
+
 std::string ProfileSyncServiceHarness::GenerateFakeOAuth2RefreshTokenString() {
   return base::StringPrintf("oauth2_refresh_token_%d",
                             ++oauth2_refesh_token_number_);