Closed Bug 675075 Opened 9 years ago Closed 9 years ago

Reinstate nsIDOMWindowInternal

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8
Tracking Status
firefox8 + fixed

People

(Reporter: mossop, Assigned: Ms2ger)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 4 obsolete files)

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.
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.
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.
(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.
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/
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?
Summary: Possibly reinstate nsIDOMWindowInternal → Reinstate nsIDOMWindowInternal
Target Milestone: --- → mozilla8
Assignee: nobody → Ms2ger
Attached patch Patch v1 (obsolete) — Splinter Review
Do I need to change classinfo as well?
Attachment #552893 - Flags: review?(Olli.Pettay)
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.
Attachment #552893 - Flags: review?(Olli.Pettay) → review-
Attached patch Support up to 64 warnings (obsolete) — Splinter Review
We're already using all 32 bits.
Attachment #552984 - Flags: review?(Olli.Pettay)
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #552893 - Attachment is obsolete: true
Attachment #552985 - Flags: review?(Olli.Pettay)
Attached patch Patch v2.1 (obsolete) — Splinter Review
Attachment #552985 - Attachment is obsolete: true
Attachment #552985 - Flags: review?(Olli.Pettay)
Attachment #552987 - Flags: review?(Olli.Pettay)
Attached patch Patch v3Splinter Review
Let's try this, then.
Attachment #552984 - Attachment is obsolete: true
Attachment #552987 - Attachment is obsolete: true
Attachment #552984 - Flags: review?(Olli.Pettay)
Attachment #552987 - Flags: review?(Olli.Pettay)
Attachment #552989 - Flags: review?(Olli.Pettay)
Attachment #552989 - Flags: review?(Olli.Pettay) → review+
Depends on: 678967
If you're looking to land this on aurora (8), please request patch approval.
This landed on m-c before the 8 merge to aurora, so is already fixed on 8 (hence the target milestone).
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.
Keywords: dev-doc-needed
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?
qa? based on comment 16.
Whiteboard: [qa?]
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+].
Whiteboard: [qa?] → [qa-]
Blocks: 897176
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.