nsIDOMWindowUtils should expose getInnerWindowWithId too

RESOLVED FIXED in mozilla24

Status

P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: zer0, Assigned: gkrizsanits)

Tracking

({dev-doc-needed})

unspecified
mozilla24
x86
Mac OS X
dev-doc-needed

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

6 years ago
Priority: -- → P2
(Reporter)

Comment 2

6 years ago
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`.
(Reporter)

Updated

6 years ago
Assignee: nobody → gkrizsanits
(Assignee)

Comment 3

6 years ago
Created attachment 738511 [details] [diff] [review]
GetWindowWithCurrentInnerId for nsDOMWindowUtils

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)
(Reporter)

Comment 4

6 years ago
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)
(Assignee)

Updated

6 years ago
Attachment #738511 - Flags: feedback?(zer0)
(Assignee)

Comment 5

6 years ago
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+
(Assignee)

Updated

6 years ago
Depends on: 861495
(Assignee)

Comment 8

6 years ago
Created attachment 747991 [details] [diff] [review]
expose GetWindowWithCurrentInnerId to services

(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)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 10

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #9)
> Perhaps getCurrentInnerWindowWithId?

Fine by me.
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
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??
(Assignee)

Comment 18

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
(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
Last Resolved: 5 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.

Comment 28

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