Closed
Bug 675075
Opened 13 years ago
Closed 13 years ago
Reinstate nsIDOMWindowInternal
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: mossop, Assigned: Ms2ger)
References
Details
(Whiteboard: [qa-])
Attachments
(1 file, 4 obsolete files)
10.47 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
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•13 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.
Comment 3•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
Assignee: nobody → Ms2ger
Assignee | ||
Comment 6•13 years ago
|
||
Do I need to change classinfo as well?
Attachment #552893 -
Flags: review?(Olli.Pettay)
Comment 7•13 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•13 years ago
|
||
We're already using all 32 bits.
Attachment #552984 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 9•13 years ago
|
||
Attachment #552893 -
Attachment is obsolete: true
Attachment #552985 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 10•13 years ago
|
||
Attachment #552985 -
Attachment is obsolete: true
Attachment #552985 -
Flags: review?(Olli.Pettay)
Attachment #552987 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 11•13 years ago
|
||
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•13 years ago
|
Attachment #552989 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 12•13 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
Closed: 13 years ago
Flags: in-testsuite-
Keywords: dev-doc-needed
Resolution: --- → FIXED
Comment 13•13 years ago
|
||
If you're looking to land this on aurora (8), please request patch approval.
Comment 14•13 years ago
|
||
This landed on m-c before the 8 merge to aurora, so is already fixed on 8 (hence the target milestone).
Comment 15•13 years ago
|
||
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•13 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?
status-firefox8:
--- → fixed
Comment 18•13 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-]
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•