Mark synced bookmark apps on non-Chrome OS as not locally installed.

Bookmark apps that are not locally installed will not be treated as
installed for the installation banner checks. In the future more changes
to their behaviour, in relation to the chrome://apps page will be
introduced.

Bug: 874841
Change-Id: I48741967a646c6aac95cde3c832b8c8860b4f7b1
Reviewed-on: https://chromium-review.googlesource.com/1177529
Commit-Queue: Ben Wells <benwells@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584304}

chrome/browser/banners/app_banner_manager_desktop.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/banners/app_banner_settings_helper.h"
 #include "chrome/browser/extensions/bookmark_app_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/web_application_info.h"
 #include "extensions/common/constants.h"
@@ -76,7 +77,7 @@ bool AppBannerManagerDesktop::IsWebAppConsideredInstalled(
     const GURL& validated_url,
     const GURL& start_url,
     const GURL& manifest_url) {
-  return extensions::BookmarkAppHelper::BookmarkOrHostedAppInstalled(
+  return extensions::BookmarkOrHostedAppInstalled(
       web_contents->GetBrowserContext(), start_url);
 }
 

chrome/browser/extensions/bookmark_app_helper.cc

@@ -37,6 +37,7 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/web_applications/components/web_app_icon_downloader.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
 #include "chrome/browser/web_applications/extensions/web_app_extension_shortcut.h"
 #include "chrome/browser/webshare/share_target_pref_helper.h"
 #include "chrome/common/chrome_features.h"
@@ -51,7 +52,6 @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
-#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/pref_names.h"
@@ -114,8 +114,11 @@ class BookmarkAppInstaller : public base::RefCounted<BookmarkAppInstaller>,
                              public content::WebContentsObserver {
  public:
   BookmarkAppInstaller(extensions::ExtensionService* service,
-                       const WebApplicationInfo& web_app_info)
-      : service_(service), web_app_info_(web_app_info) {}
+                       const WebApplicationInfo& web_app_info,
+                       bool is_locally_installed)
+      : service_(service),
+        web_app_info_(web_app_info),
+        is_locally_installed_(is_locally_installed) {}
 
   void Run() {
     for (const auto& icon : web_app_info_.icons) {
@@ -131,7 +134,7 @@ class BookmarkAppInstaller : public base::RefCounted<BookmarkAppInstaller>,
       return;
     }
 
-    FinishInstallation();
+    DoInstallation();
   }
 
   void SetupWebContents() {
@@ -190,11 +193,11 @@ class BookmarkAppInstaller : public base::RefCounted<BookmarkAppInstaller>,
         }
       }
     }
-    FinishInstallation();
+    DoInstallation();
     Release();
   }
 
-  void FinishInstallation() {
+  void DoInstallation() {
     // Ensure that all icons that are in web_app_info are present, by generating
     // icons for any sizes which have failed to download. This ensures that the
     // created manifest for the bookmark app does not contain links to icons
@@ -218,11 +221,24 @@ class BookmarkAppInstaller : public base::RefCounted<BookmarkAppInstaller>,
                                                              &web_app_info_);
     scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(service_));
     installer->set_error_on_unsupported_requirements(true);
+    installer->set_installer_callback(base::BindOnce(
+        &BookmarkAppInstaller::OnInstallationDone, this, installer));
     installer->InstallWebApp(web_app_info_);
   }
 
+  void OnInstallationDone(scoped_refptr<CrxInstaller> installer,
+                          const base::Optional<CrxInstallError>& result) {
+    // No result means success.
+    if (!result.has_value()) {
+      SetBookmarkAppIsLocallyInstalled(service_->GetBrowserContext(),
+                                       installer->extension(),
+                                       is_locally_installed_);
+    }
+  }
+
   extensions::ExtensionService* service_;
   WebApplicationInfo web_app_info_;
+  bool is_locally_installed_;
 
   std::unique_ptr<content::WebContents> web_contents_;
   std::unique_ptr<WebAppIconDownloader> web_app_icon_downloader_;
@@ -286,27 +302,6 @@ WebApplicationInfo::IconInfo BookmarkAppHelper::GenerateIconInfo(
   return icon_info;
 }
 
-// static
-bool BookmarkAppHelper::BookmarkOrHostedAppInstalled(
-    content::BrowserContext* browser_context,
-    const GURL& url) {
-  ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context);
-  const ExtensionSet& extensions = registry->enabled_extensions();
-
-  // Iterate through the extensions and extract the LaunchWebUrl (bookmark apps)
-  // or check the web extent (hosted apps).
-  for (const scoped_refptr<const Extension>& extension : extensions) {
-    if (!extension->is_hosted_app())
-      continue;
-
-    if (extension->web_extent().MatchesURL(url) ||
-        AppLaunchInfo::GetLaunchWebURL(extension.get()) == url) {
-      return true;
-    }
-  }
-  return false;
-}
-
 // static
 void BookmarkAppHelper::UpdateWebAppIconsWithoutChangingLinks(
     std::map<int, web_app::BitmapAndSource> bitmap_map,
@@ -572,6 +567,10 @@ void BookmarkAppHelper::FinishInstallation(const Extension* extension) {
   // Set the launcher type for the app.
   SetLaunchType(profile_, extension->id(), launch_type);
 
+  // Set this app to be locally installed, as it was installed from this
+  // machine.
+  SetBookmarkAppIsLocallyInstalled(profile_, extension, true);
+
   if (!contents_) {
     // The web contents can be null in tests.
     callback_.Run(extension, web_app_info_);
@@ -671,9 +670,10 @@ void BookmarkAppHelper::Observe(int type,
 }
 
 void CreateOrUpdateBookmarkApp(extensions::ExtensionService* service,
-                               WebApplicationInfo* web_app_info) {
+                               WebApplicationInfo* web_app_info,
+                               bool is_locally_installed) {
   scoped_refptr<BookmarkAppInstaller> installer(
-      new BookmarkAppInstaller(service, *web_app_info));
+      new BookmarkAppInstaller(service, *web_app_info, is_locally_installed));
   installer->Run();
 }
 

chrome/browser/extensions/bookmark_app_helper.h

@@ -29,7 +29,6 @@ class Profile;
 class SkBitmap;
 
 namespace content {
-class BrowserContext;
 class WebContents;
 }  // namespace content
 
@@ -77,11 +76,6 @@ class BookmarkAppHelper : public content::NotificationObserver {
                                                        SkColor color,
                                                        char letter);
 
-  // Returns true if a bookmark or hosted app from a given URL is already
-  // installed and enabled.
-  static bool BookmarkOrHostedAppInstalled(
-      content::BrowserContext* browser_context, const GURL& url);
-
   // It is important that the linked app information in any extension that
   // gets created from sync matches the linked app information that came from
   // sync. If there are any changes, they will be synced back to other devices
@@ -173,7 +167,8 @@ class BookmarkAppHelper : public content::NotificationObserver {
 // Creates or updates a bookmark app from the given |web_app_info|. Icons will
 // be downloaded from the icon URLs provided in |web_app_info|.
 void CreateOrUpdateBookmarkApp(ExtensionService* service,
-                               WebApplicationInfo* web_app_info);
+                               WebApplicationInfo* web_app_info,
+                               bool is_locally_installed);
 
 // Returns whether the given |url| is a valid bookmark app url.
 bool IsValidBookmarkAppUrl(const GURL& url);

chrome/browser/extensions/bookmark_app_helper_unittest.cc

@@ -15,6 +15,7 @@
 #include "chrome/browser/extensions/launch_util.h"
 #include "chrome/browser/installable/installable_data.h"
 #include "chrome/browser/web_applications/components/web_app_icon_downloader.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/extensions/manifest_handlers/app_theme_color_info.h"
@@ -518,7 +519,8 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateAndUpdateBookmarkApp) {
   web_app_info.icons.push_back(
       CreateIconInfoWithBitmap(kIconSizeSmall, SK_ColorRED));
 
-  extensions::CreateOrUpdateBookmarkApp(service_, &web_app_info);
+  extensions::CreateOrUpdateBookmarkApp(service_, &web_app_info,
+                                        false /* is_locally_installed */);
   content::RunAllTasksUntilIdle();
 
   {
@@ -533,13 +535,15 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateAndUpdateBookmarkApp) {
     EXPECT_FALSE(extensions::IconsInfo::GetIconResource(
                      extension, kIconSizeSmall, ExtensionIconSet::MATCH_EXACTLY)
                      .empty());
+    EXPECT_FALSE(BookmarkAppIsLocallyInstalled(profile(), extension));
   }
 
   web_app_info.title = base::UTF8ToUTF16(kAlternativeAppTitle);
   web_app_info.icons[0] = CreateIconInfoWithBitmap(kIconSizeLarge, SK_ColorRED);
   web_app_info.scope = GURL(kAppAlternativeScope);
 
-  extensions::CreateOrUpdateBookmarkApp(service_, &web_app_info);
+  extensions::CreateOrUpdateBookmarkApp(service_, &web_app_info,
+                                        true /* is_locally_installed */);
   content::RunAllTasksUntilIdle();
 
   {
@@ -558,6 +562,7 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateAndUpdateBookmarkApp) {
     EXPECT_FALSE(extensions::IconsInfo::GetIconResource(
                      extension, kIconSizeLarge, ExtensionIconSet::MATCH_EXACTLY)
                      .empty());
+    EXPECT_TRUE(BookmarkAppIsLocallyInstalled(profile(), extension));
   }
 }
 

chrome/browser/extensions/browsertest_util.cc

@@ -58,7 +58,8 @@ const Extension* InstallBookmarkApp(Profile* profile, WebApplicationInfo info) {
   content::WindowedNotificationObserver windowed_observer(
       NOTIFICATION_CRX_INSTALLER_DONE,
       content::NotificationService::AllSources());
-  CreateOrUpdateBookmarkApp(GetExtensionService(profile), &info);
+  CreateOrUpdateBookmarkApp(GetExtensionService(profile), &info,
+                            true /* locally_installed */);
   windowed_observer.Wait();
 
   EXPECT_EQ(++num_extensions,

chrome/browser/extensions/extension_sync_service.cc

@@ -555,7 +555,13 @@ void ExtensionSyncService::ApplyBookmarkAppSyncData(
     web_app_info.icons.push_back(icon_info);
   }
 
-  CreateOrUpdateBookmarkApp(extension_service(), &web_app_info);
+#if defined(OS_CHROMEOS)
+  bool is_locally_installed = true;
+#else
+  bool is_locally_installed = extension != nullptr;
+#endif
+  CreateOrUpdateBookmarkApp(extension_service(), &web_app_info,
+                            is_locally_installed);
 }
 
 void ExtensionSyncService::SetSyncStartFlareForTesting(

chrome/browser/sync/test/integration/two_client_apps_sync_test.cc

@@ -19,6 +19,7 @@
 #include "chrome/browser/sync/test/integration/sync_app_helper.h"
 #include "chrome/browser/sync/test/integration/sync_integration_test_util.h"
 #include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/extensions/manifest_handlers/app_theme_color_info.h"
 #include "components/sync/model/string_ordinal.h"
@@ -395,7 +396,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, BookmarkAppBasic) {
         extensions::NOTIFICATION_CRX_INSTALLER_DONE,
         content::NotificationService::AllSources());
     extensions::CreateOrUpdateBookmarkApp(GetExtensionService(GetProfile(0)),
-                                          &web_app_info);
+                                          &web_app_info,
+                                          true /* is_locally_installed */);
     windowed_observer.Wait();
     EXPECT_EQ(num_extensions,
               GetExtensionRegistry(GetProfile(0))->enabled_extensions().size());
@@ -404,7 +406,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, BookmarkAppBasic) {
     // Wait for the synced app to install.
     content::WindowedNotificationObserver windowed_observer(
         extensions::NOTIFICATION_CRX_INSTALLER_DONE,
-        base::Bind(&AllProfilesHaveSameApps));
+        base::BindRepeating(&AllProfilesHaveSameApps));
     windowed_observer.Wait();
   }
 }
@@ -425,7 +427,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, BookmarkAppMinimal) {
         extensions::NOTIFICATION_CRX_INSTALLER_DONE,
         content::NotificationService::AllSources());
     extensions::CreateOrUpdateBookmarkApp(GetExtensionService(GetProfile(0)),
-                                          &web_app_info);
+                                          &web_app_info,
+                                          true /* is_locally_installed */);
     windowed_observer.Wait();
     EXPECT_EQ(num_extensions,
               GetExtensionRegistry(GetProfile(0))->enabled_extensions().size());
@@ -434,7 +437,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, BookmarkAppMinimal) {
     // Wait for the synced app to install.
     content::WindowedNotificationObserver windowed_observer(
         extensions::NOTIFICATION_CRX_INSTALLER_DONE,
-        base::Bind(&AllProfilesHaveSameApps));
+        base::BindRepeating(&AllProfilesHaveSameApps));
     windowed_observer.Wait();
   }
 }
@@ -467,7 +470,8 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, BookmarkAppThemeColor) {
         extensions::NOTIFICATION_CRX_INSTALLER_DONE,
         content::NotificationService::AllSources());
     extensions::CreateOrUpdateBookmarkApp(GetExtensionService(GetProfile(0)),
-                                          &web_app_info);
+                                          &web_app_info,
+                                          true /* is_locally_installed */);
     windowed_observer.Wait();
     EXPECT_EQ(num_extensions,
               GetExtensionRegistry(GetProfile(0))->enabled_extensions().size());
@@ -476,7 +480,7 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, BookmarkAppThemeColor) {
     // Wait for the synced app to install.
     content::WindowedNotificationObserver windowed_observer(
         extensions::NOTIFICATION_CRX_INSTALLER_DONE,
-        base::Bind(&AllProfilesHaveSameApps));
+        base::BindRepeating(&AllProfilesHaveSameApps));
     windowed_observer.Wait();
   }
   auto* extension = GetAppByLaunchURL(web_app_info.app_url, GetProfile(1));
@@ -484,6 +488,56 @@ IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, BookmarkAppThemeColor) {
       extensions::AppThemeColorInfo::GetThemeColor(extension);
   EXPECT_EQ(SK_ColorBLUE, theme_color.value());
 }
+
+IN_PROC_BROWSER_TEST_F(TwoClientAppsSyncTest, IsLocallyInstalled) {
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(AllProfilesHaveSameApps());
+
+  size_t num_extensions =
+      GetExtensionRegistry(GetProfile(0))->enabled_extensions().size();
+
+  WebApplicationInfo web_app_info;
+  web_app_info.app_url = GURL("http://www.chromium.org/");
+  web_app_info.title = base::UTF8ToUTF16("Test name");
+  web_app_info.theme_color = SK_ColorBLUE;
+  ++num_extensions;
+  {
+    content::WindowedNotificationObserver windowed_observer(
+        extensions::NOTIFICATION_CRX_INSTALLER_DONE,
+        content::NotificationService::AllSources());
+    extensions::CreateOrUpdateBookmarkApp(GetExtensionService(GetProfile(0)),
+                                          &web_app_info,
+                                          true /* is_locally_installed */);
+    windowed_observer.Wait();
+    EXPECT_EQ(num_extensions,
+              GetExtensionRegistry(GetProfile(0))->enabled_extensions().size());
+  }
+  {
+    // Wait for the synced app to install.
+    content::WindowedNotificationObserver windowed_observer(
+        extensions::NOTIFICATION_CRX_INSTALLER_DONE,
+        base::BindRepeating(&AllProfilesHaveSameApps));
+    windowed_observer.Wait();
+
+    // The is_locally_installed pref is set in a post install task which
+    // completes asynchronously after the CRX_INSTALLER_DONE notification is
+    // sent. This test needs to wait for this to complete before checking the
+    // is_locally_installed_pref, so it waits until all tasks are complete.
+    //
+    // Other tests do not need to do this, as all the fields they check are set
+    // before the CRX_INSTALLER_DONE notification is sent.
+    //
+    // Note this cannot replace the CRX_INSTALLER_DONE notification observer as
+    // it would not wait for the sync stuff to happen.
+    content::RunAllTasksUntilIdle();
+  }
+  auto* extension = GetAppByLaunchURL(web_app_info.app_url, GetProfile(1));
+#if defined(OS_CHROMEOS)
+  EXPECT_TRUE(BookmarkAppIsLocallyInstalled(GetProfile(1), extension));
+#else
+  EXPECT_FALSE(BookmarkAppIsLocallyInstalled(GetProfile(1), extension));
+#endif
+}
 // TODO(akalin): Add tests exercising:
 //   - Offline installation/uninstallation behavior
 //   - App-specific properties

chrome/browser/web_applications/extensions/BUILD.gn

@@ -16,6 +16,8 @@ source_set("extensions") {
     "bookmark_app_installer.h",
     "bookmark_app_shortcut_installation_task.cc",
     "bookmark_app_shortcut_installation_task.h",
+    "bookmark_app_util.cc",
+    "bookmark_app_util.h",
     "pending_bookmark_app_manager.cc",
     "pending_bookmark_app_manager.h",
     "web_app_extension_shortcut.cc",

chrome/browser/web_applications/extensions/bookmark_app_util.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/web_applications/extensions/bookmark_app_util.h"
+
+#include "base/values.h"
+#include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
+#include "content/public/browser/browser_context.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/url_pattern_set.h"
+#include "url/gurl.h"
+
+namespace extensions {
+namespace {
+
+// A preference used to indicate that a bookmark apps is fully locally installed
+// on this machine. The default value (i.e. if the pref is not set) is to be
+// fully locally installed, so that hosted apps or bookmark apps created /
+// synced before this pref existed will be treated as locally installed.
+const char kPrefLocallyInstalled[] = "locallyInstalled";
+
+}  // namespace
+
+void SetBookmarkAppIsLocallyInstalled(content::BrowserContext* context,
+                                      const Extension* extension,
+                                      bool is_locally_installed) {
+  ExtensionPrefs::Get(context)->UpdateExtensionPref(
+      extension->id(), kPrefLocallyInstalled,
+      std::make_unique<base::Value>(is_locally_installed));
+}
+
+bool BookmarkAppIsLocallyInstalled(content::BrowserContext* context,
+                                   const Extension* extension) {
+  bool locally_installed;
+  if (ExtensionPrefs::Get(context)->ReadPrefAsBoolean(
+          extension->id(), kPrefLocallyInstalled, &locally_installed)) {
+    return locally_installed;
+  }
+
+  return true;
+}
+
+bool BookmarkOrHostedAppInstalled(content::BrowserContext* browser_context,
+                                  const GURL& url) {
+  ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context);
+  const ExtensionSet& extensions = registry->enabled_extensions();
+
+  // Iterate through the extensions and extract the LaunchWebUrl (bookmark apps)
+  // or check the web extent (hosted apps).
+  for (const scoped_refptr<const Extension>& extension : extensions) {
+    if (!extension->is_hosted_app())
+      continue;
+
+    if (!BookmarkAppIsLocallyInstalled(browser_context, extension.get()))
+      continue;
+
+    if (extension->web_extent().MatchesURL(url) ||
+        AppLaunchInfo::GetLaunchWebURL(extension.get()) == url) {
+      return true;
+    }
+  }
+  return false;
+}
+
+}  // namespace extensions

chrome/browser/web_applications/extensions/bookmark_app_util.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_UTIL_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_UTIL_H_
+
+namespace content {
+class BrowserContext;
+}
+
+class Extension;
+class GURL;
+
+namespace extensions {
+
+// Sets an extension pref to indicate whether the hosted app is locally
+// installed or not. When apps are not locally installed they will appear in the
+// app launcher, but will act like normal web pages when launched. For example
+// they will never open in standalone windows. They will also have different
+// commands available to them reflecting the fact that they aren't fully
+// installed.
+void SetBookmarkAppIsLocallyInstalled(content::BrowserContext* context,
+                                      const Extension* extension,
+                                      bool is_locally_installed);
+
+// Gets whether the bookmark app is locally installed. Defaults to true if the
+// extension pref that stores this isn't set.
+// Note this can be called for hosted apps which should use the default.
+bool BookmarkAppIsLocallyInstalled(content::BrowserContext* context,
+                                   const Extension* extension);
+
+// Returns true if a bookmark or hosted app from a given URL is already
+// installed and enabled.
+bool BookmarkOrHostedAppInstalled(content::BrowserContext* browser_context,
+                                  const GURL& url);
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_BOOKMARK_APP_UTIL_H_