From 05be0c3425f433f86510d726ec5417bc867c2a1a Mon Sep 17 00:00:00 2001
From: Ben Kelly <wanderview@chromium.org>
Date: Tue, 1 Feb 2022 15:22:27 -0800
Subject: [PATCH] URLPattern: Set unmatched optional groups to undefined
 instead of ''.

This addresses the issues raised in:

https://github.com/WICG/urlpattern/issues/162

The main changes in this CL are:

1. The webidl is modified to allow passed back undefined.  I'm told
   real webidl should support `(USVString or undefined)` here, but our
   webidl compiler does not support that.  So we use `any` instead.
2. We must examine if the WTF::String is null or not before populating
   the returned array.  If its null, then we convert to undefined
   instead.

It turned out, however, that StringUTF8Adaptor was also converting
empty strings to null strings.  It was necessary to fix this in order
to return undefined and empty string in the correct places in the
returned values.

Bug: 1292699
Change-Id: I27a0be2567bb9ce4592ca15a5216fd5a58bdbf17
---
 urlpattern/resources/urlpatterntestdata.json | 30 +++++++++++++-------
 urlpattern/resources/urlpatterntests.js      |  8 ++++++
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/urlpattern/resources/urlpatterntestdata.json b/urlpattern/resources/urlpatterntestdata.json
index 8992c95b59b9c53..6773648da078247 100644
--- a/urlpattern/resources/urlpatterntestdata.json
+++ b/urlpattern/resources/urlpatterntestdata.json
@@ -353,8 +353,9 @@
   {
     "pattern": [{ "pathname": "/foo/:bar?" }],
     "inputs": [{ "pathname": "/foo" }],
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
-      "pathname": { "input": "/foo", "groups": { "bar": "" } }
+      "pathname": { "input": "/foo", "groups": { "bar": null } }
     }
   },
   {
@@ -418,8 +419,9 @@
   {
     "pattern": [{ "pathname": "/foo/:bar*" }],
     "inputs": [{ "pathname": "/foo" }],
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
-      "pathname": { "input": "/foo", "groups": { "bar": "" } }
+      "pathname": { "input": "/foo", "groups": { "bar": null } }
     }
   },
   {
@@ -472,15 +474,17 @@
     "expected_obj": {
       "pathname": "/foo/*?"
     },
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
-      "pathname": { "input": "/foo", "groups": { "0": "" } }
+      "pathname": { "input": "/foo", "groups": { "0": null } }
     }
   },
   {
     "pattern": [{ "pathname": "/foo/*?" }],
     "inputs": [{ "pathname": "/foo" }],
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
-      "pathname": { "input": "/foo", "groups": { "0": "" } }
+      "pathname": { "input": "/foo", "groups": { "0": null } }
     }
   },
   {
@@ -656,15 +660,17 @@
     "expected_obj": {
       "pathname": "/foo/**"
     },
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
-      "pathname": { "input": "/foo", "groups": { "0": "" } }
+      "pathname": { "input": "/foo", "groups": { "0": null } }
     }
   },
   {
     "pattern": [{ "pathname": "/foo/**" }],
     "inputs": [{ "pathname": "/foo" }],
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
-      "pathname": { "input": "/foo", "groups": { "0": "" } }
+      "pathname": { "input": "/foo", "groups": { "0": null } }
     }
   },
   {
@@ -1810,9 +1816,10 @@
       "hostname": "(sub.)?example.com",
       "pathname": "/foo"
     },
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
       "protocol": { "input": "https", "groups": {} },
-      "hostname": { "input": "example.com", "groups": { "0": "" } },
+      "hostname": { "input": "example.com", "groups": { "0": null } },
       "pathname": { "input": "/foo", "groups": {} }
     }
   },
@@ -1848,9 +1855,10 @@
       "hostname": "(sub(?:.))?example.com",
       "pathname": "/foo"
     },
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
       "protocol": { "input": "https", "groups": {} },
-      "hostname": { "input": "example.com", "groups": { "0": "" } },
+      "hostname": { "input": "example.com", "groups": { "0": null } },
       "pathname": { "input": "/foo", "groups": {} }
     }
   },
@@ -2297,9 +2305,10 @@
       "protocol": "data",
       "pathname": "text/javascript,let x = 100/:tens?5;"
     },
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
       "protocol": { "input": "data", "groups": {} },
-      "pathname": { "input": "text/javascript,let x = 100/5;", "groups": { "tens": "" } }
+      "pathname": { "input": "text/javascript,let x = 100/5;", "groups": { "tens": null } }
     }
   },
   {
@@ -2619,8 +2628,9 @@
     "expected_obj": {
       "pathname": "*(.*)?"
     },
+    "//": "The `null` below is translated to undefined in the test harness.",
     "expected_match": {
-      "pathname": { "input": "foobar", "groups": { "0": "foobar", "1": "" }}
+      "pathname": { "input": "foobar", "groups": { "0": "foobar", "1": null }}
     }
   },
   {
diff --git a/urlpattern/resources/urlpatterntests.js b/urlpattern/resources/urlpatterntests.js
index 40b3edc3da22b99..f774699bff99c3b 100644
--- a/urlpattern/resources/urlpatterntests.js
+++ b/urlpattern/resources/urlpatterntests.js
@@ -146,6 +146,14 @@ function runTests(data) {
             expected_obj.groups['0'] = '';
           }
         }
+        // JSON does not allow us to use undefined directly, so the data file
+        // contains null instead.  Translate to the expected undefined value
+        // here.
+        for (const key in expected_obj.groups) {
+          if (expected_obj.groups[key] === null) {
+            expected_obj.groups[key] = undefined;
+          }
+        }
         assert_object_equals(exec_result[component], expected_obj,
                              `exec() result for ${component}`);
       }