Closed Bug 1009445 Opened 6 years ago Closed 6 years ago

Support AsyncEventDispatcher on non-node interfaces

Categories

(Core :: DOM: Events, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: shelly, Assigned: smaug)

Details

Attachments

(2 files, 1 obsolete file)

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.
We have various helpers in DOMEventTargetHelper.
Perhaps we could add
DispatchAsyncTrustedEvent.
Though, (new AsyncEventDispatcher(target, event))->PostDOMEvent(); is simple too
Assignee: nobody → bugs
Attached patch async_for_all_targets_2.diff (obsolete) — Splinter Review
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
Attachment #8421904 - Flags: review?(masayuki)
Attachment #8421904 - Attachment is obsolete: true
Attachment #8421904 - Flags: review?(masayuki)
Attachment #8421919 - Flags: review?(masayuki)
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.
Attachment #8421919 - Flags: review?(masayuki) → review+
(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.
Attached patch +commentsSplinter Review
https://hg.mozilla.org/mozilla-central/rev/31a250e951cf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.