Closed Bug 363067 Opened 13 years ago Closed 13 years ago

Add nsPIDOMEventTarget and kill nsIChromeEventHandler

Categories

(Core :: DOM: Events, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch something like this (obsolete) — Splinter Review
This saves few kBs on my x86_64 builds and reduces the size of nsXULElement.
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.
What is a PIDOMEventTarget?  I know what DOMEventTarget means, but the PI part confuses me.
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.
(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 ;)
Sure thing.  ;)
Attachment #247826 - Flags: review?(jst)
Blocks: 363089
Please make HasFlag public rather than making nsContentUtils a friend.
> 
> >Index: xpfe/appshell/src/nsWebShellWindow.cpp
> 
> Changes to nsWebShellWindow.cpp aren't needed.
> 

Oops, yes those are needed.
Attached patch up-to-date (obsolete) — Splinter Review
Attachment #247826 - Attachment is obsolete: true
Attachment #249261 - Flags: review?(jst)
Attachment #247826 - Flags: review?(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
Attachment #249261 - Flags: review?(jst) → review+
Attachment #249261 - Flags: superreview?(bugmail)
Now after Bug 333078, nsPIDOMEventTarget shouldn't inherit nsIDOMGCParticipant but nsISupports.
Blocks: 290127
Attached patch up-to-dateSplinter Review
Attachment #249261 - Attachment is obsolete: true
Attachment #251889 - Flags: superreview?(jonas)
Attachment #249261 - Flags: superreview?(jonas)
Blocks: 369153
Blocks: 370433
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.
Attachment #251889 - Flags: superreview?(jonas) → superreview+
Attached patch for check inSplinter Review
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 26429
You need to log in before you can comment on or make changes to this bug.