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

Let webkitURL be an alias of the URL constructor #135

Closed
foolip opened this Issue Jul 27, 2016 · 12 comments

Comments

6 participants
@foolip
Copy link
Member

foolip commented Jul 27, 2016

Measuring usage of attributes on the global object is hard because it's tainted by enumeration, but this is what we have:
https://www.chromestatus.com/metrics/feature/timeline/popularity/283

(Compare to https://www.chromestatus.com/metrics/feature/timeline/popularity/356 which is likely used very little but is also at ~0.1%.)

window.webkitURL has been deprecated in Blink for a long time, and I need to either remove it or get rid of the deprecation message. I'm leaning towards just spec'ing it, because Edge has window.webkitURL and it's trivial to support if you also have the infrastructure for other constructor aliases like WebKitAnimationEvent or, perhaps, HTMLDocument as an alias of Document (not yet implemented).

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Jul 27, 2016

What would we add, NamedConstructor=webkitURL? @bakulf thoughts?

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Jul 27, 2016

No, just the aliasing prose like HTML's

For historical reasons, Window objects must also have a writable, configurable, non-enumerable property named HTMLDocument whose value is the Document interface object.

We could consider formalizing this in IDL with some new extended attribute...

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Oct 19, 2016

@miketaylr have you seen this in the wild as a problem for Firefox? Seems like we should just add it given that all other browsers ship with it.

@foolip how many aliases do we have? Is it worth adding IDL syntax?

@foolip

This comment has been minimized.

Copy link
Member Author

foolip commented Oct 19, 2016

Blink has a FooConstructor syntax for this, this one's attribute URLConstructor webkitURL.

Other aliases are WebKitTransitionEvent, WebKitAnimationEvent, WebKitMutationObserver, webkitIDBCursor, webkitIDBDatabase, webkitIDBFactory, webkitIDBIndex, webkitIDBKeyRange, webkitIDBObjectStore, webkitIDBRequest, webkitIDBTransaction, webkitIDBCursor, webkitIDBDatabase, webkitIDBFactory, webkitIDBIndex, webkitIDBKeyRange, webkitIDBObjectStore, webkitIDBRequest, webkitIDBTransaction, webkitMediaStream, webkitRTCPeerConnection, webkitSpeechGrammar, webkitSpeechGrammarList, webkitSpeechRecognition, webkitSpeechRecognitionError, webkitSpeechRecognitionEvent, webkitAudioContext, and webkitOfflineAudioContext.

For webkitURL specifically, it'd be worth looking into the risk that it's used to sniff WebKit, in which case it could make things worse for Gecko. It has been deprecated for a long time in Blink. I'd be very interested to hear if it's created trouble for Firefox.

(More generally I'd be interested to hear what non-standard stuff would be most useful to remove from Blink, there's a lot to choose from.)

@miketaylr

This comment has been minimized.

Copy link

miketaylr commented Oct 20, 2016

@miketaylr have you seen this in the wild as a problem for Firefox? Seems like we should just add it given that all other browsers ship with it.

I'm not aware of any broken sites for Firefox due to webkitURL. Most usage I see is window.URL || window.webkitURL. But, that doesn't mean they're not out there. I'd really like to know what made Edge implement -- they've been really good at backing up these decisions with data. It's possible that having "Chrome" in their UA string has led them down code paths that require it.

(edit: after poking around a bit more on GitHub, there's quite a bit of code like https://github.com/catarak/beatsncode/blob/0bfe7025d4535a57930172c832e3fa0562f9b381/js/main.js. So even though I don't know about any broken sites, I don't doubt they're out there.)

@zcorpan

This comment has been minimized.

Copy link
Member

zcorpan commented May 22, 2017

A github search:
https://github.com/search?l=JavaScript&p=2&q=webkitURL+NOT+window.URL+NOT+self.URL+NOT+"typeof+URL"+NOT+hasOwnProperty%28%27URL%27%29&type=Code&utf8=✓

Chrome use counter is around 0.1%
https://www.chromestatus.com/metrics/feature/timeline/popularity/283

WebKit just fixed https://bugs.webkit.org/show_bug.cgi?id=172166 to stop exposing the webkitURL alias in workers.

We now have IDL syntax for this. It seems the shortest and least painful path to interop is to specify [LegacyWindowAlias=webkitURL].

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 22, 2017

Sounds reasonable. Is there a Gecko issue on adding IDL support for that extended attribute? Does IDL harness support it so I don't need to write more tests than just adding it to url/interfaces.any.js?

zcorpan added a commit that referenced this issue May 22, 2017

@zcorpan

This comment has been minimized.

Copy link
Member

zcorpan commented May 22, 2017

IDL harness support was added in web-platform-tests/wpt#5966

I can file a bug for Gecko for implementing LegacyWindowAlias.
Edit: https://bugzilla.mozilla.org/show_bug.cgi?id=1366738

@foolip

This comment has been minimized.

Copy link
Member Author

foolip commented May 29, 2017

It's possible that having "Chrome" in their UA string has led them down code paths that require it.

It also seems plausible that the opposite could also happen. Supporting certain APIs that are used to detect WebKit could also increase the pressure to put "WebKit" (or "Chrome") in the UA string.

@bakulf

This comment has been minimized.

Copy link

bakulf commented May 31, 2017

I'm OK with having [LegacyWindowAlias]. Temporary solution can be the use of NamedConstructor in gecko code.

@zcorpan

This comment has been minimized.

Copy link
Member

zcorpan commented May 31, 2017

OK great, then we can go ahead and merge.

@zcorpan zcorpan closed this in #312 May 31, 2017

zcorpan added a commit that referenced this issue May 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.