Add opaque hosts #185

annevk · 2016-12-28T17:20:28Z

For URLs without a special scheme we cannot use the host parser
directly due to compatibility issues and we need to serialize slightly
differently too.

Fixes #148.

annevk · 2016-12-28T17:20:52Z

@achristensen07 what do you think of this approach?

achristensen07 · 2016-12-28T17:35:26Z

Seems good. Make sure to add a test verifying something like "new URL('path', 'asdf://host')" serializes to 'asdf://host/path' adding the slash. I broke something by forgetting that in my initial implementation. I don't add a slash for query-only or fragment-only relative URLs, but that might be worth more discussion.

annevk · 2016-12-28T17:52:12Z

@achristensen07 running https://quuz.org/url/liveview.html#notspecial://test?x in Safari TP does yield a slash for me.

achristensen07 · 2016-12-28T17:56:49Z

Yes, but "new URL('?query', 'asdf://host')" does not add a slash right now in STP. Maybe this is inconsistent.

I think this is a good approach, but we might need another issue for these query-only and fragment-only URLs with non-special schemes.

domenic · 2016-12-28T18:10:29Z

url.bs

+for further processing.
+
+<p class="note no-backref">A <a>non-special-URL host</a> is emphatically not a <a for=/>host</a> and
+only used by <a lt="is special">non-special URLs</a>.


is only used by...?

"X is A and B" seems fine?

When you add "not"s it's confusing. "X is not A and B" could be either "X is not A and X is not B" or "X is not A and X is B".

domenic · 2016-12-28T18:12:09Z

url.bs

- <a for=url>path</a> (including empty strings), separated from each other by
- "<code>/</code>", to <var>output</var>.
+ <li>
+  <p>Otherwise, then:


Remove ", then" here

annevk · 2016-12-29T08:58:12Z

url.bs

+   <li><p>Return the result of <a lt="host parser">host parsing</a> <var>input</var>.
+  </ol>
+
+ <li><p>Return <var>input</var>.


I either need to percent-encode input here or in the state machine.

annevk · 2016-12-29T13:00:30Z

The pathname getter should probably also be adjusted in a way similar to the serializer.

URL Standard change: whatwg/url#185.

annevk · 2016-12-29T13:08:39Z

Yes, but "new URL('?query', 'asdf://host')" does not add a slash right now in STP. Maybe this is inconsistent.

Yeah I think that would be quite odd and required a slash in the tests (and standard) for now. I guess I should also test asdf://host?query. (Edit: tested and it seems that Safari TP does add a slash there.)

This is a theoretical issue once #185 lands, but as discussed it seems good to address this anyway. Fixes #79.

domenic · 2016-12-29T16:41:22Z

url.bs

   <a lt="host serializer">serialized</a>, to <var>output</var>.

+   <li><p>Otherwise, append <var>url</var>'s <a for=url>host</a>, to <var>output</var>.


s/, to output/ to output/

I think that's currently somewhat consistently "wrong"...

domenic · 2016-12-29T17:00:40Z

This doesn't seem to work either, testing in whatwg-url. In particular sc://ñ does not parse as having an empty path, but instead a path containing a single empty string item. Again could be a bug in my spec translation. I'll post a whatwg-url PR for you to review.

This matches whatwg/url#185.

annevk · 2016-12-30T11:54:45Z

Yeah, you are right. Even with EOF you get a path that is non-empty.

Another problem here is not forbidding certain code points in the host setter.

I guess I should rethink this.

The other question of course is whether all this additional complexity is worth it, versus treating non-special schemes as just having a path and not being able to be base URLs for each other. (Which is what most browsers implement at the moment.)

annevk · 2016-12-30T14:38:26Z

One thought I had is that it's perhaps worth splitting the non-special-URL host change and the change to not serialize a path if none was in the input.

But it might be easy to do the no path in input change. It only really matters if the URL is not special and you hit EOF during the host/hostname state, right? It should be easy enough to account for that.

annevk · 2017-01-05T15:14:43Z

I'm going to pursue making this work, since it seems worthwhile to support relative URLs for non-special schemes, even if it comes at a cost somewhat greater than initially anticipated. I don't think this is the reason to move to the Chrome/Firefox model of effectively setting the cannot-be-base-URL flag for all non-special URLs.

I landed a commit that addresses the issue @domenic found, but I also need to look through the tests again since I think I missed updating a couple tests (in particular setter tests).

URL Standard change: whatwg/url#185.

annevk · 2017-01-05T15:42:27Z

I also updated the tests. I didn't add any new host/hostname setter tests however.

annevk · 2017-01-05T18:53:26Z

I forgot to address "Another problem here is not forbidding certain code points in the host setter" so this is still not ready.

annevk · 2017-01-06T12:13:37Z

The other thing I noticed that while testing is that currently we allow userinfo and a port to exist while host is empty. That shouldn't be allowed (and isn't in Safari).

annevk · 2017-01-21T10:55:54Z

Also, come to think of it, it's basically the same code points that are restricted. I just removed the code points that are not problematic for non-special URLs, such as [ and ] (restricted for IPv6).

annevk · 2017-01-21T10:56:56Z

We could keep the list of restricted code points the same though if you think that is better. I don't care strongly.

achristensen07 · 2017-01-24T08:04:03Z

"https://" is now invalid in WebKit. I don't think it's worth allowing it in the spec. It's a relatively small compatibility issue.

annevk · 2017-01-24T11:27:59Z

@domenic this PR and the corresponding tests at web-platform-tests/wpt#4406 can be reviewed again. I have verified it with whatwg-url, but haven't updated my patch for it. Since it's a rather trivial change it seems good to use that as a test to ensure you get the same results.

domenic · 2017-01-24T19:37:11Z

url.bs

+            idempotent:
+            https://@/example.org/ -> https:///example.org/ -> https://example.org/ -->
+
+       <li><p>Decrease <var>pointer</var> by the number of code points in <var>buffer</var> plus


These could be separate substeps

I left this as-is since the parser generally groups steps that can reasonably be grouped.

Follows whatwg/url#185.

URL Standard change: whatwg/url#185.

Follows whatwg/url#185.

This is a theoretical issue once #185 lands, but as discussed it seems good to address this anyway. Fixes #79.

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

annevk added the needs tests Moving the issue forward requires someone to write tests label Dec 28, 2016

domenic reviewed Dec 28, 2016

View reviewed changes

annevk commented Dec 29, 2016

View reviewed changes

annevk added a commit to web-platform-tests/wpt that referenced this pull request Dec 29, 2016

URL: test host parsing of URLs with unknown schemes

2c87b7d

URL Standard change: whatwg/url#185.

annevk mentioned this pull request Dec 29, 2016

URL: test host parsing of URLs with unknown schemes web-platform-tests/wpt#4406

Merged

annevk removed the needs tests Moving the issue forward requires someone to write tests label Dec 29, 2016

annevk mentioned this pull request Dec 29, 2016

Parsing an empty host #79

Closed

annevk added a commit that referenced this pull request Dec 29, 2016

IPv4 parser: handle the empty string

e3370f1

This is a theoretical issue once #185 lands, but as discussed it seems good to address this anyway. Fixes #79.

annevk mentioned this pull request Dec 29, 2016

IPv4 parser: handle the empty string #189

Merged

domenic reviewed Dec 29, 2016

View reviewed changes

domenic added a commit to jsdom/whatwg-url that referenced this pull request Dec 29, 2016

Spec update: treat URLs without a special scheme differently

ecb808e

This matches whatwg/url#185.

domenic mentioned this pull request Dec 29, 2016

Spec update: treat URLs without a special scheme differently jsdom/whatwg-url#63

Closed

annevk mentioned this pull request Jan 3, 2017

protocol setter changing between special and non-special schemes #104

Closed

annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Jan 4, 2017

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

URL: test host parsing of URLs with unknown schemes

534bf95

URL Standard change: whatwg/url#185.

annevk removed the needs implementer interest Moving the issue forward requires implementers to express interest label Jan 5, 2017

jasnell mentioned this pull request Jan 5, 2017

url: track whatwg url issue #185 (non-special URL host) nodejs/node#10634

Closed

annevk mentioned this pull request Jan 23, 2017

Forbidden host code points #214

Closed

annevk changed the title ~~Add non-special-URL hosts~~ Add opague hosts Jan 24, 2017

annevk changed the title ~~Add opague hosts~~ Add opaque hosts Jan 24, 2017

see #214 (and mention as fixed in the final commit message)

1f2728b

oops, shouldn't be part of syntax

9035b30

annevk requested a review from domenic January 24, 2017 11:38

domenic approved these changes Jan 24, 2017

View reviewed changes

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

Spec update: add opaque hosts

ee6dc82

Follows whatwg/url#185.

domenic mentioned this pull request Jan 24, 2017

Spec update: add opaque hosts jsdom/whatwg-url#69

Merged

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

Spec update: add opaque hosts

3709e38

Follows whatwg/url#185.

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

URL: host parsing of URLs with unknown schemes

69c16f6

URL Standard change: whatwg/url#185.

annevk merged commit 3036255 into master Jan 24, 2017

annevk deleted the annevk/non-special-url-hosts branch January 24, 2017 19:53

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

Spec update: add opaque hosts

0d03572

Follows whatwg/url#185.

annevk added a commit that referenced this pull request Jan 24, 2017

IPv4 parser: handle the empty string

cb146b9

This is a theoretical issue once #185 lands, but as discussed it seems good to address this anyway. Fixes #79.

TimothyGu mentioned this pull request Apr 19, 2017

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

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opaque hosts #185

Add opaque hosts #185

annevk commented Dec 28, 2016

annevk commented Dec 28, 2016

achristensen07 commented Dec 28, 2016

annevk commented Dec 28, 2016

achristensen07 commented Dec 28, 2016

domenic Dec 28, 2016

annevk Dec 28, 2016

domenic Dec 28, 2016 •

edited

Loading

domenic Dec 28, 2016

annevk Dec 29, 2016

annevk commented Dec 29, 2016

annevk commented Dec 29, 2016 •

edited

Loading

domenic Dec 29, 2016

annevk Dec 30, 2016

domenic commented Dec 29, 2016

annevk commented Dec 30, 2016

annevk commented Dec 30, 2016

annevk commented Jan 5, 2017

annevk commented Jan 5, 2017

annevk commented Jan 5, 2017

annevk commented Jan 6, 2017

annevk commented Jan 21, 2017

annevk commented Jan 21, 2017

achristensen07 commented Jan 24, 2017

annevk commented Jan 24, 2017

domenic Jan 24, 2017

annevk Jan 24, 2017

		<a lt="host serializer">serialized</a>, to <var>output</var>.

		<li><p>Otherwise, append <var>url</var>'s <a for=url>host</a>, to <var>output</var>.

Add opaque hosts #185

Add opaque hosts #185

Conversation

annevk commented Dec 28, 2016

annevk commented Dec 28, 2016

achristensen07 commented Dec 28, 2016

annevk commented Dec 28, 2016

achristensen07 commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic Dec 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Dec 29, 2016

annevk commented Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Dec 29, 2016

annevk commented Dec 30, 2016

annevk commented Dec 30, 2016

annevk commented Jan 5, 2017

annevk commented Jan 5, 2017

annevk commented Jan 5, 2017

annevk commented Jan 6, 2017

annevk commented Jan 21, 2017

annevk commented Jan 21, 2017

achristensen07 commented Jan 24, 2017

annevk commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic Dec 28, 2016 •

edited

Loading

annevk commented Dec 29, 2016 •

edited

Loading