306974 - Remove some duplicate event dispatching code

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

Currently there are many places in content/, layout/ and dom/ (...) which
dispatch a trusted event in a similar way, like:
event = doc->createEvent(...);
event->setTrusted(PR_TRUE);
target->dispatchEvent(event);

I think the duplication happens often enough for a centralized method.
So perhaps nsContentUtils::DispatchTrustedEvent() would make sense.

Comment 1

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

Attachment: Adding nsContentUtils::DispatchTrustedEvent

Comment 2

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

Attachment: up-to-date

Comment 3

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

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

>+   * @param aDefaultAction The value returned by the
>+   *                       nsIDOMEventTarget::DispatchEvent();

How about "Set to true if default action should be taken, see
nsIDOMEventTarget::DispatchEvent"?

>+  static nsresult DispatchTrustedEvent(nsIDocument* aDoc,
>+                                       nsISupports* aTarget,
>+                                       const nsAString& aEventName,
>+                                       PRBool aCanBubble = PR_TRUE,
>+                                       PRBool aCancelable = PR_TRUE,
>+                                       PRBool *aDefaultAction = nsnull);

I'd be happy to loose the default values here. It's always nice to force people
to think about what they really want to do, especially for something as easy to
forget as this. Possibly the last default could stay.

>+nsresult
>+nsContentUtils::DispatchTrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
>+                                     const nsAString& aEventName,
>+                                     PRBool aCanBubble, PRBool aCancelable,
>+                                     PRBool *aDefaultAction)
>+{
>+  nsCOMPtr<nsIDOMDocumentEvent> docEvent(do_QueryInterface(aDoc));
>+  nsCOMPtr<nsIDOMEventTarget> target(do_QueryInterface(aTarget));
>+  if (docEvent && target) {

NS_ENSURE_TRUE(docEvent && target, NS_ERROR_INVALID_ARG);

>+    nsCOMPtr<nsIDOMEvent> event;
>+    nsresult rv =
>+      docEvent->CreateEvent(NS_LITERAL_STRING("Events"), getter_AddRefs(event));

NS_ENSURE_SUCCESS(rv, rv);

>+    nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(event));
>+    if (NS_SUCCEEDED(rv) && privateEvent) {
>+      event->InitEvent(aEventName, aCanBubble, aCancelable);
>+      privateEvent->SetTrusted(PR_TRUE);
>+      PRBool defaultActionEnabled;
>+      rv = target->DispatchEvent(event, &defaultActionEnabled);

NS_ENSURE_SUCCESS(rv, rv);

>+      if (NS_SUCCEEDED(rv) && aDefaultAction) {
>+        *aDefaultAction = defaultActionEnabled;
>+      }

You could also do:

PRBool dummy;
rv = target->DispatchEvent(event, aDefaultAction ? aDefaultAction : &dummy);

>+    }
>+    return rv;

Functions ending with |return rv| is generally a good indicator of missing
NS_ENSURE_SUCCESS.

>Index: content/base/src/nsDocument.cpp
>@@ -2211,16 +2201,18 @@ nsDocument::DispatchContentLoadedEvents(
> 
>       if (!ancestor_doc) {
>         break;
>       }
> 
>       nsCOMPtr<nsIDOMDocumentEvent> document_event =
>         do_QueryInterface(ancestor_doc);
> 
>+      nsCOMPtr<nsIDOMEvent> event;
>+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent;

Why this change?

With those changes, r=me

Comment 4

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

> >Index: content/base/src/nsDocument.cpp
> >@@ -2211,16 +2201,18 @@ nsDocument::DispatchContentLoadedEvents(
> > 
> >       if (!ancestor_doc) {
> >         break;
> >       }
> > 
> >       nsCOMPtr<nsIDOMDocumentEvent> document_event =
> >         do_QueryInterface(ancestor_doc);
> > 
> >+      nsCOMPtr<nsIDOMEvent> event;
> >+      nsCOMPtr<nsIPrivateDOMEvent> privateEvent;
> 
> Why this change?

Read the patch ;) Those were defined in the
beginning of the method, but because DOMContentLoaded
uses now nsContentUtils...

Comment 5

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

Attachment: + comments

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 198713 [details] [diff] [review]
+ comments

> Index: content/base/public/nsContentUtils.h
> ===================================================================

> +   * Works only with events, which can be created by calling

Nit: drop the comma after events

> Index: content/base/src/nsContentUtils.cpp
> ===================================================================

> +nsresult
> +nsContentUtils::DispatchTrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
> +                                     const nsAString& aEventName,
> +                                     PRBool aCanBubble, PRBool aCancelable,
> +                                     PRBool *aDefaultAction)

> +  nsCOMPtr<nsIPrivateDOMEvent> privateEvent(do_QueryInterface(event));
> +  NS_ENSURE_STATE(privateEvent);

Nit: I'd use NS_ENSURE_TRUE, it's not really ensuring some state

> Index: content/html/content/src/nsHTMLLinkElement.cpp
> ===================================================================

> +  nsContentUtils::DispatchTrustedEvent(aDoc,
> +                                       NS_STATIC_CAST(nsIDOMNode*, this),

Nit: I'd prefer if you used the right class (nsIContent) to cast to
nsISupports, here and in the other element classes.

> Index: layout/generic/nsObjectFrame.cpp
> ===================================================================

>  static void
>  FirePluginNotFoundEvent(nsIContent *aTarget)
>  {
> -  nsCOMPtr<nsIDOMDocumentEvent> eventDoc =
> -    do_QueryInterface(aTarget->GetDocument());

> +  nsContentUtils::DispatchTrustedEvent(aTarget->GetCurrentDoc(), aTarget,

Are you sure this should be GetCurrentDoc? If not, keep using GetDocument.

Nit: take care of long lines:
http://lab.cph.novell.com/~abeaufour/jst-review/?patch=198713