Various small cleanups in ProfileSyncServiceHarness

- Update some comments for accuracy
- Remove unused methods and includes
- Add some "const"s

Bug: none
Change-Id: Ia45237757b101af7ff0886dadff9ef9e622a2cc2
Reviewed-on: https://chromium-review.googlesource.com/1208526
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589141}

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

@@ -5,14 +5,11 @@
 #include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
 
 #include <cstddef>
-#include <iterator>
-#include <ostream>
 #include <sstream>
 
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
-#include "base/strings/stringprintf.h"
 #include "base/test/bind_test_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
@@ -28,16 +25,11 @@
 #include "chrome/browser/ui/webui/signin/login_ui_test_utils.h"
 #include "chrome/browser/unified_consent/unified_consent_service_factory.h"
 #include "chrome/common/channel_info.h"
-#include "chrome/common/chrome_switches.h"
-#include "components/invalidation/impl/p2p_invalidation_service.h"
 #include "components/signin/core/browser/signin_manager.h"
-#include "components/signin/core/browser/signin_metrics.h"
-#include "components/sync/base/progress_marker_map.h"
 #include "components/sync/driver/about_sync_util.h"
 #include "components/sync/engine/sync_string_conversions.h"
 #include "components/unified_consent/feature.h"
 #include "components/unified_consent/unified_consent_service.h"
-#include "google_apis/gaia/gaia_constants.h"
 #include "services/identity/public/cpp/identity_manager.h"
 #include "services/identity/public/cpp/identity_test_utils.h"
 
@@ -124,34 +116,10 @@ ProfileSyncServiceHarness::ProfileSyncServiceHarness(
       username_(username),
       password_(password),
       signin_type_(signin_type),
-      oauth2_refesh_token_number_(0),
       profile_debug_name_(profile->GetDebugName()) {}
 
 ProfileSyncServiceHarness::~ProfileSyncServiceHarness() { }
 
-bool ProfileSyncServiceHarness::SetupSync() {
-  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::SetupSyncForClearingServerData() {
-  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::SignIn() {
   // TODO(crbug.com/871221): This function should distinguish primary account
   // (aka sync account) from secondary accounts (content area signin). Let's
@@ -203,12 +171,34 @@ bool ProfileSyncServiceHarness::SignIn() {
   return false;
 }
 
+bool ProfileSyncServiceHarness::SetupSync() {
+  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::SetupSyncForClearingServerData() {
+  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 skip_passphrase_verification) {
   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;
@@ -235,7 +225,7 @@ bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
     // When unified consent given is set to |true|, the unified consent service
     // enables syncing all datatypes.
     UnifiedConsentServiceFactory::GetForProfile(profile_)
-      ->SetUnifiedConsentGiven(sync_everything);
+        ->SetUnifiedConsentGiven(sync_everything);
     if (!sync_everything) {
       service()->OnUserChoseDatatypes(sync_everything, synced_datatypes);
     }
@@ -291,6 +281,11 @@ bool ProfileSyncServiceHarness::SetupSync(syncer::ModelTypeSet synced_datatypes,
   return true;
 }
 
+void ProfileSyncServiceHarness::FinishSyncSetup() {
+  sync_blocker_.reset();
+  service()->SetFirstSetupComplete();
+}
+
 void ProfileSyncServiceHarness::StopSyncService(
     syncer::SyncService::SyncStopDataFate data_fate) {
   DVLOG(1) << "Requesting stop for service.";
@@ -334,11 +329,10 @@ bool ProfileSyncServiceHarness::StartSyncService() {
 #if !defined(OS_CHROMEOS)
 void ProfileSyncServiceHarness::SignoutSyncService() {
   DCHECK(!username_.empty());
-  // TODO(https://crbug.com/806781): This should go through IdentityManager once
-  // that supports sign-out.
-  SigninManagerFactory::GetForProfile(profile_)->SignOutAndRemoveAllAccounts(
-      signin_metrics::SIGNOUT_TEST,
-      signin_metrics::SignoutDelete::IGNORE_METRIC);
+  identity::ClearPrimaryAccount(
+      SigninManagerFactory::GetForProfile(profile_),
+      IdentityManagerFactory::GetForProfile(profile_),
+      identity::ClearPrimaryAccountPolicy::REMOVE_ALL_ACCOUNTS);
 }
 #endif  // !OS_CHROMEOS
 
@@ -362,19 +356,14 @@ bool ProfileSyncServiceHarness::AwaitMutualSyncCycleCompletion(
   return AwaitQuiescence(harnesses);
 }
 
-bool ProfileSyncServiceHarness::AwaitGroupSyncCycleCompletion(
-    const std::vector<ProfileSyncServiceHarness*>& partners) {
-  return AwaitQuiescence(partners);
-}
-
 // static
 bool ProfileSyncServiceHarness::AwaitQuiescence(
     const std::vector<ProfileSyncServiceHarness*>& clients) {
-  std::vector<ProfileSyncService*> services;
   if (clients.empty()) {
     return true;
   }
 
+  std::vector<ProfileSyncService*> services;
   for (const ProfileSyncServiceHarness* harness : clients) {
     services.push_back(harness->service());
   }
@@ -435,30 +424,6 @@ bool ProfileSyncServiceHarness::AwaitSyncSetupCompletion(
   return true;
 }
 
-std::string ProfileSyncServiceHarness::GenerateFakeOAuth2RefreshTokenString() {
-  return base::StringPrintf("oauth2_refresh_token_%d",
-                            ++oauth2_refesh_token_number_);
-}
-
-bool ProfileSyncServiceHarness::IsSyncEnabledByUser() const {
-  return service()->IsFirstSetupComplete() &&
-         !service()->HasDisableReason(
-             ProfileSyncService::DISABLE_REASON_USER_CHOICE);
-}
-
-void ProfileSyncServiceHarness::FinishSyncSetup() {
-  sync_blocker_.reset();
-  service()->SetFirstSetupComplete();
-}
-
-SyncCycleSnapshot ProfileSyncServiceHarness::GetLastCycleSnapshot() const {
-  DCHECK(service() != nullptr) << "Sync service has not yet been set up.";
-  if (service()->IsSyncActive()) {
-    return service()->GetLastCycleSnapshot();
-  }
-  return SyncCycleSnapshot();
-}
-
 bool ProfileSyncServiceHarness::EnableSyncForDatatype(
     syncer::ModelType datatype) {
   DVLOG(1) << GetClientInfoString(
@@ -591,6 +556,24 @@ bool ProfileSyncServiceHarness::DisableSyncForAllDatatypes() {
   return true;
 }
 
+SyncCycleSnapshot ProfileSyncServiceHarness::GetLastCycleSnapshot() const {
+  DCHECK(service() != nullptr) << "Sync service has not yet been set up.";
+  if (service()->IsSyncActive()) {
+    return service()->GetLastCycleSnapshot();
+  }
+  return SyncCycleSnapshot();
+}
+
+std::string ProfileSyncServiceHarness::GetServiceStatus() {
+  std::unique_ptr<base::DictionaryValue> value(
+      syncer::sync_ui_util::ConstructAboutInformation(service(),
+                                                      chrome::GetChannel()));
+  std::string service_status;
+  base::JSONWriter::WriteWithOptions(
+      *value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &service_status);
+  return service_status;
+}
+
 // TODO(sync): Clean up this method in a separate CL. Remove all snapshot fields
 // and log shorter, more meaningful messages.
 std::string ProfileSyncServiceHarness::GetClientInfoString(
@@ -622,16 +605,8 @@ std::string ProfileSyncServiceHarness::GetClientInfoString(
   return os.str();
 }
 
-bool ProfileSyncServiceHarness::IsTypePreferred(syncer::ModelType type) {
-  return service()->GetPreferredDataTypes().Has(type);
-}
-
-std::string ProfileSyncServiceHarness::GetServiceStatus() {
-  std::unique_ptr<base::DictionaryValue> value(
-      syncer::sync_ui_util::ConstructAboutInformation(service(),
-                                                      chrome::GetChannel()));
-  std::string service_status;
-  base::JSONWriter::WriteWithOptions(
-      *value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &service_status);
-  return service_status;
+bool ProfileSyncServiceHarness::IsSyncEnabledByUser() const {
+  return service()->IsFirstSetupComplete() &&
+         !service()->HasDisableReason(
+             ProfileSyncService::DISABLE_REASON_USER_CHOICE);
 }

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

@@ -41,29 +41,33 @@ class ProfileSyncServiceHarness {
       const std::string& username,
       const std::string& password,
       SigninType signin_type);
-  virtual ~ProfileSyncServiceHarness();
+  ~ProfileSyncServiceHarness();
 
-  // Creates a ProfileSyncService for the profile passed at construction and
-  // signs in without actually enabling sync the feature.
+  // Signs in to a primary account without actually enabling sync the feature.
+  // TODO(treib): Rename this to SignInToPrimaryAccount.
   bool SignIn();
 
-  // Creates a ProfileSyncService for the profile passed at construction and
-  // enables sync for all available datatypes. Returns true only after sync has
-  // been fully initialized and authenticated, and we are ready to process
-  // changes.
+  // Enables and configures sync for all available datatypes. Returns true only
+  // after sync has been fully initialized and authenticated, and we are ready
+  // to process changes.
   bool SetupSync();
 
-  // Setup sync without the authenticating through the passphrase encryption.
+  // Sets up sync without authenticating through the passphrase encryption.
   // Use this method when you need to setup a client that you're going to call
   // StopSyncService(), StartSyncService() directly after.
   bool SetupSyncForClearingServerData();
 
-  // Both SetupSync and SetupSyncForClear call into this method.
+  // Both SetupSync and SetupSyncForClearingServerData call into this method.
   // Same as the above method, but enables sync only for the datatypes contained
   // in |synced_datatypes|.
   bool SetupSync(syncer::ModelTypeSet synced_datatypes,
                  bool skip_passphrase_verification = false);
 
+  // Signals that sync setup is complete, and that PSS may begin syncing.
+  // Typically SetupSync does this automatically, but if that returned false,
+  // then setup may have been left incomplete.
+  void FinishSyncSetup();
+
   // Methods to stop and restart the sync service.
   //
   // For example, this can be used to simulate a sign-in/sign-out or can be
@@ -80,8 +84,10 @@ class ProfileSyncServiceHarness {
   bool StartSyncService();
 
 #if !defined(OS_CHROMEOS)
-  // Sign out of sync service. ChromeOS doesn't have the concept of sign-out,
-  // so this only exists on other platforms.
+  // Signs out of the primary account. ChromeOS doesn't have the concept of
+  // sign-out, so this only exists on other platforms.
+  // TODO(treib): Rename this to SignOut() - it does nothing with the
+  // SyncService.
   void SignoutSyncService();
 #endif  // !OS_CHROMEOS
 
@@ -99,13 +105,6 @@ class ProfileSyncServiceHarness {
   // exactly one client is waiting to receive those changes.
   bool AwaitMutualSyncCycleCompletion(ProfileSyncServiceHarness* partner);
 
-  // Blocks the caller until |this| completes its ongoing sync cycle and every
-  // other client in |partners| have achieved identical download progresses.
-  // Note: Use this method when exactly one client makes local change(s),
-  // and more than one client is waiting to receive those changes.
-  bool AwaitGroupSyncCycleCompletion(
-      const std::vector<ProfileSyncServiceHarness*>& partners);
-
   // Blocks the caller until every client in |clients| completes its ongoing
   // sync cycle and all the clients' progress markers match.  Note: Use this
   // method when more than one client makes local change(s), and more than one
@@ -145,21 +144,6 @@ class ProfileSyncServiceHarness {
   // Returns a snapshot of the current sync session.
   syncer::SyncCycleSnapshot GetLastCycleSnapshot() const;
 
-  // Check if |type| is being synced.
-  bool IsTypePreferred(syncer::ModelType type);
-
-  // Returns a string that can be used as the value of an oauth2 refresh token.
-  // This function guarantees that a different string is returned each time
-  // it is called.
-  std::string GenerateFakeOAuth2RefreshTokenString();
-
-  // Returns a string with relevant info about client's sync state (if
-  // available), annotated with |message|. Useful for logging.
-  std::string GetClientInfoString(const std::string& message) const;
-
-  // Signals that sync setup is complete, and that PSS may begin syncing.
-  void FinishSyncSetup();
-
  private:
   ProfileSyncServiceHarness(Profile* profile,
                             const std::string& username,
@@ -169,30 +153,30 @@ class ProfileSyncServiceHarness {
   // Gets detailed status from |service_| in pretty-printable form.
   std::string GetServiceStatus();
 
+  // Returns a string with relevant info about client's sync state (if
+  // available), annotated with |message|. Useful for logging.
+  std::string GetClientInfoString(const std::string& message) const;
+
   // Returns true if the user has enabled and configured sync for this client.
   // Note that this does not imply sync is actually running.
   bool IsSyncEnabledByUser() const;
 
-  // Sync profile associated with this sync client.
-  Profile* profile_;
+  // Profile associated with this sync client.
+  Profile* const profile_;
 
   // ProfileSyncService object associated with |profile_|.
-  browser_sync::ProfileSyncService* service_;
+  browser_sync::ProfileSyncService* const service_;
 
   // Prevents Sync from running until configuration is complete.
   std::unique_ptr<syncer::SyncSetupInProgressHandle> sync_blocker_;
 
   // Credentials used for GAIA authentication.
-  std::string username_;
-  std::string password_;
+  const std::string username_;
+  const std::string password_;
 
   // Used to decide what method of profile signin to use.
   const SigninType signin_type_;
 
-  // Number used by GenerateFakeOAuth2RefreshTokenString() to make sure that
-  // all refresh tokens used in the tests are different.
-  int oauth2_refesh_token_number_;
-
   // Used for logging.
   const std::string profile_debug_name_;