Rob Arnold [:robarnold] Assignee
	Description • 15 years ago

We actually need the native message data for the system information service to monitor changes in power status.

For taskbar previews, this avoids adding yet another [notxpcom] method to the interface and we don't have to explicitly keep track of the previews (they can be garbage collected without the need for weak references).

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 1 • 15 years ago

Just browsing, but it looks like you've got a static hash table there which will call PL_DHashTableFinish when it destructs. Static xpcom classes generally lead to crashes.

	Jim Mathies [:jimm]
	Comment 2 • 15 years ago

Would you mind doing a merge and posting a new patch? This one has gone bad. (Sorry it took so long for me to get to this.)

	Jim Mathies [:jimm]
	Comment 3 • 15 years ago

(In reply to comment #2)
> Would you mind doing a merge and posting a new patch? This one has gone bad.
> (Sorry it took so long for me to get to this.)

Actually don't worry about it, I've managed to get it applied. A end of file return on the nsWindow.cpp and the nsSwitchToUIThread removal messed things up.

	Jim Mathies [:jimm]
	Comment 4 • 15 years ago

+ * The Initial Developer of the Original Code is
+ * Netscape Communications Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2000
+ * the Initial Developer. All Rights Reserved.

nit - header updates.

+  NS_ASSERTION(!sInvisibleWindowHook, "There can only be one! (invisible window)");
+  sInvisibleWindowHook = &window.GetWindowHook();

Why are we storing the hook of the first invisible window in a static? Doesn't look like we actually use this for anything. Also see bug 509023, looks like we should be checking if this is already set and returning rather than generating an assert.

+  for (PRUint32 midx = 0; midx < length; midx++)
+    data->monitors[midx].Invoke(hWnd, nMsg, wParam, lParam, aResult);

paren this please.

+  nsresult AddHook(UINT nMsg, Callback callback, void *context);
+	nsresult RemoveHook(UINT nMsg, Callback callback, void *context);

please clean up the .h by removing tabs please. Lets also get some documentation in WinHook.cpp explaining how to use this.

+  WindowHook            mWindowHook;
+

Nix that last new line.

+  if (mWindowHook.Notify(mWnd, msg, wParam, lParam, aRetValue))
+    return PR_TRUE;

Generally speaking, I like the idea of a generic hooking mechanism. I wonder though what the performance issues are with two or three of these set. Rather than do a lookup for every msg coming into process message, maybe we should limit lookups only to messages we hook (manually added by people when they need them hooked or a filter for a block of messages in the case of a registered windows message)? 

Also, please address ben turner's comments. I'm not the right person to review that aspect of the code. Also moving this forward please get an sr from Ere or roc.

	Rob Arnold [:robarnold] Assignee
	Comment 5 • 15 years ago

Addresses review comments. In common case, overhead is a function call and a lookup into an empty hashtable.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 6 • 15 years ago

We need documentation for WindowHook describing what it actually is and how to use it. In particular you need to explain why we need WindowHook and why we can't make it the same object as nsWindow.

Tere are a number of style problems:

+  if (mWindowType == eWindowType_invisible && !WindowHook::HiddenWindowHook())
+    WindowHook::SetHiddenWindow(*this);

{}

+  if(!strcmp(topic, NS_XPCOM_SHUTDOWN_OBSERVER_ID)) {

if (

You aren't obeying the a* convention for parameter names anywhere, but you need to.

+    if (!data) return nsnull;

separate lines

+    if (window)
+      hook = &window->GetWindowHook();

{}

+    bool operator== (const CallbackData &rhs) const {
+      return cb == rhs.cb && context == rhs.context;
+    }
+    bool operator!= (const CallbackData &rhs) const {
+      return !(*this == rhs);
+    }

PRBool

+  typedef nsTArray<CallbackData> MonitorArray;

I don't think this typedef adds much value, remove it?

	Rob Arnold [:robarnold] Assignee
	Comment 7 • 15 years ago

(In reply to comment #6)
> We need documentation for WindowHook describing what it actually is and how to
> use it. In particular you need to explain why we need WindowHook and why we
> can't make it the same object as nsWindow.

Parts of toolkit will need to be able to hook the native message loop of the hidden window (Power notification). Also it reduces clutter in nsWindow but that's a secondary goal. I will write this up into a comment in WindowHook.h.

> Tere are a number of style problems:
> 
> +  if (mWindowType == eWindowType_invisible && !WindowHook::HiddenWindowHook())
> +    WindowHook::SetHiddenWindow(*this);
> 
> {}

This was done according to prevailing style in the win32 widget code for single statements (ex: nsWindow). But upon further review, both styles exist.

> 
> +    bool operator== (const CallbackData &rhs) const {
> +      return cb == rhs.cb && context == rhs.context;
> +    }
> +    bool operator!= (const CallbackData &rhs) const {
> +      return !(*this == rhs);
> +    }
> 
> PRBool

That's not C++ though...I'll do it but it feels wrong.

> +  typedef nsTArray<CallbackData> MonitorArray;
> 
> I don't think this typedef adds much value, remove it?

Saves visual space and typing in RemoveMonitor. I think it lends readability to that code too. nsTArray<CallbackData>::index_type seems much uglier to read.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 15 years ago

(In reply to comment #7)
> (In reply to comment #6)
> > We need documentation for WindowHook describing what it actually is and how
> > to use it. In particular you need to explain why we need WindowHook and why
> > we can't make it the same object as nsWindow.
>
> Parts of toolkit will need to be able to hook the native message loop of the
> hidden window (Power notification). Also it reduces clutter in nsWindow but
> that's a secondary goal. I will write this up into a comment in WindowHook.h.

The hidden window is an nsWindow though. You're actually unconvincing me that this should be separate from nsWindow :-)

> This was done according to prevailing style in the win32 widget code for
> single statements (ex: nsWindow). But upon further review, both styles exist.

Then let's favour the brand-new super-style of {} around everything.

> > +    bool operator== (const CallbackData &rhs) const {
> > +      return cb == rhs.cb && context == rhs.context;
> > +    }
> > +    bool operator!= (const CallbackData &rhs) const {
> > +      return !(*this == rhs);
> > +    }
> > 
> > PRBool
> 
> That's not C++ though...I'll do it but it feels wrong.

It's our style. A mass change away from PRBool may be something good for the future.

> > +  typedef nsTArray<CallbackData> MonitorArray;
> > 
> > I don't think this typedef adds much value, remove it?
> 
> Saves visual space and typing in RemoveMonitor. I think it lends readability
> to that code too. nsTArray<CallbackData>::index_type seems much uglier to
> read.

How about PRUint32? :-)

	Rob Arnold [:robarnold] Assignee
	Comment 9 • 15 years ago

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > We need documentation for WindowHook describing what it actually is and how
> > > to use it. In particular you need to explain why we need WindowHook and why
> > > we can't make it the same object as nsWindow.
> >
> > Parts of toolkit will need to be able to hook the native message loop of the
> > hidden window (Power notification). Also it reduces clutter in nsWindow but
> > that's a secondary goal. I will write this up into a comment in WindowHook.h.
> 
> The hidden window is an nsWindow though. You're actually unconvincing me that
> this should be separate from nsWindow :-)

Widget can't depend on toolkit so I can't use whatever interface I eventually write in nsWindow. Finding the hidden window is also a pain because we don't have a good way to enumerate windows (there are also several 'hidden' windows like nsAppShell's terrible hack). And in the future, it might be nice to move more message handlers out of nsWindow (the gfx ones may go to nsWindowGfx some day).

> 
> > > +  typedef nsTArray<CallbackData> MonitorArray;
> > > 
> > > I don't think this typedef adds much value, remove it?
> > 
> > Saves visual space and typing in RemoveMonitor. I think it lends readability
> > to that code too. nsTArray<CallbackData>::index_type seems much uglier to
> > read.
> 
> How about PRUint32? :-)

Well I find the whole PRUint32 value = -1 to be unseemly so I like the NoIndex and I think it's better to use the typedefs rather than propagate what they happen to typedef to today (if not, then why typedef at all?).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 15 years ago

(In reply to comment #9)
> Widget can't depend on toolkit so I can't use whatever interface I eventually
> write in nsWindow. Finding the hidden window is also a pain because we don't
> have a good way to enumerate windows (there are also several 'hidden' windows
> like nsAppShell's terrible hack). And in the future, it might be nice to move
> more message handlers out of nsWindow (the gfx ones may go to nsWindowGfx some
> day).

I think moving message handling out of widget would be a bad thing.

Can we broadcast power status changes through nsIObserver?

What exactly do you need for tab previews?

> > > > +  typedef nsTArray<CallbackData> MonitorArray;
> > > > 
> > > > I don't think this typedef adds much value, remove it?
> > > 
> > > Saves visual space and typing in RemoveMonitor. I think it lends readability
> > > to that code too. nsTArray<CallbackData>::index_type seems much uglier to
> > > read.
> > 
> > How about PRUint32? :-)
> 
> Well I find the whole PRUint32 value = -1 to be unseemly so I like the NoIndex
> and I think it's better to use the typedefs rather than propagate what they
> happen to typedef to today (if not, then why typedef at all?).

Documentation maybe? Using NoIndex makes sense, even if you're also using PRUint32.

	Rob Arnold [:robarnold] Assignee
	Comment 11 • 15 years ago

(In reply to comment #10)
> I think moving message handling out of widget would be a bad thing.
> 
> Can we broadcast power status changes through nsIObserver?

We have to register to receive the new power notification messages and I don't think that new code should reside in widget/

I also have a desire to merge as many of the extra windows we create as possible (nsAppShell, the DDE window) to the hidden window.

> What exactly do you need for tab previews?

I have 4 or so messages that are sent to a top level nsWindow which I'll want to hook and handle with the rest of the taskbar preview code (the window preview code in particular).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 15 years ago

(In reply to comment #11)
> We have to register to receive the new power notification messages and I don't
> think that new code should reside in widget

Why not?

> I also have a desire to merge as many of the extra windows we create as
> possible (nsAppShell, the DDE window) to the hidden window.
> 
> > What exactly do you need for tab previews?
> 
> I have 4 or so messages that are sent to a top level nsWindow which I'll want
> to hook and handle with the rest of the taskbar preview code (the window
> preview code in particular).

I think it would make sense to have some kind of global cross-platform API for getting the nsIWidget of the hidden window. That need not live in widget.

I think it would make sense for there to be a Windows-specific widget API (nsIWidgetWin?) that exposes this hooking functionality on a per-window basis.

How does that sound?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 15 years ago

As a general rule, I am extremely reluctant to add new public objects that have a 0-or-1:1 relationship with existing public objects. It's unnecessarily complex.

	Rob Arnold [:robarnold] Assignee
	Comment 14 • 15 years ago

(In reply to comment #12)
> (In reply to comment #11)
> > We have to register to receive the new power notification messages and I don't
> > think that new code should reside in widget
> 
> Why not?

It should go with the rest of the code to handle power notification in toolkit where the other cross platform code lives.

> I think it would make sense to have some kind of global cross-platform API for
> getting the nsIWidget of the hidden window. That need not live in widget.

Agreed on the API, but widget seemed like a convenient place since we have access to the HWND and message callback.

> I think it would make sense for there to be a Windows-specific widget API
> (nsIWidgetWin?) that exposes this hooking functionality on a per-window basis.
> 
> How does that sound?

That's what WindowHook is. Why add an interface and virtual functions? No need to make it XPCOM since the C++ data type are what matters.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 15 • 15 years ago

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > We have to register to receive the new power notification messages and I
> > > don't think that new code should reside in widget
> > 
> > Why not?
> 
> It should go with the rest of the code to handle power notification in toolkit
> where the other cross platform code lives.

I don't think we should mix cross-platform code with Windows-only code. If the toolkit Windows code lives in toolkit/system then I guess that's OK.
 
> > I think it would make sense to have some kind of global cross-platform API
> > for getting the nsIWidget of the hidden window. That need not live in
> > widget.
> 
> Agreed on the API, but widget seemed like a convenient place since we have
> access to the HWND and message callback.

The cross-platform code that creates the hidden window can store a reference to the window (I bet it already does) and make it available somehow. In fact I'm surprised this doesn't already exist.

> > I think it would make sense for there to be a Windows-specific widget API
> > (nsIWidgetWin?) that exposes this hooking functionality on a per-window
> > basis.
> > 
> > How does that sound?
> 
> That's what WindowHook is. Why add an interface and virtual functions? No need
> to make it XPCOM since the C++ data type are what matters.

We discussed this on IRC and I think we agreed that the hashtable goes away, and we create a new class between nsIWidget and nsWindow that exposes these methods directly on the widget.

	Rob Arnold [:robarnold] Assignee
	Comment 16 • 15 years ago

(In reply to comment #15)
> The cross-platform code that creates the hidden window can store a reference to
> the window (I bet it already does) and make it available somehow. In fact I'm
> surprised this doesn't already exist.

nsIAppShellService has a nsIXULWindow hiddenWindow attribute. From that can I QI to an nsIBaseWindow? (I don't know a good way to check this without actually running it).

> > > I think it would make sense for there to be a Windows-specific widget API
> > > (nsIWidgetWin?) that exposes this hooking functionality on a per-window
> > > basis.
> > > 
> > > How does that sound?
> > 
> > That's what WindowHook is. Why add an interface and virtual functions? No need
> > to make it XPCOM since the C++ data type are what matters.
> 
> We discussed this on IRC and I think we agreed that the hashtable goes away,
> and we create a new class between nsIWidget and nsWindow that exposes these
> methods directly on the widget.

As discussed we leave WindowHook as a member of nsWindow to avoid heap allocation. It's still not clear what the new class name should be or what its relation is to nsWindow except that it should be returned from GetNativeData via a new key/enum. XPCOM/QI need not be involved and heap allocation is undesirable.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 15 years ago

(In reply to comment #16)
> nsIAppShellService has a nsIXULWindow hiddenWindow attribute. From that can I
> QI to an nsIBaseWindow? (I don't know a good way to check this without
> actually running it).

Dunno, but there must be a way.

	Rob Arnold [:robarnold] Assignee
	Comment 18 • 15 years ago

Changes:
Dropped the hidden window pointer.
Dropped the global hashtable (this removes the need for an nsIObserver and Initialization function).
Renamed MonitorArray to CallbackDataArray to make the type clearer.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 15 years ago

+  WindowHook&             GetWindowHook() { return mWindowHook; }

Just call this WindowHook().

+    bool operator== (const CallbackData &rhs) const {
+      return cb == rhs.cb && context == rhs.context;
+    }
+    bool operator!= (const CallbackData &rhs) const {
+      return !(*this == rhs);

PRBool

+  NS_ENSURE_TRUE(nsnull == data->hook.cb, NS_ERROR_INVALID_ARG);

NS_ERROR_UNEXPECTED would be better, I think

+    return NS_ERROR_INVALID_ARG;
+  if (data->hook != cbdata)
+    return NS_ERROR_INVALID_ARG;

Ditto

+    if (!data) return nsnull;

Two lines

Wouldn't your hashtable stuff be simpler and more efficient using an nsDataHashtable?

	Rob Arnold [:robarnold] Assignee
	Comment 20 • 15 years ago

(In reply to comment #19)
> +  WindowHook&             GetWindowHook() { return mWindowHook; }
> 
> Just call this WindowHook().

I tried to initially and the compiler strongly disliked it. I suspect because it looked somewhat like a constructor but isn't.

> 
> +    bool operator== (const CallbackData &rhs) const {
> +      return cb == rhs.cb && context == rhs.context;
> +    }
> +    bool operator!= (const CallbackData &rhs) const {
> +      return !(*this == rhs);
> 
> PRBool

Thanks, I meant to do this!

> 
> +  NS_ENSURE_TRUE(nsnull == data->hook.cb, NS_ERROR_INVALID_ARG);
> 
> NS_ERROR_UNEXPECTED would be better, I think
> 
> +    return NS_ERROR_INVALID_ARG;
> +  if (data->hook != cbdata)
> +    return NS_ERROR_INVALID_ARG;
> 
> Ditto

Done.

> 
> +    if (!data) return nsnull;
> 
> Two lines

Done.

> 
> Wouldn't your hashtable stuff be simpler and more efficient using an
> nsDataHashtable?

I took a look at https://developer.mozilla.org/en/XPCOM_hashtable_guide which says that I shouldn't use a hashtable since the expected number of items is < 0 for most windows. I've implemented a simple dictionary using nsTArray.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 21 • 15 years ago

(In reply to comment #20)
> Created an attachment (id=394368) [details]
> v1.3
> 
> (In reply to comment #19)
> > +  WindowHook&             GetWindowHook() { return mWindowHook; }
> > 
> > Just call this WindowHook().
> 
> I tried to initially and the compiler strongly disliked it. I suspect because
> it looked somewhat like a constructor but isn't.

Oh yeah, that sucks!

	Rob Arnold [:robarnold] Assignee
	Comment 22 • 15 years ago

Comment on attachment 394368 [details] [diff] [review]
v1.3

Requesting approval for 1.9.2 since this is needed for taskbar previews.

	Rob Arnold [:robarnold] Assignee
	Comment 23 • 15 years ago

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/8880b3778f53

	Jacek Caban
	Comment 24 • 15 years ago

This patch broke mingw build. I get following error:

WindowHook.h:106: error: a class-key must be used when declaring a friend

The fix is trivial, I've attached the patch.

	Jim Mathies [:jimm]
	Comment 25 • 15 years ago

(In reply to comment #24)
> Created an attachment (id=394645) [details]
> mingw fix
> 
> This patch broke mingw build. I get following error:
> 
> WindowHook.h:106: error: a class-key must be used when declaring a friend
> 
> The fix is trivial, I've attached the patch.

This is being addressed in bug 510650.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 26 • 15 years ago

This blocks a blocking bug, and thus is itself a blocker.

	Jim Mathies [:jimm]
	Comment 27 • 15 years ago

pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dbd4ab7ead73

	chris hofmann
	Comment 28 • 15 years ago

can someone quickly check Bug 530962 to see if that bug is crash fall out from this change?

	Rob Arnold [:robarnold] Assignee
	Comment 29 • 15 years ago

It's not. Commented in that bug.