Skip to content

url: trim leading slashes of file URL paths - #12203

Closed
watilde wants to merge 1 commit into
nodejs:masterfrom
watilde:feature/file-slash-2
Closed

url: trim leading slashes of file URL paths#12203
watilde wants to merge 1 commit into
nodejs:masterfrom
watilde:feature/file-slash-2

Conversation

@watilde

@watildewatilde commented Apr 4, 2017•
edited by TimothyGu
Loading

Copy link
Copy Markdown
Member

It should trim the slashes after the colon into three for file URL.

Resources:

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests are included
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

url

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Apr 4, 2017
thefourtheye
thefourtheye reviewed Apr 4, 2017
View reviewed changes
Comment threadtest/fixtures/url-tests.js Outdated

@thefourtheyethefourtheyeApr 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this commented text necessary?

@watildewatildeApr 4, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

// {
// "href": "file://test/",
// "new_value": "test",
// "expected": {
// "href": "file://test/",
// "username": ""
// }
// }

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.

thefourtheye
thefourtheye reviewed Apr 4, 2017
View reviewed changes
Comment threadsrc/node_url.cc Outdated

@thefourtheyethefourtheyeApr 4, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean > 0 here?

@watildewatildeApr 4, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The updated spec says greater than 1 instead of > 0:
https://github.com/whatwg/url/blob/2f4b7f74533398c41f4316157f027a33a627caf9/url.bs#L2070

@watilde

watilde commented Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

ci: https://ci.nodejs.org/job/node-test-pull-request/7220/

@watilde

watilde commented Apr 5, 2017

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/url

TimothyGu
TimothyGu suggested changes Apr 6, 2017
View reviewed changes

@TimothyGuTimothyGu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Almost there.

Comment threadsrc/node_url.cc Outdated

@TimothyGuTimothyGuApr 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The url->flags |= URL_FLAGS_HAS_PATH; should go into this branch.

@watildewatildeApr 7, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks! The flags seem to be there around passing a value to the property.

Updates :)

Comment threadsrc/node_url.cc Outdated

@TimothyGuTimothyGuApr 6, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And this branch needs a url->flags |= URL_FLAGS_HAS_HOST;

@watilde
watildeforce-pushed the feature/file-slash-2 branch from 3209da9 to 56e52d5CompareApril 7, 2017 06:09
@watilde

watilde commented Apr 7, 2017

Copy link
Copy Markdown
MemberAuthor

new ci: https://ci.nodejs.org/job/node-test-pull-request/7260/

joyeecheung
joyeecheung reviewed Apr 8, 2017
View reviewed changes
Comment threadtest/fixtures/url-setter-tests.js Outdated

@joyeecheungjoyeecheungApr 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@watildewatildeApr 8, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

updated it to 3eff1bd

Comment threadtest/fixtures/url-tests.js Outdated

@joyeecheungjoyeecheungApr 8, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto, although this file is missing https://github.com/w3c/web-platform-tests/blob/3eff1bd564a0085347d31d80fbee13aaa2e3c2ca/url/urltestdata.json#L5380-L5451 ?

@watildewatildeApr 8, 2017•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done synchronising them all by hands, and the lines were added as a result! thanks.

@watilde
watildeforce-pushed the feature/file-slash-2 branch 2 times, most recently from 5720d56 to 50f48b6CompareApril 8, 2017 07:52
@watilde
url: trim leading slashes of file URL paths
5d0f60d
It should trim the slashes after the colon into three for file URL.
Refs: web-platform-tests/wpt#5195Fixes: nodejs#11188
@watilde
watildeforce-pushed the feature/file-slash-2 branch from 50f48b6 to 5d0f60dCompareApril 8, 2017 07:56
@watilde

watilde commented Apr 8, 2017

Copy link
Copy Markdown
MemberAuthor

CI https://ci.nodejs.org/job/node-test-pull-request/7281/ 🐈

@watilde

watilde commented Apr 9, 2017

Copy link
Copy Markdown
MemberAuthor

The windows-fanned failure looks unrelated.

@refack

refack commented Apr 10, 2017

Copy link
Copy Markdown
Contributor

The windows-fanned failure looks unrelated.

Yep failed test on windows because of #12283, CI can be considered green.

@refack

refack commented Apr 10, 2017

Copy link
Copy Markdown
Contributor

Another one for good luck
CI: https://ci.nodejs.org/job/node-test-pull-request/7295/

@watilde

watilde commented Apr 10, 2017

Copy link
Copy Markdown
MemberAuthor

@refack Thanks for following up! I'm going to merge this tonight :)

@watilde

watilde commented Apr 10, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in b470a85. Thanks!

@watildewatilde closed this Apr 10, 2017
@watilde
watilde deleted the feature/file-slash-2 branch April 10, 2017 16:58
watilde added a commit that referenced this pull request Apr 10, 2017
@watilde
url: trim leading slashes of file URL paths
b470a85
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 <timothygu99@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@italoacasas

italoacasas commented Apr 10, 2017

Copy link
Copy Markdown

cc @watilde

@TimothyGuTimothyGu mentioned this pull request Apr 19, 2017
Merged
4 tasks
@jasnelljasnell mentioned this pull request May 11, 2017
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheungjoyeecheungjoyeecheung approved these changes
+2 more reviewers
@thefourtheyethefourtheyethefourtheye left review comments
@TimothyGuTimothyGuTimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@watilde@refack@italoacasas@thefourtheye@TimothyGu@joyeecheung@nodejs-github-bot