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.

Sign up for GitHub

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

Jump to bottom

url: fix file state clarification in binding #11123

Closed
wants to merge 3 commits into from
Closed

url: fix file state clarification in binding #11123

wants to merge 3 commits into from

Conversation

watilde
Copy link
Member

@watilde watilde commented Feb 2, 2017 •
edited
Loading

The state of query and fragment in file state clarification should be adjusted after the state of the path is confirmed.

Applicable cases:

  • file:#foo => file:///#foo
  • file:?bar => file:///?bar

Fixes: #10978

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

url

@nodejs-github-bot 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 Feb 2, 2017
@TimothyGu
Copy link
Member

+1 on the change, but I'd rather wait for the spec to update (see whatwg/url#225) before applying this. Official WPT tests can also be found after web-platform-tests/wpt#4700 is merged.

@watilde
Copy link
Member Author

watilde commented Feb 2, 2017

Wow this is nice:

Official WPT tests can also be found after web-platform-tests/wpt#4700 is merged.

Then let's wait for the tests to come upstream, and update them when they get merged. After that, I will remove my test from this PR.

@TimothyGu TimothyGu added the stalled Issues and PRs that are stalled. label Feb 2, 2017
@watilde
Copy link
Member Author

watilde commented Feb 7, 2017 •
edited
Loading

Since the PRs on WHATWG and WPT were merged, I've updated this PR.

Here is the summary:

@joyeecheung
Copy link
Member

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

@watilde
Copy link
Member Author

watilde commented Feb 13, 2017 •
edited
Loading

@nodejs/url Could we possibly update the status of this PR since the spec and the test were already merged into upstream?

@joyeecheung
Copy link
Member

joyeecheung commented Feb 13, 2017 •
edited
Loading

@watilde Do you mean if this is ready to land? There are some CI checks failed so probably need to investigate first(I cannot go look into it right now because the connection to the UI is super slow from here, not sure if it's my connection or the infra?)

@watilde
url: fix file state clarification in binding
a723caa 
The state of query and fragment in file state
clarification should be adjusted after the state of
the path is confirmed.

Applicable cases:
+ `file:#foo` => `file:///#foo`
+ `file:?bar` => `file:///?bar`

Fixes: nodejs#10978
@watilde
url, test: synchronize WPT url tests for file URL
b47f916 
Added at the following PR:
+ web-platform-tests/wpt#4382
+ web-platform-tests/wpt#4700
@watilde
url: empty file URL should be parsed correctly
e39218b 
An empty file URL `file:` should be parsed to `file:///` instead of
`file://`. In the `kFile` state, the process was braked immediately
when the ch is EOL, but it should work as `default` in the kFile state
to adjust slashes.
@watilde
Copy link
Member Author

watilde commented Feb 13, 2017

test/arm looks fine:

For these two, I have no idea so far since this update shouldn't affect the cases.

Either way, I will do git rebase master, and continue to investigate then.

@jasnell
Copy link
Member

jasnell commented Feb 13, 2017

New CI: https://ci.nodejs.org/job/node-test-pull-request/6378/

The smartos failure looks unrelated. The freebsd one is... troubling. Running again

@TimothyGu TimothyGu removed the stalled Issues and PRs that are stalled. label Feb 13, 2017
@watilde
Copy link
Member Author

watilde commented Feb 14, 2017

The error on ARM looks unrelated: https://ci.nodejs.org/job/node-test-commit-arm/nodes=ubuntu1604-arm64/7719/console

jasnell pushed a commit that referenced this pull request Feb 17, 2017
@watilde @jasnell
url: fix file state clarification in binding
97d111d 
An empty file URL `file:` should be parsed to `file:///` instead of
`file://`. In the `kFile` state, the process was braked immediately
when the ch is EOL, but it should work as `default` in the kFile state
to adjust slashes.

Applicable cases:
* `file:#foo` => `file:///#foo`
* `file:?bar` => `file:///?bar`

PR-URL: #11123
Fixes: #10978
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request Feb 17, 2017
@watilde @jasnell
url, test: synchronize WPT url tests for file URL
4577775 
Added at the following PR:
* web-platform-tests/wpt#4382
* web-platform-tests/wpt#4700

PR-URL: #11123
Fixes: #10978
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 17, 2017

Landed in 97d111d...4577775

@jasnell jasnell closed this Feb 17, 2017
@watilde watilde deleted the fix-binding-url-parse branch February 17, 2017 14:51
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
@watilde @italoacasas
url: fix file state clarification in binding
1a70f6c 
An empty file URL `file:` should be parsed to `file:///` instead of
`file://`. In the `kFile` state, the process was braked immediately
when the ch is EOL, but it should work as `default` in the kFile state
to adjust slashes.

Applicable cases:
* `file:#foo` => `file:///#foo`
* `file:?bar` => `file:///?bar`

PR-URL: nodejs#11123
Fixes: nodejs#10978
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
@watilde @italoacasas
url, test: synchronize WPT url tests for file URL
f189623 
Added at the following PR:
* web-platform-tests/wpt#4382
* web-platform-tests/wpt#4700

PR-URL: nodejs#11123
Fixes: nodejs#10978
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@watilde @italoacasas
url: fix file state clarification in binding
72da362 
An empty file URL `file:` should be parsed to `file:///` instead of
`file://`. In the `kFile` state, the process was braked immediately
when the ch is EOL, but it should work as `default` in the kFile state
to adjust slashes.

Applicable cases:
* `file:#foo` => `file:///#foo`
* `file:?bar` => `file:///?bar`

PR-URL: #11123
Fixes: #10978
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@watilde @italoacasas
url, test: synchronize WPT url tests for file URL
d8cb65a 
Added at the following PR:
* web-platform-tests/wpt#4382
* web-platform-tests/wpt#4700

PR-URL: #11123
Fixes: #10978
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@watilde @TimothyGu @joyeecheung @jasnell @nodejs-github-bot