Fix CachedMetricsProfile and DemographicMetricsProvider for ChromeOS

This CL fixes the CachedMetricsProfile and DemographicMetricsProvider
for ChromeOS Ash. It introduces a profile selection strategy for
CachedMetricsProfile and skips the number of profiles on disk check in
DemographicMetricsProvider for ChromeOS Ash.

Bug: 1145655
Change-Id: I3cf69673e428cc7ff81e4d1e5f1650a6d96bdcc2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527521Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Vincent Boisselle <vincb@google.com>
Commit-Queue: Toby Huang <tobyhuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833819}

chrome/browser/metrics/cached_metrics_profile.cc

@@ -4,12 +4,30 @@
 
 #include "chrome/browser/metrics/cached_metrics_profile.h"
 
+#include "build/chromeos_buildflags.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "components/user_manager/user.h"
+#include "components/user_manager/user_manager.h"
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+
 namespace metrics {
 
+namespace {
+
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+bool IsLoggedIn() {
+  return user_manager::UserManager::IsInitialized() &&
+         user_manager::UserManager::Get()->IsUserLoggedIn();
+}
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+
+}  // namespace
+
 CachedMetricsProfile::CachedMetricsProfile() = default;
 
 CachedMetricsProfile::~CachedMetricsProfile() = default;
@@ -20,16 +38,28 @@ Profile* CachedMetricsProfile::GetMetricsProfile() {
     return nullptr;
 
   // If there is a cached profile, reuse that.  However, check that it is still
-  // valid first.
+  // valid first. This logic is valid for all platforms, including ChromeOS Ash.
   if (cached_profile_ && profile_manager->IsValidProfile(cached_profile_))
     return cached_profile_;
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+  // Select the primary user profile for ChromeOS.
+  if (!IsLoggedIn())
+    return nullptr;
+  const user_manager::User* primary_user =
+      user_manager::UserManager::Get()->GetPrimaryUser();
+  if (!primary_user || !primary_user->is_profile_created())
+    return nullptr;
+  cached_profile_ =
+      chromeos::ProfileHelper::Get()->GetProfileByUser(primary_user);
+#else
   // Find a suitable profile to use, and cache it so that we continue to report
   // statistics on the same profile.  We would simply use
   // ProfileManager::GetLastUsedProfile(), except that that has the side effect
   // of creating a profile if it does not yet exist.
   cached_profile_ = profile_manager->GetProfileByPath(
       profile_manager->GetLastUsedProfileDir(profile_manager->user_data_dir()));
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
   if (cached_profile_) {
     // Ensure that the returned profile is not an incognito profile.
     cached_profile_ = cached_profile_->GetOriginalProfile();

chrome/browser/metrics/cached_metrics_profile.h

@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_METRICS_CACHED_METRICS_PROFILE_H_
 #define CHROME_BROWSER_METRICS_CACHED_METRICS_PROFILE_H_
 
-#include "base/macros.h"
-
 class Profile;
 
 namespace metrics {
@@ -16,6 +14,8 @@ namespace metrics {
 class CachedMetricsProfile {
  public:
   CachedMetricsProfile();
+  CachedMetricsProfile(const CachedMetricsProfile&) = delete;
+  CachedMetricsProfile& operator=(const CachedMetricsProfile&) = delete;
   ~CachedMetricsProfile();
 
   // Returns the profile for which metrics will be gathered. Once a suitable
@@ -28,8 +28,6 @@ class CachedMetricsProfile {
   // its value is cached here so that GetMetricsProfile() can return a
   // consistent value.
   Profile* cached_profile_ = nullptr;
-
-  DISALLOW_COPY_AND_ASSIGN(CachedMetricsProfile);
 };
 
 }  // namespace metrics

chrome/browser/metrics/chrome_metrics_service_client.cc

@@ -386,8 +386,10 @@ bool IsProcessRunning(base::ProcessId pid) {
 class ProfileClientImpl
     : public metrics::DemographicMetricsProvider::ProfileClient {
  public:
-  ~ProfileClientImpl() override {}
   ProfileClientImpl() = default;
+  ProfileClientImpl(const ProfileClientImpl&) = delete;
+  ProfileClientImpl& operator=(const ProfileClientImpl&) = delete;
+  ~ProfileClientImpl() override = default;
 
   int GetNumberOfProfilesOnDisk() override {
     return g_browser_process->profile_manager()->GetNumberOfProfiles();
@@ -425,8 +427,6 @@ class ProfileClientImpl
  private:
   // Provides the same cached Profile each time.
   metrics::CachedMetricsProfile cached_metrics_profile_;
-
-  DISALLOW_COPY_AND_ASSIGN(ProfileClientImpl);
 };
 
 std::unique_ptr<metrics::DemographicMetricsProvider>

chrome/browser/metrics/chromeos_metrics_provider_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/multidevice_setup/multidevice_setup_client_factory.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile_manager.h"
@@ -75,6 +76,10 @@ class TestChromeOSMetricsProvider : public ChromeOSMetricsProvider {
   }
 };
 
+const AccountId account_id1(AccountId::FromUserEmail("user1@example.com"));
+const AccountId account_id2(AccountId::FromUserEmail("user2@example.com"));
+const AccountId account_id3(AccountId::FromUserEmail("user3@example.com"));
+
 }  // namespace
 
 class ChromeOSMetricsProviderTest : public testing::Test {
@@ -100,9 +105,7 @@ class ChromeOSMetricsProviderTest : public testing::Test {
     profile_manager_ = std::make_unique<TestingProfileManager>(
         TestingBrowserProcess::GetGlobal());
     ASSERT_TRUE(profile_manager_->SetUp());
-    TestingProfile* profile =
-        profile_manager_->CreateTestingProfile("test_name");
-    profile_manager_->UpdateLastUser(profile);
+    testing_profile_ = profile_manager_->CreateTestingProfile("test_name");
 
     // Set statistic provider for hardware class tests.
     chromeos::system::StatisticsProvider::SetTestProvider(
@@ -128,6 +131,7 @@ class ChromeOSMetricsProviderTest : public testing::Test {
   base::test::ScopedFeatureList scoped_feature_list_;
   chromeos::system::ScopedFakeStatisticsProvider fake_statistics_provider_;
   std::unique_ptr<TestingProfileManager> profile_manager_;
+  TestingProfile* testing_profile_ = nullptr;
   std::unique_ptr<FakeMultiDeviceSetupClientImplFactory>
       fake_multidevice_setup_client_impl_factory_;
 
@@ -138,13 +142,10 @@ class ChromeOSMetricsProviderTest : public testing::Test {
 };
 
 TEST_F(ChromeOSMetricsProviderTest, MultiProfileUserCount) {
-  const AccountId account_id1(AccountId::FromUserEmail("user1@example.com"));
-  const AccountId account_id2(AccountId::FromUserEmail("user2@example.com"));
-  const AccountId account_id3(AccountId::FromUserEmail("user3@example.com"));
-
   // |scoped_enabler| takes over the lifetime of |user_manager|.
   chromeos::FakeChromeUserManager* user_manager =
       new chromeos::FakeChromeUserManager();
+  // TODO(crbug/1154780): Overload operator-> in ScopedUserManager.
   user_manager::ScopedUserManager scoped_enabler(
       base::WrapUnique(user_manager));
   user_manager->AddKioskAppUser(account_id1);
@@ -162,13 +163,10 @@ TEST_F(ChromeOSMetricsProviderTest, MultiProfileUserCount) {
 }
 
 TEST_F(ChromeOSMetricsProviderTest, MultiProfileCountInvalidated) {
-  const AccountId account_id1(AccountId::FromUserEmail("user1@example.com"));
-  const AccountId account_id2(AccountId::FromUserEmail("user2@example.com"));
-  const AccountId account_id3(AccountId::FromUserEmail("user3@example.com"));
-
   // |scoped_enabler| takes over the lifetime of |user_manager|.
   chromeos::FakeChromeUserManager* user_manager =
       new chromeos::FakeChromeUserManager();
+  // TODO(crbug/1154780): Overload operator-> in ScopedUserManager.
   user_manager::ScopedUserManager scoped_enabler(
       base::WrapUnique(user_manager));
   user_manager->AddKioskAppUser(account_id1);
@@ -203,6 +201,18 @@ TEST_F(ChromeOSMetricsProviderTest, HasLinkedAndroidPhoneAndEnabledFeatures) {
       chromeos::multidevice_setup::mojom::Feature::kMessages,
       chromeos::multidevice_setup::mojom::FeatureState::kFurtherSetupRequired);
 
+  // |scoped_enabler| takes over the lifetime of |user_manager|.
+  chromeos::FakeChromeUserManager* user_manager =
+      new chromeos::FakeChromeUserManager();
+  // TODO(crbug/1154780): Overload operator-> in ScopedUserManager.
+  user_manager::ScopedUserManager scoped_enabler(
+      base::WrapUnique(user_manager));
+  user_manager->AddKioskAppUser(account_id1);
+  user_manager->LoginUser(account_id1);
+  const user_manager::User* primary_user = user_manager->GetPrimaryUser();
+  chromeos::ProfileHelper::Get()->SetUserToProfileMappingForTesting(
+      primary_user, testing_profile_);
+
   TestChromeOSMetricsProvider provider;
   metrics::SystemProfileProto system_profile;
   provider.ProvideSystemProfileMetrics(&system_profile);

components/metrics/demographics/BUILD.gn

@@ -16,6 +16,7 @@ static_library("demographics") {
 
   deps = [
     "//base",
+    "//build:chromeos_buildflags",
     "//components/metrics",
     "//components/pref_registry",
     "//components/prefs",
@@ -34,6 +35,7 @@ source_set("unit_tests") {
     ":demographics",
     "//base",
     "//base/test:test_support",
+    "//build:chromeos_buildflags",
     "//components/metrics",
     "//components/sync/base",
     "//components/sync/driver",

components/metrics/demographics/demographic_metrics_provider.cc

@@ -7,6 +7,7 @@
 #include "base/feature_list.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/optional.h"
+#include "build/chromeos_buildflags.h"
 #include "components/sync/driver/sync_service_utils.h"
 #include "third_party/metrics_proto/ukm/report.pb.h"
 
@@ -56,14 +57,23 @@ DemographicMetricsProvider::ProvideSyncedUserNoisedBirthYearAndGender() {
   if (!base::FeatureList::IsEnabled(kDemographicMetricsReporting))
     return base::nullopt;
 
+#if !BUILDFLAG(IS_CHROMEOS_ASH)
   // Skip if not exactly one Profile on disk. Having more than one Profile that
   // is using the browser can make demographics less relevant. This approach
   // cannot determine if there is more than 1 distinct user using the Profile.
+
+  // ChromeOS almost always has more than one profile on disk, so this check
+  // doesn't work. We have a profile selection strategy for ChromeOS, so skip
+  // this check for ChromeOS.
+  // TODO(crbug/1145655): LaCros will behave similarly to desktop Chrome and
+  // reduce the number of profiles on disk to one, so remove these #if guards
+  // after LaCros release.
   if (profile_client_->GetNumberOfProfilesOnDisk() != 1) {
     LogUserDemographicsStatusInHistogram(
         UserDemographicsStatus::kMoreThanOneProfile);
     return base::nullopt;
   }
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH)
 
   syncer::SyncService* sync_service = profile_client_->GetSyncService();
   // Skip if no sync service.

components/metrics/demographics/demographic_metrics_provider.h

@@ -53,6 +53,8 @@ class DemographicMetricsProvider : public MetricsProvider,
     virtual PrefService* GetPrefService() = 0;
 
     // Gets the network time that represents now.
+    // TODO(crbug/1145655): Remove this function and replace with
+    // base::Time::Now().
     virtual base::Time GetNetworkTime() const = 0;
   };
 

components/metrics/demographics/demographic_metrics_provider_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/simple_test_clock.h"
 #include "base/time/time.h"
+#include "build/chromeos_buildflags.h"
 #include "components/metrics/demographics/user_demographics.h"
 #include "components/metrics/metrics_log_uploader.h"
 #include "components/sync/base/sync_prefs.h"
@@ -283,6 +284,7 @@ TEST(DemographicMetricsProviderTest,
   ChromeUserMetricsExtension uma_proto;
   provider.ProvideSyncedUserNoisedBirthYearAndGender(&uma_proto);
 
+#if !BUILDFLAG(IS_CHROMEOS_ASH)
   // Expect that the UMA proto is untouched.
   EXPECT_FALSE(uma_proto.user_demographics().has_birth_year());
   EXPECT_FALSE(uma_proto.user_demographics().has_gender());
@@ -290,6 +292,16 @@ TEST(DemographicMetricsProviderTest,
   // Verify histograms.
   histogram.ExpectUniqueSample("UMA.UserDemographics.Status",
                                UserDemographicsStatus::kMoreThanOneProfile, 1);
+#else
+  // On ChromeOS, we have a profile selection strategy, so expect UMA reporting
+  // to work.
+  EXPECT_TRUE(uma_proto.user_demographics().has_birth_year());
+  EXPECT_TRUE(uma_proto.user_demographics().has_gender());
+
+  // Verify histograms.
+  histogram.ExpectUniqueSample("UMA.UserDemographics.Status",
+                               UserDemographicsStatus::kSuccess, 1);
+#endif  // !BUILDFLAG(IS_CHROMEOS_ASH)
 }
 
 TEST(DemographicMetricsProviderTest,

components/metrics/demographics/user_demographics.h

@@ -23,7 +23,7 @@ constexpr int kUserDemographicsGenderDefaultValue = -1;
 constexpr UserDemographicsProto_Gender kUserDemographicGenderDefaultEnumValue =
     UserDemographicsProto_Gender_Gender_MIN;
 
-// Default value for user gender when no value has been set.
+// Default value for user birth year when no value has been set.
 constexpr int kUserDemographicsBirthYearDefaultValue = -1;
 
 // Default value for birth year offset when no value has been set. Set to a