url: change path parsing for non-special URLs by watilde · Pull Request #12058 · nodejs/node

watilde · 2017-03-27T05:41:52Z

Updates:

url: change path parsing for non-special URLs
- Refs: Change path parsing for non-special URLs whatwg/url#213
test: synchronize WPT url setter test data, and put it as js instead of json
- Refs: url: update the processing in the scheme state. #11887 (review)
test: synchronize WPT url setter test data and url test data from Non-special URL path tests web-platform-tests/wpt#4586

Fixes #11962.

Checklist

make -j4 test
tests are included
commit message follows

Affected core subsystem(s)

url, test

watilde · 2017-03-27T07:08:19Z

cc @nodejs/url

TimothyGu

More in-depth review tomorrow, hopefully.

TimothyGu · 2017-03-27T07:10:48Z

src/node_url.cc

Isn't continue equivalent to p--?

That's right! Updated to use continue instead of p--. thanks :)

watilde · 2017-03-27T22:22:42Z

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

watilde · 2017-03-29T03:44:44Z

Failed CIs doesn't seem to be related to this PR.

watilde · 2017-03-30T07:16:29Z

rebased to resolve conflicts.
new ci: https://ci.nodejs.org/job/node-test-pull-request/7105/

TimothyGu · 2017-03-31T02:42:14Z

src/node_url.cc

The spec says "set url’s query to the empty string". A URL's query is initially null. You have to make it clear that the query in this case is empty string rather than undefined by setting URL_FLAGS_HAS_QUERY on url->flags.

Good point and good catch.

Oh good to know it! Thanks. I will update it soon.

TimothyGu · 2017-03-31T02:43:07Z

src/node_url.cc

Ditto, but in this case you have to set URL_FLAGS_HAS_FRAGMENT

watilde · 2017-04-02T06:18:37Z

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

TimothyGu · 2017-04-02T15:30:18Z

src/node_url.cc

Sorry for not being clear. Keep the url->query.clear() there, in addition to setting the flag.

oops, I misunderstood it sorry! updated.

This changes to the way path parsing for non-special URLs. It allows paths to be empty for non-special URLs and also takes that into account when serializing. Fixes: nodejs#11962 Refs: whatwg/url#213

Refs: web-platform-tests/wpt#4586 Refs: nodejs#11887

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: nodejs#11887

watilde · 2017-04-02T16:45:04Z

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

watilde · 2017-04-03T09:18:26Z

Landed in f8f46f9, 50bfef6 and 843b7e6. Thanks.

This changes to the way path parsing for non-special URLs. It allows paths to be empty for non-special URLs and also takes that into account when serializing. Fixes: #11962 Refs: whatwg/url#213 PR-URL: #12058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Refs: web-platform-tests/wpt#4586 Refs: #11887 PR-URL: #12058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: #11887 PR-URL: #12058 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: nodejs#11887 PR-URL: nodejs#12058 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 Mar 27, 2017

TimothyGu reviewed Mar 27, 2017

View reviewed changes

watilde force-pushed the feature/file-path branch from ceacc41 to b0e8cae Compare March 27, 2017 22:20

watilde mentioned this pull request Mar 29, 2017

url: repeated slashes in file URL should be arranged #11188

Closed

watilde force-pushed the feature/file-path branch from b0e8cae to 6169f87 Compare March 30, 2017 07:15

jasnell approved these changes Mar 30, 2017

View reviewed changes

joyeecheung approved these changes Mar 31, 2017

View reviewed changes

TimothyGu suggested changes Mar 31, 2017

View reviewed changes

watilde force-pushed the feature/file-path branch 2 times, most recently from a3ed1d4 to 8772487 Compare April 2, 2017 06:17

TimothyGu reviewed Apr 2, 2017

View reviewed changes

watilde added 2 commits April 3, 2017 00:47

url: change path parsing for non-special URLs

66feed8

This changes to the way path parsing for non-special URLs. It allows paths to be empty for non-special URLs and also takes that into account when serializing. Fixes: nodejs#11962 Refs: whatwg/url#213

test: synchronize WPT url test data

616a59a

Refs: web-platform-tests/wpt#4586 Refs: nodejs#11887

watilde force-pushed the feature/file-path branch from 8772487 to 303d71f Compare April 2, 2017 15:47

test: synchronize WPT url setter test data

b103e6e

Updates: + Bring tests url-setter-tests from WPT, and put it as JavaScript + Comment out unpassed tests Refs: web-platform-tests/wpt#5112 Refs: nodejs#11887

watilde force-pushed the feature/file-path branch from 303d71f to b103e6e Compare April 2, 2017 15:57

TimothyGu approved these changes Apr 2, 2017

View reviewed changes

watilde closed this Apr 3, 2017

watilde deleted the feature/file-path branch April 3, 2017 09:18

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas added the backport-to-v7.x label Apr 10, 2017

TimothyGu mentioned this pull request Apr 19, 2017

(v7.x backport) url: updates to the WHATWG URL parser #12507

Merged

4 tasks

TimothyGu added backported-to-v7.x and removed backport-requested-v7.x labels Apr 27, 2017

baby636 mentioned this pull request Jun 22, 2022

[Snyk] Security upgrade eslint from 5.16.0 to 6.6.0 baby636/node#59

Open

Uh oh!

Conversation

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

Uh oh!

Updates:

Checklist

Affected core subsystem(s)

Uh oh!

watilde commented Mar 27, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watilde commented Mar 27, 2017

Uh oh!

watilde commented Mar 29, 2017

Uh oh!

watilde commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watilde commented Apr 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

watilde commented Apr 2, 2017

Uh oh!

watilde commented Apr 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

watilde commented Mar 27, 2017 •

edited

Loading