url: handle windows drive letter in the file state by watilde · Pull Request #12808 · nodejs/node

watilde · 2017-05-03T07:32:43Z

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

make -j4 test
tests are included
commit message follows commit guidelines

Affected core subsystem(s)

url

watilde · 2017-05-03T07:47:24Z

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

rmisev · 2017-05-03T10:12:56Z

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 · 2017-05-03T10:51:17Z

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

refack · 2017-05-03T11:17:13Z

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

rmisev · 2017-05-03T11:41:27Z

src/node_url.cc

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

I see it works! Thanks for your comment :)

rmisev · 2017-05-03T11:41:29Z

src/node_url.cc

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

addaleax · 2017-05-03T13:28:44Z

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

watilde · 2017-05-03T13:37:24Z

@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 · 2017-05-03T17:35:06Z

Okay, new CI then: https://ci.nodejs.org/job/node-test-commit/9608/ :)

TimothyGu · 2017-05-04T05:03:06Z

src/node_url.cc

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 👍

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);.

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] != '#'))) {

I replaced end - pointer with remaining I added.

`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 · 2017-05-04T13:26:36Z

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

TimothyGu

LGTM!

TimothyGu · 2017-05-05T01:22:27Z

Landed in 943dd5f.

`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>

`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>

`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>

nodejs-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 May 3, 2017

watilde force-pushed the feature/file-url branch from 40f315b to c691818 Compare May 3, 2017 10:51

refack approved these changes May 3, 2017

View reviewed changes

rmisev reviewed May 3, 2017

View reviewed changes

src/node_url.cc Outdated

Copy link

Contributor

rmisev May 3, 2017

Choose a reason for hiding this comment

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

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

watilde force-pushed the feature/file-url branch from c691818 to c4a1428 Compare May 3, 2017 12:17

TimothyGu reviewed May 4, 2017

View reviewed changes

url: handle windows drive letter in the file state

6962b12

`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 force-pushed the feature/file-url branch from c4a1428 to 6962b12 Compare May 4, 2017 13:25

jasnell approved these changes May 4, 2017

View reviewed changes

joyeecheung approved these changes May 4, 2017

View reviewed changes

TimothyGu approved these changes May 4, 2017

View reviewed changes

TimothyGu closed this May 5, 2017

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

watilde mentioned this pull request May 7, 2017

(v7.x backport) url: handle windows drive letter in the file state #12880

Closed

2 tasks

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

Uh oh!

Conversation

watilde commented May 3, 2017

Checklist

Affected core subsystem(s)

Uh oh!

watilde commented May 3, 2017

Uh oh!

rmisev commented May 3, 2017

Uh oh!

watilde commented May 3, 2017

Uh oh!

refack commented May 3, 2017

Uh oh!

rmisev May 3, 2017

Choose a reason for hiding this comment

Uh oh!

watilde May 4, 2017

Choose a reason for hiding this comment

Uh oh!

rmisev May 3, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 3, 2017

Uh oh!

watilde commented May 3, 2017

Uh oh!

addaleax commented May 3, 2017

Uh oh!

TimothyGu May 4, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu May 4, 2017

Choose a reason for hiding this comment

Uh oh!

watilde May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watilde May 4, 2017

Choose a reason for hiding this comment

Uh oh!

watilde commented May 4, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented May 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

watilde May 4, 2017 •

edited

Loading