Migrate from `BrowserContext` to `Profile`.

Previously the `HoldingSpaceKeyedService` was constructed with a
`BrowserContext` and internally converted to `Profile` as needed. Now,
we'll take a `Profile` at construction time and avoid any downstream
conversions.

This CL also addresses a race condition that could occur when
partitioning `HoldingSpaceItems` by existence. Previously, input order
was not guaranteed to match output order. Now they will which should
stabilize browsertests.

Bug: 1119496
Change-Id: I8e213eeed8f4b4169bc033130fcdebf473c1fd73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2386383
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#803367}

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

@@ -29,17 +29,16 @@ ProfileManager* GetProfileManager() {
 }  // namespace
 
 // TODO(dmblack): Add a delegate for downloads.
-HoldingSpaceKeyedService::HoldingSpaceKeyedService(
-    content::BrowserContext* context,
-    const AccountId& account_id)
-    : browser_context_(context),
+HoldingSpaceKeyedService::HoldingSpaceKeyedService(Profile* profile,
+                                                   const AccountId& account_id)
+    : profile_(profile),
       account_id_(account_id),
-      holding_space_client_(Profile::FromBrowserContext(context)),
-      thumbnail_loader_(Profile::FromBrowserContext(context)) {
+      holding_space_client_(profile),
+      thumbnail_loader_(profile) {
   // 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::FromBrowserContext(context))) {
+  if (profile_manager->IsValidProfile(profile)) {
     OnProfileReady();
     return;
   }
@@ -87,8 +86,8 @@ std::vector<GURL> HoldingSpaceKeyedService::GetPinnedFiles() const {
 void HoldingSpaceKeyedService::AddScreenshot(
     const base::FilePath& screenshot_file,
     const gfx::ImageSkia& image) {
-  GURL file_system_url = holding_space_util::ResolveFileSystemUrl(
-      Profile::FromBrowserContext(browser_context_), screenshot_file);
+  GURL file_system_url =
+      holding_space_util::ResolveFileSystemUrl(profile_, screenshot_file);
   if (file_system_url.is_empty())
     return;
 
@@ -99,8 +98,8 @@ void HoldingSpaceKeyedService::AddScreenshot(
 
 void HoldingSpaceKeyedService::AddDownload(
     const base::FilePath& download_file) {
-  GURL file_system_url = holding_space_util::ResolveFileSystemUrl(
-      Profile::FromBrowserContext(browser_context_), download_file);
+  GURL file_system_url =
+      holding_space_util::ResolveFileSystemUrl(profile_, download_file);
   if (file_system_url.is_empty())
     return;
 
@@ -126,7 +125,7 @@ void HoldingSpaceKeyedService::Shutdown() {
 }
 
 void HoldingSpaceKeyedService::OnProfileAdded(Profile* profile) {
-  if (profile == Profile::FromBrowserContext(browser_context_)) {
+  if (profile == profile_) {
     profile_manager_observer_.Remove(GetProfileManager());
     OnProfileReady();
   }
@@ -166,18 +165,16 @@ void HoldingSpaceKeyedService::RemoveDownloadManagerObservers() {
 }
 
 void HoldingSpaceKeyedService::OnProfileReady() {
-  Profile* profile = Profile::FromBrowserContext(browser_context_);
-
   // The `HoldingSpaceFileSystemDelegate` monitors the file system for changes.
   delegates_.push_back(std::make_unique<HoldingSpaceFileSystemDelegate>(
-      profile, &holding_space_model_,
+      profile_, &holding_space_model_,
       /*file_removed_callback=*/
       base::BindRepeating(&HoldingSpaceKeyedService::OnFileRemoved,
                           weak_factory_.GetWeakPtr())));
 
   // The `HoldingSpacePersistenceDelegate` manages holding space persistence.
   delegates_.push_back(std::make_unique<HoldingSpacePersistenceDelegate>(
-      profile, &holding_space_model_,
+      profile_, &holding_space_model_,
       /*item_restored_callback=*/
       base::BindRepeating(&HoldingSpaceKeyedService::AddItem,
                           weak_factory_.GetWeakPtr()),
@@ -214,8 +211,7 @@ void HoldingSpaceKeyedService::OnModelRestored() {
 
   // Once the `holding_space_model_` has been restored from persistence, we can
   // start to observe the `download_manager_` to track downloaded files.
-  download_manager_ =
-      content::BrowserContext::GetDownloadManager(browser_context_);
+  download_manager_ = content::BrowserContext::GetDownloadManager(profile_);
   download_manager_->AddObserver(this);
   RetrieveDownloadHistory();
 }

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

@@ -28,10 +28,6 @@ namespace base {
 class FilePath;
 }  // namespace base
 
-namespace content {
-class BrowserContext;
-}  // namespace content
-
 namespace user_prefs {
 class PrefRegistrySyncable;
 }  // namespace user_prefs
@@ -54,8 +50,7 @@ class HoldingSpaceKeyedService : public KeyedService,
                                  public content::DownloadManager::Observer,
                                  public download::DownloadItem::Observer {
  public:
-  HoldingSpaceKeyedService(content::BrowserContext* context,
-                           const AccountId& account_id);
+  HoldingSpaceKeyedService(Profile* profile, const AccountId& account_id);
   HoldingSpaceKeyedService(const HoldingSpaceKeyedService& other) = delete;
   HoldingSpaceKeyedService& operator=(const HoldingSpaceKeyedService& other) =
       delete;
@@ -136,7 +131,7 @@ class HoldingSpaceKeyedService : public KeyedService,
 
   void RetrieveDownloadHistory();
 
-  content::BrowserContext* const browser_context_;
+  Profile* const profile_;
   const AccountId account_id_;
 
   HoldingSpaceClientImpl holding_space_client_;

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

@@ -37,17 +37,16 @@ KeyedService* HoldingSpaceKeyedServiceFactory::BuildServiceInstanceFor(
   if (!features::IsTemporaryHoldingSpaceEnabled())
     return nullptr;
 
-  user_manager::User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(
-      Profile::FromBrowserContext(context));
+  Profile* profile = Profile::FromBrowserContext(context);
+  user_manager::User* user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
   if (!user)
     return nullptr;
 
   if (user->GetType() == user_manager::USER_TYPE_KIOSK_APP)
     return nullptr;
 
-  auto* service = new HoldingSpaceKeyedService(context, user->GetAccountId());
-
-  return service;
+  return new HoldingSpaceKeyedService(profile, user->GetAccountId());
 }
 
 bool HoldingSpaceKeyedServiceFactory::ServiceIsCreatedWithBrowserContext()

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

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/ash/holding_space/holding_space_util.h"
 
+#include <map>
+
 #include "ash/public/cpp/file_icon_util.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "base/barrier_closure.h"
@@ -45,6 +47,12 @@ void PartitionItemsByExistence(Profile* profile,
     return;
   }
 
+  // Cache the original indices of the `items` being partitioned so that we can
+  // return them back in the same order after checking for existence.
+  std::map<std::string, size_t> indices_by_id;
+  for (size_t i = 0; i < items.size(); ++i)
+    indices_by_id[items.at(i)->id()] = i;
+
   auto existing_items = std::make_unique<HoldingSpaceItemPtrList>();
   auto non_existing_items = std::make_unique<HoldingSpaceItemPtrList>();
 
@@ -59,12 +67,28 @@ void PartitionItemsByExistence(Profile* profile,
       base::BindOnce(
           [](std::unique_ptr<HoldingSpaceItemPtrList> existing_items,
              std::unique_ptr<HoldingSpaceItemPtrList> non_existing_items,
+             std::map<std::string, size_t> indices_by_id,
              PartitionItemsByExistenceCallback callback) {
+            // We need to sort our partitioned vectors to match the original
+            // order that was provided at call time. This is necessary as the
+            // original order may have been lost due to race conditions when
+            // checking for item existence.
+            auto sort = [&indices_by_id](HoldingSpaceItemPtrList* items) {
+              std::sort(items->begin(), items->end(),
+                        [&indices_by_id](const auto& a, const auto& b) {
+                          return indices_by_id[a->id()] <
+                                 indices_by_id[b->id()];
+                        });
+            };
+            sort(existing_items.get());
+            sort(non_existing_items.get());
+
+            // Ownership of the partitioned vectors is passed to `callback`.
             std::move(callback).Run(std::move(*existing_items),
                                     std::move(*non_existing_items));
           },
           std::move(existing_items), std::move(non_existing_items),
-          std::move(callback)));
+          std::move(indices_by_id), std::move(callback)));
 
   // Verify existence of each holding space `item`. Upon successful check of
   // existence, each `item` should be pushed into either `existing_items` or