Embedders need to be able to reach the window root.

RESOLVED FIXED

Status

defect
RESOLVED FIXED
18 years ago
2 months ago

People

(Reporter: jst, Assigned: mpgritti)

Tracking

({embed, fixed1.7, topembed-})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-aviary1.0)

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

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

Updated

17 years ago
Blocks: 70229
Keywords: topembed+
Looks like should go to 1.0 then.
Target Milestone: --- → mozilla1.0
Reporter

Comment 2

17 years ago
Hyatt, this is what we want here, right?

Comment 3

17 years ago
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

Comment 4

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

Updated

17 years ago
Blocks: 98995
Reporter

Updated

17 years ago
Target Milestone: mozilla1.1alpha → ---
Assignee

Comment 5

15 years ago
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 ?
Assignee

Updated

15 years ago
Attachment #148929 - Attachment is obsolete: true
Assignee

Updated

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

Comment 9

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

Updated

15 years ago
Attachment #148930 - Flags: review?(bzbarsky)
Reporter

Comment 10

15 years ago
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+
Assignee

Comment 12

15 years ago
Posted patch add braces (obsolete) — Splinter Review
Attachment #148930 - Attachment is obsolete: true
Assignee: jst → marco
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Assignee

Comment 14

15 years ago
*** Bug 81835 has been marked as a duplicate of this bug. ***
Assignee

Updated

15 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 15

15 years ago
Attachment #149121 - Attachment is obsolete: true
Assignee

Comment 16

15 years ago
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 17

15 years ago
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
Assignee

Updated

15 years ago
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 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?
Assignee

Comment 20

15 years ago
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?
Assignee

Comment 22

15 years ago
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.
Whiteboard: fixed-aviary1.0

Comment 23

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

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.