url: trim leading slashes of file URL paths#12203
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added c++ 
test/fixtures/url-tests.js Outdated
There was a problem hiding this comment.
Is this commented text necessary?
There was a problem hiding this comment.
This additional is to follow the spec/test updates(web-platform-tests/wpt#5195), and we're still having some remaining unfollowed updates of the spec. When we follow them that can fix this test cases, we can take the comment out.
e.g.
node/test/fixtures/url-setter-tests.js
Lines 373 to 380 in 843b7e6
I will work on it later, but I think it's better to separate a patch from this PR since the referral link can be different.
src/node_url.cc Outdated
There was a problem hiding this comment.
The updated spec says greater than 1 instead of > 0:
https://github.com/whatwg/url/blob/2f4b7f74533398c41f4316157f027a33a627caf9/url.bs#L2070
watilde commented Apr 5, 2017
watilde commented Apr 5, 2017
cc @nodejs/url |
src/node_url.cc Outdated
There was a problem hiding this comment.
The url->flags |= URL_FLAGS_HAS_PATH; should go into this branch.
There was a problem hiding this comment.
Thanks! The flags seem to be there around passing a value to the property.
Updates :)
src/node_url.cc Outdated
There was a problem hiding this comment.
And this branch needs a url->flags |= URL_FLAGS_HAS_HOST;
watildeforce-pushed the feature/file-slash-2 branch from 3209da9 to 56e52d5Comparewatilde commented Apr 7, 2017
test/fixtures/url-setter-tests.js Outdated
There was a problem hiding this comment.
Can you update the hash of the link in the comment at the top of this file
/* WPT Refs: https://github.com/w3c/web-platform-tests/blob/e48dd15/url/setters_tests.json License: http://www.w3.org/Consortium/Legal/2008/04-testsuite-copyright.html */ This should be 3eff1bd now.
test/fixtures/url-tests.js Outdated
There was a problem hiding this comment.
Ditto, although this file is missing https://github.com/w3c/web-platform-tests/blob/3eff1bd564a0085347d31d80fbee13aaa2e3c2ca/url/urltestdata.json#L5380-L5451 ?
There was a problem hiding this comment.
Done synchronising them all by hands, and the lines were added as a result! thanks.
watildeforce-pushed the feature/file-slash-2 branch 2 times, most recently from 5720d56 to 50f48b6Compare
It should trim the slashes after the colon into three for file URL. Refs: web-platform-tests/wpt#5195Fixes: nodejs#11188
watildeforce-pushed the feature/file-slash-2 branch from 50f48b6 to 5d0f60dComparewatilde commented Apr 8, 2017
watilde commented Apr 9, 2017
The windows-fanned failure looks unrelated. |
refack commented Apr 10, 2017
Yep failed test on windows because of #12283, CI can be considered green. |
refack commented Apr 10, 2017
Another one for good luck |
watilde commented Apr 10, 2017
@refack Thanks for following up! I'm going to merge this tonight :) |
watilde commented Apr 10, 2017
Landed in b470a85. Thanks! |
It should trim the slashes after the colon into three for file URL. PR-URL: #12203 Refs: web-platform-tests/wpt#5195Fixes: #11188 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
italoacasas commented Apr 10, 2017
cc @watilde |
7 participants
It should trim the slashes after the colon into three for file URL.
Resources:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
url