675075 - Reinstate nsIDOMWindowInternal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Dave Townsend [:mossop]]

nsIDOMWindowInternal was removed which has broken a large number of add-ons. The only reason for the removal was code cleanup and it seems like we could easily leave an empty nsIDOMWindowInternal there to not break those add-ons.

I'd like to see this happen but perhaps a first step is to see if we can get data from AMO on how many add-ons would be made incompatible by this change.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

https://mxr.mozilla.org/addons/search?string=nsIDOMWindowInternal
> Found 192 matching lines in 115 files

While some add-ons in these results use nsIDOMWindowInternal in more than one file, there are still close to 100 add-ons on AMO using it.

I, too, would like this change to be delayed and have a deprecation period where developers get an indication in the Error Console that they should be using nsIDOMWindow instead. Similar to what we're doing with nsIJSON in bug 672063.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Actually, the vast majority of the callsites seem to be QIing the result of getNext() on a window enumerator.... given that Window objects have classinfo, this should be a no-op, so I wonder why so many addons are doing it.

I do agree we should have a deprecation period, but I suspect that the messaging should just be "take the QI out", not "QI to nsIDOMWindow".  Worth double-checking.

Comment 3

[Dão Gottwald [:dao]]

(In reply to comment #2)
> Actually, the vast majority of the callsites seem to be QIing the result of
> getNext() on a window enumerator.... given that Window objects have
> classinfo, this should be a no-op, so I wonder why so many addons are doing
> it.

I usually omit the QI but always felt a little guilty doing so :)
Somebody started doing this in our code base and now everyone thinks it's somehow more correct or even necessary. People copying existing code, happens all the time. I wouldn't be surprised if its on MDC as well.

Comment 4

[Matt Brubeck (:mbrubeck)]

We've used nsIDOMWindowInternal in published example code for writing restartless add-ons for Fennec, for example:

http://starkravingfinkle.org/blog/2011/01/bootstrap-jones-adventures-in-restartless-add-ons/

Comment 5

[Jorge Villalobos [:jorgev] (he/him)]

We don't want to run the bulk compatibility as late as we did for Firefox 7 due to bug 672063, so we need a decision. Are we doing this?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Do I need to change classinfo as well?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 552893 [details] [diff] [review]
Patch v1

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -1333,16 +1333,17 @@ NS_IMPL_CYCLE_COLLECTION_CLASS(nsGlobalW
> DOMCI_DATA(Window, nsGlobalWindow)
> 
> // QueryInterface implementation for nsGlobalWindow
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsGlobalWindow)
>   // Make sure this matches the cast in nsGlobalWindow::FromWrapper()
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIScriptGlobalObject)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMWindow)
>   NS_INTERFACE_MAP_ENTRY(nsIDOMJSWindow)
>+  NS_INTERFACE_MAP_ENTRY(nsIDOMWindowInternal)
Would be actually great if you could add some warning.
It would be probably enough to warn just once.


> 	nsIDOMPerformanceNavigation.idl		\
>+	nsIDOMWindowInternal.idl \
> 	$(NULL)
Missing tab before \ ?


>diff --git a/dom/interfaces/base/nsIDOMWindowInternal.idl b/dom/interfaces/base/nsIDOMWindowInternal.idl
...
>+[scriptable, uuid(8614bdb7-5b07-4d00-a7ba-4d44697a343d)]
>+interface nsIDOMWindowInternal : nsIDOMWindow {}
Missing ;?
But anyway, I think in this case you could just put nsIDOMWindowInternal to nsIDOMWindow.idl
and add some comment that the interface is deprecated.


r- since I'd like to see the warning, so that extension authors might notice that they need to update their code.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Support up to 64 warnings

We're already using all 32 bits.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2.1

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v3

Let's try this, then.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Landed: http://hg.mozilla.org/mozilla-central/rev/7acef8f218ac
Tried to fix: http://hg.mozilla.org/mozilla-central/rev/5b948d637979
Backed out: http://hg.mozilla.org/mozilla-central/rev/8994b8638803
Landed: http://hg.mozilla.org/mozilla-central/rev/32c088e2048c

Comment 13

[Asa Dotzler [:asa]]

If you're looking to land this on aurora (8), please request patch approval.

Comment 14

[Ed Morley [:emorley]]

This landed on m-c before the 8 merge to aurora, so is already fixed on 8 (hence the target milestone).

Comment 15

[Eric Shepherd [:sheppy]]

We will track documentation for this on bug 670235, where it was originally removed. I have noted in appropriate places that nsIDOMWindowInternal has been re-instated as an empty interface for compatibility.

Comment 16

[Ioana (away)]

The add-ons list in comment #1 cannot be opened without an account on  mxr.mozilla.org. I tried getting the warning about using nsIDOMWindowInternal with some add-ons recommended by Add-ons Manager (WOT, Flagfox etc) but I couldn't.

Can anyone please give some examples of add-ons that use nsIDOMWindowInternal?

Comment 17

[u279076]

qa? based on comment 16.

Comment 18

[Ioana (away)]

Changing whiteboard to [qa-] since no one seems to have the add-ons list I requested in comment #16.

If anyone finds the list or has at least one example of an add-on this issue reproduced with, please comment it here and change the whiteboard to [qa+].