Closed Bug 129602 Opened 22 years ago Closed 20 years ago

Embedders need to be able to reach the window root.

Categories

(Core Graveyard :: Embedding: APIs, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jst, Assigned: mpgritti)

References

Details

(Keywords: embed, fixed1.7, topembed-, Whiteboard: fixed-aviary1.0)

Attachments

(2 files, 3 obsolete files)

Embedders might want to reach the window root in embedding apps to register
event listeners n' such. The most convenient place to do this is in
nsIDOMWindow, but it must not be scriptable (since the window root might be a
chrome window) so we'll need to mark this new attribute [noscript]. The new
attribute should be of type nsIDOMEventTarget.
Blocks: 70229
Keywords: topembed+
Looks like should go to 1.0 then.
Target Milestone: --- → mozilla1.0
Hyatt, this is what we want here, right?
topembed- since we don't know that this is a blocker for any embeddor. It 
does sound like something we may need in the future however
Keywords: topembed+embed, topembed-
Target Milestone: mozilla1.0 → mozilla1.1alpha
Here's an embedder who might find this useful! :-)

I've been working on sidebar support in galeon and while fighting with the
"what's related" panel I've found it would be very helpful to have this toplevel
event target. I've got a workaround right now but this would allow things to
work properly.
Blocks: 98995
Target Milestone: mozilla1.1alpha → ---
I think this would be important for embedders. There is no other public way to
connect chrome listeners right now. I think it could be considered a fix for bug
81835.
Though DOMWindow is frozen now. Would it make sense to have nsIDOMWindow2.idl ?
Attachment #148929 - Attachment is obsolete: true
Attachment #148930 - Flags: superreview?(jst)
Attachment #148930 - Flags: review?(bzbarsky)
I don't know when I'll be able to get to this review, to be truthful.  I'm
rather swamped with reviews and non-Mozilla work right now.  :(

I do wonder why we can't put this on nsIDOMWindowInternal (which isn't very
internal).
(In reply to comment #8)
> I don't know when I'll be able to get to this review, to be truthful.  I'm
> rather swamped with reviews and non-Mozilla work right now.  :(

OK. I'm cancelling it and ccing some other peers.
Anyone has time to review this ?

> I do wonder why we can't put this on nsIDOMWindowInternal (which isn't very
> internal).

Connecting chrome events is really a basic functionality for embedding. I dont
think it's a good idea to expose it through an interface "which isn't very
internal", whose name suggest it isnt really public and which is not planned to
frozen.
Obviously I'm not keen of nsIDOMWindow2 too, but I cant think of any other
acceptable solution.
Attachment #148930 - Flags: review?(bzbarsky)
Comment on attachment 148930 [details] [diff] [review]
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch)

nsIDOMWindowInternal is IMO not the answer here, nsIDOMWindow2 seems
inevitable, so I'm sr'ing this change. It'll take some time before we can
freeze this new interface tho, and before we do, we should probably pick
through what's in nsIDOMWindowInternal and move over the methods/properties
that we really do want embedders to use...
Attachment #148930 - Flags: superreview?(jst) → superreview+
Comment on attachment 148930 [details] [diff] [review]
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch)

Okay, since jst is pleased, that's good enough for me.	However:

>+NS_IMETHODIMP
>+GlobalWindowImpl::GetWindowRoot(nsIDOMEventTarget **aWindowRoot)
>+{
>+  *aWindowRoot = nsnull;
>+
>+  nsIDOMWindowInternal *rootWindow = GlobalWindowImpl::GetPrivateRoot();
>+  nsCOMPtr<nsPIDOMWindow> piWin(do_QueryInterface(rootWindow));
>+  if (!piWin)
>+    return NS_OK;
>+
>+  nsIChromeEventHandler *chromeHandler = piWin->GetChromeEventHandler();
>+  if (!chromeHandler)
>+    return NS_OK;

Please follow the preferred style which includes bracing even one line
if-statements.


>+
>+#include "nsIDOMWindow.idl"
>+
>+[scriptable, uuid(65455132-b96a-40ec-adea-52fa22b1028c)]
>+interface nsIDOMWindow2 : nsIDOMWindow
>+{
>+  /**
>+   * Get the window root for this window. This is useful for hooking
>+   * up event listeners to this window and every other window nested
>+   * in the window root.
>+   */
>+  [noscript] readonly attribute nsIDOMEventTarget windowRoot;

Would it make more sense to mark the interface as noscript if the functionality
it provides is intended to be marked noscript?

r=caillon
Attachment #148930 - Flags: review+
Attached patch add braces (obsolete) — Splinter Review
Attachment #148930 - Attachment is obsolete: true
Assignee: jst → marco
checked in
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
*** Bug 81835 has been marked as a duplicate of this bug. ***
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #149121 - Attachment is obsolete: true
Comment on attachment 149676 [details] [diff] [review]
merge the uid change that was checked in

I'd like to get this in 1.7. It's an important api for embedders and it would
make the transition to embed string of GNOME users easier (GNOME 2.8 release is
too near to be able to depend on mozilla 1.8). Also it should not be risky
since it's just an addition and it didnt cause problems on the trunk.
Attachment #149676 - Flags: approval1.7?
Comment on attachment 149676 [details] [diff] [review]
merge the uid change that was checked in

a=chofmann for 1.7
Attachment #149676 - Flags: approval1.7? → approval1.7+
checked in on 1.7 brach. please mark fixed if it is.
Keywords: fixed1.7
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
this broke the 1.7 tree, did some API change?

if so, are we certain it is safe for 1.7?
That's because nsPIDOMWindow is not de-xpcomified on 1.7 branch. I'll attach a
patch in two minutes
biesi has fixed the bustage, he says it was due to deCOMtamination work on the
trunk.

can jst, caillon, and/or marco confirm that this patch should be nice and safe
for 1.7?
I think it's safe. No API changed. There is only the addition of GetWindowRoot,
which is unused in mozilla codebase. The build did break only because of the
implementation of the unused API.
This patch added nsIDOMWindow2 to the Gecko SDK, but did not mark it frozen. 
That problem has been reported as bug 271295.

In general, we should only add frozen interfaces to the Gecko SDK.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.