Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Embedders need to be able to reach the window root.

RESOLVED FIXED

Status

()

Core
Embedding: APIs
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: jst, Assigned: Marco Pesenti Gritti)

Tracking

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

Trunk
embed, fixed1.7, topembed-
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-aviary1.0)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 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

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

Comment 2

16 years ago
Created attachment 75447 [details] [diff] [review]
Expose the root tree item's chrome event handler as the window root

Hyatt, this is what we want here, right?

Comment 3

15 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

15 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

15 years ago
Blocks: 98995
(Reporter)

Updated

15 years ago
Target Milestone: mozilla1.1alpha → ---
(Assignee)

Comment 5

13 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)

Comment 6

13 years ago
Created attachment 148929 [details] [diff] [review]
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch)
(Assignee)

Comment 7

13 years ago
Created attachment 148930 [details] [diff] [review]
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch)
(Assignee)

Updated

13 years ago
Attachment #148929 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #148930 - Flags: superreview?(jst)
Attachment #148930 - Flags: review?(bzbarsky)

Comment 8

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

13 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

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

Comment 10

13 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

13 years ago
Created attachment 149121 [details] [diff] [review]
add braces
Attachment #148930 - Attachment is obsolete: true
Assignee: jst → marco
checked in
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

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

Updated

13 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

13 years ago
Created attachment 149676 [details] [diff] [review]
merge the uid change that was checked in
Attachment #149121 - Attachment is obsolete: true
(Assignee)

Comment 16

13 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

13 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

13 years ago
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 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

13 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

13 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

13 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.
You need to log in before you can comment on or make changes to this bug.