Cleanup API for file and non-special URLs #224

annevk · 2017-02-01T13:41:33Z

This:

Stops the username/password/port APIs from functioning when host is
the empty string.
Makes the host/hostname APIs work better with file URLs and adjusts
the file host state accordingly.
Make setting host/hostname to the empty string impossible when they
have a username/password/port.
Fixes file: URL with a port number through the host setter #97.

This: * Stops the username/password/port APIs from functioning when host is the empty string. * Makes the host/hostname APIs work better with file URLs and adjusts the file host state accordingly. * Make setting host/hostname to the empty string impossible when they have a username/password/port. * Fixes #97.

Also file URL host parsing. See whatwg/url#224 for the corresponding URL Standard change.

jasnell · 2017-02-01T14:40:30Z

This is good but the third bullet (make setting host/hostname to the empty string impossible when they have a username/password/port) seems a bit unnatural to me. What might be better is that setting the host/hostname to the empty string would set the username/password/port to empty strings... but of course, that has side effects that may not be obvious to most users so I'm a bit torn on it.

domenic · 2017-02-02T05:03:52Z

url.bs

+
+        <ol>
+         <li><p>If <var>state override</var> is given and <var>url</var>
+         <a>includes credentials</a>, <a>syntax violation</a>, return.


Is "return" the same as "terminate this algorithm" elsewhere?

Yeah. I'm slowly switching to Infra terminology.

I think the inconsistency is pretty confusing; when I implemented in whatwg-url I accidentally translated "return" as "bail out of the entire URL parsing algorithm".

Isn't that what's happening?

No; it's bailing out of this particular state and back into the main parser loop. I.e. it goes back to step 11 ("Keep running the following state machine by switching on state. If after a run pointer points to EOF code point, go to the next step. Otherwise, increase pointer by one and continue with the state machine.")

Err, nevermind, that is what's happening. I guess what I meant is that I translated this as "bail out of this step and go back to step 11". Basically I saw two different types of bail-outs, and since I knew that "abort these steps" meant the one, I assumed that "return" meant the other one. I think it's best to change them all at once or not at all.

domenic · 2017-02-02T05:07:40Z

When updating the setters, unfortunately we need to update HTML's location and HTMLHyperlinkElementUtils as well, usually... :-/

Follows whatwg/url#224.

annevk · 2017-02-02T09:31:41Z

@jasnell yeah, it's mostly an edge case I wanted to take care of to avoid messing up serialization. In particular creating a serialization that could not be parsed into the same components.

Also file URL host parsing. See whatwg/url#224 for the corresponding URL Standard change.

This change depends on whatwg/url#224 landing first. Tests: web-platform-tests/wpt#4696. Location API bits are not tested since we generally treat loading from file URLs as out-of-scope of the web platform, but accounting for it standards-wise seemed trivial enough.

domenic · 2017-02-03T22:16:07Z

It occurs to me this might be cleaner and require fewer changes in the dependent APIs if you still went to the host/hostname state, but as the first step it just switched to the file host state if the scheme is file and state override is given.

annevk · 2017-02-07T08:56:09Z

Would you prefer that? I have to say I have very mixed feelings about the whole state override setup.

domenic · 2017-02-07T18:10:00Z

I think I would. I found the state override setup confusing at first but now that I understand how it works it seems fine. And given that, preserving the idea that we can just do the API stuff by directly moving into the state override makes sense to me.

annevk · 2017-02-08T10:06:18Z

Done. I also verified that the tests still pass with whatwg-url.

domenic · 2017-02-08T17:17:27Z

LGTM but still would prefer not starting to use "return" without switching over everything all at once.

Also file URL host parsing. See whatwg/url#224 for the corresponding URL Standard change.

This change builds on whatwg/url#224. Tests: web-platform-tests/wpt#4696. Location API bits are not tested since we generally treat loading from file URLs as out-of-scope for the web platform, but accounting for it standards-wise seemed trivial enough.

Follows whatwg/url#224.

- Update to spec - Add opaque hosts - File state did not correctly deal with lack of base URL - Cleanup API for file and non-special URLs - Allow % and IPv6 addresses in non-special URL hosts - Use specific names for percent-encode sets - Add empty host concept for file and non-special URLs - Clarify IPv6 serializer - Fix existing mistakes - Add missing ':' to forbidden host code point list. - Correct IPv4 parser empty label behavior - Maintain type equivalence in URLContext with spec - scheme, username, and password should always be strings - host, port, query, and fragment may be strings or null - Align scheme state more closely with the spec - Make sure the `special` variable is always synced with URL_FLAG_SPECIAL. PR-URL: nodejs#12523 Fixes: nodejs#10608 Fixes: nodejs#10634 Refs: whatwg/url#185 Refs: whatwg/url#225 Refs: whatwg/url#224 Refs: whatwg/url#218 Refs: whatwg/url#243 Refs: whatwg/url#260 Refs: whatwg/url#268 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

- Update to spec - Add opaque hosts - File state did not correctly deal with lack of base URL - Cleanup API for file and non-special URLs - Allow % and IPv6 addresses in non-special URL hosts - Use specific names for percent-encode sets - Add empty host concept for file and non-special URLs - Clarify IPv6 serializer - Fix existing mistakes - Add missing ':' to forbidden host code point list. - Correct IPv4 parser empty label behavior - Maintain type equivalence in URLContext with spec - scheme, username, and password should always be strings - host, port, query, and fragment may be strings or null - Align scheme state more closely with the spec - Make sure the `special` variable is always synced with URL_FLAG_SPECIAL. PR-URL: #12523 Fixes: #10608 Fixes: #10634 Refs: whatwg/url#185 Refs: whatwg/url#225 Refs: whatwg/url#224 Refs: whatwg/url#218 Refs: whatwg/url#243 Refs: whatwg/url#260 Refs: whatwg/url#268 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Do not allow username,password,port for URLs without a host or file URLs Imports tests from web-platform-tests/wpt@0e6a90f and tracks URL spec change at whatwg/url#224  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/411)

This change builds on whatwg/url#224. Tests: web-platform-tests/wpt#4696. Location API bits are not tested since we generally treat loading from file URLs as out-of-scope for the web platform, but accounting for it standards-wise seemed trivial enough.

annevk added a commit to web-platform-tests/wpt that referenced this pull request Feb 1, 2017

URL: file and non-special URL setters

512b95e

Also file URL host parsing. See whatwg/url#224 for the corresponding URL Standard change.

annevk mentioned this pull request Feb 1, 2017

URL: file and non-special URL setters web-platform-tests/wpt#4696

Merged

file URLs can never have username/password

c40312a

annevk requested review from jasnell, domenic and achristensen07 February 1, 2017 14:22

domenic reviewed Feb 2, 2017

View reviewed changes

domenic added a commit to jsdom/whatwg-url that referenced this pull request Feb 2, 2017

Spec update: cleanup API for file and non-special URLs

f3de3fa

Follows whatwg/url#224.

domenic mentioned this pull request Feb 2, 2017

Spec update: cleanup API for file and non-special URLs jsdom/whatwg-url#71

Merged

annevk added a commit to web-platform-tests/wpt that referenced this pull request Feb 2, 2017

URL: file and non-special URL setters

04b0796

Also file URL host parsing. See whatwg/url#224 for the corresponding URL Standard change.

abstract cannot have a username/password/port for HTML

1ba8533

annevk mentioned this pull request Feb 2, 2017

Align with URL API changes for file and non-special URLs whatwg/html#2317

Merged

use state override instead for host/hostname setter

10d577f

annevk added a commit to web-platform-tests/wpt that referenced this pull request Feb 8, 2017

URL: file and non-special URL setters

0e6a90f

Also file URL host parsing. See whatwg/url#224 for the corresponding URL Standard change.

annevk merged commit cf616f9 into master Feb 8, 2017

domenic deleted the annevk/host-hostname-file-urls branch February 8, 2017 18:31

domenic added a commit to jsdom/whatwg-url that referenced this pull request Feb 8, 2017

Spec update: cleanup API for file and non-special URLs

940eca5

Follows whatwg/url#224.

domenic added a commit to jsdom/whatwg-url that referenced this pull request Feb 8, 2017

Spec update: cleanup API for file and non-special URLs

cad6187

Follows whatwg/url#224.

joyeecheung mentioned this pull request Apr 11, 2017

URL: Don't accept port numbers for file urls nodejs/node#10608

Closed

TimothyGu mentioned this pull request Apr 19, 2017

url: update WHATWG URL parser to latest spec nodejs/node#12523

Closed

4 tasks

valenting mentioned this pull request Nov 2, 2017

Do not allow username,password,port for URLs without a host or file URLs servo/rust-url#411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup API for file and non-special URLs #224

Cleanup API for file and non-special URLs #224

annevk commented Feb 1, 2017

jasnell commented Feb 1, 2017

domenic Feb 2, 2017

annevk Feb 2, 2017

domenic Feb 3, 2017

annevk Feb 7, 2017

domenic Feb 7, 2017

domenic Feb 7, 2017

domenic commented Feb 2, 2017

annevk commented Feb 2, 2017

domenic commented Feb 3, 2017

annevk commented Feb 7, 2017

domenic commented Feb 7, 2017

annevk commented Feb 8, 2017

domenic commented Feb 8, 2017

Cleanup API for file and non-special URLs #224

Cleanup API for file and non-special URLs #224

Conversation

annevk commented Feb 1, 2017

jasnell commented Feb 1, 2017

domenic Feb 2, 2017

Choose a reason for hiding this comment

annevk Feb 2, 2017

Choose a reason for hiding this comment

domenic Feb 3, 2017

Choose a reason for hiding this comment

annevk Feb 7, 2017

Choose a reason for hiding this comment

domenic Feb 7, 2017

Choose a reason for hiding this comment

domenic Feb 7, 2017

Choose a reason for hiding this comment

domenic commented Feb 2, 2017

annevk commented Feb 2, 2017

domenic commented Feb 3, 2017

annevk commented Feb 7, 2017

domenic commented Feb 7, 2017

annevk commented Feb 8, 2017

domenic commented Feb 8, 2017