Peter Van der Beken [:peterv] Assignee
	Description • 7 years ago

We do support instanceof for supplemental interfaces, but the spec doesn't, and I rely on us not doing it for bug 888600.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 7 years ago

OK.  So this only affects supplemental interfaces that are not [NoInterfaceObject], which means in practice XPathEvaluator, KeyEvent, and ChromeWindow.

The fact that XPathEvaluator and KeyEvent are even supplemental interfaces is more or less an accident of convenience plus the behavior we wanted for instanceof.  That is, we wanted key events to be instances of KeyEvent and documents to be instances of XPathEvaluator.

If we _don't_ want that, then we can just make them both non-supplemental by having a [NoInterfaceObject] supplemental interface that is implemented by both KeyEvent/KeyboardEvent and Document/XPathEvaluator respectively.

That said, with your patch "document instanceof XPathEvaluator" is still true, I suspect.  But it's not true in any other browser, so I have no problem with changing that behavior.

As for ChromeWindow, that one is even simpler, because we can just make it [NoInterfaceObject] now, I would think.  The only reason it's not is so that instanceof ChromeWindow will work, but we can just replace that by a getter on the interface that returns whether it's a ChromeWindow or not, and replace the 5 uses of "instanceof ChromeWindow" in our tree with that getter (and the two uses in the comm-central tree).  Then we won't need the ChromeWindow special-casing you had to add.  It used to be that we couldn't do this because of extensions, but at this point I think this change would be fine to make.

Thoughts?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 7 years ago

Comment on attachment 8902663 [details] [diff] [review]
v1

Clearing request pending response to comment 2.

	Peter Van der Beken [:peterv] Assignee
	Comment 4 • 7 years ago

I'm not really sure how what you're proposing for KeyEvent/XPathEvaluator will be different from what we have now? And it seems the generated code would look identical but the idl will be more complicated.

I do want to simplify things for ChromeWindow, so I'll do that.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 7 years ago

Per IRC discussion, let's fix up ChromeWindow and move the other bits to a separate bug.  You're right that the generated code would be identical; my suggestions just simplify the conceptual model and pave the way to moving in the direction mixins are moving in the spec.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 7 years ago

Comment on attachment 8914923 [details] [diff] [review]
v2

Minor gripe: it would have been pretty nice to have the "nix-instanceof" bits as a separate changeset.  Especially since we don't actually _need_ to change the instanceof Ci.nsIDOMChromeWindow bits here, since those would keep working, right?

It might still be good to have that as a separate changeset for bisectability, but probably not worth spending too much time on that.

>+    if (window && window.isChromeWindow) {

Hmm.  How sure are we that the other places don't need null-checks?

>-[test_bug1016960.html]

Why not just change the test to check that chromeWindow.isChromeWindow is true?  I think that would be better...

>+++ b/dom/bindings/BindingUtils.cpp
>+  if (clasp->mPrototypeID != prototypes::id::_ID_Count) {

I preferred the older logic, where we checked this up front before getting "instance" and "domClass" and had things less-indented.  Is there a reason to switch from that?  Probably better to put it back that way, and put the comments explaining what's going on back..

>+      if (boolp) {

The code used to be unconditional, so we never fell through to CallOrdinaryHasInstance in the CPOW case.  Why is that changing?  Should it be changing?

r=me with the above addressed

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 7 years ago

Oh, with these changes, I expect you can make nsIDOMChromeWindow non-scriptable, right?

	Peter Van der Beken [:peterv] Assignee
	Comment 9 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6723d4383dcf666f19a463a7dc8b59f1888a7ed1

	Peter Van der Beken [:peterv] Assignee
	Comment 10 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=742ad9d00ca7bc20f53595ffb5b5e80cb5432789

	Peter Van der Beken [:peterv] Assignee
	Comment 11 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> Minor gripe: it would have been pretty nice to have the "nix-instanceof"
> bits as a separate changeset.  Especially since we don't actually _need_ to
> change the instanceof Ci.nsIDOMChromeWindow bits here, since those would
> keep working, right?

I moved these changes to a separate changeset.

> >+    if (window && window.isChromeWindow) {
> 
> Hmm.  How sure are we that the other places don't need null-checks?

I did check them fwiw.

> >+++ b/dom/bindings/BindingUtils.cpp
> >+  if (clasp->mPrototypeID != prototypes::id::_ID_Count) {
> 
> I preferred the older logic, where we checked this up front before getting
> "instance" and "domClass" and had things less-indented.  Is there a reason
> to switch from that?  Probably better to put it back that way, and put the
> comments explaining what's going on back..

> The code used to be unconditional, so we never fell through to
> CallOrdinaryHasInstance in the CPOW case.  Why is that changing?  Should it
> be changing?

I undid all the changes to InterfaceHasInstance, if we don't special-case ChromeWindow it can remain as is. I do think it's a little weird that the CPOW case works differently from the non-CPOW case.

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #8)
> Oh, with these changes, I expect you can make nsIDOMChromeWindow
> non-scriptable, right?

Someone would need to remove all the QueryInterface calls too. I'll file a bug.

	Peter Van der Beken [:peterv] Assignee
	Comment 12 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b5bbc888413b87292a34fcfb4a039dc1712fe4
Bug 1383059 - Remove instanceof for supplemental interfaces. Part 1: add Window.isChromeWindow and switch |instanceof [nsIDOM]ChromeWindow| to use it instead. r=bz.

	Peter Van der Beken [:peterv] Assignee
	Comment 13 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3d4cd8f538db7e3e61998d547579cc5fd26f2b2
Bug 1383059 - Remove instanceof for supplemental interfaces. Part 2: remove support for supplemental interfaces in instanceof code and remove unnecessary ChromeWindow interface. r=bz.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 14 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f6b5bbc88841
https://hg.mozilla.org/mozilla-central/rev/a3d4cd8f538d