1009445 - Support AsyncEventDispatcher on non-node interfaces

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Shelly Lin [:shelly]]

Per bug 744896, comment 59, we are dispatching a sync event on a non-node interface (AudioTrackList and VideoTrackList), but it would be better if we can dispatch an async event.

Currently, AsyncEventDispatcher takes only a nsINode object as the event node.

Comment 1

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

We have various helpers in DOMEventTargetHelper.
Perhaps we could add
DispatchAsyncTrustedEvent.

Comment 2

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

Though, (new AsyncEventDispatcher(target, event))->PostDOMEvent(); is simple too

Comment 3

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

Attachment: async_for_all_targets_2.diff

ensureDeletionWhenFailing part is a bit odd, but it ensures that
someone is keeping the object alive, and in case
dispatch/addscriptrunner fails, AsyncEventDispatcher is deleted.
Technically RunDOMEventWhenSafe doesn't need it right now, but
in principle caller should ensure sane refcnt.

https://tbpl.mozilla.org/?tree=Try&rev=47fbab1041af

Comment 4

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

Attachment: without dom:: prefix in .cpp

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8421919 [details] [diff] [review]
without dom:: prefix in .cpp

>diff --git a/dom/events/AsyncEventDispatcher.cpp b/dom/events/AsyncEventDispatcher.cpp
>--- a/dom/events/AsyncEventDispatcher.cpp
>+++ b/dom/events/AsyncEventDispatcher.cpp
> nsresult
> AsyncEventDispatcher::PostDOMEvent()
> {
>+  nsRefPtr<AsyncEventDispatcher> ensureDeletionWhenFailing =
>+    mRefCnt.get() > 1 ? nullptr : this;
>   return NS_DispatchToCurrentThread(this);
> }
> 
> void
> AsyncEventDispatcher::RunDOMEventWhenSafe()
> {
>+  nsRefPtr<AsyncEventDispatcher> ensureDeletionWhenFailing =
>+    mRefCnt.get() > 1 ? nullptr : this;
>   nsContentUtils::AddScriptRunner(this);
> }

Hmm, I don't understand them well. Why don't you hold the instance always?

>diff --git a/dom/events/AsyncEventDispatcher.h b/dom/events/AsyncEventDispatcher.h
>--- a/dom/events/AsyncEventDispatcher.h
>+++ b/dom/events/AsyncEventDispatcher.h
>@@ -21,39 +21,39 @@ namespace mozilla {
>  * For example, you may need to fire an event from within layout, but
>  * want to ensure that the event handler doesn't mutate the DOM at
>  * the wrong time, in order to avoid resulting instability.
>  */
>  
> class AsyncEventDispatcher : public nsRunnable
> {
> public:
>-  AsyncEventDispatcher(nsINode* aEventNode, const nsAString& aEventType,
>+  AsyncEventDispatcher(dom::EventTarget* aTarget, const nsAString& aEventType,
>                        bool aBubbles, bool aDispatchChromeOnly)
>-    : mEventNode(aEventNode)
>+    : mTarget(aTarget)
>     , mEventType(aEventType)
>     , mBubbles(aBubbles)
>     , mDispatchChromeOnly(aDispatchChromeOnly)
>   {
>   }

How about duplicate this constructor? One is for chrome only which takes nsINode. The other removes aDispatchChromeOnly? Then, the MOZ_ASSERT()s in Run() are safer for most people. Although, I'm not sure if this is possible.

I'll offline today and tomorrow. So, +'ing this review with the points. If I'm wrong, please ignore and land this.

Comment 6

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> Comment on attachment 8421919 [details] [diff] [review]
> without dom:: prefix in .cpp
> 
> >diff --git a/dom/events/AsyncEventDispatcher.cpp b/dom/events/AsyncEventDispatcher.cpp
> >--- a/dom/events/AsyncEventDispatcher.cpp
> >+++ b/dom/events/AsyncEventDispatcher.cpp
> > nsresult
> > AsyncEventDispatcher::PostDOMEvent()
> > {
> >+  nsRefPtr<AsyncEventDispatcher> ensureDeletionWhenFailing =
> >+    mRefCnt.get() > 1 ? nullptr : this;
> >   return NS_DispatchToCurrentThread(this);
> > }
> > 
> > void
> > AsyncEventDispatcher::RunDOMEventWhenSafe()
> > {
> >+  nsRefPtr<AsyncEventDispatcher> ensureDeletionWhenFailing =
> >+    mRefCnt.get() > 1 ? nullptr : this;
> >   nsContentUtils::AddScriptRunner(this);
> > }
> 
> Hmm, I don't understand them well. Why don't you hold the instance always?
Just for the sake of performance. Though, perf isn't possibly too critical here.

> >+  AsyncEventDispatcher(dom::EventTarget* aTarget, const nsAString& aEventType,
> >                        bool aBubbles, bool aDispatchChromeOnly)
> >-    : mEventNode(aEventNode)
> >+    : mTarget(aTarget)
> >     , mEventType(aEventType)
> >     , mBubbles(aBubbles)
> >     , mDispatchChromeOnly(aDispatchChromeOnly)
> >   {
> >   }
> 
> How about duplicate this constructor? One is for chrome only which takes
> nsINode. The other removes aDispatchChromeOnly? Then, the MOZ_ASSERT()s in
> Run() are safer for most people. 
I could do that.

Comment 7

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

Attachment: +comments

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/31a250e951cf

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31a250e951cf