-
Notifications
You must be signed in to change notification settings - Fork 173
Remove custom [[HasInstance]] behavior from interface objects #356
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
Conversation
This was only ever implemented in Firefox. It was added in 5982ce7. after being removed as a result of https://www.w3.org/Bugs/Public/show_bug.cgi?id=12295. Closes #129.
I support this change. I think it is best to align with EcmaScript here. |
Do you want me to submit a WPT for #129 (comment) with the opposite result? Or are you taking care of it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM aside for the nit.
@@ -6384,9 +6383,8 @@ more of the following are redefined in accordance with the rules for exotic obje | |||
\[[Call]], | |||
\[[Set]], | |||
\[[DefineOwnProperty]], | |||
\[[GetOwnProperty]], | |||
\[[Delete]] and | |||
\[[HasInstance]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still modifying [[HasInstance]] for legacy callback interfaces, so you might want to keep it listed here.
I can take care of the tests; I wanted to make sure this would be merged first. |
It appears nobody implements the throwing [[HasInstance]] for NodeFilter. We should probably just remove that. |
Why is it even specified? |
Follows whatwg/webidl#356 and also adds general coverage for legacy callback interface objects.
Unclear. Maybe because it's not really a sensible thing to be testing. But per spec it will throw anyway in most cases because these objects have no |
Unimplemented everywhere
@bzbarsky OK to merge this? |
Yes. Note that Firefox is probably not going to be able change its behavior for a while; certainly not until we're able to offer a different branding-detection solution to consumers of the current behavior... |
@bzbarsky maybe we could experiment with a chrome-only-branding API? Presumably we don't need it for web sites... |
Follows whatwg/webidl#356 and also adds general coverage for legacy callback interface objects.
We could try. It'd be a bunch of work and compat risk that I'm not willing to spend time on any time in the next 6 months, personally, so just trying to set realistic expectations here. |
Bugs commented on:
Bugs for legacy callback interface objects (not directly related to this PR, but caught while writing tests in web-platform-tests/wpt#5762):
If someone could check http://w3c-test.org/WebIDL/ecmascript-binding/legacy-callback-interface-object.html in Safari Tech Preview and file a bug if necessary, I'd much appreciate it, as I'm away from my testing Mac for the next week. |
Safari TP passes all 8 tests. |
This was only ever implemented in Firefox. It was added in 5982ce7. after being removed as a result of https://www.w3.org/Bugs/Public/show_bug.cgi?id=12295.
Closes #129.
This ensures that authors can maintain the invariant they are used to where
x instanceof Y
means thatY.prototype
's methods are available onx
(which is not true in Firefox). It also aligns with normal author-created or built-in ECMAScript objects, where e.g.{} instanceof otherWindow.Object
returns false.As pointed out in #129, only @travisleithead from a non-Firefox browser expressed support for the currently-specced behavior, and they have not implemented it in the intervening 4 years; Travis, I'd be curious if your team's feelings have changed. Chrome strongly objects to the currently-specced behavior. /cc @cdumez (EDIT: I guess @samweinig is probably the right person for Web IDL) to hear about WebKit's opinion.
As such, we'd like to remove this from the spec, as it seems that it never got consensus for being added, although people may not have voiced their objections in a timely manner when it was originally proposed.
If there is a desire for adding a brand-checking API to the platform, we'd like that to be proposed separately. But simply as a matter of matching reality and improving interop, we think it's important to get the spec fixed on this matter sooner, and not block on such an API addition.
Preview | Diff