Created attachment 554038 [details] [diff] [review] Patch v1 Removes nsIDOMWindowInternal again, now that it will have been depreciated for one cycle. Is the backout of http://hg.mozilla.org/mozilla-central/rev/32c088e2048c (bug 675075), minus a couple of unrelated whitespace/#include re-ordering changes made then. Returns UUIDs to their pre- bug 675075 values, since nothing else has landed since. Presume a quick MXR search of AMO for usage of nsIDOMWindowInternal will be wanted before landing, to check that one cycle of being depreciated is enough.
I think we're going to want to leave this around for more than 1 cycle ....
I'm not sure why we'd want to do that. We should, IMO, make it clear that we do act on these deprecation warnings and that addressing them is not something to put off for a couple of years.
Why? What's the ongoing cost of leaving this as is?
Comment on attachment 554038 [details] [diff] [review] Patch v1 >- nsIDOMWindow *caller = nsContentUtils::GetWindowFromCaller(); >+ nsIDOMWindow *caller = >+ static_cast<nsIDOMWindow*>(nsContentUtils::GetWindowFromCaller()); why this change? I think this could land soon. Addon devs have plenty of time to fix their code. It will take still 12 weeks or so before FF8 is released. And 6 weeks from that before FF9 is released.
(In reply to Olli Pettay [:smaug] from comment #5) > Comment on attachment 554038 [details] [diff] [review] > Patch v1 > > > >- nsIDOMWindow *caller = nsContentUtils::GetWindowFromCaller(); > >+ nsIDOMWindow *caller = > >+ static_cast<nsIDOMWindow*>(nsContentUtils::GetWindowFromCaller()); > why this change? Because I claimed he could do a straight backout, and I forgot about that hunk.
Created attachment 554092 [details] [diff] [review] Patch v1.1 v1 minus the unwanted static_cast<nsIDOMWindow*> hunk.
(In reply to Kyle Huey [:khuey] (email@example.com) from comment #2) > I think we're going to want to leave this around for more than 1 cycle .... 1 or 2 cycles where the warning is visible in the Error Console should be enough. If add-on developers are paying attention to console errors, they should notice within a single cycle. The rest won't notice until we remove it altogether, and there's nothing we can do about that.
I still don't understand what the point is. We're breaking a bunch of addons to save 20 lines of code? QIing to nsIDOMWindowInternal is all over MDC and the web in code samples for writing Firefox addons. It seems like we're breaking backwards compatibility for no reason here.
I wouldn't say code cleanup is "no reason", but I would agree with you that it is not the most important of reasons. I don't really mind if the interface stays there forever to avoid breaking add-ons, but the backout patch included a warning indicating the interface is deprecated and will be removed. So, if we decide to keep the interface, then the warning should go.
For whenever this lands (or doesn't) in the future; has passed try: http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=f701ded4c303
I still think this should be WONTFIX. The cost of keeping this around is effectively 0.
Once the next migration occurs (in just over a week), what do people want to do here? 1) Remove nsIDOMWindowInternal for Firefox 11 now that there will have been three cycles of depreciation notices (Firefox 8+9+10). 2) Leave nsIDOMWindowInternal in place, with the depreciation notice displaying - and close this bug as WONTFIX (or else leave the decision to 6 months down the line). 3) Leave nsIDOMWindowInternal in place indefinitely, and morph this bug into removing the depreciation notice, since it would no longer be accurate. I don't really mind either way, I'd just quite like to resolve this bug and get it off my open-assigned saved search if possible :-)
I would prefer option 2, and comment #9 sums up the reasons pretty well. This is a too often used interface, and no matter what we do in terms of messaging, many add-ons will keep using it until we break it. We could revisit this decision in the future and see if it makes sense to remove the interface then, but it's hard to know what the success conditions for that would be, given that we have little knowledge of add-ons hosted outside of AMO, which are extremely popular at the moment.
I'm going to WONTFIX this and hope nobody cares enough to reopen it.
I don't think we should leave unnecessary cruft around in our code :)
There are still quite a lot of MDN pages using nsIDOMWindowInternal in their examples. https://developer.mozilla.org/en-US/search?q=nsIDOMWindowInternal Can they just be changed, or will they need the "up until version foo do this, otherwise do this" wiki template banner thing? I'm not overly familiar with how many versions back the examples/MDN docs tend to support...
We need to balance our addon base against "nice to have" maintenance. In this case the addon community clearly wins. I don't think we should leave this bug open: it is an attractive nuisance and we would not *currently* accept a patch for it. File a new bug or reopen this one if we ever come up with data saying that these old extensions don't matter.
From the documentation point of view: even if the interface will stay for the foreseeable future, do we still want to keep it used in examples in the MDN or should we edit them in order not to use it anymore? This should help prevent new code/add-ons start using it in the future.
Please remove it from examples on MDN. .QueryInterface(Components.interfaces.nsIDOMWindowInternal) is useless no-op.
As far as I can tell, all docs that used to reference nsIDOMWindowInternal have been updated. If you find any, please point them out.