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.

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

Changes URL tests for file path normalisation proposal #25716

Merged
merged 2 commits into from
Sep 30, 2020
Merged

Changes URL tests for file path normalisation proposal #25716

merged 2 commits into from
Sep 30, 2020

Conversation

alwinb
Copy link
Contributor

@alwinb alwinb commented Sep 23, 2020

Changes to the URL tests (urltestdata.json and setters_tests.json) for the proposal outlined in whatwg/url#405.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks for writing these! This looks correct to me modulo the nit.

@@ -7217,4 +7344,4 @@
"search": "",
"username": "user"
}
]
]
Copy link
Member

Choose a reason for hiding this comment

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

Please restore the newline at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't see that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just so I know how this works, does each such commit send out another approval request to each of the reviewers?

Copy link
Member

Choose a reason for hiding this comment

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

I think that depends on how you configure your GitHub notifications, but maybe? I usually wait for some kind of comment before taking another look.

annevk pushed a commit to whatwg/url that referenced this pull request Sep 30, 2020
Applicable only to file URLs:
- Leading empty path segments are no longer removed. 
- If absent, the host is copied from the base URL.
- Otherwise the host is preserved with one exception: if the host is `localhost`, then it is set to the empty string. 

Tests: web-platform-tests/wpt#25716.

JS implementation: jsdom/whatwg-url#170.

Fixes #302 and fixes #402.
@annevk annevk merged commit 050308a into web-platform-tests:master Sep 30, 2020
philn pushed a commit to philn/old-webkit that referenced this pull request Oct 4, 2020
https://bugs.webkit.org/show_bug.cgi?id=217170

Reviewed by Brady Eidson.

LayoutTests/imported/w3c:

* web-platform-tests/url/a-element-expected.txt:
* web-platform-tests/url/a-element-xhtml-expected.txt:
* web-platform-tests/url/resources/urltestdata.json:
* web-platform-tests/url/url-constructor-expected.txt:

Source/WTF:

This matches Chrome and the URL specification.
Covered by newly passing web platform tests.

I also updated the web platform tests from web-platform-tests/wpt#25716
which aligns with Safari in cases except copying of the host from base file URLs.

The implementation pushes copying from the base URL downstream in the parsing process to where it is in the URL specification
so that we can properly decide how much of the base URL to copy and so we can copy it into the right place in the result URL.

I also updated an assertion that makes sure that we re-use the input String if possible because there are cases where we copy
part of the parent URL, which is a "syntax violation" (meaning we copy the string parts and assemble a new one), then re-assemble
a new String that is equal to the input string.  This is not a problem, it just needed to be reflected in the assertion.

* wtf/URLParser.cpp:
(WTF::URLParser::URLParser):
(WTF::URLParser::parse):



git-svn-id: http://svn.webkit.org/repository/webkit/trunk@267896 268f45cc-cd09-0410-ab3c-d52691b4dbfc
watilde added a commit to watilde/node that referenced this pull request Oct 5, 2020
@alwinb alwinb deleted the pr-whatwg-url-issue-405 branch October 8, 2020 08:53
watilde added a commit to nodejs/node that referenced this pull request Oct 12, 2020
Refs: whatwg/url#544
Refs: web-platform-tests/wpt#25716

PR-URL: #35477
Fixes: #35429
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: whatwg/url#544
Refs: web-platform-tests/wpt#25716

PR-URL: nodejs#35477
Fixes: nodejs#35429
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants