DPWA: treat empty manifest id as missing

The change was requested from spec editor review w3c/manifest#988 (review). This makes manifest parsing to treat specifying empty string for id field same as it's not specified. Updated |ParseURL| to add |ignore_empty_string| param. This is needed because we currently parse url fields inconsistently. Bug: 1182363 Change-Id: Ie5f6d331da7075f7c919f6e2120ea3840fc650f6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3177726 Commit-Queue: Phillis Tang <[email protected]> Reviewed-by: Daniel Murphy <[email protected]> Cr-Commit-Position: refs/heads/main@{#924541} NOKEYCHECK=True GitOrigin-RevId: 6fc55b5ee6ccec48d2e0ee47fb0e4bed0e1f8040
mjfroman · Sep 24, 2021 · bed471a · bed471a
1 parent cfedd0d
commit bed471a
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 4 deletions.
diff --git a/blink/renderer/modules/manifest/manifest_parser.cc b/blink/renderer/modules/manifest/manifest_parser.cc
@@ -272,10 +272,13 @@ absl::optional<RGBA32> ManifestParser::ParseColor(const JSONObject* object,
 KURL ManifestParser::ParseURL(const JSONObject* object,
                               const String& key,
                               const KURL& base_url,
-                              ParseURLRestrictions origin_restriction) {
+                              ParseURLRestrictions origin_restriction,
+                              bool ignore_empty_string) {
   absl::optional<String> url_str = ParseString(object, key, NoTrim);
   if (!url_str.has_value())
     return KURL();
+  if (ignore_empty_string && url_str.value() == "")
+    return KURL();
 
   KURL resolved = KURL(base_url, *url_str);
   if (!resolved.IsValid()) {
@@ -395,7 +398,8 @@ String ManifestParser::ParseId(const JSONObject* object,
   KURL start_url_origin = KURL(SecurityOrigin::Create(start_url)->ToString());
 
   KURL id = ParseURL(object, "id", start_url_origin,
-                     ParseURLRestrictions::kSameOriginOnly);
+                     ParseURLRestrictions::kSameOriginOnly,
+                     /*ignore_empty_string=*/true);
   if (id.IsValid()) {
     ManifestUmaUtil::ParseIdResult(
         ManifestUmaUtil::ParseIdResultType::kSucceed);

diff --git a/blink/renderer/modules/manifest/manifest_parser.h b/blink/renderer/modules/manifest/manifest_parser.h
@@ -99,10 +99,14 @@ class MODULES_EXPORT ManifestParser {
   // enforce matching of the document's and parsed URL's origins.
   // Returns a KURL. If the parsing failed or origin matching was enforced but
   // not present, the returned KURL will be empty.
+  // |ignore_empty_string| treats empty string as the field missing.
+  // TODO(crbug.com/1223173): remove ignore_empty_string when all url fields are
+  // parsed the same.
   KURL ParseURL(const JSONObject* object,
                 const String& key,
                 const KURL& base_url,
-                ParseURLRestrictions origin_restriction);
+                ParseURLRestrictions origin_restriction,
+                bool ignore_empty_string = false);
 
   // Helper function to parse "enum" fields that accept a single value or a
   // list of values to allow sites to be backwards compatible with browsers that

diff --git a/blink/renderer/modules/manifest/manifest_parser_unittest.cc b/blink/renderer/modules/manifest/manifest_parser_unittest.cc
@@ -302,7 +302,7 @@ TEST_F(ManifestParserTest, IdParseRules) {
     auto& manifest =
         ParseManifest(R"({ "start_url": "/start?query=a", "id": "" })");
     ASSERT_EQ(0u, GetErrorCount());
-    EXPECT_EQ("", manifest->id);
+    EXPECT_EQ("start?query=a", manifest->id);
   }
   // Full url.
   {