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

url: handle windows drive letter in the file state #12808

Closed
wants to merge 1 commit into from

Conversation

@watilde
Copy link
Member

commented May 3, 2017

In the whatwg-url, C| should not satisfy the condition to not copy the base's path. This patch also synchronises WPT url test data to verify the update in upstream.

Refs: whatwg/url#305
Refs: web-platform-tests/wpt#5754

Checklist
Affected core subsystem(s)

url

@watilde

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

@watilde watilde added this to Landed in Spec in WHATWG URL implementation May 3, 2017

@rmisev

This comment has been minimized.

Copy link
Contributor

commented May 3, 2017

Oops! It looks like you copied other tests ... which was already in your tests file. Correct ones are commented # Windows drive letter handling with the 'file:' base URL in the https://github.com/w3c/web-platform-tests/blob/f0b286344dd2613d48643394408f4ab48ef2b33d/url/urltestdata.json#L5426-L5524

@watilde

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

Opps sorry! I re-copied them and got an error in make-test for this part(the other cases worked fine):

  {
    "input": "C|",
    "base": "file://host/dir/file",
    "href": "file:///C:",
    "protocol": "file:",
    "username": "",
    "password": "",
    "host": "",
    "hostname": "",
    "port": "",
    "pathname": "/C:",
    "search": "",
    "hash": ""
  },

I will work on it tonight. Thanks for pointing it :) @rmisev

@watilde watilde force-pushed the watilde:feature/file-url branch May 3, 2017

@refack
refack approved these changes May 3, 2017
@refack

This comment has been minimized.

Copy link
Member

commented May 3, 2017

CI: https://ci.nodejs.org/job/node-test-commit/9600/
(Interested how it behaves on Windows...)

src/node_url.cc Outdated
if ((!IsWindowsDriveLetter(ch, p[1]) ||
end - p == 1 ||
(p[2] != '/' &&
if ((end - p == 0 ||

This comment has been minimized.

Copy link
@rmisev

rmisev May 3, 2017

Contributor

The remaining means after ch, so p[0] must not be counted here. Correct to end - (p+1) == 0 or just end - p == 1

There is an example, which explains the term remaining: https://url.spec.whatwg.org/#remaining

This comment has been minimized.

Copy link
@watilde

watilde May 4, 2017

Author Member

I see it works! Thanks for your comment :)

src/node_url.cc Outdated
(p[2] != '/' &&
if ((end - p == 0 ||
!IsWindowsDriveLetter(ch, p[1]) ||
(end - p >= 2 &&

This comment has been minimized.

Copy link
@rmisev

rmisev May 3, 2017

Contributor

The same as above: end - (p+1) >= 2 or just end - p >= 3

@watilde watilde force-pushed the watilde:feature/file-url branch May 3, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented May 3, 2017

The last CI doesn’t look so good… :/

@watilde

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

@addaleax Yeah sorry, I was WIP at the time without the wip label. I pushed the fix with the review comments, but will recheck it tonight.

@addaleax

This comment has been minimized.

Copy link
Member

commented May 3, 2017

src/node_url.cc Outdated
if ((!IsWindowsDriveLetter(ch, p[1]) ||
end - p == 1 ||
(p[2] != '/' &&
if ((end - p == 1 ||

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu May 4, 2017

Member

Depending on the spec writer's intention (whatwg/url#308) changing end - p == 1 to end - p <= 1 might be more robust to cover ch == kEOL as well, but in any case it shouldn't matter since the case when ch == kEOL is covered in another case already. Also, nice catch on noticing the end - p == 1 in the original code should actually be end - p == 2 👍

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu May 4, 2017

Member

Seems like they agree with me.

We can also calculate per iteration an actual remaining variable, like we are already doing for some other functions in the file. This should eliminate any confusion around this variable. The most correct implementation would be something like const size_t remaining = end == pointer ? 0 : (end - pointer - 1);.

This comment has been minimized.

Copy link
@watilde

watilde May 4, 2017

Author Member

Woo that'd be nice! I'm going to update it 😺

it's gonna be like

const size_t remaining = end == pointer ? 0 : (end - pointer - 1);

......

if ((remaining == 0 ||
      !IsWindowsDriveLetter(ch, p[1]) ||
      (remaining >= 2 &&
       p[2] != '/' &&
       p[2] != '\\' &&
       p[2] != '?' &&
       p[2] != '#'))) {

This comment has been minimized.

Copy link
@watilde

watilde May 4, 2017

Author Member

I replaced end - pointer with remaining I added.

url: handle windows drive letter in the file state
`C|` should not satisfy the condition to not copy
the base's path. It also synchronises WPT url test data
to verify the update in upstream.

Refs: whatwg/url#305
Refs: web-platform-tests/wpt#5754

@watilde watilde force-pushed the watilde:feature/file-url branch to 6962b12 May 4, 2017

@watilde

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

@jasnell
jasnell approved these changes May 4, 2017
@TimothyGu
Copy link
Member

left a comment

LGTM!

@TimothyGu

This comment has been minimized.

Copy link
Member

commented May 5, 2017

Landed in 943dd5f.

@TimothyGu TimothyGu closed this May 5, 2017

TimothyGu added a commit that referenced this pull request May 5, 2017
url: handle windows drive letter in the file state
`C|` should not satisfy the condition to not copy
the base's path. It also synchronises WPT url test data
to verify the update in upstream.

PR-URL: #12808
Refs: whatwg/url#305
Refs: web-platform-tests/wpt#5754
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

@watilde watilde deleted the watilde:feature/file-url branch May 5, 2017

@watilde watilde moved this from Landed in Spec to Done in WHATWG URL implementation May 5, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
url: handle windows drive letter in the file state
`C|` should not satisfy the condition to not copy
the base's path. It also synchronises WPT url test data
to verify the update in upstream.

PR-URL: nodejs#12808
Refs: whatwg/url#305
Refs: web-platform-tests/wpt#5754
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
watilde added a commit to watilde/node that referenced this pull request May 7, 2017
url: handle windows drive letter in the file state
`C|` should not satisfy the condition to not copy
the base's path. It also synchronises WPT url test data
to verify the update in upstream.

PR-URL: nodejs#12808
Refs: whatwg/url#305
Refs: web-platform-tests/wpt#5754
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
watilde added a commit that referenced this pull request May 12, 2017
url: handle windows drive letter in the file state
`C|` should not satisfy the condition to not copy
the base's path. It also synchronises WPT url test data
to verify the update in upstream.

PR-URL: #12808
Refs: whatwg/url#305
Refs: web-platform-tests/wpt#5754
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8 participants
You can’t perform that action at this time.