Initial clear server data impl

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

 // Copyright 2013 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/test/integration/profile_sync_service_harness.h"
 
 #include <cstddef>
 #include <iterator>
 #include <ostream>
 #include <sstream>
@@ skipping 112 matching lines @@
 
 ProfileSyncServiceHarness::~ProfileSyncServiceHarness() { }
 
 void ProfileSyncServiceHarness::SetCredentials(const std::string& username,
                                                const std::string& password) {
   username_ = username;
   password_ = password;
 }
 
 bool ProfileSyncServiceHarness::SetupSync() {
-  bool result = SetupSync(syncer::UserSelectableTypes());
-  if (result == false) {
-    std::string status = GetServiceStatus();
-    LOG(ERROR) << profile_debug_name_
-               << ": SetupSync failed. Syncer status:\n" << status;
+  bool result = SetupSync(syncer::UserSelectableTypes(), false);
+  if (!result) {
+    LOG(ERROR) << profile_debug_name_ << ": SetupSync failed. Syncer status:\n"
+               << GetServiceStatus();
   } else {
     DVLOG(1) << profile_debug_name_ << ": SetupSync successful.";
   }
   return result;
 }
 
-bool ProfileSyncServiceHarness::SetupSync(
-    syncer::ModelTypeSet synced_datatypes) {
+bool ProfileSyncServiceHarness::SetupSyncForClear() {
+  bool result = SetupSync(syncer::UserSelectableTypes(), true);
+  if (!result) {
+    LOG(ERROR) << profile_debug_name_
+               << ": SetupSyncForClear failed. Syncer status:\n"
+               << GetServiceStatus();
+  } else {
+    DVLOG(1) << profile_debug_name_ << ": SetupSyncForClear successful.";
+  }
+  return result;
+}
+
+bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
+                                          bool forClear) {

[skym, 2017/03/08 00:23:48]

should be named for_clear.
https://google.github.io/styleguide/cppguide.html#Variable_Names

[wylieb, 2017/03/08 00:58:51]

Done.

   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
@@ skipping 16 matching lines @@
     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 (!AwaitEngineInitialization()) {
+  if (forClear && !AwaitEngineInitializationForClear()) {
+    return false;
+  } else if (!forClear && !AwaitEngineInitialization()) {
     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;
   }
 
+  // If we're setting up for clear, we don't need to wait for passphrase
+  // decryption or setup completion.

[skym, 2017/03/08 00:23:48]

I'd just drop the "passphrase description or", I'm not sure that's what I'd
categorize the passphrase logic below this as. It seems like it's setting the
passphrase. It bails if it needs to passphrase to decrypt anything.

[wylieb, 2017/03/08 00:58:51]

Done.

+  if (forClear) {
+    return true;
+  }
+
   // Set an implicit passphrase for encryption if an explicit one hasn't already
   // been set. If an explicit passphrase has been set, immediately return false,
   // since a decryption passphrase is required.
   if (!service()->IsUsingSecondaryPassphrase()) {
     service()->SetEncryptionPassphrase(password_, ProfileSyncService::IMPLICIT);
   } else {
     LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
                   " until SetDecryptionPassphrase is called.";
     return false;
   }
 
   // Wait for initial sync cycle to be completed.
   if (!AwaitSyncSetupCompletion()) {
-    LOG(ERROR) << "Initial sync cycle timed out.";
     return false;
   }
 
   return true;
 }
 
+bool ProfileSyncServiceHarness::RestartSyncService() {
+  auto blocker = service()->GetSetupInProgressHandle();

[skym, 2017/03/08 00:23:48]

I think auto is bad here. You're obscuring if this is a smart or raw pointer.

[wylieb, 2017/03/08 00:58:51]

Done.

+  DVLOG(1) << "Requesting stop for service.";
+  service()->RequestStop(ProfileSyncService::CLEAR_DATA);
+  DVLOG(1) << "Requesting start for service";
+
+  service()->RequestStart();
+  if (!AwaitEngineInitialization()) {
+    LOG(ERROR) << "AwaitEngineInitialization failed.";
+    return false;
+  }
+
+  // This passphrase should be implicit because ClearServerData should be called

[skym, 2017/03/08 00:23:48]

Erm, doesn't implicit mean that it is the GAIA password being reused as a
passphrase?
https://cs.chromium.org/chromium/src/components/sync/driver/sync_service.h?l=78

Rather, it is IsUsingSecondaryPassphrase() that should be false because
ClearServerData() was just called, right?

Although I don't get why the integ tests need to do this themselves, I thought
there was logic to automatically upgrade clients to an implict passphrase
itself.. /shrug

[wylieb, 2017/03/08 00:58:51]

Done.

+  // prior.
+  if (!service()->IsUsingSecondaryPassphrase()) {
+    service()->SetEncryptionPassphrase(password_, ProfileSyncService::IMPLICIT);
+  } else {
+    LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
+                  " until SetDecryptionPassphrase is called.";
+    return false;
+  }
+
+  blocker.reset();

[skym, 2017/03/08 00:23:48]

You shouldn't need this, it'll be destructed when it leaves scope for you,
right?

[wylieb, 2017/03/08 00:58:51]

Done.

+  service()->SetFirstSetupComplete();

[skym, 2017/03/08 00:23:48]

I'm confused why you need both the blocker and this call.

You've already called this method from FinishSyncSetup(), from SetupSync(),
right? It isn't clear to me if that has been cleared out or not as part of this
restarting of sync. It seems like once you know which it does, you can remove
one of these two.

Wait, actually, I don't see any reason to block at all, regardless of above
thinking. It seems like in the old SyncSetup, the blocking was so that we could
set the enabled data types. But they're already set before you get into this
function, right?

[wylieb, 2017/03/08 00:58:51]

Seems like something to be addressed in another CL. A change that addresses the
general question of when and why should we use a blocker, and is
SetFirstSetupComplete enough?

[skym, 2017/03/08 19:15:02]

Okay... I'm okay with you having both the SetFirstSetupComplete and blocker in
here for now. But you need to 
1. Understand why they're here and what they're adding
2. Document that reason with comments

+
+  return true;
+}
+
 bool ProfileSyncServiceHarness::AwaitMutualSyncCycleCompletion(
     ProfileSyncServiceHarness* partner) {
   std::vector<ProfileSyncServiceHarness*> harnesses;
   harnesses.push_back(this);
   harnesses.push_back(partner);
   return AwaitQuiescence(harnesses);
 }
 
 bool ProfileSyncServiceHarness::AwaitGroupSyncCycleCompletion(
     const std::vector<ProfileSyncServiceHarness*>& partners) {
@@ skipping 33 matching lines @@
   }
 
   if (HasAuthError(service())) {
     LOG(ERROR) << "Credentials were rejected. Sync cannot proceed.";
     return false;
   }
 
   return true;
 }
 
+bool ProfileSyncServiceHarness::AwaitEngineInitializationForClear() {
+  if (!EngineInitializeChecker(service()).Wait()) {
+    LOG(ERROR)
+        << "Initializing up for clear: EngineInitializeChecker timed out.";
+    return false;
+  }
+
+  if (!service()->IsEngineInitialized()) {
+    LOG(ERROR) << "Initializing up for clear: Service engine not initialized.";
+    return false;
+  }
+
+  if (HasAuthError(service())) {
+    LOG(ERROR) << "Initializing up for clear: Credentials were rejected. Sync "
+                  "cannot proceed.";
+    return false;
+  }
+
+  return true;
+}
+
 bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion() {
   if (!SyncSetupChecker(service()).Wait()) {
     LOG(ERROR) << "SyncSetupChecker timed out.";
     return false;
   }
 
   // Make sure that initial sync wasn't blocked by a missing passphrase.
   if (service()->passphrase_required_reason() == syncer::REASON_DECRYPTION) {
     LOG(ERROR) << "A passphrase is required for decryption. Sync cannot proceed"
                   " until SetDecryptionPassphrase is called.";
@@ skipping 191 matching lines @@
 
 std::string ProfileSyncServiceHarness::GetServiceStatus() {
   std::unique_ptr<base::DictionaryValue> value(
       syncer::sync_ui_util::ConstructAboutInformation(
           service(), service()->signin(), chrome::GetChannel()));
   std::string service_status;
   base::JSONWriter::WriteWithOptions(
       *value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &service_status);
   return service_status;
 }