Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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.

Comment 2

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