Use crostini::GetTerminalId while switching to Terminal System App

GetTerminalId will return the old terminal ID or the new
Terminal System App ID if the feature is on.  This allows
the new Terminal app to have the same launch and shelf
context menu items.

ExtensionApps is modified to not register TerminalSystemApp
since it will be registered as a crostini app by
CrostiniRegistryService.  This will change once System Apps
support hiding apps.  See crbug.com/1028898

Added on_initilized() OneShotEvent to AppListSyncableService
so that callbacks can be registered to run after initialization.

CrostiniRegistryService::MigrateTerminal removes old terminal
from registry and registers on_initialized() callback with
AppListSyncableService to copy attributes from old terminal to new.

MigrateTerminal will be removed after a few releases once
the TerminalSystemApp feature is removed.

Bug: 1019021
Bug: 1028898
Change-Id: Ic35488bf8606a17a8806925dfa31ed0310cc9367
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1925833
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#719891}

chrome/browser/apps/app_service/crostini_apps.cc

@@ -217,7 +217,7 @@ void CrostiniApps::OnCrostiniEnabledChanged() {
   // 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->app_id = crostini::GetTerminalId();
   app->show_in_launcher = show;
   app->show_in_search = show;
   Publish(std::move(app));
@@ -321,7 +321,7 @@ apps::mojom::IconKeyPtr CrostiniApps::NewIconKey(const std::string& app_id) {
   // Crostini Terminal icon (the UI for enabling and installing Crostini apps)
   // should be showable even before the user has installed their first Crostini
   // app and before bringing up an Crostini VM for the first time.
-  if (app_id == crostini::kCrostiniTerminalId) {
+  if (app_id == crostini::GetTerminalId()) {
     return apps::mojom::IconKey::New(
         apps::mojom::IconKey::kDoesNotChangeOverTime,
         IDR_LOGO_CROSTINI_TERMINAL, apps::IconEffects::kNone);

chrome/browser/apps/app_service/crostini_apps.h

@@ -90,6 +90,9 @@ class CrostiniApps : public KeyedService,
   void OnAppIconUpdated(const std::string& app_id,
                         ui::ScaleFactor scale_factor) override;
 
+  // Registers and unregisters terminal with AppService.
+  // TODO(crbug.com/1028898): Move this code into System Apps
+  // once it can support hiding apps.
   void OnCrostiniEnabledChanged();
 
   void LoadIconFromVM(const std::string app_id,

chrome/browser/apps/app_service/extension_apps.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/apps/app_service/app_icon_factory.h"
 #include "chrome/browser/apps/launch_service/launch_service.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/chromeos/extensions/gfx_utils.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -324,6 +325,12 @@ bool ExtensionApps::Accepts(const extensions::Extension* extension) {
     case apps::mojom::AppType::kExtension:
       return !extension->from_bookmark();
     case apps::mojom::AppType::kWeb:
+      // Crostini Terminal System App is handled by Crostini Apps.
+      // TODO(crbug.com/1028898): Register Terminal as a System App rather than
+      // a crostini app.
+      if (extension->id() == crostini::kCrostiniTerminalSystemAppId) {
+        return false;
+      }
       return extension->from_bookmark();
     default:
       NOTREACHED();

chrome/browser/chromeos/crostini/crostini_registry_service.cc

@@ -22,6 +22,8 @@
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/app_list_syncable_service.h"
+#include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/dbus/vm_applications/apps.pb.h"
 #include "components/crx_file/id_util.h"
@@ -203,7 +205,7 @@ FindAppIdResult FindAppId(const base::DictionaryValue* prefs,
                           bool ignore_space = false) {
   result->clear();
   for (const auto& item : prefs->DictItems()) {
-    if (item.first == kCrostiniTerminalId)
+    if (item.first == GetTerminalId())
       continue;
 
     if (require_startup_notify &&
@@ -489,6 +491,7 @@ CrostiniRegistryService::CrostiniRegistryService(Profile* profile)
       base_icon_path_(profile->GetPath().AppendASCII(kCrostiniIconFolder)),
       clock_(base::DefaultClock::GetInstance()) {
   RecordStartupMetrics();
+  MigrateTerminal();
 }
 
 CrostiniRegistryService::~CrostiniRegistryService() = default;
@@ -603,11 +606,15 @@ CrostiniRegistryService::GetRegisteredApps() const {
       prefs_->GetDictionary(prefs::kCrostiniRegistry);
   std::map<std::string, CrostiniRegistryService::Registration> result;
   for (const auto& item : apps->DictItems()) {
-    result.emplace(item.first, Registration(&item.second,
-                                            item.first == kCrostiniTerminalId));
+    result.emplace(item.first,
+                   Registration(&item.second, /*is_terminal_app=*/item.first ==
+                                                  GetTerminalId()));
   }
-  if (!apps->FindKey(kCrostiniTerminalId)) {
-    result.emplace(kCrostiniTerminalId, Registration(nullptr, true));
+  // TODO(crbug.com/1028898): Register Terminal as a System App rather than a
+  // crostini app.
+  if (!apps->FindKey(GetTerminalId())) {
+    result.emplace(GetTerminalId(),
+                   Registration(nullptr, /*is_terminal_app=*/true));
   }
   return result;
 }
@@ -637,7 +644,7 @@ void CrostiniRegistryService::RecordStartupMetrics() {
   size_t num_apps = 0;
 
   for (const auto& item : apps->DictItems()) {
-    if (item.first == kCrostiniTerminalId)
+    if (item.first == GetTerminalId())
       continue;
 
     const base::Value* no_display =
@@ -714,7 +721,7 @@ void CrostiniRegistryService::ClearApplicationList(
     base::DictionaryValue* apps = update.Get();
 
     for (const auto& item : apps->DictItems()) {
-      if (item.first == kCrostiniTerminalId)
+      if (item.first == GetTerminalId())
         continue;
       if (item.second.FindKey(kAppVmNameKey)->GetString() == vm_name &&
           (container_name.empty() ||
@@ -807,7 +814,7 @@ void CrostiniRegistryService::UpdateApplicationList(
     }
 
     for (const auto& item : apps->DictItems()) {
-      if (item.first == kCrostiniTerminalId)
+      if (item.first == GetTerminalId())
         continue;
       if (item.second.FindKey(kAppVmNameKey)->GetString() ==
               app_list.vm_name() &&
@@ -870,7 +877,7 @@ void CrostiniRegistryService::AppLaunched(const std::string& app_id) {
 
   base::Value* app = apps->FindKey(app_id);
   if (!app) {
-    DCHECK_EQ(app_id, kCrostiniTerminalId);
+    DCHECK_EQ(app_id, GetTerminalId());
     base::Value pref(base::Value::Type::DICTIONARY);
     SetCurrentTime(&pref, kAppLastLaunchTimeKey);
     apps->SetKey(app_id, std::move(pref));
@@ -889,7 +896,7 @@ void CrostiniRegistryService::SetCurrentTime(base::Value* dictionary,
 
 void CrostiniRegistryService::SetAppScaled(const std::string& app_id,
                                            bool scaled) {
-  DCHECK_NE(app_id, kCrostiniTerminalId);
+  DCHECK_NE(app_id, GetTerminalId());
 
   DictionaryPrefUpdate update(prefs_, prefs::kCrostiniRegistry);
   base::DictionaryValue* apps = update.Get();
@@ -979,4 +986,30 @@ void CrostiniRegistryService::OnIconInstalled(const std::string& app_id,
     obs.OnAppIconUpdated(app_id, scale_factor);
 }
 
+void CrostiniRegistryService::MigrateTerminal() const {
+  // Remove the old terminal from the registry.
+  DictionaryPrefUpdate update(profile_->GetPrefs(), prefs::kCrostiniRegistry);
+  base::DictionaryValue* apps = update.Get();
+  apps->RemoveKey(GetDeletedTerminalId());
+
+  // Transfer item attributes from old terminal to new, and delete old terminal
+  // once AppListSyncableService is initialized.
+  auto* app_list_syncable_service =
+      app_list::AppListSyncableServiceFactory::GetForProfile(profile_);
+  if (!app_list_syncable_service) {
+    return;
+  }
+  app_list_syncable_service->on_initialized().Post(
+      FROM_HERE,
+      base::BindOnce(
+          [](app_list::AppListSyncableService* service) {
+            if (service->GetSyncItem(crostini::GetDeletedTerminalId())) {
+              service->TransferItemAttributes(crostini::GetDeletedTerminalId(),
+                                              crostini::GetTerminalId());
+              service->RemoveItem(crostini::GetDeletedTerminalId());
+            }
+          },
+          base::Unretained(app_list_syncable_service)));
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_registry_service.h

@@ -216,6 +216,14 @@ class CrostiniRegistryService : public KeyedService {
   // Removes all the icons installed for an application.
   void RemoveAppData(const std::string& app_id);
 
+  // Migrates terminal from old crosh-based terminal to new Terminal System App.
+  // Old terminal is removed from registry, and launcher position and pinned
+  // attribute is copied to the new terminal.
+  // TODO(crbug.com/1019021):  Keep this code for at least 1 release after
+  // TerminalSystemApp feature is removed.  Current expectation is to remove
+  // feature in M83, this function can then be remoevd after M84.
+  void MigrateTerminal() const;
+
   // Owned by the Profile.
   Profile* const profile_;
   PrefService* const prefs_;

chrome/browser/chromeos/crostini/crostini_registry_service_unittest.cc

@@ -10,10 +10,13 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/simple_test_clock.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
 #include "chrome/browser/chromeos/crostini/crostini_test_helper.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/dbus/vm_applications/apps.pb.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/test/browser_task_environment.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -54,6 +57,7 @@ class CrostiniRegistryServiceTest : public testing::Test {
   }
 
   CrostiniRegistryService* service() { return service_.get(); }
+  Profile* profile() { return &profile_; }
 
   std::vector<std::string> GetRegisteredAppIds() {
     std::vector<std::string> result;
@@ -543,4 +547,22 @@ TEST_F(CrostiniRegistryServiceTest, SetAndGetPackageId) {
   EXPECT_EQ(result_no_package_id->PackageId(), "");
 }
 
+TEST_F(CrostiniRegistryServiceTest, MigrateTerminal) {
+  // Add prefs entry for the deleted terminal.
+  base::DictionaryValue registry;
+  registry.SetKey(GetDeletedTerminalId(), base::DictionaryValue());
+  profile()->GetPrefs()->Set(prefs::kCrostiniRegistry, std::move(registry));
+
+  // Only current terminal returned.
+  RecreateService();
+  EXPECT_THAT(GetRegisteredAppIds(),
+              testing::UnorderedElementsAre(GetTerminalId()));
+
+  // Deleted terminal removed from prefs.
+  EXPECT_FALSE(profile()
+                   ->GetPrefs()
+                   ->GetDictionary(prefs::kCrostiniRegistry)
+                   ->HasKey(GetDeletedTerminalId()));
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_util.cc

@@ -520,4 +520,13 @@ const std::string& GetTerminalId() {
   return *app_id;
 }
 
+const std::string& GetDeletedTerminalId() {
+  static const base::NoDestructor<std::string> app_id([] {
+    return base::FeatureList::IsEnabled(features::kTerminalSystemApp)
+               ? kCrostiniTerminalId
+               : kCrostiniTerminalSystemAppId;
+  }());
+  return *app_id;
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_util.h

@@ -153,8 +153,15 @@ void CloseCrostiniUpdateFilesystemView();
 // applying an Ansible playbook in the container).
 void ShowCrostiniAnsibleSoftwareConfigView(Profile* profile);
 
+// Returns App ID of the terminal app which is either the older crosh-based
+// terminal, or the new Terminal System App if the TerminalSystemApp feature
+// is enabled.
 const std::string& GetTerminalId();
 
+// Returns the alternative terminal ID to |GetTerminalId|.  This is used when
+// migrating terminals when TerminalSystemApp feature changes.
+const std::string& GetDeletedTerminalId();
+
 // We use an arbitrary well-formed extension id for the Terminal app, this
 // is equal to GenerateId("Terminal").
 constexpr char kCrostiniTerminalId[] = "oajcgpnkmhaalajejhlfpacbiokdnnfe";

chrome/browser/ui/app_list/app_list_syncable_service.cc

@@ -928,6 +928,11 @@ syncer::SyncMergeResult AppListSyncableService::MergeDataAndStartSyncing(
 
   HandleUpdateFinished();
 
+  // Check if already signaled since unit tests make multiple calls.
+  if (!on_initialized_.is_signaled()) {
+    on_initialized_.Signal();
+  }
+
   return result;
 }
 

chrome/browser/ui/app_list/app_list_syncable_service.h

@@ -15,6 +15,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/one_shot_event.h"
 #include "build/build_config.h"
 #include "chrome/browser/sync/glue/sync_start_util.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ -155,6 +156,9 @@ class AppListSyncableService : public syncer::SyncableService,
   // Returns true if this service was initialized.
   bool IsInitialized() const;
 
+  // Signalled when AppListSyncableService is Initialized.
+  const base::OneShotEvent& on_initialized() const { return on_initialized_; }
+
   // Returns true if sync was started.
   bool IsSyncing() const;
 
@@ -331,6 +335,7 @@ class AppListSyncableService : public syncer::SyncableService,
 
   // List of observers.
   base::ObserverList<Observer>::Unchecked observer_list_;
+  base::OneShotEvent on_initialized_;
 
   base::WeakPtrFactory<AppListSyncableService> weak_ptr_factory_{this};
 

chrome/browser/ui/app_list/app_service/app_service_app_item.cc

@@ -27,6 +27,12 @@ std::unique_ptr<app_list::AppContextMenu> AppServiceAppItem::MakeAppContextMenu(
     const std::string& app_id,
     AppListControllerDelegate* controller,
     bool is_platform_app) {
+  // Terminal System App uses CrostiniAppContextMenu.
+  if (app_id == crostini::kCrostiniTerminalSystemAppId) {
+    return std::make_unique<CrostiniAppContextMenu>(profile, app_id,
+                                                    controller);
+  }
+
   switch (app_type) {
     case apps::mojom::AppType::kUnknown:
     case apps::mojom::AppType::kBuiltIn:
@@ -68,9 +74,9 @@ AppServiceAppItem::AppServiceAppItem(
   } 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) {
+    // Crostini apps and the Terminal System App start in the crostini folder.
+    if (app_type_ == apps::mojom::AppType::kCrostini ||
+        id() == crostini::kCrostiniTerminalSystemAppId) {
       DCHECK(folder_id().empty());
       SetChromeFolderId(crostini::kCrostiniFolderId);
     }

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

@@ -24,7 +24,7 @@ bool CrostiniAppContextMenu::IsCommandIdEnabled(int command_id) const {
   if (command_id == ash::UNINSTALL) {
     return IsUninstallable();
   } else if (command_id == ash::STOP_APP) {
-    if (app_id() == crostini::kCrostiniTerminalId) {
+    if (app_id() == crostini::GetTerminalId()) {
       return crostini::IsCrostiniRunning(profile());
     }
   }
@@ -34,7 +34,7 @@ bool CrostiniAppContextMenu::IsCommandIdEnabled(int command_id) const {
 void CrostiniAppContextMenu::ExecuteCommand(int command_id, int event_flags) {
   switch (command_id) {
     case ash::UNINSTALL: {
-      DCHECK_NE(app_id(), crostini::kCrostiniTerminalId);
+      DCHECK_NE(app_id(), crostini::GetTerminalId());
       apps::AppServiceProxy* proxy =
           apps::AppServiceProxyFactory::GetForProfile(profile());
       DCHECK(proxy);
@@ -42,7 +42,7 @@ void CrostiniAppContextMenu::ExecuteCommand(int command_id, int event_flags) {
       return;
     }
     case ash::STOP_APP:
-      if (app_id() == crostini::kCrostiniTerminalId) {
+      if (app_id() == crostini::GetTerminalId()) {
         crostini::CrostiniManager::GetForProfile(profile())->StopVm(
             crostini::kCrostiniDefaultVmName, base::DoNothing());
         return;
@@ -62,7 +62,7 @@ void CrostiniAppContextMenu::BuildMenu(ui::SimpleMenuModel* menu_model) {
                          IDS_APP_LIST_UNINSTALL_ITEM);
   }
 
-  if (app_id() == crostini::kCrostiniTerminalId) {
+  if (app_id() == crostini::GetTerminalId()) {
     AddContextMenuOption(menu_model, ash::STOP_APP,
                          IDS_CROSTINI_SHUT_DOWN_LINUX_MENU_ITEM);
   }

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

@@ -80,7 +80,7 @@ gfx::ImageSkiaRep CrostiniAppIcon::Source::GetImageForScale(float scale) {
 
   // Host loads icon asynchronously, so use default icon so far.
   int resource_id;
-  if (host_ && host_->app_id() == crostini::kCrostiniTerminalId) {
+  if (host_ && host_->app_id() == crostini::GetTerminalId()) {
     // Don't initiate the icon request from the container because we have this
     // one already.
     resource_id = IDR_LOGO_CROSTINI_TERMINAL;

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

@@ -316,8 +316,7 @@ class AppServiceDataSource : public AppSearchProvider::DataSource,
 
       // Until it's been installed, the Crostini Terminal is hidden and
       // requires a few characters before being shown in search results.
-      if ((update.AppType() == apps::mojom::AppType::kCrostini) &&
-          (update.AppId() == crostini::kCrostiniTerminalId) &&
+      if (update.AppId() == crostini::GetTerminalId() &&
           !crostini::CrostiniFeatures::Get()->IsEnabled(profile())) {
         apps_vector->back()->set_recommendable(false);
         apps_vector->back()->set_relevance_threshold(

chrome/browser/ui/ash/launcher/crostini_shelf_context_menu.cc

@@ -69,7 +69,7 @@ void CrostiniShelfContextMenu::GetMenuModel(GetMenuModelCallback callback) {
                          IDS_APP_LIST_NEW_WINDOW);
   }
 
-  if (item().id.app_id == crostini::kCrostiniTerminalId &&
+  if (item().id.app_id == crostini::GetTerminalId() &&
       crostini::IsCrostiniRunning(controller()->profile())) {
     AddContextMenuOption(menu_model.get(), ash::STOP_APP,
                          IDS_CROSTINI_SHUT_DOWN_LINUX_MENU_ITEM);
@@ -108,7 +108,7 @@ bool CrostiniShelfContextMenu::IsCommandIdEnabled(int command_id) const {
   if (command_id == ash::UNINSTALL)
     return IsUninstallable();
   if (command_id == ash::STOP_APP &&
-      item().id.app_id == crostini::kCrostiniTerminalId) {
+      item().id.app_id == crostini::GetTerminalId()) {
     return crostini::IsCrostiniRunning(controller()->profile());
   }
   return ShelfContextMenu::IsCommandIdEnabled(command_id);
@@ -117,7 +117,7 @@ bool CrostiniShelfContextMenu::IsCommandIdEnabled(int command_id) const {
 void CrostiniShelfContextMenu::ExecuteCommand(int command_id, int event_flags) {
   switch (command_id) {
     case ash::UNINSTALL: {
-      DCHECK_NE(item().id.app_id, crostini::kCrostiniTerminalId);
+      DCHECK_NE(item().id.app_id, crostini::GetTerminalId());
       apps::AppServiceProxy* proxy =
           apps::AppServiceProxyFactory::GetForProfile(controller()->profile());
       DCHECK(proxy);
@@ -125,7 +125,7 @@ void CrostiniShelfContextMenu::ExecuteCommand(int command_id, int event_flags) {
       return;
     }
     case ash::STOP_APP:
-      if (item().id.app_id == crostini::kCrostiniTerminalId) {
+      if (item().id.app_id == crostini::GetTerminalId()) {
         crostini::CrostiniManager::GetForProfile(controller()->profile())
             ->StopVm(crostini::kCrostiniDefaultVmName, base::DoNothing());
       }

chrome/browser/ui/ash/launcher/shelf_context_menu.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.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_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/app_list/extension_uninstaller.h"
@@ -55,12 +56,13 @@ std::unique_ptr<ShelfContextMenu> ShelfContextMenu::Create(
     return std::make_unique<ArcShelfContextMenu>(controller, item, display_id);
   }
 
-  // Create an CrostiniShelfContextMenu if the item is Crostini app.
+  // Use CrostiniShelfContextMenu for crostini apps and Terminal System App.
   crostini::CrostiniRegistryService* crostini_registry_service =
       crostini::CrostiniRegistryServiceFactory::GetForProfile(
           controller->profile());
-  if (crostini_registry_service &&
-      crostini_registry_service->IsCrostiniShelfAppId(item->id.app_id)) {
+  if ((crostini_registry_service &&
+       crostini_registry_service->IsCrostiniShelfAppId(item->id.app_id)) ||
+      item->id.app_id == crostini::kCrostiniTerminalSystemAppId) {
     return std::make_unique<CrostiniShelfContextMenu>(controller, item,
                                                       display_id);
   }