Last Comment Bug 129602 - Embedders need to be able to reach the window root.
: Embedders need to be able to reach the window root.
Status: RESOLVED FIXED
fixed-aviary1.0
: embed, fixed1.7, topembed-
Product: Core
Classification: Components
Component: Embedding: APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Marco Pesenti Gritti
: Michael Dunn
: Myk Melez [:myk] [@mykmelez]
Mentors:
: 81835 (view as bug list)
Depends on:
Blocks: 70229 98995
  Show dependency treegraph
 
Reported: 2002-03-07 17:25 PST by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2004-11-22 20:56 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Expose the root tree item's chrome event handler as the window root (2.82 KB, patch)
2002-03-21 13:29 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch) (5.44 KB, patch)
2004-05-20 03:14 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch) (5.44 KB, patch)
2004-05-20 03:16 PDT, Marco Pesenti Gritti
caillon: review+
jst: superreview+
Details | Diff | Splinter Review
add braces (5.47 KB, patch)
2004-05-22 17:59 PDT, Marco Pesenti Gritti
no flags Details | Diff | Splinter Review
merge the uid change that was checked in (5.54 KB, patch)
2004-05-31 04:21 PDT, Marco Pesenti Gritti
chofmann: approval1.7+
Details | Diff | Splinter Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2002-03-07 17:25:12 PST
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.
Comment 1 Heikki Toivonen (remove -bugzilla when emailing directly) 2002-03-15 19:14:13 PST
Looks like should go to 1.0 then.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2002-03-21 13:29:38 PST
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 saari (gone) 2002-04-30 09:13:17 PDT
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
Comment 4 Philip Langdale 2002-07-20 04:52:05 PDT
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.
Comment 5 Marco Pesenti Gritti 2004-04-28 15:26:32 PDT
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 ?
Comment 6 Marco Pesenti Gritti 2004-05-20 03:14:17 PDT
Created attachment 148929 [details] [diff] [review]
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch)
Comment 7 Marco Pesenti Gritti 2004-05-20 03:16:15 PDT
Created attachment 148930 [details] [diff] [review]
Add nsIDOMWindow2 with a windowRoot property (partially based on jst patch)
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2004-05-20 09:47:34 PDT
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).
Comment 9 Marco Pesenti Gritti 2004-05-20 10:39:39 PDT
(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.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2004-05-20 14:42:00 PDT
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...
Comment 11 Christopher Aillon (sabbatical, not receiving bugmail) 2004-05-20 17:45:02 PDT
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
Comment 12 Marco Pesenti Gritti 2004-05-22 17:59:55 PDT
Created attachment 149121 [details] [diff] [review]
add braces
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2004-05-26 11:12:15 PDT
checked in
Comment 14 Marco Pesenti Gritti 2004-05-26 11:15:57 PDT
*** Bug 81835 has been marked as a duplicate of this bug. ***
Comment 15 Marco Pesenti Gritti 2004-05-31 04:21:19 PDT
Created attachment 149676 [details] [diff] [review]
merge the uid change that was checked in
Comment 16 Marco Pesenti Gritti 2004-05-31 04:24:23 PDT
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.
Comment 17 chris hofmann 2004-05-31 11:56:03 PDT
Comment on attachment 149676 [details] [diff] [review]
merge the uid change that was checked in

a=chofmann for 1.7
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2004-05-31 13:16:49 PDT
checked in on 1.7 brach. please mark fixed if it is.
Comment 19 (not reading, please use seth@sspitzer.org instead) 2004-05-31 15:44:35 PDT
this broke the 1.7 tree, did some API change?

if so, are we certain it is safe for 1.7?
Comment 20 Marco Pesenti Gritti 2004-05-31 15:52:56 PDT
That's because nsPIDOMWindow is not de-xpcomified on 1.7 branch. I'll attach a
patch in two minutes
Comment 21 (not reading, please use seth@sspitzer.org instead) 2004-05-31 15:54:03 PDT
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?
Comment 22 Marco Pesenti Gritti 2004-05-31 16:00:32 PDT
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.
Comment 23 Darin Fisher 2004-11-22 20:56:52 PST
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.

Note You need to log in before you can comment on or make changes to this bug.