dpwas: update installability to account for display_override

Takes into account the display_override manifest value when
determining if the PWA is installable.

For example, if the developer specifies:
  display: "browser"
  display_override: ["standalone"]

This should be installable, but prior to this change, chromium
would only evaluate the display property.

Since unsupported values are ignored when parsing the manifest,
only the first value in the parsed display_override array needs
to be evaluated.

To test the change use: chrome --enable-features=WebAppManifestDisplayOverride
Example websites:
 - (not installable) https://mwjacksonmsft.github.io/pwa/display-override-browser/index.html
 - (installable) https://mwjacksonmsft.github.io/pwa/display-override-custom/index.html

Explainer: https://github.com/WICG/display-override/blob/master/explainer.m
Design document: https://docs.google.com/document/d/1hEmbGVHMN38q1YTaaGccQ-Y5CHr7xIURYPRWXTuvZLo/edit?usp=sharing
I2P: https://groups.google.com/a/chromium.org/d/topic/blink-dev/WvIeZT8uSzw/discussion

Bug: 1109520
Change-Id: Ib3f7465ed36474b1166f136c3cd6655cc34add0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2333541
Commit-Queue: Mike Jackson <mjackson@microsoft.com>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796899}

chrome/browser/installable/installable_logging.cc

@@ -64,6 +64,9 @@ static const char kPreferRelatedApplicationsSupportedOnlyBetaStable[] =
     "channels on Android.";
 static const char kManifestLocationChanged[] =
     "Manifest location changed during fetch";
+static const char kManifestDisplayOverrideNotSupportedMessage[] =
+    "Manifest contains 'display_override' field, and the first supported "
+    "display mode must be one of 'standalone', 'fullscreen', or 'minimal-ui'";
 
 static const char kNotInMainFrameId[] = "not-in-main-frame";
 static const char kNotFromSecureOriginId[] = "not-from-secure-origin";
@@ -96,6 +99,8 @@ static const char kPreferRelatedApplicationsId[] =
 static const char kPreferRelatedApplicationsSupportedOnlyBetaStableId[] =
     "prefer-related-applications-only-beta-stable";
 static const char kManifestLocationChangedId[] = "manifest-location-changed";
+static const char kManifestDisplayOverrideNotSupportedId[] =
+    "manifest-display-override-not-supported";
 
 const std::string& GetMessagePrefix() {
   static base::NoDestructor<std::string> message_prefix(
@@ -200,6 +205,9 @@ std::string GetErrorMessage(InstallableStatusCode code) {
     case MANIFEST_URL_CHANGED:
       message = kManifestLocationChanged;
       break;
+    case MANIFEST_DISPLAY_OVERRIDE_NOT_SUPPORTED:
+      message = kManifestDisplayOverrideNotSupportedMessage;
+      break;
   }
 
   return message;
@@ -305,6 +313,9 @@ content::InstallabilityError GetInstallabilityError(
     case MANIFEST_URL_CHANGED:
       error_id = kManifestLocationChangedId;
       break;
+    case MANIFEST_DISPLAY_OVERRIDE_NOT_SUPPORTED:
+      error_id = kManifestDisplayOverrideNotSupportedId;
+      break;
   }
   error.error_id = error_id;
   error.installability_error_arguments = error_arguments;

chrome/browser/installable/installable_logging.h

@@ -57,6 +57,7 @@ enum InstallableStatusCode {
   PREFER_RELATED_APPLICATIONS = 36,
   PREFER_RELATED_APPLICATIONS_SUPPORTED_ONLY_BETA_STABLE = 37,
   MANIFEST_URL_CHANGED = 38,
+  MANIFEST_DISPLAY_OVERRIDE_NOT_SUPPORTED = 39,
   MAX_ERROR_CODE,
 };
 

chrome/browser/installable/installable_manager.cc

@@ -24,6 +24,7 @@
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/storage_partition.h"
+#include "content/public/common/content_features.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/url_util.h"
@@ -179,6 +180,12 @@ bool DoesManifestContainRequiredIcon(const blink::Manifest& manifest,
   return false;
 }
 
+bool ShouldRejectDisplayMode(blink::mojom::DisplayMode display_mode) {
+  return !(display_mode == blink::mojom::DisplayMode::kStandalone ||
+           display_mode == blink::mojom::DisplayMode::kFullscreen ||
+           display_mode == blink::mojom::DisplayMode::kMinimalUi);
+}
+
 // Returns true if |params| specifies a full PWA check.
 bool IsParamsForPwaCheck(const InstallableParams& params) {
   return params.valid_manifest && params.has_worker &&
@@ -656,12 +663,26 @@ bool InstallableManager::IsManifestValidForWebApp(
     is_valid = false;
   }
 
-  if (check_webapp_manifest_display &&
-      manifest.display != blink::mojom::DisplayMode::kStandalone &&
-      manifest.display != blink::mojom::DisplayMode::kFullscreen &&
-      manifest.display != blink::mojom::DisplayMode::kMinimalUi) {
-    valid_manifest_->errors.push_back(MANIFEST_DISPLAY_NOT_SUPPORTED);
-    is_valid = false;
+  if (check_webapp_manifest_display) {
+    blink::mojom::DisplayMode display_mode_to_evaluate = manifest.display;
+    InstallableStatusCode manifest_error = MANIFEST_DISPLAY_NOT_SUPPORTED;
+
+    if (base::FeatureList::IsEnabled(
+            features::kWebAppManifestDisplayOverride)) {
+      // Unsupported values are ignored when we parse the manifest, and
+      // consequently aren't in the manifest.display_override array.
+      // If this array is not empty, the first value will "win", so validate
+      // this value is installable.
+      if (!manifest.display_override.empty()) {
+        display_mode_to_evaluate = manifest.display_override[0];
+        manifest_error = MANIFEST_DISPLAY_OVERRIDE_NOT_SUPPORTED;
+      }
+    }
+
+    if (ShouldRejectDisplayMode(display_mode_to_evaluate)) {
+      valid_manifest_->errors.push_back(manifest_error);
+      is_valid = false;
+    }
   }
 
   if (!DoesManifestContainRequiredIcon(manifest, prefer_maskable_icon)) {

chrome/browser/installable/installable_manager_browsertest.cc

@@ -12,6 +12,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "chrome/browser/banners/app_banner_manager_desktop.h"
 #include "chrome/browser/installable/installable_logging.h"
@@ -24,6 +25,7 @@
 #include "chrome/common/chrome_features.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/common/content_features.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -211,6 +213,7 @@ class NestedCallbackTester {
     EXPECT_EQ(manifest_.display, data.manifest->display);
     EXPECT_EQ(manifest_.name, data.manifest->name);
     EXPECT_EQ(manifest_.short_name, data.manifest->short_name);
+    EXPECT_EQ(manifest_.display_override, data.manifest->display_override);
 
     quit_closure_.Run();
   }
@@ -1836,3 +1839,103 @@ IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest,
   ASSERT_EQ(tester->errors().size(), 1u);
   EXPECT_EQ(tester->errors()[0], MANIFEST_URL_CHANGED);
 }
+
+// A dedicated test fixture for DisplayOverride, which is supported
+// only for the new web apps mode, and requires a command line switch
+// to enable manifest parsing.
+class InstallableManagerBrowserTest_DisplayOverride
+    : public InstallableManagerBrowserTest {
+ public:
+  InstallableManagerBrowserTest_DisplayOverride() {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kWebAppManifestDisplayOverride);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest_DisplayOverride,
+                       CheckManifestOnly) {
+  base::RunLoop run_loop;
+  std::unique_ptr<CallbackTester> tester(
+      new CallbackTester(run_loop.QuitClosure()));
+
+  NavigateAndRunInstallableManager(
+      browser(), tester.get(), GetManifestParams(),
+      GetURLOfPageWithServiceWorkerAndManifest(
+          "/banners/manifest_display_override.json"));
+  run_loop.Run();
+
+  EXPECT_FALSE(tester->manifest().IsEmpty());
+  EXPECT_FALSE(tester->manifest_url().is_empty());
+  ASSERT_EQ(2u, tester->manifest().display_override.size());
+  EXPECT_EQ(blink::mojom::DisplayMode::kMinimalUi,
+            tester->manifest().display_override[0]);
+  EXPECT_EQ(blink::mojom::DisplayMode::kStandalone,
+            tester->manifest().display_override[1]);
+
+  EXPECT_TRUE(tester->primary_icon_url().is_empty());
+  EXPECT_EQ(nullptr, tester->primary_icon());
+  EXPECT_FALSE(tester->has_maskable_primary_icon());
+  EXPECT_FALSE(tester->valid_manifest());
+  EXPECT_FALSE(tester->has_worker());
+  EXPECT_TRUE(tester->splash_icon_url().is_empty());
+  EXPECT_EQ(nullptr, tester->splash_icon());
+  EXPECT_EQ(std::vector<InstallableStatusCode>{}, tester->errors());
+}
+
+IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest_DisplayOverride,
+                       ManifestDisplayOverrideReportsError) {
+  base::RunLoop run_loop;
+  std::unique_ptr<CallbackTester> tester(
+      new CallbackTester(run_loop.QuitClosure()));
+
+  NavigateAndRunInstallableManager(
+      browser(), tester.get(), GetWebAppParams(),
+      GetURLOfPageWithServiceWorkerAndManifest(
+          "/banners/manifest_display_override_contains_browser.json"));
+  run_loop.Run();
+
+  EXPECT_FALSE(tester->manifest().IsEmpty());
+  EXPECT_FALSE(tester->manifest_url().is_empty());
+  ASSERT_EQ(3u, tester->manifest().display_override.size());
+  EXPECT_EQ(blink::mojom::DisplayMode::kBrowser,
+            tester->manifest().display_override[0]);
+  EXPECT_EQ(blink::mojom::DisplayMode::kMinimalUi,
+            tester->manifest().display_override[1]);
+  EXPECT_EQ(blink::mojom::DisplayMode::kStandalone,
+            tester->manifest().display_override[2]);
+  EXPECT_EQ(
+      std::vector<InstallableStatusCode>{
+          MANIFEST_DISPLAY_OVERRIDE_NOT_SUPPORTED},
+      tester->errors());
+}
+
+IN_PROC_BROWSER_TEST_F(InstallableManagerBrowserTest_DisplayOverride,
+                       FallbackToDisplayBrowser) {
+  base::RunLoop run_loop;
+  std::unique_ptr<CallbackTester> tester(
+      new CallbackTester(run_loop.QuitClosure()));
+
+  NavigateAndRunInstallableManager(
+      browser(), tester.get(), GetWebAppParams(),
+      GetURLOfPageWithServiceWorkerAndManifest(
+          "/banners/manifest_display_override_display_is_browser.json"));
+  run_loop.Run();
+
+  EXPECT_FALSE(tester->manifest().IsEmpty());
+  EXPECT_FALSE(tester->manifest_url().is_empty());
+  ASSERT_EQ(1u, tester->manifest().display_override.size());
+  EXPECT_EQ(blink::mojom::DisplayMode::kStandalone,
+            tester->manifest().display_override[0]);
+
+  EXPECT_FALSE(tester->primary_icon_url().is_empty());
+  EXPECT_NE(nullptr, tester->primary_icon());
+  EXPECT_EQ(144, tester->primary_icon()->width());
+  EXPECT_TRUE(tester->valid_manifest());
+  EXPECT_TRUE(tester->has_worker());
+  EXPECT_TRUE(tester->splash_icon_url().is_empty());
+  EXPECT_EQ(nullptr, tester->splash_icon());
+  EXPECT_EQ(std::vector<InstallableStatusCode>{}, tester->errors());
+}

chrome/browser/installable/installable_manager_unittest.cc

@@ -5,7 +5,9 @@
 #include "chrome/browser/installable/installable_manager.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
+#include "content/public/common/content_features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/manifest/display_mode.mojom.h"
 
@@ -327,3 +329,53 @@ TEST_F(InstallableManagerUnitTest, ManifestDisplayModes) {
   EXPECT_TRUE(IsManifestValid(manifest));
   EXPECT_EQ(NO_ERROR_DETECTED, GetErrorCode());
 }
+
+class InstallableManagerUnitTest_DisplayOverride
+    : public InstallableManagerUnitTest {
+ public:
+  InstallableManagerUnitTest_DisplayOverride() {
+    scoped_feature_list_.InitAndEnableFeature(
+        features::kWebAppManifestDisplayOverride);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+TEST_F(InstallableManagerUnitTest_DisplayOverride, ManifestDisplayOverride) {
+  blink::Manifest manifest = GetValidManifest();
+
+  manifest.display_override.push_back(blink::mojom::DisplayMode::kMinimalUi);
+  EXPECT_TRUE(IsManifestValid(manifest));
+  EXPECT_EQ(NO_ERROR_DETECTED, GetErrorCode());
+
+  manifest.display_override.push_back(blink::mojom::DisplayMode::kBrowser);
+  EXPECT_TRUE(IsManifestValid(manifest));
+  EXPECT_EQ(NO_ERROR_DETECTED, GetErrorCode());
+
+  manifest.display_override.insert(manifest.display_override.begin(),
+                                   blink::mojom::DisplayMode::kStandalone);
+  EXPECT_TRUE(IsManifestValid(manifest));
+  EXPECT_EQ(NO_ERROR_DETECTED, GetErrorCode());
+
+  manifest.display_override.insert(manifest.display_override.begin(),
+                                   blink::mojom::DisplayMode::kStandalone);
+  EXPECT_TRUE(IsManifestValid(manifest));
+  EXPECT_EQ(NO_ERROR_DETECTED, GetErrorCode());
+
+  manifest.display_override.insert(manifest.display_override.begin(),
+                                   blink::mojom::DisplayMode::kBrowser);
+  EXPECT_TRUE(
+      IsManifestValid(manifest, false /* check_webapp_manifest_display */));
+  EXPECT_FALSE(IsManifestValid(manifest));
+  EXPECT_EQ(MANIFEST_DISPLAY_OVERRIDE_NOT_SUPPORTED, GetErrorCode());
+}
+
+TEST_F(InstallableManagerUnitTest_DisplayOverride, FallbackToBrowser) {
+  blink::Manifest manifest = GetValidManifest();
+
+  manifest.display = blink::mojom::DisplayMode::kBrowser;
+  manifest.display_override.push_back(blink::mojom::DisplayMode::kMinimalUi);
+  EXPECT_TRUE(IsManifestValid(manifest));
+  EXPECT_EQ(NO_ERROR_DETECTED, GetErrorCode());
+}

chrome/browser/web_applications/manifest_update_manager_browsertest.cc

@@ -826,11 +826,11 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest_DisplayOverride,
   )";
 
   OverrideManifest(kManifestTemplate,
-                   {R"([ "browser", "standalone" ])", kInstallableIconList});
+                   {R"([ "fullscreen", "standalone" ])", kInstallableIconList});
   AppId app_id = InstallWebApp();
 
   OverrideManifest(kManifestTemplate,
-                   {R"([ "browser", "minimal-ui" ])", kInstallableIconList});
+                   {R"([ "fullscreen", "minimal-ui" ])", kInstallableIconList});
   EXPECT_EQ(GetResultAfterPageLoad(GetAppURL(), &app_id),
             ManifestUpdateResult::kAppUpdated);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,
@@ -841,7 +841,7 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest_DisplayOverride,
       GetProvider().registrar().GetAppDisplayModeOverride(app_id);
 
   ASSERT_EQ(2u, app_display_mode_override.size());
-  EXPECT_EQ(DisplayMode::kBrowser, app_display_mode_override[0]);
+  EXPECT_EQ(DisplayMode::kFullscreen, app_display_mode_override[0]);
   EXPECT_EQ(DisplayMode::kMinimalUi, app_display_mode_override[1]);
 }
 
@@ -989,13 +989,13 @@ IN_PROC_BROWSER_TEST_P(ManifestUpdateManagerBrowserTest_DisplayOverride,
   )";
 
   OverrideManifest(kManifestTemplate,
-                   {R"([ "browser", "fullscreen" ])", kInstallableIconList});
+                   {R"([ "standard", "fullscreen" ])", kInstallableIconList});
   AppId app_id = InstallWebApp();
 
   // display_override contains an additional invalid value
   OverrideManifest(
       kManifestTemplate,
-      {R"([ "invalid", "browser", "fullscreen" ])", kInstallableIconList});
+      {R"([ "invalid", "standard", "fullscreen" ])", kInstallableIconList});
   EXPECT_EQ(GetResultAfterPageLoad(GetAppURL(), &app_id),
             ManifestUpdateResult::kAppUpToDate);
   histogram_tester_.ExpectBucketCount(kUpdateHistogramName,

chrome/test/data/banners/manifest_display_override_contains_browser.json

+{
+  "name": "Manifest test app",
+  "icons": [
+    {
+      "src": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAJAAAACQAQMAAADdiHD7AAAABlBMVEUAAAAAAAClZ7nPAAAACXBIWXMAAA7EAAAOxAGVKw4bAAAAGklEQVRIie3BMQEAAADCoPVPbQ0PoAAA4NcACrAAAcLJlncAAAAASUVORK5CYII=",
+      "type": "image/png",
+      "sizes": "192x192"
+    }
+  ],
+  "start_url": "manifest_test_page.html",
+  "display": "fullscreen",
+  "display_override": [ "browser", "minimal-ui", "standalone" ]
+}

chrome/test/data/banners/manifest_display_override_display_is_browser.json

+{
+  "name": "Manifest test app",
+  "icons": [
+    {
+      "src": "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAJAAAACQAQMAAADdiHD7AAAABlBMVEUAAAAAAAClZ7nPAAAACXBIWXMAAA7EAAAOxAGVKw4bAAAAGklEQVRIie3BMQEAAADCoPVPbQ0PoAAA4NcACrAAAcLJlncAAAAASUVORK5CYII=",
+      "type": "image/png",
+      "sizes": "192x192"
+    }
+  ],
+  "start_url": "manifest_test_page.html",
+  "display": "browser",
+  "display_override": [ "standalone" ]
+}

tools/metrics/histograms/enums.xml

@@ -2401,6 +2401,7 @@ Unknown properties are collapsed to zero. -->
       label="prefer_related_applications is only supported on Chrome Beta
              & Stable on Android"/>
   <int value="38" label="Manifest URL was changed during fetch"/>
+  <int value="39" label="Manifest display_override value not supported"/>
 </enum>
 
 <enum name="AppBannersInstallEvent">