Enable holding space feature flag by default.

Note that this CL also updates the lifecycle of the holding space keyed
service. Previously it was created with browser context but is now
delayed until start of the user session.

Also note modifications to DownloadManagerUtils to address tests broken
by this CL which assumed DownloadManagerImpl would not be created until
after tests bodies executed.

Bug: 1155387
Change-Id: I7e0f226cb275465c72eb8d63f1348b44f5412685
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2572694
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838744}

ash/capture_mode/capture_mode_controller.cc

@@ -703,8 +703,11 @@ void CaptureModeController::OnImageFileSaved(
   CopyImageToClipboard(image);
   ShowPreviewNotification(path, image, CaptureModeType::kImage);
 
-  if (features::IsTemporaryHoldingSpaceEnabled())
-    HoldingSpaceController::Get()->client()->AddScreenshot(path);
+  if (features::IsTemporaryHoldingSpaceEnabled()) {
+    HoldingSpaceClient* client = HoldingSpaceController::Get()->client();
+    if (client)  // May be `nullptr` in tests.
+      client->AddScreenshot(path);
+  }
 }
 
 void CaptureModeController::OnVideoFileStatus(bool success) {
@@ -729,8 +732,9 @@ void CaptureModeController::OnVideoFileSaved(bool success) {
         (base::TimeTicks::Now() - recording_start_time_).InSeconds());
 
     if (features::IsTemporaryHoldingSpaceEnabled()) {
-      HoldingSpaceController::Get()->client()->AddScreenRecording(
-          current_video_file_path_);
+      HoldingSpaceClient* client = HoldingSpaceController::Get()->client();
+      if (client)  // May be `nullptr` in tests.
+        client->AddScreenRecording(current_video_file_path_);
     }
   }
 

ash/public/cpp/ash_features.cc

@@ -144,7 +144,7 @@ const base::Feature kNotificationsInContextMenu{
     "NotificationsInContextMenu", base::FEATURE_DISABLED_BY_DEFAULT};
 
 const base::Feature kTemporaryHoldingSpace{"TemporaryHoldingSpace",
-                                           base::FEATURE_DISABLED_BY_DEFAULT};
+                                           base::FEATURE_ENABLED_BY_DEFAULT};
 
 const base::Feature kTemporaryHoldingSpacePreviews{
     "TemporaryHoldingSpacePreviews", base::FEATURE_DISABLED_BY_DEFAULT};

chrome/browser/chromeos/login/session/user_session_initializer.cc

@@ -30,7 +30,9 @@
 #include "chrome/browser/component_updater/sth_set_component_remover.h"
 #include "chrome/browser/google/google_brand_chromeos.h"
 #include "chrome/browser/net/nss_context.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/ash/clipboard_image_model_factory_impl.h"
+#include "chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h"
 #include "chrome/browser/ui/ash/media_client_impl.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/constants/chromeos_features.h"
@@ -205,8 +207,14 @@ void UserSessionInitializer::InitializePrimaryProfileServices(
 }
 
 void UserSessionInitializer::OnUserSessionStarted(bool is_primary_user) {
+  Profile* profile = ProfileManager::GetActiveUserProfile();
+  DCHECK(profile);
+
+  // Ensure that the `HoldingSpaceKeyedService` for `profile` is created.
+  ash::HoldingSpaceKeyedServiceFactory::GetInstance()->GetService(profile);
+
   if (is_primary_user) {
-    DCHECK_NE(primary_profile_, nullptr);
+    DCHECK_EQ(primary_profile_, profile);
 
     plugin_vm::PluginVmManager* plugin_vm_manager =
         plugin_vm::PluginVmManagerFactory::GetForProfile(primary_profile_);

chrome/browser/download/download_browsertest.cc

@@ -4268,7 +4268,50 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DownloadLargeDataURL) {
 // Testing the behavior of resuming with only in-progress download manager.
 class InProgressDownloadTest : public DownloadTest {
  public:
-  void SetUpOnMainThread() override { EXPECT_TRUE(CheckTestDir()); }
+  InProgressDownloadTest() {
+    // The in progress download manager will be released from
+    // `DownloadManagerUtils` during creation of the `DownloadManagerImpl`. As
+    // the `DownloadManagerImpl` may be created before test bodies can run,
+    // register a callback to cache a pointer before release occurs.
+    DownloadManagerUtils::
+        SetRetrieveInProgressDownloadManagerCallbackForTesting(
+            base::BindRepeating(
+                &InProgressDownloadTest::set_in_progress_manager,
+                base::Unretained(this)));
+  }
+
+  // DownloadTest:
+  void SetUpOnMainThread() override {
+    EXPECT_TRUE(CheckTestDir());
+
+    if (!in_progress_manager_) {
+      // This will only occur if `DownloadManagerImpl` has not already been
+      // created in which case the in progress download manager has not yet been
+      // released from `DownloadManagerUtils`.
+      set_in_progress_manager(
+          DownloadManagerUtils::GetInProgressDownloadManager(
+              browser()->profile()->GetProfileKey()));
+    }
+
+    // As a pointer to the in progress download manager has now been cached,
+    // watching for release from `DownloadManagerUtils` (if it has not already
+    // occurred) is no longer necessary.
+    DownloadManagerUtils::
+        SetRetrieveInProgressDownloadManagerCallbackForTesting(
+            base::NullCallback());
+  }
+
+  download::InProgressDownloadManager* in_progress_manager() {
+    return in_progress_manager_;
+  }
+
+  void set_in_progress_manager(
+      download::InProgressDownloadManager* in_progress_manager) {
+    in_progress_manager_ = in_progress_manager;
+  }
+
+ private:
+  download::InProgressDownloadManager* in_progress_manager_ = nullptr;
 };
 
 // Check that if a download exists in both in-progress and history DB,
@@ -4290,9 +4333,6 @@ IN_PROC_BROWSER_TEST_F(InProgressDownloadTest,
   std::string guid = base::GenerateGUID();
 
   // Wait for in-progress download manager to initialize.
-  download::InProgressDownloadManager* in_progress_manager =
-      DownloadManagerUtils::GetInProgressDownloadManager(
-          browser()->profile()->GetProfileKey());
   download::SimpleDownloadManagerCoordinator* coordinator =
       SimpleDownloadManagerCoordinatorFactory::GetForKey(
           browser()->profile()->GetProfileKey());
@@ -4307,9 +4347,9 @@ IN_PROC_BROWSER_TEST_F(InProgressDownloadTest,
   std::vector<GURL> url_chain;
   url_chain.emplace_back(url);
   base::Time current_time = base::Time::Now();
-  in_progress_manager->AddInProgressDownloadForTest(
+  in_progress_manager()->AddInProgressDownloadForTest(
       std::make_unique<download::DownloadItemImpl>(
-          in_progress_manager, guid, 1 /* id */,
+          in_progress_manager(), guid, 1 /* id */,
           target_path.AddExtensionASCII("crdownload"), target_path, url_chain,
           GURL() /* referrer_url */, GURL() /* site_url */,
           GURL() /* tab_url */, GURL() /* tab_referrer_url */,
@@ -4359,9 +4399,6 @@ IN_PROC_BROWSER_TEST_F(InProgressDownloadTest,
   std::string guid = base::GenerateGUID();
 
   // Wait for in-progress download manager to initialize.
-  download::InProgressDownloadManager* in_progress_manager =
-      DownloadManagerUtils::GetInProgressDownloadManager(
-          browser()->profile()->GetProfileKey());
   download::SimpleDownloadManagerCoordinator* coordinator =
       SimpleDownloadManagerCoordinatorFactory::GetForKey(
           browser()->profile()->GetProfileKey());
@@ -4382,14 +4419,14 @@ IN_PROC_BROWSER_TEST_F(InProgressDownloadTest,
   params->set_file_path(target_path);
   params->set_transient(true);
   params->set_require_safety_checks(false);
-  in_progress_manager->DownloadUrl(std::move(params));
+  in_progress_manager()->DownloadUrl(std::move(params));
   auto params2 = std::make_unique<DownloadUrlParameters>(
       url, TRAFFIC_ANNOTATION_FOR_TESTS);
   params2->set_guid(guid);
   params2->set_file_path(target_path);
   params2->set_transient(true);
   params2->set_require_safety_checks(false);
-  in_progress_manager->DownloadUrl(std::move(params2));
+  in_progress_manager()->DownloadUrl(std::move(params2));
   coordinator_waiter.WaitForDownloadCreation(1);
   download::DownloadItem* download = coordinator->GetDownloadByGuid(guid);
   ASSERT_TRUE(download);

chrome/browser/download/download_manager_utils.cc

@@ -43,6 +43,17 @@ InProgressManagerMap& GetInProgressManagerMap() {
   return *map;
 }
 
+// Returns a callback to be invoked during `RetrieveInProgressDownloadManager()`
+// to provide an opportunity to cache a pointer to the in progress download
+// manager being released.
+base::RepeatingCallback<void(download::InProgressDownloadManager*)>&
+GetRetrieveInProgressDownloadManagerCallback() {
+  static base::NoDestructor<
+      base::RepeatingCallback<void(download::InProgressDownloadManager*)>>
+      callback;
+  return *callback;
+}
+
 // Ignores origin security check. DownloadManagerImpl will provide its own
 // implementation when InProgressDownloadManager object is passed to it.
 bool IgnoreOriginSecurityCheck(const GURL& url) {
@@ -71,6 +82,8 @@ DownloadManagerUtils::RetrieveInProgressDownloadManager(Profile* profile) {
   ProfileKey* key = profile->GetProfileKey();
   GetInProgressDownloadManager(key);
   auto& map = GetInProgressManagerMap();
+  if (GetRetrieveInProgressDownloadManagerCallback())
+    GetRetrieveInProgressDownloadManagerCallback().Run(map[key].get());
   return map[key].release();
 }
 
@@ -129,3 +142,11 @@ DownloadManagerUtils::GetInProgressDownloadManager(ProfileKey* key) {
   }
   return map[key].get();
 }
+
+// static
+void DownloadManagerUtils::
+    SetRetrieveInProgressDownloadManagerCallbackForTesting(
+        base::RepeatingCallback<void(download::InProgressDownloadManager*)>
+            callback) {
+  GetRetrieveInProgressDownloadManagerCallback() = callback;
+}

chrome/browser/download/download_manager_utils.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_UTILS_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_MANAGER_UTILS_H_
 
+#include "base/callback_forward.h"
 #include "base/macros.h"
 
 class Profile;
@@ -29,6 +30,13 @@ class DownloadManagerUtils {
   static download::InProgressDownloadManager* GetInProgressDownloadManager(
       ProfileKey* key);
 
+  // Registers a `callback` to be run during subsequent invocations of
+  // `RetrieveInProgressDownloadManager()`, providing an opportunity to cache
+  // a pointer to the in progress download manager being released.
+  static void SetRetrieveInProgressDownloadManagerCallbackForTesting(
+      base::RepeatingCallback<void(download::InProgressDownloadManager*)>
+          callback);
+
  private:
   DISALLOW_COPY_AND_ASSIGN(DownloadManagerUtils);
 };

chrome/browser/download/save_page_browsertest.cc

@@ -802,15 +802,13 @@ IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, MAYBE_SavePageAsMHTML) {
 #endif
 
   // Save the file as MHTML. Run until save completes.
+  base::RunLoop run_loop;
+  content::SavePackageFinishedObserver observer(
+      content::BrowserContext::GetDownloadManager(browser()->profile()),
+      run_loop.QuitClosure());
   ASSERT_TRUE(select_file_dialog_factory->GetLastDialog()->CallFileSelected(
       full_file_name, "mhtml"));
-  {
-    base::RunLoop run_loop;
-    content::SavePackageFinishedObserver observer(
-        content::BrowserContext::GetDownloadManager(browser()->profile()),
-        run_loop.QuitClosure());
-    run_loop.Run();
-  }
+  run_loop.Run();
 
   ASSERT_TRUE(VerifySavePackageExpectations(browser(), url));
   persisted.WaitForPersisted();

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.cc

@@ -90,9 +90,12 @@ HoldingSpaceKeyedService::HoldingSpaceKeyedService(Profile* profile,
   // the first time that holding space became available, this will no-op.
   holding_space_prefs::MarkTimeOfFirstAvailability(profile_->GetPrefs());
 
+  ProfileManager* const profile_manager = GetProfileManager();
+  if (!profile_manager)  // May be `nullptr` in tests.
+    return;
+
   // The associated profile may not be ready yet. If it is, we can immediately
   // proceed with profile dependent initialization.
-  ProfileManager* const profile_manager = GetProfileManager();
   if (profile_manager->IsValidProfile(profile)) {
     OnProfileReady();
     return;
@@ -103,9 +106,10 @@ HoldingSpaceKeyedService::HoldingSpaceKeyedService(Profile* profile,
 }
 
 HoldingSpaceKeyedService::~HoldingSpaceKeyedService() {
-  if (HoldingSpaceController::Get()) {
-    // For BrowserWithTestWindowTest that releases profile and its keyed
-    // services before ash Shell.
+  if (chromeos::PowerManagerClient::Get())
+    chromeos::PowerManagerClient::Get()->RemoveObserver(this);
+
+  if (HoldingSpaceController::Get()) {  // May be `nullptr` in tests.
     HoldingSpaceController::Get()->RegisterClientAndModelForUser(
         account_id_, /*client=*/nullptr, /*model=*/nullptr);
   }
@@ -286,12 +290,14 @@ void HoldingSpaceKeyedService::OnProfileAdded(Profile* profile) {
 void HoldingSpaceKeyedService::OnProfileReady() {
   // Observe suspend status - the delegates will be shutdown during suspend.
   if (chromeos::PowerManagerClient::Get())
-    power_manager_observer_.Observe(chromeos::PowerManagerClient::Get());
+    chromeos::PowerManagerClient::Get()->AddObserver(this);
 
   InitializeDelegates();
 
-  HoldingSpaceController::Get()->RegisterClientAndModelForUser(
-      account_id_, &holding_space_client_, &holding_space_model_);
+  if (HoldingSpaceController::Get()) {  // May be `nullptr` in tests.
+    HoldingSpaceController::Get()->RegisterClientAndModelForUser(
+        account_id_, &holding_space_client_, &holding_space_model_);
+  }
 }
 
 void HoldingSpaceKeyedService::SuspendImminent(

chrome/browser/ui/ash/holding_space/holding_space_keyed_service.h

@@ -143,10 +143,6 @@ class HoldingSpaceKeyedService : public KeyedService,
   base::ScopedObservation<ProfileManager, ProfileManagerObserver>
       profile_manager_observer_{this};
 
-  base::ScopedObservation<chromeos::PowerManagerClient,
-                          chromeos::PowerManagerClient::Observer>
-      power_manager_observer_{this};
-
   base::WeakPtrFactory<HoldingSpaceKeyedService> weak_factory_{this};
 };
 

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.cc

@@ -54,11 +54,6 @@ KeyedService* HoldingSpaceKeyedServiceFactory::BuildServiceInstanceFor(
   return new HoldingSpaceKeyedService(profile, user->GetAccountId());
 }
 
-bool HoldingSpaceKeyedServiceFactory::ServiceIsCreatedWithBrowserContext()
-    const {
-  return true;
-}
-
 void HoldingSpaceKeyedServiceFactory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   HoldingSpaceKeyedService::RegisterProfilePrefs(registry);

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_factory.h

@@ -28,7 +28,6 @@ class HoldingSpaceKeyedServiceFactory
   // BrowserContextKeyedServiceFactory:
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
-  bool ServiceIsCreatedWithBrowserContext() const override;
   void RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable* registry) override;
 

chrome/browser/ui/ash/holding_space/holding_space_keyed_service_unittest.cc

@@ -1359,6 +1359,8 @@ TEST_F(HoldingSpaceKeyedServiceTest, RemoveOlderFilesFromPersistance) {
 
 TEST_F(HoldingSpaceKeyedServiceTest, AddDownloadItem) {
   TestingProfile* profile = GetProfile();
+  HoldingSpaceModelAttachedWaiter(profile).Wait();
+
   // Create a test downloads mount point.
   std::unique_ptr<ScopedTestMountPoint> downloads_mount =
       ScopedTestMountPoint::CreateAndMountDownloads(profile);

components/download/internal/common/in_progress_download_manager.cc

@@ -680,6 +680,7 @@ void InProgressDownloadManager::NotifyDownloadsInitialized() {
 void InProgressDownloadManager::AddInProgressDownloadForTest(
     std::unique_ptr<download::DownloadItemImpl> download) {
   in_progress_downloads_.push_back(std::move(download));
+  NotifyDownloadsInitialized();
 }
 
 void InProgressDownloadManager::CancelUrlDownload(