Add blob URLs into urltestdata.json #4941
Conversation
|
Notifying @Sebmaster, @annevk, @domenic, @mikewest, @rubys, @smola, @tomalec, @xiaojunwu, and @zcorpan. (Learn how reviewing works.) |
|
I guess I'm not against landing this, but whatwg/url#127 might change the outcome of these tests with regards to the origin field. @domenic? |
|
I think what we're seeing here is confusion from implementers as to what the origin of completely-synthetic blob URLs should be. I.e., if I do I'm not sure what the desired answer to that is according to whatwg/url#127. I guess that is what the comments from whatwg/url#127 (comment) onward say. If we really don't know the answer, I think a better approach here would be to remove the |
|
Per the current URL Standard it's pretty clear what it should be. That should just work. Per that issue it's something we might want to change at which point you'd get different results. |
|
Well, you might get different results. |
|
Sure, I'm just saying it sounds like this section of the spec isn't really settled, so it makes more sense to me to make it like file URLs (we don't test or spec the origin right now) than to treat it as settled and testable. |
|
Fair enough. @watilde want to remove the origin lines for now? |
|
@annevk Sure, I will do that now! |
|
I've removed the origin lines, and CI liked it :) |
| @@ -5644,5 +5644,15 @@ | |||
| "input": "non-special://[:80/", | |||
| "base": "about:blank", | |||
| "failure": true | |||
| }, | |||
| { | |||
| "input": "blob:https://example.com:443/", | |||
There was a problem hiding this comment.
These tests are missing all the other fields, which causes them to fail in the WPT test runner. You need base, href, username, password, host, hostname, port, search, and hash.
watilde
force-pushed
the
blob-urls
branch
2 times, most recently
from
87f615e to
e4cb43f
Compare
|
@watilde sorry, I did not see that you pushed a new commit addressing my review. However, why did you allow failure for parsing blob URLs? |
|
@domenic Oh sorry never mind it please 🙇 I wanted to make sure the failed ci was from my commit or not. |
|
Oh I see :(. It looks like the CI's attempts to run Chrome are not working great the moment... I'll try to poke some people. |
|
It might be worth trying to rebase just in case they've been making changes to the CI stuff |
Related link: + nodejs/node#11426
|
@domenic Hmm it seems to be still happening. I will do rebase again tomorrow or later then. Thanks anyway! |
|
@annevk informs me the review issues were fixed, and the CI problem looks like a bug in Chrome, so merging. |
In the getter of the origin in URL, the URL that has blob protocol will be parsed in a function called "originFor". Add test cases into the url-tests-additional fixture to test that. Refs: web-platform-tests/wpt#4941
In the getter of the origin in URL, the URL that has blob protocol will be parsed in a function called "originFor". Add test cases into the url-tests-additional fixture to test that. Refs: web-platform-tests/wpt#4941 PR-URL: #11426 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
In the getter of the origin in URL, the URL that has blob protocol will be parsed in a function called "originFor". Add test cases into the url-tests-additional fixture to test that. Refs: web-platform-tests/wpt#4941 PR-URL: #11426 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Related link: