WebApp: Extract UpdateWebAppInfoFromManifest util.

We will reuse it in WebAppInstallManager.

This is a cut-and-paste CL, no behavior change.

Bug: 875698
Change-Id: If126780727852bb0ce53979b027a01b1c00b58bc
Reviewed-on: https://chromium-review.googlesource.com/c/1314016Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605232}

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/components/web_app_install_utils.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"
@@ -59,6 +60,7 @@
 #include "extensions/common/url_pattern.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_request.h"
+#include "third_party/blink/public/common/manifest/manifest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 #if defined(OS_MACOSX)
@@ -246,48 +248,6 @@ class BookmarkAppInstaller : public base::RefCounted<BookmarkAppInstaller>,
 
 }  // namespace
 
-// static
-void BookmarkAppHelper::UpdateWebAppInfoFromManifest(
-    const blink::Manifest& manifest,
-    WebApplicationInfo* web_app_info,
-    ForInstallableSite for_installable_site) {
-  if (!manifest.short_name.is_null())
-    web_app_info->title = manifest.short_name.string();
-
-  // Give the full length name priority.
-  if (!manifest.name.is_null())
-    web_app_info->title = manifest.name.string();
-
-  // Set the url based on the manifest value, if any.
-  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
-    if (!manifest.scope.is_empty())
-      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)
-    web_app_info->theme_color = *manifest.theme_color;
-
-  // If any icons are specified in the manifest, they take precedence over any
-  // we picked up from the web_app stuff.
-  if (!manifest.icons.empty()) {
-    web_app_info->icons.clear();
-    for (const auto& icon : manifest.icons) {
-      // TODO(benwells): Take the declared icon density and sizes into account.
-      WebApplicationInfo::IconInfo info;
-      info.url = icon.src;
-      web_app_info->icons.push_back(info);
-    }
-  }
-}
-
 // static
 void BookmarkAppHelper::UpdateWebAppIconsWithoutChangingLinks(
     std::map<int, web_app::BitmapAndSource> bitmap_map,
@@ -314,7 +274,6 @@ void BookmarkAppHelper::UpdateWebAppIconsWithoutChangingLinks(
   }
 }
 
-
 BookmarkAppHelper::BookmarkAppHelper(Profile* profile,
                                      WebApplicationInfo web_app_info,
                                      content::WebContents* contents,
@@ -324,6 +283,7 @@ BookmarkAppHelper::BookmarkAppHelper(Profile* profile,
       web_app_info_(web_app_info),
       crx_installer_(CrxInstaller::CreateSilent(
           ExtensionSystem::Get(profile)->extension_service())),
+      for_installable_site_(web_app::ForInstallableSite::kUnknown),
       install_source_(install_source),
       weak_factory_(this) {
   if (contents)
@@ -376,7 +336,7 @@ void BookmarkAppHelper::Create(const CreateBookmarkAppCallback& callback) {
                              weak_factory_.GetWeakPtr()));
     }
   } else {
-    for_installable_site_ = ForInstallableSite::kNo;
+    for_installable_site_ = web_app::ForInstallableSite::kNo;
     OnIconsDownloaded(true, std::map<GURL, std::vector<SkBitmap>>());
   }
 }
@@ -397,11 +357,11 @@ void BookmarkAppHelper::OnDidPerformInstallableCheck(
 
   for_installable_site_ =
       data.error_code == NO_ERROR_DETECTED && !shortcut_app_requested_
-          ? ForInstallableSite::kYes
-          : ForInstallableSite::kNo;
+          ? web_app::ForInstallableSite::kYes
+          : web_app::ForInstallableSite::kNo;
 
-  UpdateWebAppInfoFromManifest(*data.manifest, &web_app_info_,
-                               for_installable_site_);
+  web_app::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.
@@ -506,7 +466,7 @@ void BookmarkAppHelper::OnIconsDownloaded(
   }
 
   if (base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing) &&
-      for_installable_site_ == ForInstallableSite::kYes) {
+      for_installable_site_ == web_app::ForInstallableSite::kYes) {
     web_app_info_.open_as_window = true;
     chrome::ShowPWAInstallDialog(
         contents_, web_app_info_,
@@ -548,7 +508,7 @@ void BookmarkAppHelper::OnBubbleCompleted(
     crx_installer_->InstallWebApp(web_app_info_);
 
     if (InstallableMetrics::IsReportableInstallSource(install_source_) &&
-        for_installable_site_ == ForInstallableSite::kYes) {
+        for_installable_site_ == web_app::ForInstallableSite::kYes) {
       InstallableMetrics::TrackInstallEvent(install_source_);
     }
   } else {
@@ -563,8 +523,8 @@ void BookmarkAppHelper::FinishInstallation(const Extension* extension) {
       web_app_info_.open_as_window ? LAUNCH_TYPE_WINDOW : LAUNCH_TYPE_REGULAR;
 
   if (base::FeatureList::IsEnabled(::features::kDesktopPWAWindowing)) {
-    DCHECK_NE(ForInstallableSite::kUnknown, for_installable_site_);
-    launch_type = for_installable_site_ == ForInstallableSite::kYes
+    DCHECK_NE(web_app::ForInstallableSite::kUnknown, for_installable_site_);
+    launch_type = for_installable_site_ == web_app::ForInstallableSite::kYes
                       ? LAUNCH_TYPE_WINDOW
                       : LAUNCH_TYPE_REGULAR;
   }

chrome/browser/extensions/bookmark_app_helper.h

@@ -21,7 +21,6 @@
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
-#include "third_party/blink/public/common/manifest/manifest.h"
 
 class WebAppIconDownloader;
 struct InstallableData;
@@ -33,6 +32,10 @@ namespace content {
 class WebContents;
 }  // namespace content
 
+namespace web_app {
+enum class ForInstallableSite;
+}  // namespace web_app
+
 namespace extensions {
 class CrxInstaller;
 class Extension;
@@ -41,12 +44,6 @@ class ExtensionService;
 // A helper class for creating bookmark apps from a WebContents.
 class BookmarkAppHelper : public content::NotificationObserver {
  public:
-  enum class ForInstallableSite {
-    kYes,
-    kNo,
-    kUnknown,
-  };
-
   typedef base::Callback<void(const Extension*, const WebApplicationInfo&)>
       CreateBookmarkAppCallback;
 
@@ -63,11 +60,6 @@ class BookmarkAppHelper : public content::NotificationObserver {
                     WebappInstallSource install_source);
   ~BookmarkAppHelper() override;
 
-  // Update the given WebApplicationInfo with information from the manifest.
-  static void UpdateWebAppInfoFromManifest(const blink::Manifest& manifest,
-                                           WebApplicationInfo* web_app_info,
-                                           ForInstallableSite installable_site);
-
   // 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
@@ -181,7 +173,7 @@ class BookmarkAppHelper : public content::NotificationObserver {
 
   InstallableManager* installable_manager_;
 
-  ForInstallableSite for_installable_site_ = ForInstallableSite::kUnknown;
+  web_app::ForInstallableSite for_installable_site_;
 
   base::Optional<LaunchType> forced_launch_type_;
 

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/components/web_app_install_utils.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"
@@ -38,21 +39,16 @@ namespace extensions {
 
 namespace {
 
-using ForInstallableSite = BookmarkAppHelper::ForInstallableSite;
+using ForInstallableSite = web_app::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";
 const char kAppScope[] = "http://www.chromium.org/scope/";
 const char kAppAlternativeScope[] = "http://www.chromium.org/new/";
 const char kAppDefaultScope[] = "http://www.chromium.org/";
 const char kAppTitle[] = "Test title";
-const char kAppShortName[] = "Test short name";
 const char kAlternativeAppTitle[] = "Different test title";
 const char kAppDescription[] = "Test description";
-const char kAppIcon1[] = "fav1.png";
-const char kAppIcon2[] = "fav2.png";
-const char kAppIcon3[] = "fav3.png";
 const char kAppIconURL1[] = "http://foo.com/1.png";
 const char kAppIconURL2[] = "http://foo.com/2.png";
 
@@ -626,80 +622,6 @@ TEST_F(BookmarkAppHelperExtensionServiceTest, CreateAndUpdateBookmarkApp) {
   }
 }
 
-TEST_F(BookmarkAppHelperTest, UpdateWebAppInfoFromManifest) {
-  WebApplicationInfo web_app_info;
-  web_app_info.title = base::UTF8ToUTF16(kAlternativeAppTitle);
-  web_app_info.app_url = GURL(kAlternativeAppUrl);
-  WebApplicationInfo::IconInfo info;
-  info.url = GURL(kAppIcon1);
-  web_app_info.icons.push_back(info);
-
-  blink::Manifest manifest;
-  manifest.start_url = GURL(kAppUrl);
-  manifest.short_name = base::NullableString16(base::UTF8ToUTF16(kAppShortName),
-                                               false);
-
-  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);
-
-  // The icon info from |web_app_info| should be left as is, since the manifest
-  // doesn't have any icon information.
-  EXPECT_EQ(1u, web_app_info.icons.size());
-  EXPECT_EQ(GURL(kAppIcon1), web_app_info.icons[0].url);
-
-  // Test that |manifest.name| takes priority over |manifest.short_name|, and
-  // that icons provided by the manifest replace icons in |web_app_info|.
-  manifest.name = base::NullableString16(base::UTF8ToUTF16(kAppTitle), false);
-
-  blink::Manifest::ImageResource icon;
-  icon.src = GURL(kAppIcon2);
-  manifest.icons.push_back(icon);
-  icon.src = GURL(kAppIcon3);
-  manifest.icons.push_back(icon);
-
-  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());
-  EXPECT_EQ(GURL(kAppIcon2), web_app_info.icons[0].url);
-  EXPECT_EQ(GURL(kAppIcon3), web_app_info.icons[1].url);
-}
-
-// Tests "scope" is only set for installable sites.
-TEST_F(BookmarkAppHelperTest, UpdateWebAppInfoFromManifestInstallableSite) {
-  {
-    blink::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);
-  }
-
-  {
-    blink::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);
-  }
-
-  {
-    blink::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, IsValidBookmarkAppUrl) {
   EXPECT_TRUE(IsValidBookmarkAppUrl(GURL("https://chromium.org")));
   EXPECT_TRUE(IsValidBookmarkAppUrl(GURL("https://www.chromium.org")));

chrome/browser/web_applications/components/BUILD.gn

@@ -14,6 +14,8 @@ source_set("components") {
     "web_app_helpers.h",
     "web_app_icon_generator.cc",
     "web_app_icon_generator.h",
+    "web_app_install_utils.cc",
+    "web_app_install_utils.h",
     "web_app_shortcut.cc",
     "web_app_shortcut.h",
     "web_app_shortcut_chromeos.cc",
@@ -78,6 +80,7 @@ source_set("unit_tests") {
     "web_app_helpers_unittest.cc",
     "web_app_icon_downloader_unittest.cc",
     "web_app_icon_generator_unittest.cc",
+    "web_app_install_utils_unittest.cc",
     "web_app_shortcut_mac_unittest.mm",
     "web_app_shortcut_unittest.cc",
   ]

chrome/browser/web_applications/components/web_app_install_utils.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/components/web_app_install_utils.h"
+
+#include "chrome/common/web_application_info.h"
+#include "third_party/blink/public/common/manifest/manifest.h"
+
+namespace web_app {
+
+void UpdateWebAppInfoFromManifest(const blink::Manifest& manifest,
+                                  WebApplicationInfo* web_app_info,
+                                  ForInstallableSite for_installable_site) {
+  if (!manifest.short_name.is_null())
+    web_app_info->title = manifest.short_name.string();
+
+  // Give the full length name priority.
+  if (!manifest.name.is_null())
+    web_app_info->title = manifest.name.string();
+
+  // Set the url based on the manifest value, if any.
+  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
+    if (!manifest.scope.is_empty())
+      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)
+    web_app_info->theme_color = *manifest.theme_color;
+
+  // If any icons are specified in the manifest, they take precedence over any
+  // we picked up from the web_app stuff.
+  if (!manifest.icons.empty()) {
+    web_app_info->icons.clear();
+    for (const auto& icon : manifest.icons) {
+      // TODO(benwells): Take the declared icon density and sizes into account.
+      WebApplicationInfo::IconInfo info;
+      info.url = icon.src;
+      web_app_info->icons.push_back(info);
+    }
+  }
+}
+
+}  // namespace web_app

chrome/browser/web_applications/components/web_app_install_utils.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_COMPONENTS_WEB_APP_INSTALL_UTILS_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_WEB_APP_INSTALL_UTILS_H_
+
+struct WebApplicationInfo;
+
+namespace blink {
+struct Manifest;
+}
+
+namespace web_app {
+
+enum class ForInstallableSite {
+  kYes,
+  kNo,
+  kUnknown,
+};
+
+// Update the given WebApplicationInfo with information from the manifest.
+void UpdateWebAppInfoFromManifest(const blink::Manifest& manifest,
+                                  WebApplicationInfo* web_app_info,
+                                  ForInstallableSite installable_site);
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_COMPONENTS_WEB_APP_INSTALL_UTILS_H_

chrome/browser/web_applications/components/web_app_install_utils_unittest.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/components/web_app_install_utils.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/common/web_application_info.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/manifest/manifest.h"
+
+namespace web_app {
+
+namespace {
+const char kAppIcon1[] = "fav1.png";
+const char kAppIcon2[] = "fav2.png";
+const char kAppIcon3[] = "fav3.png";
+const char kAppShortName[] = "Test short name";
+const char kAppTitle[] = "Test title";
+const char kAppUrl[] = "http://www.chromium.org/index.html";
+const char kAlternativeAppUrl[] = "http://www.notchromium.org";
+const char kAlternativeAppTitle[] = "Different test title";
+}  // namespace
+
+TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifest) {
+  WebApplicationInfo web_app_info;
+  web_app_info.title = base::UTF8ToUTF16(kAlternativeAppTitle);
+  web_app_info.app_url = GURL(kAlternativeAppUrl);
+  WebApplicationInfo::IconInfo info;
+  info.url = GURL(kAppIcon1);
+  web_app_info.icons.push_back(info);
+
+  blink::Manifest manifest;
+  manifest.start_url = GURL(kAppUrl);
+  manifest.short_name =
+      base::NullableString16(base::UTF8ToUTF16(kAppShortName), false);
+
+  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);
+
+  // The icon info from |web_app_info| should be left as is, since the manifest
+  // doesn't have any icon information.
+  EXPECT_EQ(1u, web_app_info.icons.size());
+  EXPECT_EQ(GURL(kAppIcon1), web_app_info.icons[0].url);
+
+  // Test that |manifest.name| takes priority over |manifest.short_name|, and
+  // that icons provided by the manifest replace icons in |web_app_info|.
+  manifest.name = base::NullableString16(base::UTF8ToUTF16(kAppTitle), false);
+
+  blink::Manifest::ImageResource icon;
+  icon.src = GURL(kAppIcon2);
+  manifest.icons.push_back(icon);
+  icon.src = GURL(kAppIcon3);
+  manifest.icons.push_back(icon);
+
+  UpdateWebAppInfoFromManifest(manifest, &web_app_info,
+                               ForInstallableSite::kNo);
+  EXPECT_EQ(base::UTF8ToUTF16(kAppTitle), web_app_info.title);
+
+  EXPECT_EQ(2u, web_app_info.icons.size());
+  EXPECT_EQ(GURL(kAppIcon2), web_app_info.icons[0].url);
+  EXPECT_EQ(GURL(kAppIcon3), web_app_info.icons[1].url);
+}
+
+// Tests "scope" is only set for installable sites.
+TEST(WebAppInstallUtils, UpdateWebAppInfoFromManifestInstallableSite) {
+  {
+    blink::Manifest manifest;
+    manifest.start_url = GURL(kAppUrl);
+    WebApplicationInfo web_app_info;
+    UpdateWebAppInfoFromManifest(manifest, &web_app_info,
+                                 ForInstallableSite::kUnknown);
+    EXPECT_EQ(GURL(), web_app_info.scope);
+  }
+
+  {
+    blink::Manifest manifest;
+    manifest.start_url = GURL(kAppUrl);
+    WebApplicationInfo web_app_info;
+    UpdateWebAppInfoFromManifest(manifest, &web_app_info,
+                                 ForInstallableSite::kNo);
+    EXPECT_EQ(GURL(), web_app_info.scope);
+  }
+
+  {
+    blink::Manifest manifest;
+    manifest.start_url = GURL(kAppUrl);
+    WebApplicationInfo web_app_info;
+    UpdateWebAppInfoFromManifest(manifest, &web_app_info,
+                                 ForInstallableSite::kYes);
+
+    EXPECT_NE(GURL(), web_app_info.scope);
+  }
+}
+
+}  // namespace web_app