sync_ui_util: Only take a Profile* param, not other KeyedService*s

If these functions need the Profile anyway, they can just grab any
KeyedServices they require themselves. No need to make clients get them
and pass them in.

Bug: 911153
Change-Id: I31e9ecd3739e06fd2d4c9da00a1727cf9833e116
Reviewed-on: https://chromium-review.googlesource.com/c/1454539Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Hector Carmona <hcarmona@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#630705}

chrome/browser/browsing_data/counters/browsing_data_counter_utils.cc

@@ -54,12 +54,7 @@ bool ShouldShowCookieException(Profile* profile) {
   }
 #if BUILDFLAG(ENABLE_DICE_SUPPORT)
   if (AccountConsistencyModeManager::IsDiceEnabledForProfile(profile)) {
-    // TODO(http://crbug.com/890796): Migrate this part once sync_ui_util has
-    // been migrated to the IdentityManager.
-    sync_ui_util::MessageType sync_status = sync_ui_util::GetStatus(
-        profile, ProfileSyncServiceFactory::GetSyncServiceForProfile(profile),
-        IdentityManagerFactory::GetForProfile(profile));
-    return sync_status == sync_ui_util::SYNCED;
+    return sync_ui_util::GetStatus(profile) == sync_ui_util::SYNCED;
   }
 #endif
   return false;

chrome/browser/extensions/api/browsing_data/browsing_data_api.cc

@@ -149,13 +149,7 @@ bool IsRemovalPermitted(int removal_mask, PrefService* prefs) {
 
 // Returns true if Sync is currently running (i.e. enabled and not in error).
 bool IsSyncRunning(Profile* profile) {
-  syncer::SyncService* sync_service =
-      ProfileSyncServiceFactory::GetSyncServiceForProfile(profile);
-  identity::IdentityManager* identity_manager =
-      IdentityManagerFactory::GetForProfile(profile);
-  sync_ui_util::MessageType sync_status =
-      sync_ui_util::GetStatus(profile, sync_service, identity_manager);
-  return sync_status == sync_ui_util::SYNCED;
+  return sync_ui_util::GetStatus(profile) == sync_ui_util::SYNCED;
 }
 }  // namespace
 

chrome/browser/extensions/api/browsing_data/browsing_data_test.cc

@@ -422,9 +422,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowsingDataTest, Syncing) {
       ProfileSyncServiceFactory::GetForProfile(profile);
   sync_service->GetUserSettings()->SetFirstSetupComplete();
 
-  sync_ui_util::MessageType sync_status =
-      sync_ui_util::GetStatus(profile, sync_service, identity_manager);
-  ASSERT_EQ(sync_ui_util::SYNCED, sync_status);
+  ASSERT_EQ(sync_ui_util::SYNCED, sync_ui_util::GetStatus(profile));
   // Clear browsing data.
   auto function = base::MakeRefCounted<BrowsingDataRemoveFunction>();
   EXPECT_EQ(NULL, RunFunctionAndReturnSingleResult(
@@ -459,10 +457,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowsingDataTest, SyncError) {
               CREDENTIALS_REJECTED_BY_SERVER));
 
   // Sync is not running.
-  sync_ui_util::MessageType sync_status = sync_ui_util::GetStatus(
-      profile, ProfileSyncServiceFactory::GetForProfile(profile),
-      identity_manager);
-  ASSERT_NE(sync_ui_util::SYNCED, sync_status);
+  ASSERT_NE(sync_ui_util::SYNCED, sync_ui_util::GetStatus(profile));
   // Clear browsing data.
   auto function = base::MakeRefCounted<BrowsingDataRemoveFunction>();
   EXPECT_EQ(NULL, RunFunctionAndReturnSingleResult(

chrome/browser/sync/profile_sync_test_util.cc

@@ -72,18 +72,6 @@ ProfileSyncService::InitParams CreateProfileSyncServiceParamsForTest(
   return init_params;
 }
 
-std::unique_ptr<TestingProfile> MakeSignedInTestingProfile() {
-  std::unique_ptr<TestingProfile> profile =
-      IdentityTestEnvironmentProfileAdaptor::
-          CreateProfileForIdentityTestEnvironment();
-  auto identity_test_env_profile_adaptor =
-      std::make_unique<IdentityTestEnvironmentProfileAdaptor>(profile.get());
-
-  identity_test_env_profile_adaptor->identity_test_env()->SetPrimaryAccount(
-      "test@mail.com");
-  return profile;
-}
-
 std::unique_ptr<KeyedService> BuildMockProfileSyncService(
     content::BrowserContext* context) {
   return std::make_unique<NiceMock<browser_sync::ProfileSyncServiceMock>>(

chrome/browser/sync/profile_sync_test_util.h

@@ -19,7 +19,6 @@
 
 class KeyedService;
 class Profile;
-class TestingProfile;
 
 namespace content {
 class BrowserContext;
@@ -49,10 +48,6 @@ CreateProfileSyncServiceParamsForTest(
     std::unique_ptr<syncer::SyncClient> sync_client,
     Profile* profile);
 
-// A utility used by sync tests to create a TestingProfile with a Google
-// Services username stored in a (Testing)PrefService.
-std::unique_ptr<TestingProfile> MakeSignedInTestingProfile();
-
 // Helper routine to be used in conjunction with
 // BrowserContextKeyedServiceFactory::SetTestingFactory().
 std::unique_ptr<KeyedService> BuildMockProfileSyncService(

chrome/browser/sync/sync_ui_util.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/sync/sync_ui_util.h"
 
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/signin/signin_error_controller_factory.h"
 #include "chrome/browser/signin/signin_util.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
@@ -286,13 +287,14 @@ MessageType GetStatusLabelsImpl(const syncer::SyncService* service,
 }  // namespace
 
 MessageType GetStatusLabels(Profile* profile,
-                            const syncer::SyncService* service,
-                            identity::IdentityManager* identity_manager,
                             base::string16* status_label,
                             base::string16* link_label,
                             ActionType* action_type) {
-  DCHECK(service);
-
+  DCHECK(profile);
+  syncer::SyncService* service =
+      ProfileSyncServiceFactory::GetSyncServiceForProfile(profile);
+  identity::IdentityManager* identity_manager =
+      IdentityManagerFactory::GetForProfile(profile);
   const bool is_user_signout_allowed =
       signin_util::IsUserSignoutAllowedForProfile(profile);
   GoogleServiceAuthError auth_error =
@@ -301,6 +303,12 @@ MessageType GetStatusLabels(Profile* profile,
                              auth_error, status_label, link_label, action_type);
 }
 
+MessageType GetStatus(Profile* profile) {
+  ActionType action_type = NO_ACTION;
+  return GetStatusLabels(profile, /*status_label=*/nullptr,
+                         /*link_label=*/nullptr, &action_type);
+}
+
 #if !defined(OS_CHROMEOS)
 AvatarSyncErrorType GetMessagesForAvatarSyncError(
     Profile* profile,
@@ -375,16 +383,6 @@ AvatarSyncErrorType GetMessagesForAvatarSyncError(
 }
 #endif  // !defined(OS_CHROMEOS)
 
-MessageType GetStatus(Profile* profile,
-                      const syncer::SyncService* service,
-                      identity::IdentityManager* identity_manager) {
-  DCHECK(service);
-  ActionType action_type = NO_ACTION;
-  return GetStatusLabels(profile, service, identity_manager,
-                         /*status_label=*/nullptr, /*link_label=*/nullptr,
-                         &action_type);
-}
-
 bool ShouldRequestSyncConfirmation(const syncer::SyncService* service) {
   return !service->IsLocalSyncEnabled() &&
          service->GetUserSettings()->IsSyncRequested() &&

chrome/browser/sync/sync_ui_util.h

@@ -10,10 +10,6 @@
 
 class Profile;
 
-namespace identity {
-class IdentityManager;
-}
-
 namespace syncer {
 class SyncService;
 }  // namespace syncer
@@ -43,6 +39,8 @@ enum AvatarSyncErrorType {
   NO_SYNC_ERROR,                     // No sync error.
   MANAGED_USER_UNRECOVERABLE_ERROR,  // Unrecoverable error for managed users.
   UNRECOVERABLE_ERROR,               // Unrecoverable error for regular users.
+  // TODO(crbug.com/911153): Remove this value. It is never returned, but some
+  // clients still check for it.
   SUPERVISED_USER_AUTH_ERROR,  // Auth token error for supervised users.
   AUTH_ERROR,                  // Authentication error.
   UPGRADE_CLIENT_ERROR,        // Out-of-date client error.
@@ -50,16 +48,18 @@ enum AvatarSyncErrorType {
   SETTINGS_UNCONFIRMED_ERROR,  // Sync settings dialog not confirmed yet.
 };
 
-// Create status and link labels for the current status labels and link text
-// by querying |service|.
+// Returns the high-level sync status, and populates status and link label
+// strings for the current sync status by querying |profile|.
 // |status_label| and |link_label| must either be both null or both non-null.
 MessageType GetStatusLabels(Profile* profile,
-                            const syncer::SyncService* service,
-                            identity::IdentityManager* identity_manager,
                             base::string16* status_label,
                             base::string16* link_label,
                             ActionType* action_type);
 
+// Convenience version of GetStatusLabels for when you're not interested in the
+// actual labels, only in the return value.
+MessageType GetStatus(Profile* profile);
+
 #if !defined(OS_CHROMEOS)
 // Gets the error message and button label for the sync errors that should be
 // exposed to the user through the titlebar avatar button.
@@ -69,10 +69,6 @@ AvatarSyncErrorType GetMessagesForAvatarSyncError(
     int* button_string_id);
 #endif
 
-MessageType GetStatus(Profile* profile,
-                      const syncer::SyncService* service,
-                      identity::IdentityManager* identity_manager);
-
 // Whether sync is currently blocked from starting because the sync
 // confirmation dialog hasn't been shown. Note that once the dialog is
 // showing (i.e. IsFirstSetupInProgress() is true), this will return false.

chrome/browser/sync/sync_ui_util_unittest.cc

@@ -2,40 +2,33 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <stddef.h>
-
 #include <memory>
 #include <set>
+#include <string>
 
 #include "base/bind.h"
-#include "base/macros.h"
+#include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/signin/chrome_signin_client_factory.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "chrome/browser/signin/identity_test_environment_profile_adaptor.h"
-#include "chrome/browser/signin/signin_error_controller_factory.h"
-#include "chrome/browser/sync/profile_sync_test_util.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/sync/sync_ui_util.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/sync/driver/test_sync_service.h"
-#include "content/public/test/test_browser_thread.h"
+#include "components/sync/engine/sync_engine.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "services/identity/public/cpp/identity_manager.h"
 #include "services/identity/public/cpp/identity_test_environment.h"
 #include "services/identity/public/cpp/identity_test_utils.h"
 #include "services/identity/public/cpp/primary_account_mutator.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using content::BrowserThread;
 using syncer::TestSyncService;
-using ::testing::_;
-using ::testing::AtMost;
-using ::testing::NiceMock;
-using ::testing::Return;
-using ::testing::ReturnRef;
-using ::testing::SetArgPointee;
+
+namespace {
 
 // A number of distinct states of the SyncService can be generated for tests.
 enum DistinctState {
@@ -51,8 +44,6 @@ enum DistinctState {
   NUMBER_OF_STATUS_CASES
 };
 
-namespace {
-
 const char kTestGaiaId[] = "gaia-id-test_user@test.com";
 const char kTestUser[] = "test_user@test.com";
 const char kRefreshToken[] = "refresh_token";
@@ -203,13 +194,31 @@ sync_ui_util::ActionType GetActionTypeforDistinctCase(int case_number) {
   }
 }
 
+std::unique_ptr<KeyedService> BuildTestSyncService(
+    content::BrowserContext* context) {
+  return std::make_unique<TestSyncService>();
+}
+
+std::unique_ptr<TestingProfile> BuildTestingProfile() {
+  return IdentityTestEnvironmentProfileAdaptor::
+      CreateProfileForIdentityTestEnvironment(
+          {{ProfileSyncServiceFactory::GetInstance(),
+            base::BindRepeating(&BuildTestSyncService)}});
+}
+
+std::unique_ptr<TestingProfile> BuildSignedInTestingProfile() {
+  std::unique_ptr<TestingProfile> profile = BuildTestingProfile();
+  IdentityTestEnvironmentProfileAdaptor identity_adaptor(profile.get());
+  identity_adaptor.identity_test_env()->SetPrimaryAccount(kTestUser);
+  return profile;
+}
+
 // This test ensures that each distinctive SyncService status will return a
 // unique combination of status and link messages from GetStatusLabels().
 TEST_F(SyncUIUtilTest, DistinctCasesReportUniqueMessageSets) {
   std::set<base::string16> messages;
   for (int idx = 0; idx != NUMBER_OF_STATUS_CASES; idx++) {
-    std::unique_ptr<Profile> profile = IdentityTestEnvironmentProfileAdaptor::
-        CreateProfileForIdentityTestEnvironment();
+    std::unique_ptr<Profile> profile = BuildTestingProfile();
 
     IdentityTestEnvironmentProfileAdaptor env_adaptor(profile.get());
     identity::IdentityTestEnvironment* environment =
@@ -227,13 +236,14 @@ TEST_F(SyncUIUtilTest, DistinctCasesReportUniqueMessageSets) {
     // Need a primary account signed in before calling GetDistinctCase().
     environment->MakePrimaryAccountAvailable(kTestUser);
 
-    TestSyncService service;
-    GetDistinctCase(&service, identity_manager, idx);
+    TestSyncService* service = static_cast<TestSyncService*>(
+        ProfileSyncServiceFactory::GetSyncServiceForProfile(profile.get()));
+    GetDistinctCase(service, identity_manager, idx);
     base::string16 status_label;
     base::string16 link_label;
     sync_ui_util::ActionType action_type = sync_ui_util::NO_ACTION;
-    sync_ui_util::GetStatusLabels(profile.get(), &service, identity_manager,
-                                  &status_label, &link_label, &action_type);
+    sync_ui_util::GetStatusLabels(profile.get(), &status_label, &link_label,
+                                  &action_type);
 
     EXPECT_EQ(GetActionTypeforDistinctCase(idx), action_type)
         << "Wrong action returned for case #" << idx;
@@ -256,22 +266,21 @@ TEST_F(SyncUIUtilTest, DistinctCasesReportUniqueMessageSets) {
 }
 
 TEST_F(SyncUIUtilTest, UnrecoverableErrorWithActionableError) {
-  std::unique_ptr<Profile> profile(MakeSignedInTestingProfile());
-  identity::IdentityManager* identity_manager =
-      IdentityManagerFactory::GetForProfile(profile.get());
+  std::unique_ptr<Profile> profile = BuildSignedInTestingProfile();
 
-  TestSyncService service;
-  service.SetFirstSetupComplete(true);
-  service.SetDisableReasons(
+  TestSyncService* service = static_cast<TestSyncService*>(
+      ProfileSyncServiceFactory::GetSyncServiceForProfile(profile.get()));
+  service->SetFirstSetupComplete(true);
+  service->SetDisableReasons(
       syncer::SyncService::DISABLE_REASON_UNRECOVERABLE_ERROR);
 
   // First time action is not set. We should get unrecoverable error.
-  service.SetDetailedSyncStatus(true, syncer::SyncStatus());
+  service->SetDetailedSyncStatus(true, syncer::SyncStatus());
 
   base::string16 link_label;
   base::string16 unrecoverable_error_status_label;
   sync_ui_util::ActionType action_type = sync_ui_util::NO_ACTION;
-  sync_ui_util::GetStatusLabels(profile.get(), &service, identity_manager,
+  sync_ui_util::GetStatusLabels(profile.get(),
                                 &unrecoverable_error_status_label, &link_label,
                                 &action_type);
 
@@ -282,11 +291,10 @@ TEST_F(SyncUIUtilTest, UnrecoverableErrorWithActionableError) {
   // from previous one.
   syncer::SyncStatus status;
   status.sync_protocol_error.action = syncer::UPGRADE_CLIENT;
-  service.SetDetailedSyncStatus(true, status);
+  service->SetDetailedSyncStatus(true, status);
   base::string16 upgrade_client_status_label;
-  sync_ui_util::GetStatusLabels(profile.get(), &service, identity_manager,
-                                &upgrade_client_status_label, &link_label,
-                                &action_type);
+  sync_ui_util::GetStatusLabels(profile.get(), &upgrade_client_status_label,
+                                &link_label, &action_type);
   // Expect an explicit 'client upgrade' action.
   EXPECT_EQ(sync_ui_util::UPGRADE_CLIENT, action_type);
 
@@ -294,24 +302,23 @@ TEST_F(SyncUIUtilTest, UnrecoverableErrorWithActionableError) {
 }
 
 TEST_F(SyncUIUtilTest, ActionableErrorWithPassiveMessage) {
-  std::unique_ptr<Profile> profile(MakeSignedInTestingProfile());
-  identity::IdentityManager* identity_manager =
-      IdentityManagerFactory::GetForProfile(profile.get());
+  std::unique_ptr<Profile> profile = BuildSignedInTestingProfile();
 
-  TestSyncService service;
-  service.SetFirstSetupComplete(true);
-  service.SetDisableReasons(
+  TestSyncService* service = static_cast<TestSyncService*>(
+      ProfileSyncServiceFactory::GetSyncServiceForProfile(profile.get()));
+  service->SetFirstSetupComplete(true);
+  service->SetDisableReasons(
       syncer::SyncService::DISABLE_REASON_UNRECOVERABLE_ERROR);
 
   // Set action to UPGRADE_CLIENT.
   syncer::SyncStatus status;
   status.sync_protocol_error.action = syncer::UPGRADE_CLIENT;
-  service.SetDetailedSyncStatus(true, status);
+  service->SetDetailedSyncStatus(true, status);
 
   base::string16 first_actionable_error_status_label;
   base::string16 link_label;
   sync_ui_util::ActionType action_type = sync_ui_util::NO_ACTION;
-  sync_ui_util::GetStatusLabels(profile.get(), &service, identity_manager,
+  sync_ui_util::GetStatusLabels(profile.get(),
                                 &first_actionable_error_status_label,
                                 &link_label, &action_type);
   // Expect a 'client upgrade' call to action.
@@ -319,11 +326,11 @@ TEST_F(SyncUIUtilTest, ActionableErrorWithPassiveMessage) {
 
   // This time set action to ENABLE_SYNC_ON_ACCOUNT.
   status.sync_protocol_error.action = syncer::ENABLE_SYNC_ON_ACCOUNT;
-  service.SetDetailedSyncStatus(true, status);
+  service->SetDetailedSyncStatus(true, status);
 
   base::string16 second_actionable_error_status_label;
   action_type = sync_ui_util::NO_ACTION;
-  sync_ui_util::GetStatusLabels(profile.get(), &service, identity_manager,
+  sync_ui_util::GetStatusLabels(profile.get(),
                                 &second_actionable_error_status_label,
                                 &link_label, &action_type);
   // Expect a passive message instead of a call to action.
@@ -334,21 +341,19 @@ TEST_F(SyncUIUtilTest, ActionableErrorWithPassiveMessage) {
 }
 
 TEST_F(SyncUIUtilTest, SyncSettingsConfirmationNeededTest) {
-  std::unique_ptr<Profile> profile(MakeSignedInTestingProfile());
-  identity::IdentityManager* identity_manager =
-      IdentityManagerFactory::GetForProfile(profile.get());
+  std::unique_ptr<Profile> profile = BuildSignedInTestingProfile();
 
-  TestSyncService service;
-  service.SetFirstSetupComplete(false);
-  ASSERT_TRUE(sync_ui_util::ShouldRequestSyncConfirmation(&service));
+  TestSyncService* service = static_cast<TestSyncService*>(
+      ProfileSyncServiceFactory::GetSyncServiceForProfile(profile.get()));
+  service->SetFirstSetupComplete(false);
+  ASSERT_TRUE(sync_ui_util::ShouldRequestSyncConfirmation(service));
 
   base::string16 actionable_error_status_label;
   base::string16 link_label;
   sync_ui_util::ActionType action_type = sync_ui_util::NO_ACTION;
 
-  sync_ui_util::GetStatusLabels(profile.get(), &service, identity_manager,
-                                &actionable_error_status_label, &link_label,
-                                &action_type);
+  sync_ui_util::GetStatusLabels(profile.get(), &actionable_error_status_label,
+                                &link_label, &action_type);
 
   EXPECT_EQ(action_type, sync_ui_util::CONFIRM_SYNC_SETTINGS);
 }

chrome/browser/ui/webui/settings/people_handler.cc

@@ -928,8 +928,7 @@ PeopleHandler::GetSyncStatusDictionary() {
   sync_ui_util::ActionType action_type = sync_ui_util::NO_ACTION;
 
   bool status_has_error =
-      sync_ui_util::GetStatusLabels(profile_, service, identity_manager,
-                                    &status_label, &link_label,
+      sync_ui_util::GetStatusLabels(profile_, &status_label, &link_label,
                                     &action_type) == sync_ui_util::SYNC_ERROR;
   sync_status->SetString("statusText", status_label);
   sync_status->SetString("statusActionText", link_label);

chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc

@@ -250,9 +250,7 @@ void ClearBrowsingDataHandler::HandleClearBrowsingData(
   // However, if Sync is in error, clearing cookies should pause it.
   std::unique_ptr<AccountReconcilor::ScopedSyncedDataDeletion>
       scoped_data_deletion;
-  sync_ui_util::MessageType sync_status = sync_ui_util::GetStatus(
-      profile_, sync_service_, IdentityManagerFactory::GetForProfile(profile_));
-  if (sync_status == sync_ui_util::SYNCED) {
+  if (sync_ui_util::GetStatus(profile_) == sync_ui_util::SYNCED) {
     scoped_data_deletion = AccountReconcilorFactory::GetForProfile(profile_)
                                ->GetScopedSyncDataDeletion();
   }