Skip to content

Commit

Permalink
URLPattern: Set unmatched optional groups to undefined instead of ''.
Browse files Browse the repository at this point in the history 
This addresses the issues raised in:

whatwg/urlpattern#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.

Bug: 1292699
Change-Id: I27a0be2567bb9ce4592ca15a5216fd5a58bdbf17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3428631
Reviewed-by: Jeremy Roman <[email protected]>
Commit-Queue: Ben Kelly <[email protected]>
Cr-Commit-Position: refs/heads/main@{#968611}
NOKEYCHECK=True
GitOrigin-RevId: 27016ffa44b1b63cfef634f009ad881f90ee6e6c
  • Loading branch information
@wanderview @copybara-github
wanderview authored and copybara-github committed Feb 8, 2022
1 parent 78d643b commit ca3678e
Show file tree
Hide file tree
Showing 6 changed files with 105 additions and 51 deletions.
94 changes: 61 additions & 33 deletions blink/renderer/modules/url_pattern/url_pattern.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include "base/strings/string_util.h"
#include "third_party/blink/renderer/bindings/core/v8/script_regexp.h"
#include "third_party/blink/renderer/bindings/core/v8/to_v8_traits.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_union_urlpatterninit_usvstring.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_url_pattern_component_result.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_url_pattern_init.h"
Expand Down Expand Up @@ -181,11 +182,33 @@ void ApplyInit(const URLPatternInit* init,
}

URLPatternComponentResult* MakeURLPatternComponentResult(
ScriptState* script_state,
const String& input,
const Vector<std::pair<String, String>>& group_values) {
auto* result = URLPatternComponentResult::Create();
result->setInput(input);
result->setGroups(group_values);

// Convert null WTF::String values to v8::Undefined. We have to do this
// manually because the webidl compiler compiler does not currently
// support `(USVString or undefined)` in a record value.
// TODO(crbug.com/1293259): Use webidl `(USVString or undefined)` when
// available in the webidl compiler.
HeapVector<std::pair<String, ScriptValue>> v8_group_values;
v8_group_values.ReserveCapacity(group_values.size());
for (const auto& pair : group_values) {
v8::Local<v8::Value> v8_value;
if (pair.second.IsNull()) {
v8_value = v8::Undefined(script_state->GetIsolate());
} else {
v8_value = ToV8Traits<IDLUSVString>::ToV8(script_state, pair.second)
.ToLocalChecked();
}
v8_group_values.emplace_back(
pair.first,
ScriptValue(script_state->GetIsolate(), std::move(v8_value)));
}

result->setGroups(std::move(v8_group_values));
return result;
}

Expand Down Expand Up @@ -338,33 +361,34 @@ URLPattern::URLPattern(Component* protocol,
search_(search),
hash_(hash) {}

bool URLPattern::test(
const V8URLPatternInput* input,
const String& base_url,
ExceptionState& exception_state) const {
return Match(input, base_url, /*result=*/nullptr, exception_state);
bool URLPattern::test(ScriptState* script_state,
const V8URLPatternInput* input,
const String& base_url,
ExceptionState& exception_state) const {
return Match(script_state, input, base_url, /*result=*/nullptr,
exception_state);
}

bool URLPattern::test(
const V8URLPatternInput* input,
ExceptionState& exception_state) const {
return test(input, /*base_url=*/String(), exception_state);
bool URLPattern::test(ScriptState* script_state,
const V8URLPatternInput* input,
ExceptionState& exception_state) const {
return test(script_state, input, /*base_url=*/String(), exception_state);
}

URLPatternResult* URLPattern::exec(
const V8URLPatternInput* input,
const String& base_url,
ExceptionState& exception_state) const {
URLPatternResult* URLPattern::exec(ScriptState* script_state,
const V8URLPatternInput* input,
const String& base_url,
ExceptionState& exception_state) const {
URLPatternResult* result = URLPatternResult::Create();
if (!Match(input, base_url, result, exception_state))
if (!Match(script_state, input, base_url, result, exception_state))
return nullptr;
return result;
}

URLPatternResult* URLPattern::exec(
const V8URLPatternInput* input,
ExceptionState& exception_state) const {
return exec(input, /*base_url=*/String(), exception_state);
URLPatternResult* URLPattern::exec(ScriptState* script_state,
const V8URLPatternInput* input,
ExceptionState& exception_state) const {
return exec(script_state, input, /*base_url=*/String(), exception_state);
}

String URLPattern::protocol() const {
Expand Down Expand Up @@ -441,7 +465,8 @@ void URLPattern::Trace(Visitor* visitor) const {
ScriptWrappable::Trace(visitor);
}

bool URLPattern::Match(const V8URLPatternInput* input,
bool URLPattern::Match(ScriptState* script_state,
const V8URLPatternInput* input,
const String& base_url,
URLPatternResult* result,
ExceptionState& exception_state) const {
Expand Down Expand Up @@ -570,19 +595,22 @@ bool URLPattern::Match(const V8URLPatternInput* input,

result->setInputs(std::move(inputs));

result->setProtocol(
MakeURLPatternComponentResult(protocol, protocol_group_list));
result->setUsername(
MakeURLPatternComponentResult(username, username_group_list));
result->setPassword(
MakeURLPatternComponentResult(password, password_group_list));
result->setHostname(
MakeURLPatternComponentResult(hostname, hostname_group_list));
result->setPort(MakeURLPatternComponentResult(port, port_group_list));
result->setPathname(
MakeURLPatternComponentResult(pathname, pathname_group_list));
result->setSearch(MakeURLPatternComponentResult(search, search_group_list));
result->setHash(MakeURLPatternComponentResult(hash, hash_group_list));
result->setProtocol(MakeURLPatternComponentResult(script_state, protocol,
protocol_group_list));
result->setUsername(MakeURLPatternComponentResult(script_state, username,
username_group_list));
result->setPassword(MakeURLPatternComponentResult(script_state, password,
password_group_list));
result->setHostname(MakeURLPatternComponentResult(script_state, hostname,
hostname_group_list));
result->setPort(
MakeURLPatternComponentResult(script_state, port, port_group_list));
result->setPathname(MakeURLPatternComponentResult(script_state, pathname,
pathname_group_list));
result->setSearch(
MakeURLPatternComponentResult(script_state, search, search_group_list));
result->setHash(
MakeURLPatternComponentResult(script_state, hash, hash_group_list));

return true;
}
Expand Down
15 changes: 10 additions & 5 deletions blink/renderer/modules/url_pattern/url_pattern.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,20 @@ class MODULES_EXPORT URLPattern : public ScriptWrappable {
Component* hash,
base::PassKey<URLPattern> key);

bool test(const V8URLPatternInput* input,
bool test(ScriptState* script_state,
const V8URLPatternInput* input,
const String& base_url,
ExceptionState& exception_state) const;
bool test(const V8URLPatternInput* input,
bool test(ScriptState* script_state,
const V8URLPatternInput* input,
ExceptionState& exception_state) const;

URLPatternResult* exec(const V8URLPatternInput* input,
URLPatternResult* exec(ScriptState* script_state,
const V8URLPatternInput* input,
const String& base_url,
ExceptionState& exception_state) const;
URLPatternResult* exec(const V8URLPatternInput* input,
URLPatternResult* exec(ScriptState* script_state,
const V8URLPatternInput* input,
ExceptionState& exception_state) const;

String protocol() const;
Expand All @@ -79,7 +83,8 @@ class MODULES_EXPORT URLPattern : public ScriptWrappable {
// A utility function to determine if a given `input` matches the pattern or
// not. Returns `true` if there is a match and `false` otherwise. If
// `result` is not nullptr then the URLPatternResult contents will be filled.
bool Match(const V8URLPatternInput* input,
bool Match(ScriptState* script_state,
const V8URLPatternInput* input,
const String& base_url,
URLPatternResult* result,
ExceptionState& exception_state) const;
Expand Down
4 changes: 2 additions & 2 deletions blink/renderer/modules/url_pattern/url_pattern.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ enum URLPatternComponent { "protocol", "username", "password", "hostname",
[RaisesException, Measure]
constructor(optional URLPatternInput input = {}, optional USVString baseURL);

[RaisesException, Measure]
[RaisesException, CallWith=ScriptState, Measure]
boolean test(optional URLPatternInput input = {}, optional USVString baseURL);

[RaisesException, Measure]
[RaisesException, CallWith=ScriptState, Measure]
URLPatternResult? exec(optional URLPatternInput input = {}, optional USVString baseURL);

readonly attribute USVString protocol;
Expand Down
5 changes: 4 additions & 1 deletion blink/renderer/modules/url_pattern/url_pattern_component_result.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,8 @@
// https://wicg.github.io/urlpattern/
dictionary URLPatternComponentResult {
USVString input;
record<USVString, USVString> groups;

// TODO(crbug.com/1293259): Use `(USVString or undefined)` instead of `any`
// when supported by the webidl compiler.
record<USVString, any> groups;
};
30 changes: 20 additions & 10 deletions blink/web_tests/external/wpt/urlpattern/resources/urlpatterntestdata.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 } }
}
},
{
Expand Down Expand Up @@ -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 } }
}
},
{
Expand Down Expand Up @@ -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 } }
}
},
{
Expand Down Expand Up @@ -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 } }
}
},
{
Expand Down Expand Up @@ -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": {} }
}
},
Expand Down Expand Up @@ -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": {} }
}
},
Expand Down Expand Up @@ -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 } }
}
},
{
Expand Down Expand Up @@ -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 }}
}
},
{
Expand Down
8 changes: 8 additions & 0 deletions blink/web_tests/external/wpt/urlpattern/resources/urlpatterntests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}`);
}
Expand Down

0 comments on commit ca3678e

Please sign in to comment.