Closed
Bug 852687
Opened 12 years ago
Closed 12 years ago
nsIDOMWindowUtils should expose getInnerWindowWithId too
Categories
(Add-on SDK Graveyard :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla24
People
(Reporter: zer0, Assigned: gkrizsanits)
References
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 1 obsolete file)
3.19 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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`.
Reporter | ||
Comment 1•12 years ago
|
||
Priority: -- → P2
Reporter | ||
Comment 2•12 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•12 years ago
|
Assignee: nobody → gkrizsanits
Assignee | ||
Comment 3•12 years ago
|
||
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•12 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•12 years ago
|
Attachment #738511 -
Flags: feedback?(zer0)
Assignee | ||
Comment 5•12 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)
![]() |
||
Comment 6•12 years ago
|
||
It seems reasonable in general, but note bug 861495. Perhaps this should also live somewhere else?
![]() |
||
Comment 7•12 years ago
|
||
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 | ||
Comment 8•12 years ago
|
||
(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•12 years ago
|
Attachment #738511 -
Attachment is obsolete: true
![]() |
||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #9)
> Perhaps getCurrentInnerWindowWithId?
Fine by me.
Assignee | ||
Comment 11•12 years ago
|
||
Comment 12•12 years ago
|
||
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Assignee | ||
Comment 15•12 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•12 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.
![]() |
||
Comment 17•12 years ago
|
||
Uh... why does the IDL change require clobbering anything??
Assignee | ||
Comment 18•12 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.
Assignee | ||
Comment 19•12 years ago
|
||
crossing fingers:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37e606b595f7
![]() |
||
Comment 20•12 years ago
|
||
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•12 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?
![]() |
||
Comment 22•12 years ago
|
||
> 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.
Assignee | ||
Comment 23•12 years ago
|
||
Comment 24•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/37e606b595f7
https://hg.mozilla.org/mozilla-central/rev/2614fc7d4908
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 25•12 years ago
|
||
(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.
![]() |
||
Comment 26•12 years ago
|
||
> It is, actually.
Huh. Do you know the mechanism?
Comment 27•12 years ago
|
||
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•12 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.
Description
•