folder persistence on crostini side

Adds the relevant pieces to crostini that allow it to declare that one
of ash's folders should be persistent (i.e. alive with only 1 item in
it). We do this by adding an observer which updates the folder's
metadata with the persistence property.

This CL also includes some refactoring of the unit tests here so that
they better reflect how chrome and ash communicate, and so the
expectations are a bit nicer to work with.

Bug: 925052
Change-Id: Ie0c211cf9783fa2318d1e35d1b25827c0fdc048f
Reviewed-on: https://chromium-review.googlesource.com/c/1469194
Commit-Queue: Nic Hollingum <hollingum@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634498}

chrome/browser/ui/app_list/crostini/crostini_app_model_builder.cc

@@ -16,11 +16,45 @@
 #include "components/prefs/pref_change_registrar.h"
 #include "ui/base/l10n/l10n_util.h"
 
+// Folder items are created by the Ash process and their existence is
+// communicated to chrome via the AppListClient. Therefore, crostini has an
+// observer that listens for the creation of its folder, and updates the
+// properties accordingly.
+class CrostiniAppModelBuilder::CrostiniFolderObserver
+    : public AppListModelUpdaterObserver {
+ public:
+  explicit CrostiniFolderObserver(CrostiniAppModelBuilder* parent)
+      : parent_(parent) {}
+
+  ~CrostiniFolderObserver() override = default;
+
+  void OnAppListItemAdded(ChromeAppListItem* item) override {
+    if (item->id() != crostini::kCrostiniFolderId)
+      return;
+    // Persistence is not recorded by the sync, so we always set it.
+    item->SetIsPersistent(true);
+
+    // Either the name and position will be in the sync, or we set them
+    // manually.
+    if (parent_->GetSyncItem(crostini::kCrostiniFolderId))
+      return;
+    item->SetName(
+        l10n_util::GetStringUTF8(IDS_APP_LIST_CROSTINI_DEFAULT_FOLDER_NAME));
+    item->SetDefaultPositionIfApplicable(parent_->model_updater());
+  }
+
+ private:
+  CrostiniAppModelBuilder* parent_;
+};
+
 CrostiniAppModelBuilder::CrostiniAppModelBuilder(
     AppListControllerDelegate* controller)
     : AppListModelBuilder(controller, CrostiniAppItem::kItemType) {}
 
 CrostiniAppModelBuilder::~CrostiniAppModelBuilder() {
+  if (crostini_folder_observer_) {
+    model_updater()->RemoveObserver(crostini_folder_observer_.get());
+  }
   // We don't need to remove ourself from the registry's observer list as these
   // are both KeyedServices (this class lives on AppListSyncableService).
 }
@@ -40,6 +74,13 @@ void CrostiniAppModelBuilder::BuildModel() {
       crostini::prefs::kCrostiniEnabled,
       base::BindRepeating(&CrostiniAppModelBuilder::OnCrostiniEnabledChanged,
                           base::Unretained(this)));
+
+  // We register an observer against the model_updater in order to track
+  // creation and deletion of the crostini folder.
+  if (model_updater()) {
+    crostini_folder_observer_ = std::make_unique<CrostiniFolderObserver>(this);
+    model_updater()->AddObserver(crostini_folder_observer_.get());
+  }
 }
 
 void CrostiniAppModelBuilder::InsertCrostiniAppItem(
@@ -56,7 +97,6 @@ void CrostiniAppModelBuilder::InsertCrostiniAppItem(
   if (registration.NoDisplay())
     return;
 
-  MaybeCreateRootFolder();
   InsertApp(std::make_unique<CrostiniAppItem>(profile(), model_updater(),
                                               GetSyncItem(app_id), app_id,
                                               registration.Name()));
@@ -120,21 +160,3 @@ void CrostiniAppModelBuilder::OnCrostiniEnabledChanged() {
     RemoveApp(crostini::kCrostiniTerminalId, unsynced_change);
   }
 }
-
-void CrostiniAppModelBuilder::MaybeCreateRootFolder() {
-  // If a sync item exists for the root folder, then it has been created
-  // already.
-  const app_list::AppListSyncableService::SyncItem* sync_item =
-      GetSyncItem(crostini::kCrostiniFolderId);
-  if (sync_item)
-    return;
-
-  std::unique_ptr<ChromeAppListItem> crositini_folder =
-      std::make_unique<ChromeAppListItem>(
-          profile(), crostini::kCrostiniFolderId, model_updater());
-  crositini_folder->SetChromeIsFolder(true);
-  crositini_folder->SetName(
-      l10n_util::GetStringUTF8(IDS_APP_LIST_CROSTINI_DEFAULT_FOLDER_NAME));
-  crositini_folder->SetDefaultPositionIfApplicable(model_updater());
-  InsertApp(std::move(crositini_folder));
-}

chrome/browser/ui/app_list/crostini/crostini_app_model_builder.h

@@ -4,6 +4,8 @@
 #ifndef CHROME_BROWSER_UI_APP_LIST_CROSTINI_CROSTINI_APP_MODEL_BUILDER_H_
 #define CHROME_BROWSER_UI_APP_LIST_CROSTINI_CROSTINI_APP_MODEL_BUILDER_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
 #include "chrome/browser/ui/app_list/app_list_model_builder.h"
@@ -20,6 +22,10 @@ class CrostiniAppModelBuilder
   ~CrostiniAppModelBuilder() override;
 
  private:
+  // This observer will be used to update the properties of the crostini folder
+  // when ash creates it.
+  class CrostiniFolderObserver;
+
   // AppListModelBuilder:
   void BuildModel() override;
 
@@ -38,15 +44,13 @@ class CrostiniAppModelBuilder
 
   void OnCrostiniEnabledChanged();
 
-  // Creates root folder for Crostini apps in case it was not created (in which
-  // case the sync item will not exist). Once it is created, a sync item is
-  // allocated, and it will be reused to restore the root folder on demand
-  // automatically.
-  void MaybeCreateRootFolder();
-
   // Observer Crostini installation so we can start showing The Terminal app.
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
 
+  // Observer that listens for crostini folder creation and sets its properties
+  // accordingly.
+  std::unique_ptr<AppListModelUpdaterObserver> crostini_folder_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(CrostiniAppModelBuilder);
 };
 

chrome/browser/ui/app_list/test/fake_app_list_model_updater.cc

@@ -10,7 +10,8 @@
 #include "chrome/browser/ui/app_list/chrome_app_list_item.h"
 #include "extensions/common/constants.h"
 
-FakeAppListModelUpdater::FakeAppListModelUpdater() = default;
+FakeAppListModelUpdater::FakeAppListModelUpdater(Profile* profile)
+    : profile_(profile) {}
 
 FakeAppListModelUpdater::~FakeAppListModelUpdater() = default;
 
@@ -214,6 +215,17 @@ void FakeAppListModelUpdater::UpdateAppItemFromSyncItem(
   }
 }
 
+void FakeAppListModelUpdater::OnFolderCreated(
+    ash::mojom::AppListItemMetadataPtr folder) {
+  std::unique_ptr<ChromeAppListItem> stub_folder =
+      std::make_unique<ChromeAppListItem>(profile_, folder->id, this);
+
+  for (AppListModelUpdaterObserver& observer : observers_)
+    observer.OnAppListItemAdded(stub_folder.get());
+
+  AddItem(std::move(stub_folder));
+}
+
 void FakeAppListModelUpdater::AddObserver(
     AppListModelUpdaterObserver* observer) {
   observers_.AddObserver(observer);

chrome/browser/ui/app_list/test/fake_app_list_model_updater.h

@@ -18,7 +18,7 @@ class ChromeAppListItem;
 
 class FakeAppListModelUpdater : public AppListModelUpdater {
  public:
-  FakeAppListModelUpdater();
+  explicit FakeAppListModelUpdater(Profile* profile = nullptr);
   ~FakeAppListModelUpdater() override;
 
   // For AppListModel:
@@ -62,7 +62,7 @@ class FakeAppListModelUpdater : public AppListModelUpdater {
     return search_results_;
   }
 
-  void OnFolderCreated(ash::mojom::AppListItemMetadataPtr folder) override {}
+  void OnFolderCreated(ash::mojom::AppListItemMetadataPtr folder) override;
   void OnFolderDeleted(ash::mojom::AppListItemMetadataPtr item) override {}
   void OnItemUpdated(ash::mojom::AppListItemMetadataPtr item) override {}
   void OnPageBreakItemAdded(const std::string& id,
@@ -77,6 +77,7 @@ class FakeAppListModelUpdater : public AppListModelUpdater {
   std::vector<std::unique_ptr<ChromeAppListItem>> items_;
   std::vector<ChromeSearchResult*> search_results_;
   base::ObserverList<AppListModelUpdaterObserver> observers_;
+  Profile* profile_;
 
   ash::mojom::AppListItemMetadataPtr FindOrCreateOemFolder(
       const std::string& oem_folder_name,