Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

URL and HTML: correct protocol setter tests #38032

Merged
merged 5 commits into from
Jan 24, 2023
Merged

URL and HTML: correct protocol setter tests #38032

merged 5 commits into from
Jan 24, 2023

Conversation

annevk
Copy link
Member

@annevk annevk commented Jan 18, 2023

@annevk annevk requested a review from TimothyGu January 18, 2023 10:31
@annevk annevk mentioned this pull request Jan 18, 2023
Closed
domenic
domenic approved these changes Jan 19, 2023
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but some suggestions for improved coverage / better assertion messages

html/browsers/history/the-location-interface/location-protocol-setter-sameish.html Outdated
assert_equals(self[index].location.protocol, "http:");
});
self[index].location.protocol = val;
}, `Set location.protocol to ${encodeURI(val)} (percent-encoded)`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're actually setting it to the outputted value percent-decoded, right?

Note that you might be able to get away with no encoding in the assert message at all; I kind of remember the WPT harness doing some escaping for us.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It doesn't seem to do any escaping if I remove encodeURI(). It does seem like I could also invoke escape() but that appears to do the same thing for these inputs.

I'll update the message to "(percent-encoded here for clarity)".

html/browsers/history/the-location-interface/location-protocol-setter.html Outdated Show resolved Hide resolved
url/resources/setters_tests.json Show resolved Hide resolved
@foolip
Copy link
Member

foolip commented Jan 23, 2023

@annevk does the flakiness of /html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html make any sense to you?

@annevk
Copy link
Member Author

annevk commented Jan 23, 2023

I have updated the timeout to match the -weird-.html file where it did get updated to be quite a bit longer.

@annevk annevk merged commit 865a920 into master Jan 24, 2023
@annevk annevk deleted the annevk/location-setter branch January 24, 2023 12:58
This was referenced Jan 24, 2023
Closed
Open
rmisev added a commit to rmisev/web-platform-tests that referenced this pull request Jan 24, 2023
@rmisev
URL: fix setters tests
e42def2 
These tests were introduced in the web-platform-tests#38032

According to the URL specification U+000D is a newline character and must be removed before further processing.
@rmisev rmisev mentioned this pull request Jan 24, 2023
Merged
annevk added a commit that referenced this pull request Jan 26, 2023
@rmisev @annevk
URL: fix setters tests
f1ade79 
These tests were introduced in the #38032.

U+000D is a newline character and must be removed before further processing.

Closes #38182.

Co-authored-by: Anne van Kesteren <[email protected]>
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 1, 2023
@rmisev @moz-wptsync-bot
Bug 1812147 [wpt PR 38150] - URL: fix setters tests, a=testonly
f278efa 
Automatic update from web-platform-tests
URL: fix setters tests

These tests were introduced in the web-platform-tests/wpt#38032.

U+000D is a newline character and must be removed before further processing.

Closes #38182.

Co-authored-by: Anne van Kesteren <[email protected]>
--

wpt-commits: f1ade799d04b72b0ff75c13e988744c2cd873741
wpt-pr: 38150
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 3, 2023
@rmisev @moz-wptsync-bot
Bug 1812147 [wpt PR 38150] - URL: fix setters tests, a=testonly
d7d5072 
Automatic update from web-platform-tests
URL: fix setters tests

These tests were introduced in the web-platform-tests/wpt#38032.

U+000D is a newline character and must be removed before further processing.

Closes #38182.

Co-authored-by: Anne van Kesteren <[email protected]>
--

wpt-commits: f1ade799d04b72b0ff75c13e988744c2cd873741
wpt-pr: 38150
@cdumez cdumez mentioned this pull request Feb 28, 2023
Merged
webkit-early-warning-system pushed a commit to cdumez/WebKit that referenced this pull request Mar 1, 2023
@cdumez
Strip tab and newline from Location/URL/<a>/<area>'s protocol setter
0c6de66 
https://bugs.webkit.org/show_bug.cgi?id=251087
rdar://104599983

Reviewed by Alex Christensen.

Our URL parser was already skipping newlines and tabs in the scheme while
parsing. However, URL::setProtocol() would call URLParser::maybeCanonicalizeScheme()
before parsing and this function would fail if the protocol contains any
newlines or tabs.

* LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html:
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-sameish-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-sameish.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter.html:
Resync tests after web-platform-tests/wpt#38032.

* LayoutTests/imported/w3c/web-platform-tests/url/url-setters-stripping.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/url/url-setters-stripping.any.worker-expected.txt:
Rebaseline tests that are now passing.

* Source/WTF/wtf/URLParser.cpp:
(WTF::URLParser::maybeCanonicalizeScheme):
Update URLParser::maybeCanonicalizeScheme() to allow for newlines and tabs in
the scheme since it is allowed by the specification and our URL parser actually
handles them fine.

Canonical link: https://commits.webkit.org/261017@main
marcoscaceres pushed a commit that referenced this pull request Mar 28, 2023
@rmisev @annevk @marcoscaceres
URL: fix setters tests
69b29fb 
These tests were introduced in the #38032.

U+000D is a newline character and must be removed before further processing.

Closes #38182.

Co-authored-by: Anne van Kesteren <[email protected]>
aperezdc pushed a commit to WebKit/WebKit that referenced this pull request Apr 6, 2023
@cdumez @aperezdc
Cherry-pick 261017@main (0c6de66). https://bugs.webkit.org/show_bug.c…
d77f824 
…gi?id=251087

    Strip tab and newline from Location/URL/<a>/<area>'s protocol setter
    https://bugs.webkit.org/show_bug.cgi?id=251087
    rdar://104599983

    Reviewed by Alex Christensen.

    Our URL parser was already skipping newlines and tabs in the scheme while
    parsing. However, URL::setProtocol() would call URLParser::maybeCanonicalizeScheme()
    before parsing and this function would fail if the protocol contains any
    newlines or tabs.

    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-expected.txt:
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html:
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-sameish-expected.txt: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-sameish.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter.html:
    Resync tests after web-platform-tests/wpt#38032.

    * LayoutTests/imported/w3c/web-platform-tests/url/url-setters-stripping.any-expected.txt:
    * LayoutTests/imported/w3c/web-platform-tests/url/url-setters-stripping.any.worker-expected.txt:
    Rebaseline tests that are now passing.

    * Source/WTF/wtf/URLParser.cpp:
    (WTF::URLParser::maybeCanonicalizeScheme):
    Update URLParser::maybeCanonicalizeScheme() to allow for newlines and tabs in
    the scheme since it is allowed by the specification and our URL parser actually
    handles them fine.

    Canonical link: https://commits.webkit.org/261017@main
aperezdc pushed a commit to WebKit/WebKit that referenced this pull request Apr 6, 2023
@cdumez @aperezdc
Cherry-pick 261017@main (0c6de66). https://bugs.webkit.org/show_bug.c…
c9c32c1 
…gi?id=251087

    Strip tab and newline from Location/URL/<a>/<area>'s protocol setter
    https://bugs.webkit.org/show_bug.cgi?id=251087
    rdar://104599983

    Reviewed by Alex Christensen.

    Our URL parser was already skipping newlines and tabs in the scheme while
    parsing. However, URL::setProtocol() would call URLParser::maybeCanonicalizeScheme()
    before parsing and this function would fail if the protocol contains any
    newlines or tabs.

    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-expected.txt:
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html:
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-sameish-expected.txt: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter-sameish.html: Added.
    * LayoutTests/imported/w3c/web-platform-tests/html/browsers/history/the-location-interface/location-protocol-setter.html:
    Resync tests after web-platform-tests/wpt#38032.

    * LayoutTests/imported/w3c/web-platform-tests/url/url-setters-stripping.any-expected.txt:
    * LayoutTests/imported/w3c/web-platform-tests/url/url-setters-stripping.any.worker-expected.txt:
    Rebaseline tests that are now passing.

    * Source/WTF/wtf/URLParser.cpp:
    (WTF::URLParser::maybeCanonicalizeScheme):
    Update URLParser::maybeCanonicalizeScheme() to allow for newlines and tabs in
    the scheme since it is allowed by the specification and our URL parser actually
    handles them fine.

    Canonical link: https://commits.webkit.org/261017@main
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jun 1, 2024
@rmisev @moz-wptsync-bot
Bug 1812147 [wpt PR 38150] - URL: fix setters tests, a=testonly
ce8b48a 
Automatic update from web-platform-tests
URL: fix setters tests

These tests were introduced in the web-platform-tests/wpt#38032.

U+000D is a newline character and must be removed before further processing.

Closes #38182.

Co-authored-by: Anne van Kesteren <[email protected]>
--

wpt-commits: f1ade799d04b72b0ff75c13e988744c2cd873741
wpt-pr: 38150
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic approved these changes

@TimothyGu TimothyGu Awaiting requested review from TimothyGu

@achristensen07 achristensen07 Awaiting requested review from achristensen07

@foolip foolip Awaiting requested review from foolip

@GPHemsley GPHemsley Awaiting requested review from GPHemsley

@jdm jdm Awaiting requested review from jdm

@jgraham jgraham Awaiting requested review from jgraham

@mikewest mikewest Awaiting requested review from mikewest

@zqzhang zqzhang Awaiting requested review from zqzhang

Assignees

@jgraham jgraham

Labels
html url
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@annevk @foolip @domenic @jgraham @wpt-pr-bot