Skip to content

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

Merged
merged 3 commits into from
May 4, 2017

Conversation

domenic
Copy link
Member

@domenic domenic commented May 1, 2017

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 that Y.prototype's methods are available on x (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

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.
@cdumez
Copy link

cdumez commented May 2, 2017

I support this change. I think it is best to align with EcmaScript here.

@annevk
Copy link
Member

annevk commented May 2, 2017

Do you want me to submit a WPT for #129 (comment) with the opposite result? Or are you taking care of it?

Copy link
Collaborator

@tobie tobie left a 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]].
Copy link
Collaborator

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.

@domenic
Copy link
Member Author

domenic commented May 2, 2017

I can take care of the tests; I wanted to make sure this would be merged first.

@domenic
Copy link
Member Author

domenic commented May 2, 2017

It appears nobody implements the throwing [[HasInstance]] for NodeFilter. We should probably just remove that.

@tobie
Copy link
Collaborator

tobie commented May 2, 2017

It appears nobody implements the throwing [[HasInstance]] for NodeFilter. We should probably just remove that.

Why is it even specified?

domenic added a commit to web-platform-tests/wpt that referenced this pull request May 2, 2017
Follows whatwg/webidl#356 and also adds general coverage for legacy callback interface objects.
@domenic
Copy link
Member Author

domenic commented May 2, 2017

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 .prototype; see forthcoming tests.

@domenic
Copy link
Member Author

domenic commented May 3, 2017

@bzbarsky OK to merge this?

@bzbarsky
Copy link
Collaborator

bzbarsky commented May 4, 2017

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

@annevk
Copy link
Member

annevk commented May 4, 2017

@bzbarsky maybe we could experiment with a chrome-only-branding API? Presumably we don't need it for web sites...

@domenic domenic merged commit d0899e9 into master May 4, 2017
@domenic domenic deleted the hasinstance-non-exotic branch May 4, 2017 05:11
domenic added a commit to web-platform-tests/wpt that referenced this pull request May 4, 2017
Follows whatwg/webidl#356 and also adds general coverage for legacy callback interface objects.
@bzbarsky
Copy link
Collaborator

bzbarsky commented May 4, 2017

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.

@domenic
Copy link
Member Author

domenic commented May 4, 2017

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.

@annevk
Copy link
Member

annevk commented May 4, 2017

Safari TP passes all 8 tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Should probably spec a hasInstance for DOM interface objects
5 participants