Reinstate nsIDOMWindowInternal

RESOLVED FIXED in Firefox 8

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: mossop, Assigned: Ms2ger)

Tracking

Trunk
mozilla8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox8+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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)

Updated

6 years ago
Assignee: nobody → Ms2ger
(Assignee)

Comment 6

6 years ago
Created attachment 552893 [details] [diff] [review]
Patch v1

Do I need to change classinfo as well?
Attachment #552893 - Flags: review?(Olli.Pettay)

Comment 7

6 years ago
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-
(Assignee)

Comment 8

6 years ago
Created attachment 552984 [details] [diff] [review]
Support up to 64 warnings

We're already using all 32 bits.
Attachment #552984 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 9

6 years ago
Created attachment 552985 [details] [diff] [review]
Patch v2
Attachment #552893 - Attachment is obsolete: true
Attachment #552985 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 10

6 years ago
Created attachment 552987 [details] [diff] [review]
Patch v2.1
Attachment #552985 - Attachment is obsolete: true
Attachment #552985 - Flags: review?(Olli.Pettay)
Attachment #552987 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 11

6 years ago
Created attachment 552989 [details] [diff] [review]
Patch v3

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)

Updated

6 years ago
Attachment #552989 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 12

6 years ago
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Keywords: dev-doc-needed
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Depends on: 678967

Comment 13

6 years ago
If you're looking to land this on aurora (8), please request patch approval.
tracking-firefox8: ? → +
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

Comment 16

6 years ago
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?]

Updated

6 years ago
status-firefox8: --- → fixed

Comment 18

6 years ago
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
You need to log in before you can comment on or make changes to this bug.