WebApps: Moves implementation of FindAppWithUrlInScope into AppRegistrar

Previously, WebApps and BookmarkApps had their own implementations of
FindAppWithUrlInScope, that did essentially the same thing. However, the
BookmarkApps implementation didn't take match length into account (the
longest match should always be returned). Instead of adding this sort
to BookmarkApps, this moves the implementation into the generic
AppRegistrar class, seeing as we have all the primitives we require
there.

In addition, this changes the behaviour of WebAppRegistrar::GetAppScope
to return base::nullopt instead of an empty scope. This fixes the broken
behaviour of AppRegistrar::IsAppShortcut.

Bug: 1023244
Change-Id: I66aee439a1fbe8f1718f7ca351d37692ff8c3e46
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1909055
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715811}

chrome/browser/web_applications/components/app_registrar.cc

@@ -85,6 +85,36 @@ bool AppRegistrar::HasExternalAppWithInstallSource(
       profile()->GetPrefs(), app_id, install_source);
 }
 
+base::Optional<AppId> AppRegistrar::FindAppWithUrlInScope(
+    const GURL& url) const {
+  const std::string url_path = url.spec();
+
+  base::Optional<AppId> best_app_id;
+  size_t best_app_path_length = 0U;
+  bool best_app_is_shortcut = true;
+
+  for (const AppId& app_id : GetAppIds()) {
+    // TODO(crbug.com/910016): Treat shortcuts as PWAs.
+    bool app_is_shortcut = IsShortcutApp(app_id);
+    if (app_is_shortcut && !best_app_is_shortcut)
+      continue;
+
+    const base::Optional<GURL> scope = GetAppScope(app_id);
+    const std::string app_path =
+        scope ? scope->spec() : GetAppLaunchURL(app_id).Resolve(".").spec();
+
+    if ((app_path.size() > best_app_path_length ||
+         (best_app_is_shortcut && !app_is_shortcut)) &&
+        base::StartsWith(url_path, app_path, base::CompareCase::SENSITIVE)) {
+      best_app_id = app_id;
+      best_app_path_length = app_path.size();
+      best_app_is_shortcut = app_is_shortcut;
+    }
+  }
+
+  return best_app_id;
+}
+
 std::vector<AppId> AppRegistrar::FindAppsInScope(const GURL& scope) const {
   std::string scope_str = scope.spec();
 

chrome/browser/web_applications/components/app_registrar.h

@@ -61,11 +61,6 @@ class AppRegistrar {
       const AppId& app_id,
       ExternalInstallSource install_source) const;
 
-  // Searches for the first app id in the registry for which the |url| is in
-  // scope.
-  virtual base::Optional<AppId> FindAppWithUrlInScope(
-      const GURL& url) const = 0;
-
   // Count a number of all apps which are installed by user (non-default).
   // Requires app registry to be in a ready state.
   virtual int CountUserInstalledApps() const = 0;
@@ -82,6 +77,10 @@ class AppRegistrar {
 
   virtual std::vector<AppId> GetAppIds() const = 0;
 
+  // Searches for the first app id in the registry for which the |url| is in
+  // scope.
+  base::Optional<AppId> FindAppWithUrlInScope(const GURL& url) const;
+
   // Finds all apps that are installed under |scope|.
   std::vector<AppId> FindAppsInScope(const GURL& scope) const;
 

chrome/browser/web_applications/extensions/bookmark_app_registrar.cc

@@ -54,19 +54,6 @@ bool BookmarkAppRegistrar::WasInstalledByUser(
   return extension && !extension->was_installed_by_default();
 }
 
-base::Optional<web_app::AppId> BookmarkAppRegistrar::FindAppWithUrlInScope(
-    const GURL& url) const {
-  const Extension* extension = util::GetInstalledPwaForUrl(profile(), url);
-
-  if (!extension)
-    extension = GetInstalledShortcutForUrl(profile(), url);
-
-  if (extension)
-    return extension->id();
-
-  return base::nullopt;
-}
-
 int BookmarkAppRegistrar::CountUserInstalledApps() const {
   return CountUserInstalledBookmarkApps(profile());
 }

chrome/browser/web_applications/extensions/bookmark_app_registrar.h

@@ -30,8 +30,6 @@ class BookmarkAppRegistrar : public web_app::AppRegistrar,
   bool WasExternalAppUninstalledByUser(
       const web_app::AppId& app_id) const override;
   bool WasInstalledByUser(const web_app::AppId& app_id) const override;
-  base::Optional<web_app::AppId> FindAppWithUrlInScope(
-      const GURL& url) const override;
   int CountUserInstalledApps() const override;
   std::string GetAppShortName(const web_app::AppId& app_id) const override;
   std::string GetAppDescription(const web_app::AppId& app_id) const override;

chrome/browser/web_applications/test/test_app_registrar.cc

@@ -86,12 +86,6 @@ bool TestAppRegistrar::HasExternalAppWithInstallSource(
   return it != installed_apps_.end();
 }
 
-base::Optional<AppId> TestAppRegistrar::FindAppWithUrlInScope(
-    const GURL& url) const {
-  NOTIMPLEMENTED();
-  return base::nullopt;
-}
-
 int TestAppRegistrar::CountUserInstalledApps() const {
   NOTIMPLEMENTED();
   return 0;

chrome/browser/web_applications/test/test_app_registrar.h

@@ -50,7 +50,6 @@ class TestAppRegistrar : public AppRegistrar {
   bool HasExternalAppWithInstallSource(
       const AppId& app_id,
       ExternalInstallSource install_source) const override;
-  base::Optional<AppId> FindAppWithUrlInScope(const GURL& url) const override;
   int CountUserInstalledApps() const override;
   std::string GetAppShortName(const AppId& app_id) const override;
   std::string GetAppDescription(const AppId& app_id) const override;

chrome/browser/web_applications/web_app_registrar.cc

@@ -45,36 +45,6 @@ bool WebAppRegistrar::WasInstalledByUser(const AppId& app_id) const {
   return true;
 }
 
-base::Optional<AppId> WebAppRegistrar::FindAppWithUrlInScope(
-    const GURL& url) const {
-  const std::string url_path = url.spec();
-
-  base::Optional<AppId> best_app;
-  size_t best_path_length = 0U;
-  bool best_is_shortcut = true;
-
-  for (const WebApp& app : AllApps()) {
-    // TODO(crbug.com/915038): Implement and use WebApp::IsShortcut().
-    // TODO(crbug.com/910016): Treat shortcuts as PWAs.
-    bool app_is_shortcut = app.scope().is_empty();
-    if (app_is_shortcut && !best_is_shortcut)
-      continue;
-
-    const std::string app_path = app.scope().is_empty()
-                                     ? app.launch_url().Resolve(".").spec()
-                                     : app.scope().spec();
-    if ((app_path.size() > best_path_length ||
-         (best_is_shortcut && !app_is_shortcut)) &&
-        base::StartsWith(url_path, app_path, base::CompareCase::SENSITIVE)) {
-      best_app = app.app_id();
-      best_path_length = app_path.size();
-      best_is_shortcut = app_is_shortcut;
-    }
-  }
-
-  return best_app;
-}
-
 int WebAppRegistrar::CountUserInstalledApps() const {
   NOTIMPLEMENTED();
 
@@ -113,7 +83,17 @@ const GURL& WebAppRegistrar::GetAppLaunchURL(const AppId& app_id) const {
 
 base::Optional<GURL> WebAppRegistrar::GetAppScope(const AppId& app_id) const {
   auto* web_app = GetAppById(app_id);
-  return web_app ? base::Optional<GURL>(web_app->scope()) : base::nullopt;
+  if (!web_app)
+    return base::nullopt;
+
+  // TODO(crbug.com/910016): Treat shortcuts as PWAs.
+  // Shortcuts on the WebApp system have empty scopes, while the implementation
+  // of IsShortcutApp just checks if the scope is |base::nullopt|, so make sure
+  // we return |base::nullopt| rather than an empty scope.
+  if (web_app->scope().is_empty())
+    return base::nullopt;
+
+  return web_app->scope();
 }
 
 DisplayMode WebAppRegistrar::GetAppDisplayMode(const AppId& app_id) const {

chrome/browser/web_applications/web_app_registrar.h

@@ -37,7 +37,6 @@ class WebAppRegistrar : public AppRegistrar {
   bool IsLocallyInstalled(const AppId& app_id) const override;
   bool WasExternalAppUninstalledByUser(const AppId& app_id) const override;
   bool WasInstalledByUser(const AppId& app_id) const override;
-  base::Optional<AppId> FindAppWithUrlInScope(const GURL& url) const override;
   int CountUserInstalledApps() const override;
   std::string GetAppShortName(const AppId& app_id) const override;
   std::string GetAppDescription(const AppId& app_id) const override;