497178 - Make nsPIDOMEventTarget::GetListenerManager to return nsIEventListenerManager*

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Attachment: Not yet tested, requires trunk patch for bug 491134.

This should reduce unnecessary Addref/Release

Comment 1

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

Attachment: small tweaks

Needs still testing.

Comment 2

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

Attachment: v3

Comment 3

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

Attachment: v4

Adding without strong ref is always ok, removing is ok if one listener is removed.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 382733 [details] [diff] [review]
v4

>diff --git a/content/base/src/nsGenericElement.cpp b/content/base/src/nsGenericElement.cpp

> nsDOMEventRTTearoff::GetDOM3EventTarget(nsIDOM3EventTarget **aTarget)

>+  nsIEventListenerManager* listener_manager = mNode->GetListenerManager(PR_TRUE);

Long line.

> nsDOMEventRTTearoff::AddEventListener(const nsAString& aType,

>+  nsIEventListenerManager* listener_manager = mNode->GetListenerManager(PR_TRUE);

Long line.

> nsGenericElement::GetEventListenerManagerForAttr(nsIEventListenerManager** aManager,
>                                                  nsISupports** aTarget,
>                                                  PRBool* aDefer)
> {
>-  nsresult rv = GetListenerManager(PR_TRUE, aManager);
>-  if (NS_SUCCEEDED(rv)) {
>-    NS_ADDREF(*aTarget = static_cast<nsIContent*>(this));
>-  }
>+  *aManager = GetListenerManager(PR_TRUE);
>   *aDefer = PR_TRUE;
>-  return rv;
>+  NS_ENSURE_STATE(*aManager);
>+  NS_ADDREF(*aManager);
>+  NS_ADDREF(*aTarget = static_cast<nsIContent*>(this));
>+  return NS_OK;

The one caller to GetEventListenerManagerForAttr checks for success rv and non-null manager, maybe it should just check for non-null manager?

>diff --git a/content/events/public/nsPIDOMEventTarget.h b/content/events/public/nsPIDOMEventTarget.h

>+  virtual nsIEventListenerManager* GetListenerManager(PRBool aCreateIfNotFound) = 0;

Long line.

>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp

>-nsGlobalWindow::GetListenerManager(PRBool aCreateIfNotFound,
>-                                   nsIEventListenerManager** aResult)
>-{
>-  FORWARD_TO_INNER_CREATE(GetListenerManager, (aCreateIfNotFound, aResult));
>+nsIEventListenerManager*
>+nsGlobalWindow::GetListenerManager(PRBool aCreateIfNotFound)
>+{
>+  if (IsOuterWindow()) {
>+    if (!mInnerWindow) {
>+      NS_ENSURE_TRUE(!mIsClosed, nsnull);
>+      nsCOMPtr<nsIDOMDocument> doc;
>+      nsresult rv = GetDocument(getter_AddRefs(doc));
>+      NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && mInnerWindow, nsnull);
>+    }
>+    return GetCurrentInnerWindowInternal()->GetListenerManager(aCreateIfNotFound);                      \
>+  }

I think you should change FORWARD_TO_INNER_CREATE so it has a err_rval argument, just like FORWARD_TO_INNER.

> 
>   if (!mListenerManager) {
>     if (!aCreateIfNotFound) {
>-      *aResult = nsnull;
>-      return NS_OK;
>+      return nsnull;
>     }
> 
>     static NS_DEFINE_CID(kEventListenerManagerCID,
>                          NS_EVENTLISTENERMANAGER_CID);
>     nsresult rv;
> 
>     mListenerManager = do_CreateInstance(kEventListenerManagerCID, &rv);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    NS_ENSURE_SUCCESS(rv, nsnull);

Don't need rv anymore, just check mListenerManager.

>diff --git a/dom/base/nsWindowRoot.cpp b/dom/base/nsWindowRoot.cpp

>+nsWindowRoot::GetListenerManager(PRBool aCreateIfNotFound)

>     nsresult rv;
>     mListenerManager = do_CreateInstance(kEventListenerManagerCID, &rv);
>-    if (NS_FAILED(rv)) return rv;
>+    if (NS_FAILED(rv)) return nsnull;

Don't need rv anymore, just check mListenerManager.

Comment 5

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

Attachment: +comments

Comment 6

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

http://hg.mozilla.org/mozilla-central/rev/8d5c08070a23