[files] Separate ItemSuggestCache construction from its provider.

This CL separates out the construction for ItemSuggestCache and
ZeroStateDriveProvider. This allows for dependency injection, which will
help with ZSDP unit tests.

Also, result callbacks will now be registered via RegisterCallback
instead of being passed into the constructor of ItemSuggestCache. Allows
multiple callbacks to be registered if necessary.

Bug: 1348449
Change-Id: I2986c5968da042d24c7a1dd6ce6a9035cc82ebdc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3828340
Commit-Queue: Rachel Wong <wrong@chromium.org>
Reviewed-by: Amanda Deacon <amandadeacon@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1034990}

chrome/browser/ui/app_list/search/files/item_suggest_cache.cc

@@ -10,6 +10,7 @@
 #include "ash/constants/ash_pref_names.h"
 #include "ash/public/cpp/app_list/app_list_features.h"
 #include "base/bind.h"
+#include "base/callback_list.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/strings/strcat.h"
@@ -201,20 +202,23 @@ ItemSuggestCache::Results::~Results() = default;
 
 ItemSuggestCache::ItemSuggestCache(
     Profile* profile,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-    base::RepeatingCallback<void()> on_results_updated)
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
     : made_request_(false),
       enabled_(kEnabled.Get()),
       server_url_(kServerUrl.Get()),
       multiple_queries_per_session_(kMultipleQueriesPerSession.Get()),
-      on_results_updated_(on_results_updated),
       profile_(profile),
       url_loader_factory_(std::move(url_loader_factory)) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
 ItemSuggestCache::~ItemSuggestCache() = default;
 
+base::CallbackListSubscription ItemSuggestCache::RegisterCallback(
+    ItemSuggestCache::OnResultsCallback callback) {
+  return on_results_callback_list_.Add(std::move(callback));
+}
+
 absl::optional<ItemSuggestCache::Results> ItemSuggestCache::GetResults() {
   // Return a copy because a pointer to |results_| will become invalid whenever
   // the cache is updated.
@@ -387,7 +391,7 @@ void ItemSuggestCache::OnJsonParsed(
     LogStatus(Status::kOk);
     LogLatency(base::TimeTicks::Now() - update_start_time_);
     results_ = std::move(results.value());
-    on_results_updated_.Run();
+    on_results_callback_list_.Notify();
   }
 }
 

chrome/browser/ui/app_list/search/files/item_suggest_cache.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_APP_LIST_SEARCH_FILES_ITEM_SUGGEST_CACHE_H_
 #define CHROME_BROWSER_UI_APP_LIST_SEARCH_FILES_ITEM_SUGGEST_CACHE_H_
 
+#include "base/callback_list.h"
 #include "base/feature_list.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
@@ -56,13 +57,18 @@ class ItemSuggestCache {
 
   ItemSuggestCache(
       Profile* profile,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      base::RepeatingCallback<void()> on_results_updated);
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
   ~ItemSuggestCache();
 
   ItemSuggestCache(const ItemSuggestCache&) = delete;
   ItemSuggestCache& operator=(const ItemSuggestCache&) = delete;
 
+  using OnResultsCallback = base::RepeatingCallback<void()>;
+  using OnResultsCallbackList = base::RepeatingCallbackList<void()>;
+
+  // Registers a callback to be run whenever the results are updated.
+  base::CallbackListSubscription RegisterCallback(OnResultsCallback callback);
+
   // Returns the results currently in the cache. A null result indicates that
   // the cache has not been successfully updated.
   absl::optional<ItemSuggestCache::Results> GetResults();
@@ -154,7 +160,8 @@ class ItemSuggestCache {
   // Whether we should query item suggest more than once per session.
   const bool multiple_queries_per_session_;
 
-  base::RepeatingCallback<void()> on_results_updated_;
+  // List of callbacks to run when results are updated.
+  OnResultsCallbackList on_results_callback_list_;
 
   Profile* profile_;
   std::unique_ptr<signin::PrimaryAccountAccessTokenFetcher> token_fetcher_;

chrome/browser/ui/app_list/search/files/item_suggest_cache_unittest.cc

@@ -207,8 +207,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheDisabledByExperiment) {
   scoped_feature_list_.InitAndEnableFeatureWithParameters(
       feature_, {{"enabled", "false"}});
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   itemSuggestCache->UpdateCache();
   task_environment_.RunUntilIdle();
   histogram_tester_.ExpectUniqueSample(
@@ -218,8 +217,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheDisabledByExperiment) {
 TEST_F(ItemSuggestCacheTest, UpdateCacheDisabledByPolicy) {
   profile_->GetPrefs()->SetBoolean(drive::prefs::kDisableDrive, true);
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   itemSuggestCache->UpdateCache();
   task_environment_.RunUntilIdle();
   histogram_tester_.ExpectUniqueSample(
@@ -231,8 +229,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheServerUrlIsNotHttps) {
       feature_,
       {{"server_url", "http://appsitemsuggest-pa.googleapis.com/v1/items"}});
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   itemSuggestCache->UpdateCache();
   task_environment_.RunUntilIdle();
   histogram_tester_.ExpectUniqueSample(
@@ -243,8 +240,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheServerUrlIsNotGoogleDomain) {
   scoped_feature_list_.InitAndEnableFeatureWithParameters(
       feature_, {{"server_url", "https://foo.com"}});
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   itemSuggestCache->UpdateCache();
   task_environment_.RunUntilIdle();
   histogram_tester_.ExpectUniqueSample(
@@ -253,8 +249,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheServerUrlIsNotGoogleDomain) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheServerNoAuthToken) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   itemSuggestCache->UpdateCache();
   task_environment_.RunUntilIdle();
   histogram_tester_.ExpectUniqueSample(
@@ -263,8 +258,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheServerNoAuthToken) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheInsufficientResourcesError) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -282,8 +276,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheInsufficientResourcesError) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheNetError) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -301,8 +294,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheNetError) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCache5kkError) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -324,8 +316,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCache5kkError) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCache4kkError) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -347,8 +338,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCache4kkError) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCache3kkError) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -370,8 +360,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCache3kkError) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheEmptyResponse) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -387,8 +376,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheEmptyResponse) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheInvalidResponse) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -406,8 +394,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheInvalidResponse) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheConversionFailure) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -430,8 +417,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheConversionFailure) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheConversionEmptyResults) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -455,8 +441,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheConversionEmptyResults) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheSavesResults) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);
@@ -479,8 +464,7 @@ TEST_F(ItemSuggestCacheTest, UpdateCacheSavesResults) {
 
 TEST_F(ItemSuggestCacheTest, UpdateCacheSmallTimeBetweenUpdates) {
   std::unique_ptr<ItemSuggestCache> itemSuggestCache =
-      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_,
-                                         base::DoNothing());
+      std::make_unique<ItemSuggestCache>(profile_, shared_url_loader_factory_);
   identity_test_env_->MakePrimaryAccountAvailable(kEmail,
                                                   signin::ConsentLevel::kSync);
   identity_test_env_->SetAutomaticIssueOfAccessTokens(true);

chrome/browser/ui/app_list/search/files/zero_state_drive_provider.cc

@@ -148,26 +148,30 @@ ZeroStateDriveProvider::ZeroStateDriveProvider(
     Profile* profile,
     SearchController* search_controller,
     drive::DriveIntegrationService* drive_service,
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
+    std::unique_ptr<ItemSuggestCache> item_suggest_cache)
     : profile_(profile),
       drive_service_(drive_service),
       session_manager_(session_manager::SessionManager::Get()),
       construction_time_(base::Time::Now()),
-      item_suggest_cache_(
-          profile,
-          std::move(url_loader_factory),
-          base::BindRepeating(&ZeroStateDriveProvider::OnCacheUpdated,
-                              base::Unretained(this))),
+      item_suggest_cache_(std::move(item_suggest_cache)),
       max_last_modified_time_(base::Days(base::GetFieldTrialParamByFeatureAsInt(
           ash::features::kProductivityLauncher,
           "max_last_modified_time",
           8))) {
-  DCHECK(profile_);
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(profile_);
+  DCHECK(item_suggest_cache_);
+
   task_runner_ = base::ThreadPool::CreateSequencedTaskRunner(
       {base::TaskPriority::USER_BLOCKING, base::MayBlock(),
        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
 
+  // It's safe to use Unretained(this) by contract of the
+  // CallbackListSubscription.
+  item_suggest_subscription_ =
+      item_suggest_cache_->RegisterCallback(base::BindRepeating(
+          &ZeroStateDriveProvider::OnCacheUpdated, base::Unretained(this)));
+
   if (drive_service_) {
     if (drive_service_->IsMounted()) {
       // DriveFS is mounted, so we can fetch results immediately.
@@ -277,7 +281,7 @@ void ZeroStateDriveProvider::StartZeroState() {
   weak_factory_.InvalidateWeakPtrs();
 
   // Get the most recent results from the cache.
-  cache_results_ = item_suggest_cache_.GetResults();
+  cache_results_ = item_suggest_cache_->GetResults();
   if (!cache_results_) {
     LogStatus(Status::kNoResults);
     return;
@@ -395,7 +399,7 @@ void ZeroStateDriveProvider::OnCacheUpdated() {
 
 void ZeroStateDriveProvider::MaybeUpdateCache() {
   if (base::Time::Now() - kFirstUpdateDelay > construction_time_) {
-    item_suggest_cache_.UpdateCache();
+    item_suggest_cache_->UpdateCache();
   }
 }
 

chrome/browser/ui/app_list/search/files/zero_state_drive_provider.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "ash/components/drivefs/mojom/drivefs.mojom.h"
+#include "base/callback_list.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "base/task/sequenced_task_runner.h"
@@ -33,11 +34,10 @@ class ZeroStateDriveProvider : public SearchProvider,
                                public session_manager::SessionManagerObserver,
                                public chromeos::PowerManagerClient::Observer {
  public:
-  ZeroStateDriveProvider(
-      Profile* profile,
-      SearchController* search_controller,
-      drive::DriveIntegrationService* drive_service,
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
+  ZeroStateDriveProvider(Profile* profile,
+                         SearchController* search_controller,
+                         drive::DriveIntegrationService* drive_service,
+                         std::unique_ptr<ItemSuggestCache> item_suggest_cache);
   ~ZeroStateDriveProvider() override;
 
   ZeroStateDriveProvider(const ZeroStateDriveProvider&) = delete;
@@ -115,7 +115,8 @@ class ZeroStateDriveProvider : public SearchProvider,
 
   const base::Time construction_time_;
 
-  ItemSuggestCache item_suggest_cache_;
+  std::unique_ptr<ItemSuggestCache> item_suggest_cache_;
+  base::CallbackListSubscription item_suggest_subscription_;
 
   // The most recent results retrieved from |item_suggested_cache_|. This is
   // updated on a call to Start and is used only to store the results until

chrome/browser/ui/app_list/search/files/zero_state_drive_provider_unittest.cc

@@ -30,7 +30,7 @@ class ZeroStateDriveProviderTest : public testing::Test {
     provider_ = std::make_unique<ZeroStateDriveProvider>(
         profile_.get(), nullptr,
         drive::DriveIntegrationServiceFactory::GetForProfile(profile_.get()),
-        nullptr);
+        std::make_unique<ItemSuggestCache>(profile_.get(), nullptr));
     session_manager_ = std::make_unique<session_manager::SessionManager>();
     provider_->set_session_manager_for_testing(session_manager_.get());
   }

chrome/browser/ui/app_list/search/search_controller_factory.cc

@@ -168,8 +168,9 @@ std::unique_ptr<SearchController> CreateSearchController(
         std::make_unique<ZeroStateDriveProvider>(
             profile, controller.get(),
             drive::DriveIntegrationServiceFactory::GetForProfile(profile),
-            profile->GetDefaultStoragePartition()
-                ->GetURLLoaderFactoryForBrowserProcess()));
+            std::make_unique<ItemSuggestCache>(
+                profile, profile->GetDefaultStoragePartition()
+                             ->GetURLLoaderFactoryForBrowserProcess())));
   }
 
   if (app_list_features::IsLauncherSettingsSearchEnabled()) {