Enable PWA install from 3-dot menu even if site prefers related apps

This CL creates a new "installable" state in AppBannerManager to distinguish
whether Chrome provides the option to install (kByUserRequest) or whether
Chrome promotes installation for the site (kPromotable).

Before this CL we disabled PWA installation entirely if the site indicated
it preferred a related app in its manifest. Now we just use it as a signal
to avoid promoting install to the user but still provide them the option
in the 3-dot menu.

Bug: 964314, 966831
Change-Id: Ic68d21bbca021c4fad45f8225e097b1df3a46b97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1630128
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664072}

chrome/browser/banners/app_banner_manager.cc

@@ -346,18 +346,31 @@ void AppBannerManager::OnDidPerformInstallableWebAppCheck(
     TrackDisplayEvent(DISPLAY_EVENT_WEB_APP_BANNER_REQUESTED);
 
   auto error = data.errors.empty() ? NO_ERROR_DETECTED : data.errors[0];
-  SetInstallableWebAppCheckResult(error == NO_ERROR_DETECTED
-                                      ? InstallableWebAppCheckResult::kYes
-                                      : InstallableWebAppCheckResult::kNo);
-
   if (error != NO_ERROR_DETECTED) {
     if (error == NO_MATCHING_SERVICE_WORKER)
       TrackDisplayEvent(DISPLAY_EVENT_LACKS_SERVICE_WORKER);
 
+    SetInstallableWebAppCheckResult(InstallableWebAppCheckResult::kNo);
     Stop(error);
     return;
   }
 
+  if (CheckIfInstalled()) {
+    banners::TrackDisplayEvent(banners::DISPLAY_EVENT_INSTALLED_PREVIOUSLY);
+    SetInstallableWebAppCheckResult(InstallableWebAppCheckResult::kNo);
+    Stop(ALREADY_INSTALLED);
+    return;
+  }
+
+  if (manifest_.prefer_related_applications) {
+    SetInstallableWebAppCheckResult(
+        InstallableWebAppCheckResult::kByUserRequest);
+    Stop(PREFER_RELATED_APPLICATIONS);
+    return;
+  }
+
+  SetInstallableWebAppCheckResult(InstallableWebAppCheckResult::kPromotable);
+
   DCHECK(data.has_worker && data.valid_manifest);
   DCHECK(!data.primary_icon_url.is_empty());
   DCHECK(data.primary_icon);
@@ -365,11 +378,6 @@ void AppBannerManager::OnDidPerformInstallableWebAppCheck(
   primary_icon_url_ = data.primary_icon_url;
   primary_icon_ = *data.primary_icon;
 
-  if (CheckIfInstalled()) {
-    banners::TrackDisplayEvent(banners::DISPLAY_EVENT_INSTALLED_PREVIOUSLY);
-    Stop(ALREADY_INSTALLED);
-    return;
-  }
 
   // If we triggered the installability check on page load, then it's possible
   // we don't have enough engagement yet. If that's the case, return here but
@@ -449,11 +457,6 @@ InstallableStatusCode AppBannerManager::TerminationCode() const {
   return NO_ERROR_DETECTED;
 }
 
-bool AppBannerManager::IsInstallableWebApp() const {
-  return installable_web_app_check_result_ ==
-         InstallableWebAppCheckResult::kYes;
-}
-
 void AppBannerManager::SetInstallableWebAppCheckResult(
     InstallableWebAppCheckResult result) {
   if (installable_web_app_check_result_ == result)
@@ -464,15 +467,16 @@ void AppBannerManager::SetInstallableWebAppCheckResult(
   switch (result) {
     case InstallableWebAppCheckResult::kUnknown:
       break;
-    case InstallableWebAppCheckResult::kYes:
-      last_installable_web_app_scope_ = manifest_.scope;
-      DCHECK(!last_installable_web_app_scope_.is_empty());
+    case InstallableWebAppCheckResult::kPromotable:
+      last_promotable_web_app_scope_ = manifest_.scope;
+      DCHECK(!last_promotable_web_app_scope_.is_empty());
       install_animation_pending_ =
           AppBannerSettingsHelper::CanShowInstallTextAnimation(
-              web_contents(), last_installable_web_app_scope_);
+              web_contents(), last_promotable_web_app_scope_);
       break;
+    case InstallableWebAppCheckResult::kByUserRequest:
     case InstallableWebAppCheckResult::kNo:
-      last_installable_web_app_scope_ = GURL();
+      last_promotable_web_app_scope_ = GURL();
       install_animation_pending_ = false;
       break;
   }
@@ -641,28 +645,39 @@ bool AppBannerManager::IsRunning() const {
 base::string16 AppBannerManager::GetInstallableWebAppName(
     content::WebContents* web_contents) {
   AppBannerManager* manager = FromWebContents(web_contents);
-  if (!manager || !manager->IsInstallableWebApp())
+  if (!manager)
     return base::string16();
-  return manager->GetAppName();
+  switch (manager->installable_web_app_check_result_) {
+    case InstallableWebAppCheckResult::kUnknown:
+    case InstallableWebAppCheckResult::kNo:
+      return base::string16();
+    case InstallableWebAppCheckResult::kByUserRequest:
+    case InstallableWebAppCheckResult::kPromotable:
+      return manager->GetAppName();
+  }
 }
 
-bool AppBannerManager::IsProbablyInstallableWebApp() const {
-  if (IsInstallableWebApp())
-    return true;
-  return installable_web_app_check_result_ ==
-             InstallableWebAppCheckResult::kUnknown &&
-         last_installable_web_app_scope_.is_valid() &&
-         base::StartsWith(web_contents()->GetLastCommittedURL().spec(),
-                          last_installable_web_app_scope_.spec(),
-                          base::CompareCase::SENSITIVE);
+bool AppBannerManager::IsProbablyPromotableWebApp() const {
+  switch (installable_web_app_check_result_) {
+    case InstallableWebAppCheckResult::kUnknown:
+      return last_promotable_web_app_scope_.is_valid() &&
+             base::StartsWith(web_contents()->GetLastCommittedURL().spec(),
+                              last_promotable_web_app_scope_.spec(),
+                              base::CompareCase::SENSITIVE);
+    case InstallableWebAppCheckResult::kNo:
+    case InstallableWebAppCheckResult::kByUserRequest:
+      return false;
+    case InstallableWebAppCheckResult::kPromotable:
+      return true;
+  }
 }
 
 bool AppBannerManager::MaybeConsumeInstallAnimation() {
-  DCHECK(IsProbablyInstallableWebApp());
+  DCHECK(IsProbablyPromotableWebApp());
   if (!install_animation_pending_)
     return false;
   AppBannerSettingsHelper::RecordInstallTextAnimationShown(
-      web_contents(), last_installable_web_app_scope_);
+      web_contents(), last_promotable_web_app_scope_);
   install_animation_pending_ = false;
   return true;
 }

chrome/browser/banners/app_banner_manager.h

@@ -105,9 +105,14 @@ class AppBannerManager : public content::WebContentsObserver,
     COMPLETE,
   };
 
-  // Installable describes whether a site satisifes the installablity
+  // Installable describes to what degree a site satisifes the installablity
   // requirements.
-  enum class InstallableWebAppCheckResult { kUnknown, kNo, kYes };
+  enum class InstallableWebAppCheckResult {
+    kUnknown,
+    kNo,
+    kByUserRequest,
+    kPromotable,
+  };
 
   // Retrieves the platform specific instance of AppBannerManager from
   // |web_contents|.
@@ -131,10 +136,10 @@ class AppBannerManager : public content::WebContentsObserver,
   static base::string16 GetInstallableWebAppName(
       content::WebContents* web_contents);
 
-  // Returns whether installability checks have passed (e.g. having a service
-  // worker fetch event) or have passed previously within the current manifest
-  // scope.
-  bool IsProbablyInstallableWebApp() const;
+  // Returns whether installability checks satisfy promotion requirements
+  // (e.g. having a service worker fetch event) or have passed previously within
+  // the current manifest scope.
+  bool IsProbablyPromotableWebApp() const;
 
   // Each successful installability check gets to show one animation prompt,
   // this returns and consumes the animation prompt if it is available.
@@ -167,9 +172,7 @@ class AppBannerManager : public content::WebContentsObserver,
   // GetAppIdentifier() must return a valid value for this method to work.
   bool CheckIfShouldShowBanner();
 
-  // Called when the current site is eligible to show a banner. Returns true if
-  // the banner should not be shown because the site is already installed, and
-  // false if the banner should be shown because the site is not yet installed.
+  // Returns whether the site is already installed as a web app.
   bool CheckIfInstalled();
 
   // Return a string identifying this app for metrics.
@@ -337,7 +340,6 @@ class AppBannerManager : public content::WebContentsObserver,
   // Returns a status code based on the current state, to log when terminating.
   InstallableStatusCode TerminationCode() const;
 
-  bool IsInstallableWebApp() const;
   void SetInstallableWebAppCheckResult(InstallableWebAppCheckResult result);
 
   // Fetches the data required to display a banner for the current page.
@@ -361,9 +363,9 @@ class AppBannerManager : public content::WebContentsObserver,
   bool install_animation_pending_;
   InstallableWebAppCheckResult installable_web_app_check_result_;
 
-  // The scope of the most recent installability check if successful otherwise
-  // invalid.
-  GURL last_installable_web_app_scope_;
+  // The scope of the most recent installability check that passes promotability
+  // requirements, otherwise invalid.
+  GURL last_promotable_web_app_scope_;
 
   base::ObserverList<Observer, true> observer_list_;
 

chrome/browser/banners/app_banner_manager_browsertest.cc

@@ -468,4 +468,20 @@ IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, WebAppBannerReprompt) {
                                 SHOWING_WEB_APP_BANNER, 1);
 }
 
+IN_PROC_BROWSER_TEST_F(AppBannerManagerBrowserTest, PreferRelatedApplications) {
+  std::unique_ptr<AppBannerManagerTest> manager(
+      CreateAppBannerManager(browser()));
+  base::HistogramTester histograms;
+
+  GURL test_url = embedded_test_server()->GetURL(
+      "/banners/manifest_test_page.html?manifest="
+      "manifest_prefer_related_apps_empty.json");
+  TriggerBannerFlowWithNavigation(browser(), manager.get(), test_url,
+                                  false /* expected_will_show */,
+                                  State::COMPLETE);
+
+  histograms.ExpectUniqueSample(banners::kInstallableStatusCodeHistogram,
+                                PREFER_RELATED_APPLICATIONS, 1);
+}
+
 }  // namespace banners

chrome/browser/installable/installable_manager.cc

@@ -577,11 +577,6 @@ bool InstallableManager::IsManifestValidForWebApp(
     return false;
   }
 
-  if (manifest.prefer_related_applications) {
-    is_valid = false;
-    valid_manifest_->errors.push_back(PREFER_RELATED_APPLICATIONS);
-  }
-
   if (!manifest.start_url.is_valid()) {
     valid_manifest_->errors.push_back(START_URL_NOT_VALID);
     is_valid = false;

chrome/browser/installable/installable_manager_browsertest.cc

@@ -634,8 +634,7 @@ IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
     EXPECT_FALSE(tester->has_worker());
     EXPECT_EQ(
         std::vector<InstallableStatusCode>(
-            {PREFER_RELATED_APPLICATIONS, START_URL_NOT_VALID,
-             MANIFEST_MISSING_NAME_OR_SHORT_NAME,
+            {START_URL_NOT_VALID, MANIFEST_MISSING_NAME_OR_SHORT_NAME,
              MANIFEST_DISPLAY_NOT_SUPPORTED, MANIFEST_MISSING_SUITABLE_ICON}),
         tester->errors());
   }
@@ -1629,8 +1628,7 @@ IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
   run_loop.Run();
 
   EXPECT_EQ(std::vector<InstallableStatusCode>(
-                {PREFER_RELATED_APPLICATIONS, START_URL_NOT_VALID,
-                 MANIFEST_MISSING_NAME_OR_SHORT_NAME,
+                {START_URL_NOT_VALID, MANIFEST_MISSING_NAME_OR_SHORT_NAME,
                  MANIFEST_DISPLAY_NOT_SUPPORTED, MANIFEST_MISSING_SUITABLE_ICON,
                  NO_URL_FOR_SERVICE_WORKER, NO_ACCEPTABLE_ICON}),
             tester->errors());
@@ -1652,8 +1650,7 @@ IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
 IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
                        GetAllErrorsWithPlayAppManifest) {
   EXPECT_EQ(std::vector<std::string>(
-                {GetErrorMessage(PREFER_RELATED_APPLICATIONS),
-                 GetErrorMessage(START_URL_NOT_VALID),
+                {GetErrorMessage(START_URL_NOT_VALID),
                  GetErrorMessage(MANIFEST_MISSING_NAME_OR_SHORT_NAME),
                  GetErrorMessage(MANIFEST_DISPLAY_NOT_SUPPORTED),
                  GetErrorMessage(MANIFEST_MISSING_SUITABLE_ICON),

chrome/browser/installable/installable_manager_unittest.cc

@@ -213,11 +213,3 @@ TEST_F(InstallableManagerUnitTest, ManifestDisplayModes) {
   EXPECT_TRUE(IsManifestValid(manifest));
   EXPECT_EQ(NO_ERROR_DETECTED, GetErrorCode());
 }
-
-TEST_F(InstallableManagerUnitTest, PreferRelatedApplications) {
-  blink::Manifest manifest = GetValidManifest();
-
-  manifest.prefer_related_applications = true;
-  EXPECT_FALSE(IsManifestValid(manifest));
-  EXPECT_EQ(PREFER_RELATED_APPLICATIONS, GetErrorCode());
-}

chrome/browser/ui/views/page_action/pwa_install_view.cc

@@ -35,12 +35,12 @@ bool PwaInstallView::Update() {
   if (!manager)
     return false;
 
-  bool is_probably_installable = manager->IsProbablyInstallableWebApp();
+  bool is_probably_promotable = manager->IsProbablyPromotableWebApp();
   auto* tab_helper =
       web_app::WebAppTabHelperBase::FromWebContents(web_contents);
   bool is_installed = tab_helper && tab_helper->HasAssociatedApp();
 
-  bool show_install_button = is_probably_installable && !is_installed;
+  bool show_install_button = is_probably_promotable && !is_installed;
 
   if (show_install_button && manager->MaybeConsumeInstallAnimation())
     AnimateIn(base::nullopt);

chrome/browser/ui/views/page_action/pwa_install_view_browsertest.cc

@@ -326,3 +326,20 @@ IN_PROC_BROWSER_TEST_F(PwaInstallViewBrowserTest, BouncedInstallMeasured) {
 IN_PROC_BROWSER_TEST_F(PwaInstallViewBrowserTest, BouncedInstallIgnored) {
   TestInstallBounce(base::TimeDelta::FromMinutes(70), 0);
 }
+
+// Omnibox install button shouldn't show for a PWA that prefers a related app
+// over the web app.
+IN_PROC_BROWSER_TEST_F(PwaInstallViewBrowserTest,
+                       SuppressForPreferredRelatedApp) {
+  NavigateToURL(
+      https_server_.GetURL("/banners/manifest_test_page.html?manifest="
+                           "manifest_prefer_related_apps_empty.json"));
+  ASSERT_TRUE(app_banner_manager_->WaitForInstallableCheck());
+  EXPECT_FALSE(pwa_install_view_->GetVisible());
+
+  // Site should still be considered installable even if it's not promoted in
+  // the omnibox.
+  EXPECT_TRUE(base::EqualsASCII(
+      banners::AppBannerManager::GetInstallableWebAppName(web_contents_),
+      "Manifest prefer related apps empty"));
+}

chrome/test/data/banners/manifest_prefer_related_apps_empty.json

+{
+  "name": "Manifest prefer related apps empty",
+  "icons": [
+    {
+      "src": "/banners/image-512px.png",
+      "sizes": "512x512",
+      "type": "image/png"
+    }
+  ],
+  "scope": ".",
+  "start_url": ".",
+  "display": "standalone",
+  "prefer_related_applications": true
+}