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: change path parsing for non-special URLs #12058

Closed
wants to merge 3 commits into from

Conversation

@watilde
Copy link
Member

commented Mar 27, 2017

Updates:

Fixes #11962.

Checklist
  • make -j4 test
  • tests are included
  • commit message follows
Affected core subsystem(s)

url, test

@watilde

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2017

@TimothyGu
Copy link
Member

left a comment

More in-depth review tomorrow, hopefully.

src/node_url.cc Outdated
@@ -878,7 +880,7 @@ namespace url {
}
url->port = base->port;
state = kPath;
continue;
p--;

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Mar 27, 2017

Member

Isn't continue equivalent to p--?

This comment has been minimized.

Copy link
@watilde

watilde Mar 27, 2017

Author Member

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

@watilde watilde force-pushed the watilde:feature/file-path branch Mar 27, 2017

@watilde

This comment has been minimized.

Copy link
Member Author

commented Mar 27, 2017

@watilde

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2017

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

@watilde watilde force-pushed the watilde:feature/file-path branch Mar 30, 2017

@watilde

This comment has been minimized.

Copy link
Member Author

commented Mar 30, 2017

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

src/node_url.cc Outdated
continue;
}
} else if (!has_state_override && ch == '?') {
url->query.clear();

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Mar 31, 2017

Member

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.

This comment has been minimized.

Copy link
@jasnell

jasnell Mar 31, 2017

Member

Good point and good catch.

This comment has been minimized.

Copy link
@watilde

watilde Apr 2, 2017

Author Member

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

src/node_url.cc Outdated
url->query.clear();
state = kQuery;
} else if (!has_state_override && ch == '#') {
url->fragment.clear();

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Mar 31, 2017

Member

Ditto, but in this case you have to set URL_FLAGS_HAS_FRAGMENT

@watilde watilde force-pushed the watilde:feature/file-path branch 2 times, most recently Apr 2, 2017

@watilde

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2017

continue;
}
} else if (!has_state_override && ch == '?') {
url->flags |= URL_FLAGS_HAS_QUERY;

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 2, 2017

Member

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

This comment has been minimized.

Copy link
@watilde

watilde Apr 2, 2017

Author Member

oops, I misunderstood it sorry! updated.

watilde added 2 commits Mar 27, 2017
url: change path parsing for non-special URLs
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

@watilde watilde force-pushed the watilde:feature/file-path branch Apr 2, 2017

test: synchronize WPT url setter test data
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

@watilde watilde force-pushed the watilde:feature/file-path branch to b103e6e Apr 2, 2017

@watilde

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2017

@watilde

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2017

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

@watilde watilde closed this Apr 3, 2017

@watilde watilde deleted the watilde:feature/file-path branch Apr 3, 2017

watilde added a commit that referenced this pull request Apr 3, 2017
url: change path parsing for non-special URLs
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>
watilde added a commit that referenced this pull request Apr 3, 2017
test: synchronize WPT url test data
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>
watilde added a commit that referenced this pull request Apr 3, 2017
test: synchronize WPT url setter test data
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>
@jasnell jasnell referenced this pull request Apr 4, 2017
italoacasas added a commit to italoacasas/node that referenced this pull request Apr 10, 2017
test: synchronize WPT url setter test data
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>
@TimothyGu TimothyGu referenced this pull request Apr 19, 2017
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
6 participants
You can’t perform that action at this time.