desktop-pwas: Only add a scope for installable sites i.e. PWAs

Also changes Installable to an enum class to avoid accidental
implicit casts to bools.

Bug: 801940
Change-Id: I186d878b338375e339b3b5eb24fa5f2abd6c975a
Reviewed-on: https://chromium-review.googlesource.com/981835Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550502}

chrome/browser/extensions/bookmark_app_helper.cc

@@ -339,7 +339,8 @@ namespace extensions {
 // static
 void BookmarkAppHelper::UpdateWebAppInfoFromManifest(
     const content::Manifest& manifest,
-    WebApplicationInfo* web_app_info) {
+    WebApplicationInfo* web_app_info,
+    ForInstallableSite for_installable_site) {
   if (!manifest.short_name.is_null())
     web_app_info->title = manifest.short_name.string();
 
@@ -351,6 +352,7 @@ void BookmarkAppHelper::UpdateWebAppInfoFromManifest(
   if (manifest.start_url.is_valid())
     web_app_info->app_url = manifest.start_url;
 
+  if (for_installable_site == ForInstallableSite::kYes) {
     // If there is no scope present, use 'start_url' without the filename as the
     // scope. This does not match the spec but it matches what we do on Android.
     // See: https://github.com/w3c/manifest/issues/550
@@ -358,6 +360,7 @@ void BookmarkAppHelper::UpdateWebAppInfoFromManifest(
       web_app_info->scope = manifest.scope;
     else if (manifest.start_url.is_valid())
       web_app_info->scope = manifest.start_url.Resolve(".");
+  }
 
   if (manifest.theme_color != content::Manifest::kInvalidOrMissingColor)
     web_app_info->theme_color = static_cast<SkColor>(manifest.theme_color);
@@ -595,7 +598,7 @@ void BookmarkAppHelper::Create(const CreateBookmarkAppCallback& callback) {
                              weak_factory_.GetWeakPtr()));
     }
   } else {
-    installable_ = INSTALLABLE_NO;
+    for_installable_site_ = ForInstallableSite::kNo;
     OnIconsDownloaded(true, std::map<GURL, std::vector<SkBitmap>>());
   }
 }
@@ -607,10 +610,12 @@ void BookmarkAppHelper::OnDidPerformInstallableCheck(
   if (contents_->IsBeingDestroyed())
     return;
 
-  installable_ =
-      data.error_code == NO_ERROR_DETECTED ? INSTALLABLE_YES : INSTALLABLE_NO;
+  for_installable_site_ = data.error_code == NO_ERROR_DETECTED
+                              ? ForInstallableSite::kYes
+                              : ForInstallableSite::kNo;
 
-  UpdateWebAppInfoFromManifest(*data.manifest, &web_app_info_);
+  UpdateWebAppInfoFromManifest(*data.manifest, &web_app_info_,
+                               for_installable_site_);
 
   // TODO(mgiuca): Web Share Target should have its own flag, rather than using
   // the experimental-web-platform-features flag. https://crbug.com/736178.
@@ -716,7 +721,7 @@ void BookmarkAppHelper::OnIconsDownloaded(
   }
 
   if (base::FeatureList::IsEnabled(features::kDesktopPWAWindowing) &&
-      installable_ == INSTALLABLE_YES) {
+      for_installable_site_ == ForInstallableSite::kYes) {
     web_app_info_.open_as_window = true;
     chrome::ShowPWAInstallDialog(
         contents_, web_app_info_,
@@ -738,7 +743,7 @@ void BookmarkAppHelper::OnBubbleCompleted(
     crx_installer_->InstallWebApp(web_app_info_);
 
     if (InstallableMetrics::IsReportableInstallSource(install_source_) &&
-        installable_ == INSTALLABLE_YES) {
+        for_installable_site_ == ForInstallableSite::kYes) {
       InstallableMetrics::TrackInstallEvent(install_source_);
     }
   } else {
@@ -754,8 +759,8 @@ void BookmarkAppHelper::FinishInstallation(const Extension* extension) {
                                            : extensions::LAUNCH_TYPE_REGULAR;
 
   if (base::FeatureList::IsEnabled(features::kDesktopPWAWindowing)) {
-    DCHECK_NE(INSTALLABLE_UNKNOWN, installable_);
-    launch_type = installable_ == INSTALLABLE_YES
+    DCHECK_NE(ForInstallableSite::kUnknown, for_installable_site_);
+    launch_type = for_installable_site_ == ForInstallableSite::kYes
                       ? extensions::LAUNCH_TYPE_WINDOW
                       : extensions::LAUNCH_TYPE_REGULAR;
   }

chrome/browser/extensions/bookmark_app_helper.h

@@ -39,6 +39,12 @@ class Extension;
 // A helper class for creating bookmark apps from a WebContents.
 class BookmarkAppHelper : public content::NotificationObserver {
  public:
+  enum class ForInstallableSite {
+    kYes,
+    kNo,
+    kUnknown,
+  };
+
   struct BitmapAndSource {
     BitmapAndSource();
     BitmapAndSource(const GURL& source_url_p, const SkBitmap& bitmap_p);
@@ -66,7 +72,8 @@ class BookmarkAppHelper : public content::NotificationObserver {
 
   // Update the given WebApplicationInfo with information from the manifest.
   static void UpdateWebAppInfoFromManifest(const content::Manifest& manifest,
-                                           WebApplicationInfo* web_app_info);
+                                           WebApplicationInfo* web_app_info,
+                                           ForInstallableSite installable_site);
 
   // This finds the closest not-smaller bitmap in |bitmaps| for each size in
   // |sizes| and resizes it to that size. This returns a map of sizes to bitmaps
@@ -131,12 +138,6 @@ class BookmarkAppHelper : public content::NotificationObserver {
   FRIEND_TEST_ALL_PREFIXES(BookmarkAppHelperTest,
                            CreateWindowedPWAIntoAppWindow);
 
-  enum Installable {
-    INSTALLABLE_YES,
-    INSTALLABLE_NO,
-    INSTALLABLE_UNKNOWN,
-  };
-
   // Called after the bubble has been shown, and the user has either accepted or
   // the dialog was dismissed.
   void OnBubbleCompleted(bool user_accepted,
@@ -170,7 +171,7 @@ class BookmarkAppHelper : public content::NotificationObserver {
 
   InstallableManager* installable_manager_;
 
-  Installable installable_ = INSTALLABLE_UNKNOWN;
+  ForInstallableSite for_installable_site_ = ForInstallableSite::kUnknown;
 
   // The mechanism via which the app creation was triggered.
   WebappInstallSource install_source_;

chrome/browser/extensions/bookmark_app_helper_unittest.cc

@@ -36,6 +36,8 @@ namespace extensions {
 
 namespace {
 
+using ForInstallableSite = BookmarkAppHelper::ForInstallableSite;
+
 const char kManifestUrl[] = "http://www.chromium.org/manifest.json";
 const char kAppUrl[] = "http://www.chromium.org/index.html";
 const char kAlternativeAppUrl[] = "http://www.notchromium.org";
@@ -298,7 +300,8 @@ class TestBookmarkAppHelper : public BookmarkAppHelper {
 
   void CompleteInstallableCheck(const char* manifest_url,
                                 const content::Manifest& manifest,
-                                bool installable) {
+                                ForInstallableSite for_installable_site) {
+    bool installable = for_installable_site == ForInstallableSite::kYes;
     InstallableData data = {
         installable ? NO_ERROR_DETECTED : MANIFEST_DISPLAY_NOT_SUPPORTED,
         GURL(manifest_url),
@@ -346,7 +349,8 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateBookmarkApp) {
   helper.Create(base::Bind(&TestBookmarkAppHelper::CreationComplete,
                            base::Unretained(&helper)));
 
-  helper.CompleteInstallableCheck(kManifestUrl, content::Manifest(), false);
+  helper.CompleteInstallableCheck(kManifestUrl, content::Manifest(),
+                                  ForInstallableSite::kNo);
 
   std::map<GURL, std::vector<SkBitmap> > icon_map;
   icon_map[GURL(kAppUrl)].push_back(
@@ -373,7 +377,20 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateBookmarkApp) {
           .is_null());
 }
 
-TEST_F(BookmarkAppHelperExtensionServiceTest, CreateBookmarkAppWithManifest) {
+class BookmarkAppHelperExtensionServiceInstallableSiteTest
+    : public BookmarkAppHelperExtensionServiceTest,
+      public ::testing::WithParamInterface<ForInstallableSite> {
+ public:
+  BookmarkAppHelperExtensionServiceInstallableSiteTest() {}
+  ~BookmarkAppHelperExtensionServiceInstallableSiteTest() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(
+      BookmarkAppHelperExtensionServiceInstallableSiteTest);
+};
+
+TEST_P(BookmarkAppHelperExtensionServiceInstallableSiteTest,
+       CreateBookmarkAppWithManifest) {
   WebApplicationInfo web_app_info;
 
   std::unique_ptr<content::WebContents> contents(
@@ -387,7 +404,7 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateBookmarkAppWithManifest) {
   manifest.name = base::NullableString16(base::UTF8ToUTF16(kAppTitle), false);
   manifest.scope = GURL(kAppScope);
   manifest.theme_color = SK_ColorBLUE;
-  helper.CompleteInstallableCheck(kManifestUrl, manifest, true);
+  helper.CompleteInstallableCheck(kManifestUrl, manifest, GetParam());
 
   std::map<GURL, std::vector<SkBitmap> > icon_map;
   helper.CompleteIconDownload(true, icon_map);
@@ -401,16 +418,21 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateBookmarkAppWithManifest) {
   EXPECT_TRUE(extension->from_bookmark());
   EXPECT_EQ(kAppTitle, extension->name());
   EXPECT_EQ(GURL(kAppUrl), AppLaunchInfo::GetLaunchWebURL(extension));
-  EXPECT_EQ(GURL(kAppScope), GetScopeURLFromBookmarkApp(extension));
   EXPECT_EQ(SK_ColorBLUE, AppThemeColorInfo::GetThemeColor(extension).value());
   EXPECT_FALSE(
       AppBannerSettingsHelper::GetSingleBannerEvent(
           contents.get(), manifest.start_url, manifest.start_url.spec(),
           AppBannerSettingsHelper::APP_BANNER_EVENT_DID_ADD_TO_HOMESCREEN)
           .is_null());
+
+  if (GetParam() == ForInstallableSite::kYes) {
+    EXPECT_EQ(GURL(kAppScope), GetScopeURLFromBookmarkApp(extension));
+  } else {
+    EXPECT_EQ(GURL(), GetScopeURLFromBookmarkApp(extension));
+  }
 }
 
-TEST_F(BookmarkAppHelperExtensionServiceTest,
+TEST_P(BookmarkAppHelperExtensionServiceInstallableSiteTest,
        CreateBookmarkAppWithManifestIcons) {
   WebApplicationInfo web_app_info;
   std::unique_ptr<content::WebContents> contents(
@@ -428,7 +450,7 @@ TEST_F(BookmarkAppHelperExtensionServiceTest,
   manifest.icons.push_back(icon);
   icon.src = GURL(kAppIconURL2);
   manifest.icons.push_back(icon);
-  helper.CompleteInstallableCheck(kManifestUrl, manifest, true);
+  helper.CompleteInstallableCheck(kManifestUrl, manifest, GetParam());
 
   // Favicon URLs are ignored because the site has a manifest with icons.
   EXPECT_FALSE(helper.favicon_downloader()->need_favicon_urls_);
@@ -451,9 +473,48 @@ TEST_F(BookmarkAppHelperExtensionServiceTest,
   EXPECT_TRUE(extension->from_bookmark());
   EXPECT_EQ(kAppTitle, extension->name());
   EXPECT_EQ(GURL(kAppUrl), AppLaunchInfo::GetLaunchWebURL(extension));
+
+  if (GetParam() == ForInstallableSite::kYes) {
     EXPECT_EQ(GURL(kAppScope), GetScopeURLFromBookmarkApp(extension));
+  } else {
+    EXPECT_EQ(GURL(), GetScopeURLFromBookmarkApp(extension));
+  }
+}
+
+TEST_P(BookmarkAppHelperExtensionServiceInstallableSiteTest,
+       CreateBookmarkAppWithManifestNoScope) {
+  WebApplicationInfo web_app_info;
+
+  std::unique_ptr<content::WebContents> contents(
+      content::WebContentsTester::CreateTestWebContents(profile(), nullptr));
+  TestBookmarkAppHelper helper(service_, web_app_info, contents.get());
+  helper.Create(base::Bind(&TestBookmarkAppHelper::CreationComplete,
+                           base::Unretained(&helper)));
+
+  content::Manifest manifest;
+  manifest.start_url = GURL(kAppUrl);
+  manifest.name = base::NullableString16(base::UTF8ToUTF16(kAppTitle), false);
+  helper.CompleteInstallableCheck(kManifestUrl, manifest, GetParam());
+
+  std::map<GURL, std::vector<SkBitmap>> icon_map;
+  helper.CompleteIconDownload(true, icon_map);
+  content::RunAllTasksUntilIdle();
+  EXPECT_TRUE(helper.extension());
+
+  const Extension* extension =
+      service_->GetInstalledExtension(helper.extension()->id());
+  if (GetParam() == ForInstallableSite::kYes) {
+    EXPECT_EQ(GURL(kAppDefaultScope), GetScopeURLFromBookmarkApp(extension));
+  } else {
+    EXPECT_EQ(GURL(), GetScopeURLFromBookmarkApp(extension));
+  }
 }
 
+INSTANTIATE_TEST_CASE_P(/* no prefix */,
+                        BookmarkAppHelperExtensionServiceInstallableSiteTest,
+                        ::testing::Values(ForInstallableSite::kNo,
+                                          ForInstallableSite::kYes));
+
 TEST_F(BookmarkAppHelperExtensionServiceTest,
        CreateBookmarkAppDefaultLauncherContainers) {
   auto scoped_feature_list = std::make_unique<base::test::ScopedFeatureList>();
@@ -473,7 +534,8 @@ TEST_F(BookmarkAppHelperExtensionServiceTest,
     helper.Create(base::Bind(&TestBookmarkAppHelper::CreationComplete,
                              base::Unretained(&helper)));
 
-    helper.CompleteInstallableCheck(kManifestUrl, manifest, true);
+    helper.CompleteInstallableCheck(kManifestUrl, manifest,
+                                    ForInstallableSite::kYes);
     helper.CompleteIconDownload(true, icon_map);
 
     content::RunAllTasksUntilIdle();
@@ -489,7 +551,8 @@ TEST_F(BookmarkAppHelperExtensionServiceTest,
     helper.Create(base::Bind(&TestBookmarkAppHelper::CreationComplete,
                              base::Unretained(&helper)));
 
-    helper.CompleteInstallableCheck(kManifestUrl, manifest, false);
+    helper.CompleteInstallableCheck(kManifestUrl, manifest,
+                                    ForInstallableSite::kNo);
     helper.CompleteIconDownload(true, icon_map);
 
     content::RunAllTasksUntilIdle();
@@ -502,31 +565,6 @@ TEST_F(BookmarkAppHelperExtensionServiceTest,
   }
 }
 
-TEST_F(BookmarkAppHelperExtensionServiceTest,
-       CreateBookmarkAppWithManifestNoScope) {
-  WebApplicationInfo web_app_info;
-
-  std::unique_ptr<content::WebContents> contents(
-      content::WebContentsTester::CreateTestWebContents(profile(), nullptr));
-  TestBookmarkAppHelper helper(service_, web_app_info, contents.get());
-  helper.Create(base::Bind(&TestBookmarkAppHelper::CreationComplete,
-                           base::Unretained(&helper)));
-
-  content::Manifest manifest;
-  manifest.start_url = GURL(kAppUrl);
-  manifest.name = base::NullableString16(base::UTF8ToUTF16(kAppTitle), false);
-  helper.CompleteInstallableCheck(kManifestUrl, manifest, true);
-
-  std::map<GURL, std::vector<SkBitmap>> icon_map;
-  helper.CompleteIconDownload(true, icon_map);
-  content::RunAllTasksUntilIdle();
-  EXPECT_TRUE(helper.extension());
-
-  const Extension* extension =
-      service_->GetInstalledExtension(helper.extension()->id());
-  EXPECT_EQ(GURL(kAppDefaultScope), GetScopeURLFromBookmarkApp(extension));
-}
-
 TEST_F(BookmarkAppHelperExtensionServiceTest,
        CreateBookmarkAppWithoutManifest) {
   WebApplicationInfo web_app_info;
@@ -540,7 +578,8 @@ TEST_F(BookmarkAppHelperExtensionServiceTest,
   helper.Create(base::Bind(&TestBookmarkAppHelper::CreationComplete,
                            base::Unretained(&helper)));
 
-  helper.CompleteInstallableCheck(kManifestUrl, content::Manifest(), false);
+  helper.CompleteInstallableCheck(kManifestUrl, content::Manifest(),
+                                  ForInstallableSite::kNo);
   std::map<GURL, std::vector<SkBitmap>> icon_map;
   helper.CompleteIconDownload(true, icon_map);
   content::RunAllTasksUntilIdle();
@@ -712,7 +751,8 @@ TEST_F(BookmarkAppHelperTest, UpdateWebAppInfoFromManifest) {
   manifest.short_name = base::NullableString16(base::UTF8ToUTF16(kAppShortName),
                                                false);
 
-  BookmarkAppHelper::UpdateWebAppInfoFromManifest(manifest, &web_app_info);
+  BookmarkAppHelper::UpdateWebAppInfoFromManifest(manifest, &web_app_info,
+                                                  ForInstallableSite::kNo);
   EXPECT_EQ(base::UTF8ToUTF16(kAppShortName), web_app_info.title);
   EXPECT_EQ(GURL(kAppUrl), web_app_info.app_url);
 
@@ -731,7 +771,8 @@ TEST_F(BookmarkAppHelperTest, UpdateWebAppInfoFromManifest) {
   icon.src = GURL(kAppIcon3);
   manifest.icons.push_back(icon);
 
-  BookmarkAppHelper::UpdateWebAppInfoFromManifest(manifest, &web_app_info);
+  BookmarkAppHelper::UpdateWebAppInfoFromManifest(
+      manifest, &web_app_info, BookmarkAppHelper::ForInstallableSite::kNo);
   EXPECT_EQ(base::UTF8ToUTF16(kAppTitle), web_app_info.title);
 
   EXPECT_EQ(2u, web_app_info.icons.size());
@@ -739,6 +780,38 @@ TEST_F(BookmarkAppHelperTest, UpdateWebAppInfoFromManifest) {
   EXPECT_EQ(GURL(kAppIcon3), web_app_info.icons[1].url);
 }
 
+// Tests "scope" is only set for installable sites.
+TEST_F(BookmarkAppHelperTest, UpdateWebAppInfoFromManifestInstallableSite) {
+  {
+    content::Manifest manifest;
+    manifest.start_url = GURL(kAppUrl);
+    WebApplicationInfo web_app_info;
+    BookmarkAppHelper::UpdateWebAppInfoFromManifest(
+        manifest, &web_app_info,
+        BookmarkAppHelper::ForInstallableSite::kUnknown);
+    EXPECT_EQ(GURL(), web_app_info.scope);
+  }
+
+  {
+    content::Manifest manifest;
+    manifest.start_url = GURL(kAppUrl);
+    WebApplicationInfo web_app_info;
+    BookmarkAppHelper::UpdateWebAppInfoFromManifest(
+        manifest, &web_app_info, BookmarkAppHelper::ForInstallableSite::kNo);
+    EXPECT_EQ(GURL(), web_app_info.scope);
+  }
+
+  {
+    content::Manifest manifest;
+    manifest.start_url = GURL(kAppUrl);
+    WebApplicationInfo web_app_info;
+    BookmarkAppHelper::UpdateWebAppInfoFromManifest(
+        manifest, &web_app_info, BookmarkAppHelper::ForInstallableSite::kYes);
+
+    EXPECT_NE(GURL(), web_app_info.scope);
+  }
+}
+
 TEST_F(BookmarkAppHelperTest, ConstrainBitmapsToSizes) {
   std::set<int> desired_sizes;
   desired_sizes.insert(16);

chrome/browser/extensions/favicon_downloader.h

@@ -22,7 +22,7 @@ struct FaviconURL;
 }
 
 namespace extensions {
-FORWARD_DECLARE_TEST(BookmarkAppHelperExtensionServiceTest,
+FORWARD_DECLARE_TEST(BookmarkAppHelperExtensionServiceInstallableSiteTest,
                      CreateBookmarkAppWithManifestIcons);
 }
 
@@ -55,7 +55,8 @@ class FaviconDownloader : public content::WebContentsObserver {
 
  private:
   friend class TestFaviconDownloader;
-  FRIEND_TEST_ALL_PREFIXES(extensions::BookmarkAppHelperExtensionServiceTest,
+  FRIEND_TEST_ALL_PREFIXES(
+      extensions::BookmarkAppHelperExtensionServiceInstallableSiteTest,
       CreateBookmarkAppWithManifestIcons);
 
   // Initiates a download of the image at |url| and returns the download id.