Reland "Create updater::UsageStatsProvider"

Change 6334513 was reverted because the test helper
AnyAppUsageUsageEnabled was not called from branded mac builds.
This removes the function.

original commit: commit 736662131c

Bug: 371595849
Change-Id: I669bcf5abc7214c8ddcf0be7b3626575b9e202fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6344256
Commit-Queue: Felipe Pazos <fpazos@google.com>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Noah Rose Ledesma <noahrose@google.com>
Cr-Commit-Position: refs/heads/main@{#1431277}

chrome/updater/app/app_install.cc

@@ -124,7 +124,9 @@ void AppInstall::SendPing(int exit_code, base::OnceClosure callback) {
           base::BindOnce(
               [](base::OnceClosure callback, UpdaterScope scope,
                  int exit_code) {
-                if (exit_code == kErrorOk || !AnyAppUsageStatsAllowed(scope)) {
+                if (exit_code == kErrorOk ||
+                    !UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+                        scope)) {
                   std::move(callback).Run();
                   return;
                 }

chrome/updater/app/server/win/com_classes_legacy.cc

@@ -1286,7 +1286,7 @@ void LegacyAppCommandWebImpl::SendPing(UpdaterScope scope,
         scoped_refptr<PersistedData> persisted_data =
             config->GetUpdaterPersistedData();
         if (!persisted_data->GetUsageStatsEnabled() &&
-            !AnyAppUsageStatsAllowed(scope)) {
+            !UsageStatsProvider::Create()->AnyAppEnablesUsageStats(scope)) {
           return;
         }
 

chrome/updater/crash_client.cc

@@ -139,7 +139,7 @@ bool CrashClient::InitializeCrashReporting(UpdaterScope updater_scope) {
       (base::Environment::Create()->GetVar(kUsageStatsEnabled,
                                            &env_usage_stats) &&
        env_usage_stats == kUsageStatsEnabledValueEnabled) ||
-      AnyAppUsageStatsAllowed(updater_scope)) {
+      UsageStatsProvider::Create()->AnyAppEnablesUsageStats(updater_scope)) {
     crashpad::Settings* crashpad_settings = database_->GetSettings();
     CHECK(crashpad_settings);
     crashpad_settings->SetUploadsEnabled(true);

chrome/updater/update_usage_stats_task.cc

@@ -4,8 +4,10 @@
 
 #include "chrome/updater/update_usage_stats_task.h"
 
+#include "base/functional/callback.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/sequence_checker.h"
+#include "base/task/thread_pool.h"
 #include "chrome/updater/crash_client.h"
 #include "chrome/updater/persisted_data.h"
 #include "chrome/updater/updater_scope.h"
@@ -29,4 +31,19 @@ void UpdateUsageStatsTask::SetUsageStatsEnabled(
   CrashClient::GetInstance()->SetUploadsEnabled(enabled);
 }
 
+void UpdateUsageStatsTask::Run(base::OnceClosure callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE, {base::MayBlock()},
+      base::BindOnce(
+          [](UpdaterScope scope) {
+            return UsageStatsProvider::Create()->AnyAppEnablesUsageStats(scope);
+          },
+          scope_),
+      base::BindOnce(&UpdateUsageStatsTask::SetUsageStatsEnabled, this,
+                     persisted_data_)
+          .Then(std::move(callback)));
+}
+
 }  // namespace updater

chrome/updater/update_usage_stats_task.h

@@ -5,19 +5,49 @@
 #ifndef CHROME_UPDATER_UPDATE_USAGE_STATS_TASK_H_
 #define CHROME_UPDATER_UPDATE_USAGE_STATS_TASK_H_
 
+#include <memory>
+#include <string>
+
 #include "base/functional/callback_forward.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/sequence_checker.h"
+#include "build/build_config.h"
 #include "chrome/updater/updater_scope.h"
 
+namespace base {
+class FilePath;
+}
+
 namespace updater {
 
-// Checks if any app besides Omaha 4 or CECA is allowed to send usage stats.
-bool AnyAppUsageStatsAllowed(UpdaterScope scope);
-
 class PersistedData;
 
+// A UsageStatsProvider evaluates the usage stat state of apps on the system to
+// determine whether or not the updater is allowed to send usage stats.
+class UsageStatsProvider {
+ public:
+  virtual ~UsageStatsProvider() = default;
+
+  // Returns true if any app besides Omaha 4 or CECA is allowed to send usage
+  // stats. The function looks at apps installed on the system to check if they
+  // have usage stats enabled. This information is stored in the registry on
+  // Windows, and in a crashpad database found in the `ApplicationSupport`
+  // directory on MacOS.
+  virtual bool AnyAppEnablesUsageStats(UpdaterScope scope) = 0;
+  static std::unique_ptr<UsageStatsProvider> Create();
+
+ private:
+#if BUILDFLAG(IS_WIN)
+  static std::unique_ptr<UsageStatsProvider> Create(
+      const std::wstring& system_key,
+      const std::wstring& user_key);
+#elif BUILDFLAG(IS_MAC)
+  static std::unique_ptr<UsageStatsProvider> Create(
+      const base::FilePath& app_directory);
+#endif
+};
+
 class UpdateUsageStatsTask
     : public base::RefCountedThreadSafe<UpdateUsageStatsTask> {
  public:

chrome/updater/update_usage_stats_task_linux.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/updater/update_usage_stats_task.h"
 
+#include <memory>
 #include <utility>
 
 #include "base/functional/bind.h"
@@ -15,20 +16,16 @@
 
 namespace updater {
 
-bool AnyAppUsageStatsAllowed(UpdaterScope scope) {
+class UsageStatsProviderImpl : public UsageStatsProvider {
+ public:
+  UsageStatsProviderImpl() = default;
+
   // TODO(crbug.com/40821596): Implement.
-  return false;
+  bool AnyAppEnablesUsageStats(UpdaterScope scope) override { return false; }
-}
+};
 
-void UpdateUsageStatsTask::Run(base::OnceClosure callback) {
+std::unique_ptr<UsageStatsProvider> UsageStatsProvider::Create() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return std::make_unique<UsageStatsProviderImpl>();
-
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&AnyAppUsageStatsAllowed, scope_),
-      base::BindOnce(&UpdateUsageStatsTask::SetUsageStatsEnabled, this,
-                     persisted_data_)
-          .Then(std::move(callback)));
 }
 
 }  // namespace updater

chrome/updater/update_usage_stats_task_mac.mm

@@ -14,10 +14,6 @@
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
-#include "base/functional/bind.h"
-#include "base/functional/callback.h"
-#include "base/task/thread_pool.h"
-#include "chrome/updater/persisted_data.h"
 #include "chrome/updater/updater_branding.h"
 #include "chrome/updater/updater_scope.h"
 #include "chrome/updater/util/mac_util.h"
@@ -56,22 +52,6 @@ std::vector<base::FilePath> GetAppSupportDirectoriesForScope(
   return app_support_dirs;
 }
 
-// Returns all directories under "Application Support/<company name>" for the
-// given scope.
-std::vector<base::FilePath> GetAppDirectoriesForScope(UpdaterScope scope) {
-  std::vector<base::FilePath> all_apps;
-  for (const base::FilePath& app_support_dir :
-       GetAppSupportDirectoriesForScope(scope)) {
-    base::FileEnumerator(app_support_dir.Append(COMPANY_SHORTNAME_STRING),
-                         /*recursive=*/false,
-                         base::FileEnumerator::FileType::DIRECTORIES)
-        .ForEach([&all_apps](const base::FilePath& app) {
-          all_apps.push_back(app);
-        });
-  }
-  return all_apps;
-}
-
 // Returns true if the directory contains a crashpad database with uploads
 // enabled.
 bool AppAllowsUsageStats(const base::FilePath& app_directory) {
@@ -90,26 +70,50 @@ bool AppAllowsUsageStats(const base::FilePath& app_directory) {
 
 }  // namespace
 
-// Returns true if any app directory under "Application Support/<company name>"
+class UsageStatsProviderImpl : public UsageStatsProvider {
-// for the given scope has usage stats enabled. Google Chrome channels all
+ public:
-// follow this pattern.
+  explicit UsageStatsProviderImpl(const base::FilePath& install_directory)
-bool AnyAppUsageStatsAllowed(UpdaterScope scope) {
+      : install_directory_(install_directory) {}
-  return std::ranges::any_of(
+
-      GetAppDirectoriesForScope(scope), [](const base::FilePath& app_dir) {
+  // Returns true if any app directory under
-        return app_dir.BaseName().value() != PRODUCT_FULLNAME_STRING &&
+  // "Application Support/<install_directory_>" for the given scope has
-               AppAllowsUsageStats(app_dir);
+  // usage stats enabled.
-      });
+  bool AnyAppEnablesUsageStats(UpdaterScope scope) override {
+    return std::ranges::any_of(
+        GetAppDirectories(scope), [](const base::FilePath& app_dir) {
+          return app_dir.BaseName().value() != PRODUCT_FULLNAME_STRING &&
+                 AppAllowsUsageStats(app_dir);
+        });
+  }
+
+  std::vector<base::FilePath> GetAppDirectories(UpdaterScope scope) {
+    std::vector<base::FilePath> all_apps;
+    for (const base::FilePath& app_support_dir :
+         GetAppSupportDirectoriesForScope(scope)) {
+      base::FileEnumerator(app_support_dir.Append(install_directory_),
+                           /*recursive=*/false,
+                           base::FileEnumerator::FileType::DIRECTORIES)
+          .ForEach([&all_apps](const base::FilePath& app) {
+            all_apps.push_back(app);
+          });
+    }
+    return all_apps;
+  }
+
+ private:
+  base::FilePath install_directory_;
+};
+
+// Returns a UsageStatsProvider that checks usage stats opt in for apps found
+// under "Application Support/<COMPANY_NAME>." Google Chrome channels all follow
+// this pattern.
+std::unique_ptr<UsageStatsProvider> UsageStatsProvider::Create() {
+  return UsageStatsProvider::Create(base::FilePath(COMPANY_SHORTNAME_STRING));
 }
 
-void UpdateUsageStatsTask::Run(base::OnceClosure callback) {
+std::unique_ptr<UsageStatsProvider> UsageStatsProvider::Create(
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    const base::FilePath& install_directory) {
-
+  return std::make_unique<UsageStatsProviderImpl>(install_directory);
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&AnyAppUsageStatsAllowed, scope_),
-      base::BindOnce(&UpdateUsageStatsTask::SetUsageStatsEnabled, this,
-                     persisted_data_)
-          .Then(std::move(callback)));
 }
 
 }  // namespace updater

chrome/updater/update_usage_stats_task_unittest.cc

@@ -117,7 +117,8 @@ class UpdateUsageStatsTaskTest : public testing::Test {
 TEST_F(UpdateUsageStatsTaskTest, NoApps) {
   ClearAppUsageStats("app1");
   ClearAppUsageStats("app2");
-  ASSERT_FALSE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+  ASSERT_FALSE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+      GetUpdaterScopeForTesting()));
 }
 
 TEST_F(UpdateUsageStatsTaskTest, OneAppEnabled) {
@@ -126,7 +127,8 @@ TEST_F(UpdateUsageStatsTaskTest, OneAppEnabled) {
     ClearAppUsageStats("app2");
     SetAppUsageStats(key_path, "app1", true);
     SetAppUsageStats(key_path, "app2", false);
-    ASSERT_TRUE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+    ASSERT_TRUE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+        GetUpdaterScopeForTesting()));
   }
 }
 
@@ -136,7 +138,8 @@ TEST_F(UpdateUsageStatsTaskTest, ZeroAppsEnabled) {
     ClearAppUsageStats("app2");
     SetAppUsageStats(key_path, "app1", false);
     SetAppUsageStats(key_path, "app2", false);
-    ASSERT_FALSE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+    ASSERT_FALSE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+        GetUpdaterScopeForTesting()));
   }
 }
 
@@ -147,11 +150,13 @@ TEST_F(UpdateUsageStatsTaskTest,
   }
   SetAppUsageStats(CLIENT_STATE_MEDIUM_KEY, "app1", false);
   SetAppUsageStats(CLIENT_STATE_KEY, "app1", true);
-  ASSERT_FALSE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+  ASSERT_FALSE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+      GetUpdaterScopeForTesting()));
 
   SetAppUsageStats(CLIENT_STATE_MEDIUM_KEY, "app1", true);
   SetAppUsageStats(CLIENT_STATE_KEY, "app1", false);
-  ASSERT_TRUE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+  ASSERT_TRUE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+      GetUpdaterScopeForTesting()));
 }
 
 #elif !BUILDFLAG(IS_MAC) || !BUILDFLAG(GOOGLE_CHROME_BRANDING)
@@ -161,7 +166,8 @@ TEST_F(UpdateUsageStatsTaskTest,
 TEST_F(UpdateUsageStatsTaskTest, NoApps) {
   ClearAppUsageStats("app1");
   ClearAppUsageStats("app2");
-  ASSERT_FALSE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+  ASSERT_FALSE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+      GetUpdaterScopeForTesting()));
 }
 
 // TODO(crbug.com/40821596): Enable tests once the feature is implemented.
@@ -169,14 +175,16 @@ TEST_F(UpdateUsageStatsTaskTest, NoApps) {
 TEST_F(UpdateUsageStatsTaskTest, OneAppEnabled) {
   SetAppUsageStats("app1", true);
   SetAppUsageStats("app2", false);
-  ASSERT_TRUE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+  ASSERT_TRUE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+      GetUpdaterScopeForTesting()));
 }
 #endif  // !BUILDFLAG(IS_LINUX)
 
 TEST_F(UpdateUsageStatsTaskTest, ZeroAppsEnabled) {
   SetAppUsageStats("app1", false);
   SetAppUsageStats("app2", false);
-  ASSERT_FALSE(AnyAppUsageStatsAllowed(GetUpdaterScopeForTesting()));
+  ASSERT_FALSE(UsageStatsProvider::Create()->AnyAppEnablesUsageStats(
+      GetUpdaterScopeForTesting()));
 }
 #endif
 

chrome/updater/update_usage_stats_task_win.cc

@@ -5,102 +5,105 @@
 #include "chrome/updater/update_usage_stats_task.h"
 
 #include <algorithm>
+#include <memory>
 #include <string>
 #include <vector>
 
-#include "base/containers/contains.h"
-#include "base/functional/bind.h"
-#include "base/functional/callback.h"
 #include "base/logging.h"
 #include "base/strings/strcat.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/task/thread_pool.h"
 #include "base/win/registry.h"
 #include "base/win/windows_types.h"
 #include "chrome/updater/app/app_utils.h"
-#include "chrome/updater/constants.h"
 #include "chrome/updater/persisted_data.h"
 #include "chrome/updater/util/win_util.h"
 #include "chrome/updater/win/win_constants.h"
 
 namespace updater {
 
-namespace {
+class UsageStatsProviderImpl : public UsageStatsProvider {
+ public:
+  UsageStatsProviderImpl(const std::wstring& system_key,
+                         const std::wstring& user_key)
+      : system_key_(system_key), user_key_(user_key) {}
 
-bool AppAllowsUsageStats(UpdaterScope scope, const std::string& id) {
+  bool AnyAppEnablesUsageStats(UpdaterScope scope) override {
-  const std::wstring& app_id = base::UTF8ToWide(id);
+    bool allowed = std::ranges::any_of(
-  DWORD usagestats = 0;
+        GetAppIdsForScope(scope), [this, &scope](const std::string& app_id) {
-  if (IsSystemInstall(scope) &&
+          if (!IsUpdaterOrCompanionApp(app_id) &&
-      base::win::RegKey(UpdaterScopeToHKeyRoot(scope),
+              AppAllowsUsageStats(scope, app_id)) {
-                        base::StrCat({CLIENT_STATE_MEDIUM_KEY, app_id}).c_str(),
+            VLOG(2) << "usage stats enabled by app " << app_id;
-                        Wow6432(KEY_READ))
+            return true;
-              .ReadValueDW(L"usagestats", &usagestats) == ERROR_SUCCESS) {
+          }
-    return usagestats == 1;
+          return false;
-  }
+        });
 
-  if (base::win::RegKey(UpdaterScopeToHKeyRoot(scope),
+    if (!allowed) {
-                        GetAppClientStateKey(app_id).c_str(), Wow6432(KEY_READ))
+      VLOG(2) << "no app enables usage stats";
-          .ReadValueDW(L"usagestats", &usagestats) == ERROR_SUCCESS) {
-    return usagestats == 1;
-  }
-
-  return false;
-}
-
-bool AppInVectorAllowsUsageStats(UpdaterScope scope,
-                                 const std::vector<std::string>& app_ids) {
-  return std::ranges::any_of(app_ids, [&scope](const std::string& app_id) {
-    return AppAllowsUsageStats(scope, app_id);
-  });
-}
-
-// Returns all app ids which are not the Updater or CECA.
-std::vector<std::string> FilterOtherAppIds(std::vector<std::string> app_ids) {
-  std::erase_if(app_ids, IsUpdaterOrCompanionApp);
-  return app_ids;
-}
-
-std::vector<std::string> GetAppIdsForScope(UpdaterScope scope) {
-  const HKEY root = UpdaterScopeToHKeyRoot(scope);
-  std::vector<std::wstring> subkeys;
-  if (IsSystemInstall(scope)) {
-    subkeys.push_back(CLIENT_STATE_MEDIUM_KEY);
-  }
-  subkeys.push_back(CLIENT_STATE_KEY);
-
-  std::vector<std::string> app_ids;
-  for (const auto& subkey : subkeys) {
-    for (base::win::RegistryKeyIterator it(root, subkey.c_str(),
-                                           KEY_WOW64_32KEY);
-         it.Valid(); ++it) {
-      app_ids.push_back(base::WideToUTF8(it.Name()));
     }
+    return allowed;
   }
 
-  return app_ids;
+ private:
+  std::vector<std::string> GetAppIdsForScope(UpdaterScope scope) {
+    const HKEY root = UpdaterScopeToHKeyRoot(scope);
+    std::vector<std::wstring> subkeys({user_key_});
+    if (IsSystemInstall(scope)) {
+      subkeys.push_back(system_key_);
+    }
+
+    std::vector<std::string> app_ids;
+    for (const auto& subkey : subkeys) {
+      for (base::win::RegistryKeyIterator it(root, subkey.c_str(),
+                                             KEY_WOW64_32KEY);
+           it.Valid(); ++it) {
+        app_ids.push_back(base::WideToUTF8(it.Name()));
+      }
+    }
+
+    return app_ids;
+  }
+
+  bool AppAllowsUsageStats(UpdaterScope scope, const std::string& id) {
+    const std::wstring& app_id = base::UTF8ToWide(id);
+    DWORD usagestats = 0;
+    if (IsSystemInstall(scope) &&
+        base::win::RegKey(UpdaterScopeToHKeyRoot(scope),
+                          base::StrCat({system_key_, app_id}).c_str(),
+                          Wow6432(KEY_READ))
+                .ReadValueDW(L"usagestats", &usagestats) == ERROR_SUCCESS) {
+      return usagestats == 1;
+    }
+
+    if (base::win::RegKey(UpdaterScopeToHKeyRoot(scope),
+                          base::StrCat({user_key_, app_id}).c_str(),
+                          Wow6432(KEY_READ))
+            .ReadValueDW(L"usagestats", &usagestats) == ERROR_SUCCESS) {
+      return usagestats == 1;
+    }
+
+    return false;
+  }
+
+  std::wstring system_key_;
+  std::wstring user_key_;
+};
+
+// Returns a usage stats provider that checks for apps under the
+// CLIENT_STATE_MEDIUM_KEY and CLIENT_STATE_KEY registry keys. The updater
+// stores installation and usage stat information in these keys.
+std::unique_ptr<UsageStatsProvider> UsageStatsProvider::Create() {
+  return UsageStatsProvider::Create(
+      /*system_key=*/CLIENT_STATE_MEDIUM_KEY, /*user_key=*/CLIENT_STATE_KEY);
 }
 
-}  // namespace
+// Returns a usage stats provider that checks apps installed under the
-
+// `system_key` and `user_key` in the registry. The updater
-// Check the registry to see if an app besides the Updater and Companion app
+// stores installation and usage stat information in these keys.
-// support usage stat reporting.
+std::unique_ptr<UsageStatsProvider> UsageStatsProvider::Create(
-bool AnyAppUsageStatsAllowed(UpdaterScope scope) {
+    const std::wstring& system_key,
-  bool allowed = AppInVectorAllowsUsageStats(
+    const std::wstring& user_key) {
-      scope, FilterOtherAppIds(GetAppIdsForScope(scope)));
+  return std::make_unique<UsageStatsProviderImpl>(system_key, user_key);
-  VLOG(2) << (allowed ? "usagestats enabled by another app"
-                      : "no app enables usagestats");
-  return allowed;
-}
-
-void UpdateUsageStatsTask::Run(base::OnceClosure callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, {base::MayBlock()},
-      base::BindOnce(&AnyAppUsageStatsAllowed, scope_),
-      base::BindOnce(&UpdateUsageStatsTask::SetUsageStatsEnabled, this,
-                     persisted_data_)
-          .Then(std::move(callback)));
 }
 
 }  // namespace updater

chrome/updater/win/installer/installer.cc

@@ -455,7 +455,8 @@ ProcessExitResult InstallerMain(HMODULE module,
            L" ", cmd_line_args.get()}));
 
   const UpdaterScope scope = GetUpdaterScopeForCommandLine(command_line);
-  usage_stats_enable = AnyAppUsageStatsAllowed(scope);
+  usage_stats_enable =
+      UsageStatsProvider::Create()->AnyAppEnablesUsageStats(scope);
   const std::optional<tagging::TagArgs> tag_args =
       GetTagArgsForCommandLine(command_line).tag_args;
   if (tag_args) {