Use app ids from CrostiniAppRegistry in ShelfIDs.

This patch moves the CreateCrostiniAppId from crostini_util.cc to the
CrostiniAppRegistry and makes it look for a matching app registration.
This will be useful when we want to make launching of apps focus
existing windows if they exist.

Bug: 819444
Change-Id: Ibef29d4441ea6f4aaab3ea45cce0982493f69a47
Reviewed-on: https://chromium-review.googlesource.com/1004564
Commit-Queue: Timothy Loh <timloh@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552531}

chrome/browser/chromeos/crostini/crostini_registry_service.cc

@@ -4,9 +4,11 @@
 
 #include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
 
+#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/app_list/crostini/crostini_util.h"
 #include "chromeos/dbus/vm_applications/apps.pb.h"
 #include "components/crx_file/id_util.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -20,6 +22,16 @@ namespace crostini {
 
 namespace {
 
+// Prefixes of the ApplicationId set on exo windows.
+constexpr char kArcWindowAppIdPrefix[] = "org.chromium.arc";
+constexpr char kCrostiniWindowAppIdPrefix[] = "org.chromium.termina.";
+// This comes after kCrostiniWindowAppIdPrefix
+constexpr char kWMClassPrefix[] = "wmclass.";
+
+// This prefix is used when generating the crostini app list id, and used as a
+// prefix when generating shelf ids for windows we couldn't match to an app.
+constexpr char kCrostiniAppIdPrefix[] = "crostini:";
+
 constexpr char kCrostiniRegistryPref[] = "crostini.registry";
 
 // Keys for the Dictionary stored in prefs for each app.
@@ -31,8 +43,6 @@ constexpr char kAppMimeTypesKey[] = "mime_types";
 constexpr char kAppNameKey[] = "name";
 constexpr char kAppNoDisplayKey[] = "no_display";
 
-constexpr char kCrostiniAppIdPrefix[] = "crostini:";
-
 std::string GenerateAppId(const std::string& desktop_file_id,
                           const std::string& vm_name,
                           const std::string& container_name) {
@@ -125,6 +135,68 @@ CrostiniRegistryService::CrostiniRegistryService(Profile* profile)
 
 CrostiniRegistryService::~CrostiniRegistryService() = default;
 
+std::string CrostiniRegistryService::GetCrostiniShelfAppId(
+    const std::string& window_app_id,
+    const std::string* window_startup_id) {
+  if (base::StartsWith(window_app_id, kArcWindowAppIdPrefix,
+                       base::CompareCase::SENSITIVE))
+    return std::string();
+
+  // TODO(timloh): We should:
+  // - Use and store startup notify values so we can check against it.
+  // - Store StartUpWMClass values and give these priority over matching the
+  // desktop file id for non-wayland apps
+
+  base::StringPiece key;
+  if (base::StartsWith(window_app_id, kCrostiniWindowAppIdPrefix,
+                       base::CompareCase::SENSITIVE)) {
+    base::StringPiece suffix(
+        window_app_id.begin() + strlen(kCrostiniWindowAppIdPrefix),
+        window_app_id.end());
+
+    // If we don't have an id to match to a desktop file, just return the window
+    // app id, which is already prefixed.
+    if (!base::StartsWith(suffix, kWMClassPrefix, base::CompareCase::SENSITIVE))
+      return kCrostiniAppIdPrefix + window_app_id;
+    key = suffix.substr(strlen(kWMClassPrefix));
+  } else {
+    key = window_app_id;
+  }
+
+  std::string result;
+  const base::DictionaryValue* apps =
+      prefs_->GetDictionary(kCrostiniRegistryPref);
+  for (const auto& item : apps->DictItems()) {
+    const std::string& desktop_file_id =
+        item.second
+            .FindKeyOfType(kAppDesktopFileIdKey, base::Value::Type::STRING)
+            ->GetString();
+    if (!EqualsCaseInsensitiveASCII(key, desktop_file_id))
+      continue;
+
+    if (!result.empty())
+      return kCrostiniAppIdPrefix + window_app_id;
+    result = item.first;
+  }
+
+  if (!result.empty())
+    return result;
+  return kCrostiniAppIdPrefix + window_app_id;
+}
+
+bool CrostiniRegistryService::IsCrostiniShelfAppId(
+    const std::string& shelf_app_id) {
+  if (base::StartsWith(shelf_app_id, kCrostiniAppIdPrefix,
+                       base::CompareCase::SENSITIVE))
+    return true;
+  // TODO(timloh): Return true for the default Terminal app.
+  // TODO(timloh): We need to handle desktop files that have been removed.
+  // For example, running windows with a no-longer-valid app id will try to
+  // use the ExtensionContextMenuModel.
+  return prefs_->GetDictionary(kCrostiniRegistryPref)->FindKey(shelf_app_id) !=
+         nullptr;
+}
+
 std::vector<std::string> CrostiniRegistryService::GetRegisteredAppIds() const {
   const base::DictionaryValue* apps =
       prefs_->GetDictionary(kCrostiniRegistryPref);

chrome/browser/chromeos/crostini/crostini_registry_service.h

@@ -31,6 +31,25 @@ namespace crostini {
 // the VM isn't running. The registrations here correspond to .desktop files,
 // which are detailed in the spec:
 // https://www.freedesktop.org/wiki/Specifications/desktop-entry-spec/
+
+// This class deals with several types of IDs, including:
+// 1) Desktop File IDs (desktop_file_id):
+//    - As per the desktop entry spec.
+// 2) Crostini App List Ids (app_id):
+//    - Valid extensions ids for apps stored in the registry, derived from the
+//    desktop file id, vm name, and container name.
+//    - The default Terminal app is a special case app list item, without an
+//    entry in the registry.
+// 3) Exo Window App Ids (window_app_id):
+//    - Retrieved from exo::ShellSurface::GetApplicationId()
+//    - For Wayland apps, this is the surface class of the app
+//    - For X apps, this is of the form org.chromium.termina.wmclass.foo when
+//    WM_CLASS is set to foo, or otherwise some string prefixed by
+//    "org.chromium.termina." when WM_CLASS is not set.
+// 4) Shelf App Ids (shelf_app_id):
+//    - Used in ash::ShelfID::app_id
+//    - Either a Window App Id prefixed by "crostini:" or a Crostini App Id.
+//    - For pinned apps, this is a Crostini App Id.
 class CrostiniRegistryService : public KeyedService {
  public:
   struct Registration {
@@ -80,6 +99,20 @@ class CrostiniRegistryService : public KeyedService {
   explicit CrostiniRegistryService(Profile* profile);
   ~CrostiniRegistryService() override;
 
+  // Returns a shelf app id for an exo window id.
+  //
+  // If the given window app id is not for Crostini (i.e. Arc++), returns an
+  // empty string. If we can uniquely identify a registry entry, returns the
+  // crostini app id for that. Otherwise, returns the |window_app_id|, possibly
+  // prefixed "org.chromium.termina.wayland." if it was not already prefixed.
+  //
+  // As the window app id is derived from fields set by the app itself, it is
+  // possible for an app to masquerade as a different app.
+  std::string GetCrostiniShelfAppId(const std::string& window_app_id,
+                                    const std::string* window_startup_id);
+  // Returns whether the app_id is a Crostini app id.
+  bool IsCrostiniShelfAppId(const std::string& shelf_app_id);
+
   std::vector<std::string> GetRegisteredAppIds() const;
 
   // Return null if |app_id| is not found in the registry.

chrome/browser/chromeos/crostini/crostini_registry_service_unittest.cc

@@ -65,6 +65,10 @@ class CrostiniRegistryServiceTest : public testing::Test {
     return app_list;
   }
 
+  std::string WindowIdForWMClass(const std::string& wm_class) {
+    return "org.chromium.termina.wmclass." + wm_class;
+  }
+
   CrostiniRegistryService* service() { return service_.get(); }
 
  private:
@@ -177,4 +181,40 @@ TEST_F(CrostiniRegistryServiceTest, MultipleContainers) {
               testing::UnorderedElementsAre(app_id_1, app_id_3, new_app_id));
 }
 
+TEST_F(CrostiniRegistryServiceTest, GetCrostiniAppIdNoStartupID) {
+  ApplicationList app_list = BasicAppList("app", "vm", "container");
+  *app_list.add_apps() = BasicApp("cool.app");
+  *app_list.add_apps() = BasicApp("super");
+  service()->UpdateApplicationList(app_list);
+
+  service()->UpdateApplicationList(BasicAppList("super", "vm 2", "container"));
+
+  EXPECT_THAT(service()->GetRegisteredAppIds(), testing::SizeIs(4));
+
+  EXPECT_EQ(
+      service()->GetCrostiniShelfAppId(WindowIdForWMClass("App"), nullptr),
+      GenerateAppId("app", "vm", "container"));
+  EXPECT_EQ(
+      service()->GetCrostiniShelfAppId(WindowIdForWMClass("cool.app"), nullptr),
+      GenerateAppId("cool.app", "vm", "container"));
+
+  EXPECT_EQ(
+      service()->GetCrostiniShelfAppId(WindowIdForWMClass("super"), nullptr),
+      "crostini:" + WindowIdForWMClass("super"));
+  EXPECT_EQ(service()->GetCrostiniShelfAppId(
+                "org.chromium.termina.wmclientleader.1234", nullptr),
+            "crostini:org.chromium.termina.wmclientleader.1234");
+  EXPECT_EQ(service()->GetCrostiniShelfAppId("org.chromium.termina.xid.654321",
+                                             nullptr),
+            "crostini:org.chromium.termina.xid.654321");
+
+  EXPECT_EQ(service()->GetCrostiniShelfAppId("cool.app", nullptr),
+            GenerateAppId("cool.app", "vm", "container"));
+  EXPECT_EQ(service()->GetCrostiniShelfAppId("fancy.app", nullptr),
+            "crostini:fancy.app");
+
+  EXPECT_EQ(service()->GetCrostiniShelfAppId("org.chromium.arc.h", nullptr),
+            std::string());
+}
+
 }  // namespace crostini

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

@@ -9,12 +9,6 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/chrome_features.h"
 
-namespace {
-
-constexpr char kCrostiniAppIdPrefix[] = "crostini:";
-
-}  // namespace
-
 bool IsCrostiniAllowed() {
   return virtual_machines::AreVirtualMachinesAllowedByVersionAndChannel() &&
          virtual_machines::AreVirtualMachinesAllowedByPolicy();
@@ -32,13 +26,3 @@ bool IsCrostiniInstalled() {
 bool IsCrostiniRunning() {
   return false;
 }
-
-std::string CreateCrostiniAppId(const std::string& window_app_id) {
-  DCHECK(!IsCrostiniAppId(window_app_id));
-  return kCrostiniAppIdPrefix + window_app_id;
-}
-
-bool IsCrostiniAppId(const std::string& app_id) {
-  return strncmp(app_id.c_str(), kCrostiniAppIdPrefix,
-                 sizeof(kCrostiniAppIdPrefix) - 1) == 0;
-}

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

@@ -5,8 +5,6 @@
 #ifndef CHROME_BROWSER_UI_APP_LIST_CROSTINI_CROSTINI_UTIL_H_
 #define CHROME_BROWSER_UI_APP_LIST_CROSTINI_CROSTINI_UTIL_H_
 
-#include <string>
-
 // Returns true if crostini is allowed to run.
 // Otherwise, returns false, e.g. if crostini is not available on the device,
 // or it is in the flow to set up managed account creation.
@@ -15,12 +13,6 @@ bool IsCrostiniAllowed();
 // Returns true if crostini UI can be shown. Implies crostini is allowed to run.
 bool IsExperimentalCrostiniUIAvailable();
 
-// Returns a Crostini app ID from a Aura window app ID.
-std::string CreateCrostiniAppId(const std::string& window_app_id);
-
-// Returns true if the app_id is a Crostini app ID.
-bool IsCrostiniAppId(const std::string& app_id);
-
 constexpr char kCrostiniDefaultVmName[] = "termina";
 constexpr char kCrostiniDefaultContainerName[] = "penguin";
 constexpr char kCrostiniCroshBuiltinAppId[] =

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

@@ -8,6 +8,9 @@
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/public/cpp/window_properties.h"
 #include "base/bind.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.h"
 #include "chrome/browser/ui/app_list/crostini/crostini_util.h"
 #include "chrome/browser/ui/ash/launcher/app_window_base.h"
 #include "chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h"
@@ -20,12 +23,6 @@
 #include "ui/display/manager/display_manager.h"
 #include "ui/views/widget/widget.h"
 
-namespace {
-
-constexpr char kArcAppIdPrefix[] = "org.chromium.arc";
-
-}  // namespace
-
 CrostiniAppWindowShelfController::CrostiniAppWindowShelfController(
     ChromeLauncherController* owner)
     : AppWindowLauncherController(owner) {
@@ -62,30 +59,32 @@ void CrostiniAppWindowShelfController::OnWindowVisibilityChanged(
   if (!visible)
     return;
 
-  const std::string* window_app_id =
-      exo::ShellSurface::GetApplicationId(window);
-  if (window_app_id == nullptr)
-    return;
-
-  // Skip handling ARC++ windows.
-  if (strncmp(window_app_id->c_str(), kArcAppIdPrefix,
-              sizeof(kArcAppIdPrefix) - 1) == 0)
-    return;
-
   // Skip when this window has been handled. This can happen when the window
   // becomes visible again.
   auto app_window_it = aura_window_to_app_window_.find(window);
   if (app_window_it != aura_window_to_app_window_.end())
     return;
 
-  RegisterAppWindow(window, window_app_id);
+  const std::string* window_app_id =
+      exo::ShellSurface::GetApplicationId(window);
+  if (window_app_id == nullptr)
+    return;
+  crostini::CrostiniRegistryService* registry_service =
+      crostini::CrostiniRegistryServiceFactory::GetForProfile(
+          owner()->profile());
+  const std::string& shelf_app_id = registry_service->GetCrostiniShelfAppId(
+      *window_app_id, exo::ShellSurface::GetStartupId(window));
+  // Non-crostini apps (i.e. arc++) are filtered out here.
+  if (shelf_app_id.empty())
+    return;
+
+  RegisterAppWindow(window, shelf_app_id);
 }
 
 void CrostiniAppWindowShelfController::RegisterAppWindow(
     aura::Window* window,
-    const std::string* window_app_id) {
-  const std::string crostini_app_id = CreateCrostiniAppId(*window_app_id);
-  const ash::ShelfID shelf_id(crostini_app_id);
+    const std::string& shelf_app_id) {
+  const ash::ShelfID shelf_id(shelf_app_id);
   views::Widget* widget = views::Widget::GetWidgetForNativeWindow(window);
   aura_window_to_app_window_[window] =
       std::make_unique<AppWindowBase>(shelf_id, widget);

chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.h

@@ -45,8 +45,7 @@ class CrostiniAppWindowShelfController : public AppWindowLauncherController,
   using AuraWindowToAppWindow =
       std::map<aura::Window*, std::unique_ptr<AppWindowBase>>;
 
-  void RegisterAppWindow(aura::Window* window,
-                         const std::string* window_app_id);
+  void RegisterAppWindow(aura::Window* window, const std::string& shelf_app_id);
   void UnregisterAppWindow(AppWindowBase* app_window);
 
   // AppWindowLauncherController:

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

@@ -9,9 +9,10 @@
 
 #include "ash/public/cpp/shelf_model.h"
 #include "base/metrics/user_metrics.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service.h"
+#include "chrome/browser/chromeos/crostini/crostini_registry_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
-#include "chrome/browser/ui/app_list/crostini/crostini_util.h"
 #include "chrome/browser/ui/ash/launcher/arc_launcher_context_menu.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller_util.h"
@@ -40,7 +41,9 @@ std::unique_ptr<LauncherContextMenu> LauncherContextMenu::Create(
   }
 
   // Create an CrostiniShelfContextMenu if the item is Crostini app.
-  if (IsCrostiniAppId(item->id.app_id)) {
+  if (crostini::CrostiniRegistryServiceFactory::GetForProfile(
+          controller->profile())
+          ->IsCrostiniShelfAppId(item->id.app_id)) {
     return std::make_unique<CrostiniShelfContextMenu>(controller, item,
                                                       display_id);
   }