Fix CrostiniAppModelBuilderTest for App Service

The App Service's apps::CrostiniApps class now listens for the
preference that enables or disables Crostini, and shows or hides the
Crostini Terminal app respectively.

The App Service + App List UI integration code (the
c/b/ui/app_list/app_service_* changes) also now hard-codes a Crostini
app folder, the same as the existing behavior (with the App Service
disabled), hard-coded into c/b/ui/app_list/crostini/*.

This commit also makes further changes to CrostiniAppModelBuilderTest so
that it works with the Mojo IPC based App Service.

Together, this fixes `unit_tests --enable-features=AppServiceAsh
--gtest_filter="CrostiniAppModelBuilderTest.*"`. It fails before and
passes after this commit.

Note that `unit_tests --gtest_filter="CrostiniAppModelBuilderTest.*"`,
without AppServiceAsh enabled, passes both before and after this commit.

BUG=826982

Change-Id: Id2bd1aa9d4de0389aa94eb5c1ce8211fe5997a11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1598896
Commit-Queue: Nigel Tao <nigeltao@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660687}

chrome/browser/apps/app_service/app_service_proxy_impl.cc

@@ -179,6 +179,14 @@ apps::IconLoader* AppServiceProxyImpl::OverrideInnerIconLoaderForTesting(
   return old;
 }
 
+void AppServiceProxyImpl::ReInitializeCrostiniForTesting(Profile* profile) {
+#if defined(OS_CHROMEOS)
+  if (app_service_.is_bound()) {
+    crostini_apps_.ReInitializeForTesting(app_service_, profile);
+  }
+#endif
+}
+
 void AppServiceProxyImpl::Shutdown() {
 #if defined(OS_CHROMEOS)
   if (app_service_.is_bound()) {

chrome/browser/apps/app_service/app_service_proxy_impl.h

@@ -66,6 +66,7 @@ class AppServiceProxyImpl : public KeyedService,
 
   apps::IconLoader* OverrideInnerIconLoaderForTesting(
       apps::IconLoader* icon_loader);
+  void ReInitializeCrostiniForTesting(Profile* profile);
 
  private:
   // An adapter, presenting an IconLoader interface based on the underlying

chrome/browser/apps/app_service/crostini_apps.cc

@@ -8,9 +8,12 @@
 
 #include "chrome/browser/apps/app_service/dip_px_util.h"
 #include "chrome/browser/apps/app_service/launch_util.h"
+#include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/grit/chrome_unscaled_resources.h"
+#include "components/prefs/pref_change_registrar.h"
 
 // TODO(crbug.com/826982): the equivalent of
 // CrostiniAppModelBuilder::MaybeCreateRootFolder. Does some sort of "root
@@ -19,7 +22,11 @@
 
 namespace apps {
 
-CrostiniApps::CrostiniApps() : binding_(this), registry_(nullptr) {}
+CrostiniApps::CrostiniApps()
+    : binding_(this),
+      profile_(nullptr),
+      registry_(nullptr),
+      crostini_enabled_(false) {}
 
 CrostiniApps::~CrostiniApps() {
   if (registry_) {
@@ -29,6 +36,10 @@ CrostiniApps::~CrostiniApps() {
 
 void CrostiniApps::Initialize(const apps::mojom::AppServicePtr& app_service,
                               Profile* profile) {
+  profile_ = nullptr;
+  registry_ = nullptr;
+  crostini_enabled_ = false;
+
   if (!crostini::IsCrostiniUIAllowedForProfile(profile)) {
     return;
   }
@@ -36,15 +47,34 @@ void CrostiniApps::Initialize(const apps::mojom::AppServicePtr& app_service,
   if (!registry_) {
     return;
   }
+  profile_ = profile;
+  crostini_enabled_ = crostini::IsCrostiniEnabled(profile);
 
   registry_->AddObserver(this);
 
+  pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
+  pref_change_registrar_->Init(profile->GetPrefs());
+  pref_change_registrar_->Add(
+      crostini::prefs::kCrostiniEnabled,
+      base::BindRepeating(&CrostiniApps::OnCrostiniEnabledChanged,
+                          base::Unretained(this)));
+
   apps::mojom::PublisherPtr publisher;
   binding_.Bind(mojo::MakeRequest(&publisher));
   app_service->RegisterPublisher(std::move(publisher),
                                  apps::mojom::AppType::kCrostini);
 }
 
+void CrostiniApps::ReInitializeForTesting(
+    const apps::mojom::AppServicePtr& app_service,
+    Profile* profile) {
+  // Some test code creates a profile (and therefore profile-linked services
+  // like the App Service) before it creates the fake user that lets
+  // IsCrostiniUIAllowedForProfile return true. To work around that, we issue a
+  // second Initialize call.
+  Initialize(app_service, profile);
+}
+
 void CrostiniApps::Connect(apps::mojom::SubscriberPtr subscriber,
                            apps::mojom::ConnectOptionsPtr opts) {
   std::vector<apps::mojom::AppPtr> apps;
@@ -141,6 +171,21 @@ void CrostiniApps::OnAppIconUpdated(const std::string& app_id,
   Publish(std::move(app));
 }
 
+void CrostiniApps::OnCrostiniEnabledChanged() {
+  crostini_enabled_ = profile_ && crostini::IsCrostiniEnabled(profile_);
+  auto show = crostini_enabled_ ? apps::mojom::OptionalBool::kTrue
+                                : apps::mojom::OptionalBool::kFalse;
+
+  // The Crostini Terminal app is a hard-coded special case. It is the entry
+  // point to installing other Crostini apps.
+  apps::mojom::AppPtr app = apps::mojom::App::New();
+  app->app_type = apps::mojom::AppType::kCrostini;
+  app->app_id = crostini::kCrostiniTerminalId;
+  app->show_in_launcher = show;
+  app->show_in_search = show;
+  Publish(std::move(app));
+}
+
 void CrostiniApps::LoadIconFromVM(const std::string app_id,
                                   apps::mojom::IconCompression icon_compression,
                                   int32_t size_hint_in_dip,
@@ -209,10 +254,12 @@ apps::mojom::AppPtr CrostiniApps::Convert(
   app->recommendable = apps::mojom::OptionalBool::kTrue;
   app->searchable = apps::mojom::OptionalBool::kTrue;
 
-  // TODO(crbug.com/826982): if Crostini isn't enabled, don't show the Terminal
-  // item until it becomes enabled.
   auto show = !registration.NoDisplay() ? apps::mojom::OptionalBool::kTrue
                                         : apps::mojom::OptionalBool::kFalse;
+  if (registration.is_terminal_app()) {
+    show = crostini_enabled_ ? apps::mojom::OptionalBool::kTrue
+                             : apps::mojom::OptionalBool::kFalse;
+  }
   app->show_in_launcher = show;
   app->show_in_search = show;
   // TODO(crbug.com/955937): Enable once Crostini apps are managed inside App

chrome/browser/apps/app_service/crostini_apps.h

@@ -18,6 +18,7 @@
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/interface_ptr_set.h"
 
+class PrefChangeRegistrar;
 class Profile;
 
 namespace apps {
@@ -35,6 +36,9 @@ class CrostiniApps : public KeyedService,
   void Initialize(const apps::mojom::AppServicePtr& app_service,
                   Profile* profile);
 
+  void ReInitializeForTesting(const apps::mojom::AppServicePtr& app_service,
+                              Profile* profile);
+
  private:
   enum class PublishAppIDType {
     kInstall,
@@ -69,6 +73,8 @@ class CrostiniApps : public KeyedService,
   void OnAppIconUpdated(const std::string& app_id,
                         ui::ScaleFactor scale_factor) override;
 
+  void OnCrostiniEnabledChanged();
+
   void LoadIconFromVM(const std::string app_id,
                       apps::mojom::IconCompression icon_compression,
                       int32_t size_hint_in_dip,
@@ -88,10 +94,15 @@ class CrostiniApps : public KeyedService,
   mojo::Binding<apps::mojom::Publisher> binding_;
   mojo::InterfacePtrSet<apps::mojom::Subscriber> subscribers_;
 
+  Profile* profile_;
+
+  std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
   crostini::CrostiniRegistryService* registry_;
 
   apps_util::IncrementingIconKeyFactory icon_key_factory_;
 
+  bool crostini_enabled_;
+
   base::WeakPtrFactory<CrostiniApps> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(CrostiniApps);

chrome/browser/ui/app_list/app_service_app_item.cc

@@ -7,6 +7,7 @@
 #include "ash/public/cpp/app_list/app_list_config.h"
 #include "base/bind.h"
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/ui/app_list/app_list_controller_delegate.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_context_menu.h"
 #include "chrome/browser/ui/app_list/crostini/crostini_app_context_menu.h"
@@ -60,6 +61,13 @@ AppServiceAppItem::AppServiceAppItem(
     UpdateFromSync(sync_item);
   } else {
     SetDefaultPositionIfApplicable(model_updater);
+
+    // Crostini hard-codes its own folder. As Crostini apps are created from
+    // scratch, we move them to a default folder.
+    if (app_type_ == apps::mojom::AppType::kCrostini) {
+      DCHECK(folder_id().empty());
+      SetChromeFolderId(crostini::kCrostiniFolderId);
+    }
   }
 
   // Set model updater last to avoid being called during construction.

chrome/browser/ui/app_list/app_service_app_model_builder.cc

@@ -5,14 +5,56 @@
 #include "chrome/browser/ui/app_list/app_service_app_model_builder.h"
 
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/ui/app_list/app_service_app_item.h"
+#include "chrome/grit/generated_resources.h"
 #include "chrome/services/app_service/public/cpp/app_service_proxy.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.
+//
+// Folders are an App List UI concept, not intrinsic to apps, so this
+// Crostini-specific feature is implemented here (chrome/browser/ui/app_list)
+// instead of in the App Service per se.
+class AppServiceAppModelBuilder::CrostiniFolderObserver
+    : public AppListModelUpdaterObserver {
+ public:
+  explicit CrostiniFolderObserver(AppServiceAppModelBuilder* 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:
+  AppServiceAppModelBuilder* parent_;
+};
 
 AppServiceAppModelBuilder::AppServiceAppModelBuilder(
     AppListControllerDelegate* controller)
     : AppListModelBuilder(controller, AppServiceAppItem::kItemType) {}
 
-AppServiceAppModelBuilder::~AppServiceAppModelBuilder() = default;
+AppServiceAppModelBuilder::~AppServiceAppModelBuilder() {
+  if (model_updater() && crostini_folder_observer_) {
+    model_updater()->RemoveObserver(crostini_folder_observer_.get());
+  }
+}
 
 void AppServiceAppModelBuilder::BuildModel() {
   apps::AppServiceProxy* proxy =
@@ -26,6 +68,11 @@ void AppServiceAppModelBuilder::BuildModel() {
     // in AppServiceProxyFactory::GetForProfile about whether
     // apps::AppServiceProxy::Get should return nullptr for incognito profiles.
   }
+
+  if (model_updater()) {
+    crostini_folder_observer_ = std::make_unique<CrostiniFolderObserver>(this);
+    model_updater()->AddObserver(crostini_folder_observer_.get());
+  }
 }
 
 void AppServiceAppModelBuilder::OnAppUpdate(const apps::AppUpdate& update) {

chrome/browser/ui/app_list/app_service_app_model_builder.h

@@ -19,12 +19,16 @@ class AppServiceAppModelBuilder : public AppListModelBuilder,
   ~AppServiceAppModelBuilder() override;
 
  private:
+  class CrostiniFolderObserver;
+
   // AppListModelBuilder overrides:
   void BuildModel() override;
 
   // apps::AppRegistryCache::Observer overrides:
   void OnAppUpdate(const apps::AppUpdate& update) override;
 
+  std::unique_ptr<AppListModelUpdaterObserver> crostini_folder_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(AppServiceAppModelBuilder);
 };
 

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

@@ -7,6 +7,8 @@
 #include <utility>
 #include <vector>
 
+#include "base/run_loop.h"
+#include "chrome/browser/apps/app_service/app_service_proxy_impl.h"
 #include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
 #include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.h"
 #include "chrome/browser/chromeos/crostini/crostini_test_helper.h"
@@ -18,6 +20,7 @@
 #include "chrome/common/chrome_features.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/test/base/testing_profile.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
 #include "extensions/browser/extension_system.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -74,8 +77,54 @@ class CrostiniAppModelBuilderTest : public AppListTestBase {
   ~CrostiniAppModelBuilderTest() override {}
 
   void SetUp() override {
+    // This brings up the (testing) profile, which creates various
+    // profile-linked services, including the App Service (if it is enabled).
+    // Call this "LINE A" (see below).
     AppListTestBase::SetUp();
+
+    // This sets up some Crostini-specific things, including configuring a fake
+    // user such that crostini::IsCrostiniUIAllowedForProfile() returns true.
+    // Call this "LINE B" (see below).
     test_helper_ = std::make_unique<CrostiniTestHelper>(profile());
+
+    // Some tests add apps to the registry, which queues (asynchronous) icon
+    // loading requests, which depends on D-Bus. These requests are merely
+    // queued, not executed, so without further action, D-Bus can be ignored.
+    //
+    // Separately, the App Service is a Mojo IPC service, and explicit
+    // RunUntilIdle calls are required to pump the IPCs, not just during this
+    // SetUp method, but also during the actual test code below. Those calls
+    // have a side effect of executing those icon loading requests.
+    //
+    // It is simpler if those RunUntilIdle calls are unconditional, so we also
+    // initialize D-Bus (if it wasn't already initialized) regardless of
+    // whether the App Service is enabled.
+    initialized_dbus_ = !chromeos::DBusThreadManager::IsInitialized();
+    if (initialized_dbus_) {
+      chromeos::DBusThreadManager::Initialize();
+    }
+
+    if (base::FeatureList::IsEnabled(features::kAppServiceAsh)) {
+      // The two lines ("LINE A" and "LINE B") of code, above, happen in a
+      // specific order: the AppListTestBase superclass' SetUp should happen
+      // before CrostiniAppModelBuilderTest's subclass-specific set-up.
+      //
+      // However, the App Service is a browser-context KeyedService, so it is
+      // created and initialized during LINE A, before the fake Crostini user
+      // is configured during LINE B. Without further action, in these tests
+      // (but not in production which looks at real users, not fakes), the App
+      // Service serves no Crostini apps, as at the time it looked, the
+      // profile/user doesn't have Crostini enabled.
+      //
+      // We therefore manually have the App Service re-examine whether Crostini
+      // is enabled for this profile, to pick up the side effects of LINE B.
+      apps::AppServiceProxyImpl::GetImplForTesting(profile())
+          ->ReInitializeCrostiniForTesting(profile());
+
+      // As mentioned above, explicit RunUntilIdle calls pump the Mojo IPCs.
+      base::RunLoop().RunUntilIdle();
+    }
+
     CreateBuilder();
   }
 
@@ -83,6 +132,16 @@ class CrostiniAppModelBuilderTest : public AppListTestBase {
     ResetBuilder();
     test_helper_.reset();
     AppListTestBase::TearDown();
+
+    if (initialized_dbus_) {
+      // CrostiniManager is a browser-context KeyedService, and its destructor
+      // assumes that DBus is running (so that it can remove some DBus-related
+      // observers).
+      //
+      // We therefore destroy the profile before we shut down DBus.
+      profile_.reset();
+      chromeos::DBusThreadManager::Shutdown();
+    }
   }
 
  protected:
@@ -91,6 +150,8 @@ class CrostiniAppModelBuilderTest : public AppListTestBase {
   }
 
   size_t GetModelItemCount() const {
+    // Pump the Mojo IPCs.
+    base::RunLoop().RunUntilIdle();
     return sync_service_->GetModelUpdater()->ItemCount();
   }
 
@@ -137,6 +198,8 @@ class CrostiniAppModelBuilderTest : public AppListTestBase {
       app_list::AppListSyncableService::ScopedModelUpdaterFactoryForTest>
       model_updater_factory_scope_;
 
+  bool initialized_dbus_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(CrostiniAppModelBuilderTest);
 };