363067 - Add nsPIDOMEventTarget and kill nsIChromeEventHandler

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: something like this

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Alex Vincent [:WeirdAl]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(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 ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sure thing.  ;)

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Please make HasFlag public rather than making nsContentUtils a friend.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Oops, yes those are needed.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 10

[Johnny Stenback (:jst)]

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

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: up-to-date

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for check in