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

Consider removing the gopher special-case. #342

Closed
mikewest opened this issue Aug 16, 2017 · 13 comments · Fixed by #453
Closed

Consider removing the gopher special-case. #342

mikewest opened this issue Aug 16, 2017 · 13 comments · Fixed by #453

Comments

@mikewest
Copy link
Member

gopher is called out as a special scheme with a default port of 70 (https://url.spec.whatwg.org/#url-miscellaneous). Browsers generally implement this bit of the spec. I'd suggest that it's time we stopped doing so.

So far as I can tell, Chrome's support for gopher is limited to URL parsing. The protocol itself isn't supported, and gopher: URLs in the address bar are treated as searches. Gecko looks like it has similar behavior.

Perhaps we could consider dropping the special-casing in URL, and just treating gopher: like any other scheme that the browser doesn't understand. That is, instead of parsing gopher://hostname.goes.here like we'd parse http://hostname.goes.here, we'd parse it like not-a-scheme://hostname.goes.here. This wouldn't preclude user agents from supporting the protocol with external handlers, of course, but it would allow them to stop pretending that they understand the scheme when they clearly don't.

WDYT, @annevk, @mcmanus, @sleevi, @travisleithead, @achristensen07?

@annevk
Copy link
Member

annevk commented Aug 16, 2017

Would you also argue that once we drop support for FTP, we start parsing those URLs differently? I guess I don't care too strongly either way, but it seems a little strange just in order to get rid of lookup entry.

cc @valenting

@mikewest
Copy link
Member Author

Would you also argue that once we drop support for FTP, we start parsing those URLs differently?

Yes. I can't think of a good reason to treat protocols that user agents don't actually understand differently than other protocols that user agents don't understand. We don't have special processing for ssh:, for instance, and I'd suggest that that's more widely used than gopher:.

I guess I don't care too strongly either way, but it seems a little strange just in order to get rid of lookup entry.

It's certainly not much code, and has little practical impact either way. But removing it will mean that I never have to reassure myself that Chromium's net stack doesn't actually support Gopher even through kGopherScheme is defined in Chromium's URL library, nor will gopher: come up when discussing UI treatment of various schemes in the context of various stages of HTTP BAD with good folks like @estark37.

It's cruft that seems very safe to remove.

@annevk
Copy link
Member

annevk commented Aug 16, 2017

We don't have special processing for blob: either, yet that is used. That's not really an argument one way or another in my opinion. If I have a JavaScript application that parses URLs or implements Gopher suddenly gopher:///test/ would end up parsing differently and perhaps break things. It's probably okay, but not great.

@mikewest
Copy link
Member Author

We don't have special processing for blob: either, yet that is used.

We do have special processing for blob: in URL, both in parsing and in mapping the URL to an origin. Moreover, blobs are in active use with semantics interoperably implemented by user agents. They're a little different than gopher:. :)

If I have a JavaScript application that parses URLs or implements Gopher suddenly gopher:///test/ would end up parsing differently and perhaps break things. It's probably okay, but not great.

I think that the changes to the gopher: test results in https://github.com/w3c/web-platform-tests/tree/master/url would cause some churn. I'd be pretty surprised if that churn broke any applications, because I'd be surprised if gopher: was in anything resembling wide use.

@annevk
Copy link
Member

annevk commented Aug 16, 2017

I don't really see the point of breaking the parser behavior, especially since you indicate you plan on continuing to do so. I don't think that's a good idea as it erodes trust in stability of standards.

(There's no parsing for blob: URLs in the URL Standard. That should be handled by a higher processing layer (same as with data: URLs). What some browsers have as URL parser is not generic and considered a bug as that's rather hard to reconcile with the idea of the parser being mostly scheme-agnostic.)

@mikewest
Copy link
Member Author

I don't really see the point of breaking the parser behavior, especially since you indicate you plan on continuing to do so. I don't think that's a good idea as it erodes trust in stability of standards.

I didn't mean to imply that we'd drop ftp: URL parsing the instant we dropped support for the FTP protocol. Given the usage, I expect that to take some time to be able to do safely, as with many other deprecations. Still, it seems like the right eventual direction, doesn't it? If there's no special behavior, special-casing the scheme is just cruft.

There's no parsing for blob: URLs in the URL Standard.

Steps 3-6 of https://url.spec.whatwg.org/#url-parsing process blob: distinctly from other scheme types. But I think I agree with the wider claim that the parser should be scheme-agnostic.

@annevk
Copy link
Member

annevk commented Aug 16, 2017

If there's no special behavior, special-casing the scheme is just cruft.

But there is special parsing behavior and there's no security risk associated with it. It seems weird to change the parsing of URLs over time. The whole idea of this effort is to put a stop to that and give folks consistent behavior.

@jasnell
Copy link
Collaborator

jasnell commented Aug 16, 2017

In general, I'd prefer to keep the current special casing in place unless there's enough of a significant justification for breaking, and I don't believe the reason given here meets that bar.

@achristensen07
Copy link
Collaborator

Having it or removing it makes no difference to me. I’ve seen no reported problems involving gopher url parsing

@mnot
Copy link
Member

mnot commented Aug 16, 2017

@jasnell afraid of the 418 pitchfork-wielding mob being reactivated, eh? ;)

@achristensen07
Copy link
Collaborator

I've implemented this change in https://bugs.webkit.org/show_bug.cgi?id=201852
Firefox already doesn't parse gopher as a special scheme. Let's change the spec.

caiolima pushed a commit to caiolima/webkit that referenced this issue Sep 17, 2019
https://bugs.webkit.org/show_bug.cgi?id=201852

Patch by Alex Christensen <achristensen@webkit.org> on 2019-09-16
Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

* web-platform-tests/url/a-element-expected.txt:
* web-platform-tests/url/a-element-origin-expected.txt:
* web-platform-tests/url/a-element-origin-xhtml-expected.txt:
* web-platform-tests/url/a-element-xhtml-expected.txt:
* web-platform-tests/url/url-constructor-expected.txt:
* web-platform-tests/url/url-origin-expected.txt:
* web-platform-tests/url/url-setters-expected.txt:

Source/WTF:

There is little meaningful content on gopher servers, and WebKit does not actually support gopher.
This makes WebKit match the behavior of Gecko and goes along with a change proposed at
whatwg/url#342

* wtf/URLParser.cpp:
(WTF::URLParser::defaultPortForProtocol):
(WTF::scheme):
(WTF::URLParser::copyURLPartsUntil):
(WTF::URLParser::parse):

Tools:

* TestWebKitAPI/Tests/WTF/URLParser.cpp:
(TestWebKitAPI::TEST_F):
* TestWebKitAPI/Tests/WebCore/URLParserTextEncoding.cpp:
(TestWebKitAPI::TEST_F):
* TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:

LayoutTests:

* fast/url/invalid-urls-utf8-expected.txt:
* fast/url/invalid-urls-utf8.html:
* fast/url/segments-expected.txt:
* fast/url/segments.html:
* fast/url/standard-url-expected.txt:
* fast/url/standard-url.html:
* fetch/fetch-urls.json:
* http/wpt/html/browsers/history/the-location-interface/location-protocol-setter-non-broken-async-delegate-expected.txt:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@249941 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@mikewest
Copy link
Member Author

FWIW: I'm planning on following @achristensen07's example in https://chromium-review.googlesource.com/c/chromium/src/+/612983. I think we've both just checked-in failing expectations for the existing URL tests. Our results differ due to https://crbug.com/869291 (I think WebKit's correct and Chrome is wrong), but it's probably a good idea to update the test expectations accordingly.

@annevk
Copy link
Member

annevk commented Oct 14, 2019

I'm still not convinced this is a worthwhile endeavor, but as everyone is planning on shipping this it would be nice if someone wrote a PR and updated WPT.

achristensen07 added a commit to achristensen07/url that referenced this issue Oct 17, 2019
achristensen07 added a commit to achristensen07/wpt that referenced this issue Oct 17, 2019
annevk pushed a commit to web-platform-tests/wpt that referenced this issue Oct 18, 2019
zloirock added a commit to zloirock/core-js that referenced this issue Oct 18, 2019
rmisev added a commit that referenced this issue Oct 18, 2019
After #453 the "gopher:" is not special anymore.
According to #342 an intention is that gopher's URL's origin would be opaque (null).
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 24, 2019
…emes, a=testonly

Automatic update from web-platform-tests
URL: make gopher behave like unrecognized schemes

See whatwg/url#342 for context.
--

wpt-commits: 8a9d0ccbeabbec3df4c38a394651d1b53dd2f8c5
wpt-pr: 19770
xeonchen pushed a commit to xeonchen/gecko that referenced this issue Oct 24, 2019
…emes, a=testonly

Automatic update from web-platform-tests
URL: make gopher behave like unrecognized schemes

See whatwg/url#342 for context.
--

wpt-commits: 8a9d0ccbeabbec3df4c38a394651d1b53dd2f8c5
wpt-pr: 19770
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 27, 2019
…emes, a=testonly

Automatic update from web-platform-tests
URL: make gopher behave like unrecognized schemes

See whatwg/url#342 for context.
--

wpt-commits: 8a9d0ccbeabbec3df4c38a394651d1b53dd2f8c5
wpt-pr: 19770

UltraBlame original commit: 993dacde06c1a64aac243210577193125b8948a1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 27, 2019
…emes, a=testonly

Automatic update from web-platform-tests
URL: make gopher behave like unrecognized schemes

See whatwg/url#342 for context.
--

wpt-commits: 8a9d0ccbeabbec3df4c38a394651d1b53dd2f8c5
wpt-pr: 19770

UltraBlame original commit: 993dacde06c1a64aac243210577193125b8948a1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 27, 2019
…emes, a=testonly

Automatic update from web-platform-tests
URL: make gopher behave like unrecognized schemes

See whatwg/url#342 for context.
--

wpt-commits: 8a9d0ccbeabbec3df4c38a394651d1b53dd2f8c5
wpt-pr: 19770

UltraBlame original commit: 993dacde06c1a64aac243210577193125b8948a1
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=201852

Patch by Alex Christensen <achristensen@webkit.org> on 2019-09-16
Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

* web-platform-tests/url/a-element-expected.txt:
* web-platform-tests/url/a-element-origin-expected.txt:
* web-platform-tests/url/a-element-origin-xhtml-expected.txt:
* web-platform-tests/url/a-element-xhtml-expected.txt:
* web-platform-tests/url/url-constructor-expected.txt:
* web-platform-tests/url/url-origin-expected.txt:
* web-platform-tests/url/url-setters-expected.txt:

Source/WTF:

There is little meaningful content on gopher servers, and WebKit does not actually support gopher.
This makes WebKit match the behavior of Gecko and goes along with a change proposed at
whatwg/url#342

* wtf/URLParser.cpp:
(WTF::URLParser::defaultPortForProtocol):
(WTF::scheme):
(WTF::URLParser::copyURLPartsUntil):
(WTF::URLParser::parse):

Tools:

* TestWebKitAPI/Tests/WTF/URLParser.cpp:
(TestWebKitAPI::TEST_F):
* TestWebKitAPI/Tests/WebCore/URLParserTextEncoding.cpp:
(TestWebKitAPI::TEST_F):
* TestWebKitAPI/Tests/WebKitCocoa/WKURLSchemeHandler-1.mm:

LayoutTests:

* fast/url/invalid-urls-utf8-expected.txt:
* fast/url/invalid-urls-utf8.html:
* fast/url/segments-expected.txt:
* fast/url/segments.html:
* fast/url/standard-url-expected.txt:
* fast/url/standard-url.html:
* fetch/fetch-urls.json:
* http/wpt/html/browsers/history/the-location-interface/location-protocol-setter-non-broken-async-delegate-expected.txt:

Canonical link: https://commits.webkit.org/215492@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@249941 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants