Last Comment Bug 675075 - Reinstate nsIDOMWindowInternal
: Reinstate nsIDOMWindowInternal
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
Depends on: 678967
Blocks: 670235 897176
  Show dependency treegraph
 
Reported: 2011-07-28 15:56 PDT by Dave Townsend [:mossop]
Modified: 2013-12-27 14:30 PST (History)
20 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch v1 (9.14 KB, patch)
2011-08-13 12:09 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review-
Details | Diff | Splinter Review
Support up to 64 warnings (1.59 KB, patch)
2011-08-14 09:10 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2 (9.58 KB, patch)
2011-08-14 09:11 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2.1 (8.63 KB, patch)
2011-08-14 09:20 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v3 (10.47 KB, patch)
2011-08-14 09:57 PDT, :Ms2ger (⌚ UTC+1/+2)
bugs: review+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2011-07-28 15:56:24 PDT
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] 2011-07-28 16:41:16 PDT
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 [:bz] (still a bit busy) 2011-07-28 19:06:08 PDT
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] 2011-07-29 00:22:39 PDT
(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) 2011-08-05 15:11:36 PDT
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] 2011-08-11 17:18:43 PDT
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 (⌚ UTC+1/+2) 2011-08-13 12:09:45 PDT
Created attachment 552893 [details] [diff] [review]
Patch v1

Do I need to change classinfo as well?
Comment 7 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-08-14 06:51:11 PDT
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 (⌚ UTC+1/+2) 2011-08-14 09:10:27 PDT
Created attachment 552984 [details] [diff] [review]
Support up to 64 warnings

We're already using all 32 bits.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-08-14 09:11:10 PDT
Created attachment 552985 [details] [diff] [review]
Patch v2
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-08-14 09:20:08 PDT
Created attachment 552987 [details] [diff] [review]
Patch v2.1
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-08-14 09:57:54 PDT
Created attachment 552989 [details] [diff] [review]
Patch v3

Let's try this, then.
Comment 13 Asa Dotzler [:asa] 2011-08-25 14:57:41 PDT
If you're looking to land this on aurora (8), please request patch approval.
Comment 14 Ed Morley [:emorley] 2011-08-25 15:01:44 PDT
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] 2011-09-30 13:10:30 PDT
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) 2011-10-05 04:35:21 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 10:44:46 PDT
qa? based on comment 16.
Comment 18 Ioana (away) 2011-11-07 05:38:58 PST
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+].

Note You need to log in before you can comment on or make changes to this bug.