Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Description • 18 years ago

I think nsINode, nsGlobalWindow and nsWindowRoot could extend "nsPIDOMEventTarget". That interface should include all the methods
needed for event dispatching. This way nsIChromeEventHandler could
be removed and nsEventDispatcher simplified.
Patch coming.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 1 • 18 years ago

This saves few kBs on my x86_64 builds and reduces the size of nsXULElement.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 2 • 18 years ago

Comment on attachment 247826 [details] [diff] [review]
something like this

> nsresult
> nsContentUtils::GetListenerManager(nsINode *aNode,
>                                    PRBool aCreateIfNotFound,
>-                                   nsIEventListenerManager **aResult,
>-                                   PRBool *aCreated)
>+                                   nsIEventListenerManager **aResult)
> {
>   *aResult = nsnull;
>-  *aCreated = PR_FALSE;
> 
>+  if (!aCreateIfNotFound && !aNode->HasFlag(NODE_HAS_LISTENERMANAGER)) {
>+    *aResult = nsnull;
>+    return NS_OK;

+ *aResult = nsnull; isn't needed


>Index: xpfe/appshell/src/nsWebShellWindow.cpp

Changes to nsWebShellWindow.cpp aren't needed.

	Alex Vincent [:WeirdAl]
	Comment 3 • 18 years ago

What is a PIDOMEventTarget?  I know what DOMEventTarget means, but the PI part confuses me.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 18 years ago

Hey, could we kill off nsIDOMEventReceiver in the process too?  :)

Alex, look at the patch.  This is an interface for the internal event dispatch methods.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 5 • 18 years ago

(In reply to comment #4)
> Hey, could we kill off nsIDOMEventReceiver in the process too?  :)

Yes, yes!

But perhaps I could do that in a separate bug, please ;)

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 18 years ago

Sure thing.  ;)

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 18 years ago

Please make HasFlag public rather than making nsContentUtils a friend.

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 8 • 18 years ago

> 
> >Index: xpfe/appshell/src/nsWebShellWindow.cpp
> 
> Changes to nsWebShellWindow.cpp aren't needed.
> 

Oops, yes those are needed.

	Johnny Stenback (:jst)
	Comment 10 • 18 years ago

Comment on attachment 249261 [details] [diff] [review]
up-to-date

 nsEventTargetChainItem::nsEventTargetChainItem(nsISupports* aTarget,
-                                               PRBool aTargetIsChromeHandler,
                                                nsEventTargetChainItem* aChild)
-: mNode(nsnull), mChild(aChild), mParent(nsnull), mFlags(0), mItemFlags(0)
+: mChild(aChild), mParent(nsnull), mFlags(0), mItemFlags(0)
 {
[...]
+  mTarget = do_QueryInterface(aTarget);

This could also be initialized above combine calling the nsCOMPtr ctor w/o an argument and doing the assign here.

r=jst

	Olli Pettay [:smaug][bugs@pettay.fi] Assignee
	Comment 11 • 18 years ago

Now after Bug 333078, nsPIDOMEventTarget shouldn't inherit nsIDOMGCParticipant but nsISupports.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 13 • 18 years ago

Comment on attachment 251889 [details] [diff] [review]
up-to-date

You've left a few variables and functions named (Get|m|)ChromeEventHandler. Would be good to have those renamed, though if you want to do that as a separate file that's fine with me.

>Index: content/events/src/nsEventDispatcher.cpp

> nsEventTargetChainItem::nsEventTargetChainItem(nsISupports* aTarget,
>-                                               PRBool aTargetIsChromeHandler,
>                                                nsEventTargetChainItem* aChild)
>-: mNode(nsnull), mChild(aChild), mParent(nsnull), mFlags(0), mItemFlags(0)
>+: mChild(aChild), mParent(nsnull), mFlags(0), mItemFlags(0)
> {
...
>+  mTarget = do_QueryInterface(aTarget);
> }

You can put the mTarget initialization in the colon-list as well.

> nsEventTargetChainItem::CurrentTarget()
> {
...
>+  return mTarget;
> }

Inline this method?

>Index: docshell/base/nsDocShell.cpp

>+nsDocShell::GetChromeEventHandler(nsIDOMEventTarget** aChromeEventHandler)
> {
>     NS_ENSURE_ARG_POINTER(aChromeEventHandler);
>-
>-    *aChromeEventHandler = mChromeEventHandler;
>+    nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mChromeEventHandler);
>+    *aChromeEventHandler = target;
>     NS_IF_ADDREF(*aChromeEventHandler);
>     return NS_OK;
> }

Use .swap here instead?

sr=me with that fixed.