[unified-consent] Record settings histogram and add browser tests

- Record settings histogram also when unified consent is disabled.

- Add browser test class for the UnifiedConsentService and add
  tests for the settings histogram.
  Note: More tests for the consent service will be added in a
  follow-up CL. Test support for Android and ChromeOS will be
  added in the future.

Bug: 908061, 888527
Change-Id: I4fa3ade5b8d3147c40032a3da631523e94ca09a3
Reviewed-on: https://chromium-review.googlesource.com/c/1352255
Commit-Queue: Thomas Tangl <tangltom@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612188}

chrome/browser/unified_consent/unified_consent_browsertest.cc

+// Copyright 2018 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 <memory>
+
+#include "base/test/metrics/histogram_tester.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/browser/sync/profile_sync_test_util.h"
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/unified_consent/chrome_unified_consent_service_client.h"
+#include "chrome/browser/unified_consent/unified_consent_service_factory.h"
+#include "chrome/test/base/scoped_testing_local_state.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/sync/test/fake_server/fake_server_network_resources.h"
+#include "components/unified_consent/scoped_unified_consent.h"
+#include "components/unified_consent/unified_consent_metrics.h"
+#include "components/unified_consent/unified_consent_service.h"
+
+namespace unified_consent {
+namespace {
+
+class UnifiedConsentBrowserTest : public SyncTest {
+ public:
+  UnifiedConsentBrowserTest() : SyncTest(SINGLE_CLIENT) {}
+  ~UnifiedConsentBrowserTest() override = default;
+
+  void DisableGoogleServices() {
+    ChromeUnifiedConsentServiceClient consent_service_client(
+        browser()->profile()->GetPrefs());
+    for (int i = 0;
+         i <= static_cast<int>(UnifiedConsentServiceClient::Service::kLast);
+         ++i) {
+      UnifiedConsentServiceClient::Service service =
+          static_cast<UnifiedConsentServiceClient::Service>(i);
+      if (consent_service_client.IsServiceSupported(service)) {
+        consent_service_client.SetServiceEnabled(service, false);
+        EXPECT_EQ(UnifiedConsentServiceClient::ServiceState::kDisabled,
+                  consent_service_client.GetServiceState(service));
+      }
+    }
+  }
+
+  void EnableSync() {
+    Profile* profile = browser()->profile();
+    ProfileSyncServiceFactory::GetForProfile(profile)
+        ->OverrideNetworkResourcesForTest(
+            std::make_unique<fake_server::FakeServerNetworkResources>(
+                GetFakeServer()->AsWeakPtr()));
+
+    std::unique_ptr<ProfileSyncServiceHarness> harness =
+        ProfileSyncServiceHarness::Create(
+            profile, "user@gmail.com", "fake_password",
+            ProfileSyncServiceHarness::SigninType::FAKE_SIGNIN);
+    EXPECT_TRUE(harness->SetupSync());
+  }
+
+  UnifiedConsentService* consent_service() {
+    return UnifiedConsentServiceFactory::GetForProfile(browser()->profile());
+  }
+
+ protected:
+  base::HistogramTester histogram_tester_;
+
+  DISALLOW_COPY_AND_ASSIGN(UnifiedConsentBrowserTest);
+};
+
+class UnifiedConsentDisabledBrowserTest : public UnifiedConsentBrowserTest {
+ public:
+  UnifiedConsentDisabledBrowserTest()
+      : scoped_unified_consent_disabled(UnifiedConsentFeatureState::kDisabled) {
+  }
+  ~UnifiedConsentDisabledBrowserTest() override = default;
+
+ private:
+  ScopedUnifiedConsent scoped_unified_consent_disabled;
+
+  DISALLOW_COPY_AND_ASSIGN(UnifiedConsentDisabledBrowserTest);
+};
+
+// Tests that the settings histogram is recorded if unified consent is enabled.
+// The histogram is recorded during profile initialization.
+IN_PROC_BROWSER_TEST_F(UnifiedConsentBrowserTest, PRE_SettingsHistogram_None) {
+  DisableGoogleServices();
+}
+
+IN_PROC_BROWSER_TEST_F(UnifiedConsentBrowserTest, SettingsHistogram_None) {
+  histogram_tester_.ExpectUniqueSample(
+      "UnifiedConsent.SyncAndGoogleServicesSettings",
+      metrics::SettingsHistogramValue::kNone, 1);
+}
+
+// Tests that the settings histogram is recorded if unified consent is disabled.
+// The histogram is recorded during profile initialization.
+IN_PROC_BROWSER_TEST_F(UnifiedConsentDisabledBrowserTest,
+                       PRE_SettingsHistogram_None) {
+  DisableGoogleServices();
+}
+
+IN_PROC_BROWSER_TEST_F(UnifiedConsentDisabledBrowserTest,
+                       SettingsHistogram_None) {
+  histogram_tester_.ExpectUniqueSample(
+      "UnifiedConsent.SyncAndGoogleServicesSettings",
+      metrics::SettingsHistogramValue::kNone, 1);
+}
+
+// Tests that all service entries in the settings histogram are recorded after
+// enabling them.
+IN_PROC_BROWSER_TEST_F(UnifiedConsentBrowserTest,
+                       PRE_SettingsHistogram_AllGoogleServicesEnabled) {
+  EnableSync();
+  consent_service()->EnableGoogleServices();
+}
+
+IN_PROC_BROWSER_TEST_F(UnifiedConsentBrowserTest,
+                       SettingsHistogram_AllGoogleServicesEnabled) {
+  histogram_tester_.ExpectBucketCount(
+      "UnifiedConsent.SyncAndGoogleServicesSettings",
+      metrics::SettingsHistogramValue::kNone, 0);
+  histogram_tester_.ExpectBucketCount(
+      "UnifiedConsent.SyncAndGoogleServicesSettings",
+      metrics::SettingsHistogramValue::kAllServicesWereEnabled, 1);
+  histogram_tester_.ExpectBucketCount(
+      "UnifiedConsent.SyncAndGoogleServicesSettings",
+      metrics::SettingsHistogramValue::kUrlKeyedAnonymizedDataCollection, 1);
+  histogram_tester_.ExpectBucketCount(
+      "UnifiedConsent.SyncAndGoogleServicesSettings",
+      metrics::SettingsHistogramValue::kSafeBrowsingExtendedReporting, 1);
+  histogram_tester_.ExpectBucketCount(
+      "UnifiedConsent.SyncAndGoogleServicesSettings",
+      metrics::SettingsHistogramValue::kSpellCheck, 1);
+  histogram_tester_.ExpectTotalCount(
+      "UnifiedConsent.SyncAndGoogleServicesSettings", 4);
+}
+
+}  // namespace
+}  // namespace unified_consent

chrome/browser/unified_consent/unified_consent_service_factory.cc

@@ -14,8 +14,12 @@
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
 #include "components/unified_consent/feature.h"
+#include "components/unified_consent/unified_consent_metrics.h"
 #include "components/unified_consent/unified_consent_service.h"
 
+using unified_consent::UnifiedConsentService;
+using unified_consent::metrics::RecordSettingsHistogram;
+
 UnifiedConsentServiceFactory::UnifiedConsentServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "UnifiedConsentService",
@@ -27,9 +31,9 @@ UnifiedConsentServiceFactory::UnifiedConsentServiceFactory()
 UnifiedConsentServiceFactory::~UnifiedConsentServiceFactory() = default;
 
 // static
-unified_consent::UnifiedConsentService*
-UnifiedConsentServiceFactory::GetForProfile(Profile* profile) {
-  return static_cast<unified_consent::UnifiedConsentService*>(
+UnifiedConsentService* UnifiedConsentServiceFactory::GetForProfile(
+    Profile* profile) {
+  return static_cast<UnifiedConsentService*>(
       GetInstance()->GetServiceForBrowserContext(profile, true));
 }
 
@@ -40,28 +44,32 @@ UnifiedConsentServiceFactory* UnifiedConsentServiceFactory::GetInstance() {
 
 void UnifiedConsentServiceFactory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
-  unified_consent::UnifiedConsentService::RegisterPrefs(registry);
+  UnifiedConsentService::RegisterPrefs(registry);
 }
 
 KeyedService* UnifiedConsentServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   Profile* profile = Profile::FromBrowserContext(context);
+  PrefService* pref_service = profile->GetPrefs();
+  auto service_client =
+      std::make_unique<ChromeUnifiedConsentServiceClient>(pref_service);
+  // Record settings for pre- and post-UnifiedConsent users.
+  RecordSettingsHistogram(service_client.get(), pref_service);
+
   syncer::SyncService* sync_service =
       ProfileSyncServiceFactory::GetSyncServiceForBrowserContext(profile);
   if (!sync_service)
     return nullptr;
 
   if (!unified_consent::IsUnifiedConsentFeatureEnabled()) {
-    ChromeUnifiedConsentServiceClient service_client(profile->GetPrefs());
-    unified_consent::UnifiedConsentService::RollbackIfNeeded(
-        profile->GetPrefs(), sync_service, &service_client);
+    UnifiedConsentService::RollbackIfNeeded(pref_service, sync_service,
+                                            service_client.get());
     return nullptr;
   }
 
-  return new unified_consent::UnifiedConsentService(
-      std::make_unique<ChromeUnifiedConsentServiceClient>(profile->GetPrefs()),
-      profile->GetPrefs(), IdentityManagerFactory::GetForProfile(profile),
-      sync_service);
+  return new UnifiedConsentService(
+      std::move(service_client), pref_service,
+      IdentityManagerFactory::GetForProfile(profile), sync_service);
 }
 
 bool UnifiedConsentServiceFactory::ServiceIsNULLWhileTesting() const {

chrome/test/BUILD.gn

@@ -1161,7 +1161,10 @@ test("browser_tests") {
           [ "../browser/ssl/captive_portal_blocking_page_browsertest.cc" ]
     }
     if (enable_dice_support) {
-      sources += [ "../browser/signin/dice_browsertest.cc" ]
+      sources += [
+        "../browser/signin/dice_browsertest.cc",
+        "../browser/unified_consent/unified_consent_browsertest.cc",
+      ]
     }
     if (!enable_one_click_signin) {
       sources -= [ "../browser/ui/sync/one_click_signin_links_delegate_impl_browsertest.cc" ]

components/unified_consent/unified_consent_metrics.cc

@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "components/prefs/pref_service.h"
+#include "components/unified_consent/pref_names.h"
 
 namespace {
 
@@ -33,6 +34,28 @@ void RecordConsentBumpEligibility(bool eligible) {
                         eligible);
 }
 
+void RecordSettingsHistogram(UnifiedConsentServiceClient* service_client,
+                             PrefService* pref_service) {
+  bool metric_recorded = false;
+
+  metric_recorded |= RecordSettingsHistogramFromPref(
+      prefs::kAllUnifiedConsentServicesWereEnabled, pref_service,
+      metrics::SettingsHistogramValue::kAllServicesWereEnabled);
+  metric_recorded |= RecordSettingsHistogramFromPref(
+      prefs::kUrlKeyedAnonymizedDataCollectionEnabled, pref_service,
+      metrics::SettingsHistogramValue::kUrlKeyedAnonymizedDataCollection);
+  metric_recorded |= RecordSettingsHistogramFromService(
+      service_client,
+      UnifiedConsentServiceClient::Service::kSafeBrowsingExtendedReporting,
+      metrics::SettingsHistogramValue::kSafeBrowsingExtendedReporting);
+  metric_recorded |= RecordSettingsHistogramFromService(
+      service_client, UnifiedConsentServiceClient::Service::kSpellCheck,
+      metrics::SettingsHistogramValue::kSpellCheck);
+
+  if (!metric_recorded)
+    RecordSettingsHistogramSample(metrics::SettingsHistogramValue::kNone);
+}
+
 void RecordSettingsHistogramSample(SettingsHistogramValue value) {
   UMA_HISTOGRAM_ENUMERATION(kSyncAndGoogleServicesSettingsHistogram, value);
 }

components/unified_consent/unified_consent_metrics.h

@@ -70,6 +70,11 @@ void RecordConsentBumpMetric(UnifiedConsentBumpAction action);
 // be called at startup.
 void RecordConsentBumpEligibility(bool eligible);
 
+// Records settings entries in the kSyncAndGoogleServicesSettingsHistogram.
+// kNone is recorded when none of the settings is enabled.
+void RecordSettingsHistogram(UnifiedConsentServiceClient* service_client,
+                             PrefService* pref_service);
+
 // Records a sample in the kSyncAndGoogleServicesSettingsHistogram. Wrapped in a
 // function to avoid code size issues caused by histogram macros.
 void RecordSettingsHistogramSample(SettingsHistogramValue value);

components/unified_consent/unified_consent_service.cc

@@ -42,8 +42,6 @@ UnifiedConsentService::UnifiedConsentService(
 
   identity_manager_->AddObserver(this);
   sync_service_->AddObserver(this);
-
-  RecordSettingsHistogram();
 }
 
 UnifiedConsentService::~UnifiedConsentService() {}
@@ -274,27 +272,6 @@ bool UnifiedConsentService::AreAllOnByDefaultPrivacySettingsOn() {
   return true;
 }
 
-void UnifiedConsentService::RecordSettingsHistogram() {
-  bool metric_recorded = false;
-
-  metric_recorded |= RecordSettingsHistogramFromPref(
-      prefs::kAllUnifiedConsentServicesWereEnabled, pref_service_,
-      metrics::SettingsHistogramValue::kAllServicesWereEnabled);
-  metric_recorded |= RecordSettingsHistogramFromPref(
-      prefs::kUrlKeyedAnonymizedDataCollectionEnabled, pref_service_,
-      metrics::SettingsHistogramValue::kUrlKeyedAnonymizedDataCollection);
-  metric_recorded |= RecordSettingsHistogramFromService(
-      service_client_.get(),
-      UnifiedConsentServiceClient::Service::kSafeBrowsingExtendedReporting,
-      metrics::SettingsHistogramValue::kSafeBrowsingExtendedReporting);
-  metric_recorded |= RecordSettingsHistogramFromService(
-      service_client_.get(), UnifiedConsentServiceClient::Service::kSpellCheck,
-      metrics::SettingsHistogramValue::kSpellCheck);
-
-  if (!metric_recorded)
-    RecordSettingsHistogramSample(metrics::SettingsHistogramValue::kNone);
-}
-
 void UnifiedConsentService::CheckConsentBumpEligibility() {
   // Only check eligility if the user was eligible before.
   if (!ShouldShowConsentBump()) {

components/unified_consent/unified_consent_service.h

@@ -104,10 +104,6 @@ class UnifiedConsentService : public KeyedService,
   // Checks if all on-by-default non-personalized services are on.
   bool AreAllOnByDefaultPrivacySettingsOn();
 
-  // Records a sample for each bucket enabled by the user (except kNone).
-  // kNone is recorded when none of the other buckets are recorded.
-  void RecordSettingsHistogram();
-
   // This method is called on startup to check the eligibility criteria for
   // showing the consent bump. The check is only done when the profile was
   // eligible before. If the user is not eligible anymore, the

components/unified_consent/unified_consent_service_unittest.cc

@@ -40,6 +40,8 @@ class TestSyncService : public syncer::TestSyncService {
 
  private:
   syncer::SyncServiceObserver* observer_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(TestSyncService);
 };
 
 const char kSpellCheckDummyEnabled[] = "spell_check_dummy.enabled";
@@ -97,6 +99,8 @@ class FakeUnifiedConsentServiceClient : public UnifiedConsentServiceClient {
   static std::map<Service, bool> is_not_supported_;
 
   PrefService* pref_service_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeUnifiedConsentServiceClient);
 };
 
 std::map<Service, bool> FakeUnifiedConsentServiceClient::service_enabled_;
@@ -191,6 +195,8 @@ class UnifiedConsentServiceTest : public testing::Test {
   std::unique_ptr<FakeUnifiedConsentServiceClient> service_client_;
 
   std::unique_ptr<ScopedUnifiedConsent> scoped_unified_consent_;
+
+  DISALLOW_COPY_AND_ASSIGN(UnifiedConsentServiceTest);
 };
 
 TEST_F(UnifiedConsentServiceTest, DefaultValuesWhenSignedOut) {
@@ -414,60 +420,6 @@ TEST_F(UnifiedConsentServiceTest, Rollback_UserOptedIntoUnifiedConsent) {
             service_client_->GetServiceState(Service::kContextualSearch));
 }
 
-TEST_F(UnifiedConsentServiceTest, SettingsHistogram_None) {
-  base::HistogramTester histogram_tester;
-  // Disable all services.
-  sync_service_.GetUserSettings()->SetChosenDataTypes(false,
-                                                      syncer::ModelTypeSet());
-  CreateConsentService();
-
-  histogram_tester.ExpectUniqueSample(
-      "UnifiedConsent.SyncAndGoogleServicesSettings",
-      metrics::SettingsHistogramValue::kNone, 1);
-}
-
-TEST_F(UnifiedConsentServiceTest, SettingsHistogram_UnifiedConsentGiven) {
-  base::HistogramTester histogram_tester;
-  // Unified consent is given.
-  identity_test_environment_.SetPrimaryAccount("testaccount");
-  pref_service_.SetInteger(
-      prefs::kUnifiedConsentMigrationState,
-      static_cast<int>(unified_consent::MigrationState::kCompleted));
-  pref_service_.SetBoolean(prefs::kAllUnifiedConsentServicesWereEnabled, true);
-  CreateConsentService(true);
-
-  histogram_tester.ExpectBucketCount(
-      "UnifiedConsent.SyncAndGoogleServicesSettings",
-      metrics::SettingsHistogramValue::kNone, 0);
-  histogram_tester.ExpectBucketCount(
-      "UnifiedConsent.SyncAndGoogleServicesSettings",
-      metrics::SettingsHistogramValue::kAllServicesWereEnabled, 1);
-  histogram_tester.ExpectBucketCount(
-      "UnifiedConsent.SyncAndGoogleServicesSettings",
-      metrics::SettingsHistogramValue::kUrlKeyedAnonymizedDataCollection, 1);
-  histogram_tester.ExpectBucketCount(
-      "UnifiedConsent.SyncAndGoogleServicesSettings",
-      metrics::SettingsHistogramValue::kSafeBrowsingExtendedReporting, 1);
-  histogram_tester.ExpectBucketCount(
-      "UnifiedConsent.SyncAndGoogleServicesSettings",
-      metrics::SettingsHistogramValue::kSpellCheck, 1);
-  histogram_tester.ExpectTotalCount(
-      "UnifiedConsent.SyncAndGoogleServicesSettings", 4);
-}
-
-TEST_F(UnifiedConsentServiceTest, SettingsHistogram_NoUnifiedConsentGiven) {
-  base::HistogramTester histogram_tester;
-  // Unified consent is not given. Only spellcheck is enabled.
-  pref_service_.SetBoolean(kSpellCheckDummyEnabled, true);
-  CreateConsentService();
-
-  // kUserEvents should have no sample even though the sync preference is set,
-  // because the user is not signed in.
-  histogram_tester.ExpectUniqueSample(
-      "UnifiedConsent.SyncAndGoogleServicesSettings",
-      metrics::SettingsHistogramValue::kSpellCheck, 1);
-}
-
 TEST_F(UnifiedConsentServiceTest, ConsentBump_EligibleOnSecondStartup) {
   base::HistogramTester histogram_tester;
 

ios/chrome/browser/unified_consent/unified_consent_service_factory.cc

@@ -8,6 +8,7 @@
 #include "components/keyed_service/ios/browser_state_dependency_manager.h"
 #include "components/sync/driver/sync_service.h"
 #include "components/unified_consent/feature.h"
+#include "components/unified_consent/unified_consent_metrics.h"
 #include "components/unified_consent/unified_consent_service.h"
 #include "ios/chrome/browser/application_context.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
@@ -62,6 +63,10 @@ UnifiedConsentServiceFactory::BuildServiceInstanceFor(
   syncer::SyncService* sync_service =
       ProfileSyncServiceFactory::GetForBrowserState(browser_state);
 
+  // Record settings for pre- and post-UnifiedConsent users.
+  unified_consent::metrics::RecordSettingsHistogram(service_client.get(),
+                                                    user_pref_service);
+
   if (!unified_consent::IsUnifiedConsentFeatureEnabled()) {
     unified_consent::UnifiedConsentService::RollbackIfNeeded(
         user_pref_service, sync_service, service_client.get());