Closed Bug 852687 Opened 11 years ago Closed 11 years ago

nsIDOMWindowUtils should expose getInnerWindowWithId too

Categories

(Add-on SDK Graveyard :: General, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: zer0, Assigned: gkrizsanits)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

Currently nsIDOMWindowUtils exposes only `getOuterWindowWithId`, that basically wraps `nsGlobalWindow::GetOuterWindowWithId()`. But nsGlobalWindow also has a method called `GetInnerWindowWithId()`, and that's what we need most in our SDK modules (e.g. Panel and the new coming Style), in order to pass only the window's id instead the whole window reference – that sometimes is not doable or suggested.

We should have the same wrapper in nsIDOMWindowUtils for `GetInnerWindowWithId`.
After a chat with Gabor I understand that this bug was misleading. We do not need the "inner window", that should be never exposed to JS, we need the window associated with the inner id given, only if the inner id is referred to a window currently displayed in a "outer window"; so something like `GetWindowWithCurrentInnerID`.
Assignee: nobody → gkrizsanits
Since the current inner id is available from js already, I think it's OK to expose a chrome function that look up a window based on the id of their current inner id if it helps people.

So I look up the inner by id if there isn't any I return null, if there is one, I check its outer and if the outer's current inner is still the one with the given id I return it (else return null).

On the other hand as a 2nd thought:
Matteo: could you provide me an example where this is useful? (when the inner ID is available the outer ID should be too, you can look up by other and then check if it's inner ID is the same...)
Attachment #738511 - Flags: feedback?(zer0)
So, here a specific use case: we have several `Style` object that can be applied to different `window`, and we want to know at which `window` a specific style is applied to.
So, at the moment we store the `window.document` (we can't store `window` because the document inside could change its content when the `window` reference is still the same), but it would be better if instead of storing an object reference, we could store just a string, the `inner id` – that is also used by several observer notifications.

Unfortunately at the moment there is no way to get back the `window` (outer) reference by the inner id, and I'd like to avoid to map manually outer id / inner id and keep trace of any changes. Of course an inner id could be not valid anymore or not associated to an outer window, in that case it's correct return `null` for that given id.

Is that answer to your question?
Flags: needinfo?(gkrizsanits)
Attachment #738511 - Flags: feedback?(zer0)
Comment on attachment 738511 [details] [diff] [review]
GetWindowWithCurrentInnerId for nsDOMWindowUtils

Review of attachment 738511 [details] [diff] [review]:
-----------------------------------------------------------------

What do you think about this API? It can be done from JS but it's really a bit more painful than it should be.
Attachment #738511 - Flags: feedback?(bzbarsky)
Flags: needinfo?(gkrizsanits)
It seems reasonable in general, but note bug 861495.  Perhaps this should also live somewhere else?
Comment on attachment 738511 [details] [diff] [review]
GetWindowWithCurrentInnerId for nsDOMWindowUtils

Oh, and fix the argument name in the idl?  ;)
Attachment #738511 - Flags: feedback?(bzbarsky) → feedback+
Depends on: 861495
(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 738511 [details] [diff] [review]
> GetWindowWithCurrentInnerId for nsDOMWindowUtils
> 
> Oh, and fix the argument name in the idl?  ;)

Whoops... I also managed not to forget about changing the iid this time... So my plan is to ask Matteo to add a jetpack based test for this. But if that does not work out for some reason I can add a chrome test.
Attachment #747991 - Flags: review?(bzbarsky)
Attachment #738511 - Attachment is obsolete: true
Comment on attachment 747991 [details] [diff] [review]
expose GetWindowWithCurrentInnerId to services

>+  nsIDOMWindow getWindowWithCurrentInnerId(in unsigned long long 

Perhaps getCurrentInnerWindowWithId?

r=me
Attachment #747991 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #9)
> Perhaps getCurrentInnerWindowWithId?

Fine by me.
(In reply to Ed Morley [:edmorley UTC+1] from comment #14)
> Backed out for browser-chrome failures:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=22937048&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=22937092&tree=Mozilla-
> Inbound
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f04b74f092

This is very weird... I did a try push yesterday and there I didn't see these failures. Also, only thing this patch does is adding a function to the mediator that is NEVER called by any test, so how could that break things? Plus, there is some problem with try since I cannot open my try push from yesterday any longer.
This is likely the result of an extra clobber that is needed because of the idl change. The last try push is a bit outdated, so just in case here is a new one:

https://tbpl.mozilla.org/?tree=Try&rev=668c41e73ed0

If it's green we'll clobber and try it again to push it to inbound.
Uh... why does the IDL change require clobbering anything??
(In reply to Boris Zbarsky (:bz) from comment #17)
> Uh... why does the IDL change require clobbering anything??

Good question, but this is not the first time. And it's not happening on every platform.

Btw it looks pretty green on try again.
Not the first time for this IDL file, or in general?

If changing an IDL file ever has this behavior we have a serious problem, imo, that we need to fix.  Please do make sure that's filed instead of just randomly clobbering every time you change an IDL file...

On a separate note, the patch you pushed doesn't change the nsIWindowMediator IID.  It probably should have.
(In reply to Boris Zbarsky (:bz) from comment #20)
> Not the first time for this IDL file, or in general?

I think this is the 2nd time something like this pops up, in both cases I was suggested to do a clobber, in both cases it was about changing an interface.

> If changing an IDL file ever has this behavior we have a serious problem,
> imo, that we need to fix.  Please do make sure that's filed instead of just
> randomly clobbering every time you change an IDL file...

I was assuming that this is a known issues since in neither cases were anyone surprised except me...

> On a separate note, the patch you pushed doesn't change the
> nsIWindowMediator IID.  It probably should have.

Indeed it seems like I changed CID instead accidentally... is it Ok if I file a follow up? Can this in some way be the reason for tests failing or shall we still file a bug about this clobbering issue?
> I was assuming that this is a known issues since in neither cases were anyone surprised
> except me...

That's ... unfortunate.  I'm pretty surprised, and not in a good way.  ;)

> is it Ok if I file a follow up? 

Just check in an IID rev.  r=me.

> Can this in some way be the reason for tests failing

I thought about it, but seems unlikely.
https://hg.mozilla.org/mozilla-central/rev/37e606b595f7
https://hg.mozilla.org/mozilla-central/rev/2614fc7d4908
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Boris Zbarsky (:bz) from comment #22)
> > Can this in some way be the reason for tests failing
> 
> I thought about it, but seems unlikely.

It is, actually.
> It is, actually.

Huh.  Do you know the mechanism?
I don't know what the bug is, exactly, but there's been multiple cases where tests failed because an xpidl change wasn't picked up, and they always missed a uuid change.
dev-doc-needed => getCurrentInnerWindowWithId method has been added to nsIWindowMediator
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.