Closed Bug 1361259 Opened 4 years ago Closed 4 years ago

Use NewRunnableMethod() to simplify the code of MediaEventSource

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(5 files)

Bug 1361259. P1 - let ListenerBase inherit RevocableToken.
59 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1361259. P2 - use NewRunnableMethod() to pass event data to the listener function.
59 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1361259. P3 - remove unused code.
59 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1361259. P4 - enforce copy in NonExclusive mode for each listener must get a copy.
59 bytes, text/x-review-board-request
gerald
: review+
Details
Bug 1361259. P5 - fix MediaEventSource::CopyEvent2 which is broken by P2.
59 bytes, text/x-review-board-request
gerald
: review+
Details 
NewRunnableMethod() has all the machinery to send data to another thread.
Assignee: nobody → jwwang
Priority: -- → P3
Attachment #8863599 - Flags: review?(gsquelart)
Attachment #8863600 - Flags: review?(gsquelart)
Attachment #8863601 - Flags: review?(gsquelart)
Attachment #8863602 - Flags: review?(gsquelart)
Attachment #8863603 - Flags: review?(gsquelart)
Comment on attachment 8863599 [details]
Bug 1361259. P1 - let ListenerBase inherit RevocableToken.

https://reviewboard.mozilla.org/r/135378/#review138304

::: commit-message-2e7c1:3
(Diff revision 1)
> +This is needed by P2 where |Listener| must be ref-counted so we can use
> +NewRunnableMethod() to pass event data to the listener function.

It took me a while to understand the whole reasoning for making destructors virtual!
As it's not obvious in this patch, could you please add a mention that Listeners will also be ref-counted through their RevocableToken base type when stored in ListenerHelper::mHelper.
Attachment #8863599 - Flags: review?(gsquelart) → review+
Comment on attachment 8863600 [details]
Bug 1361259. P2 - use NewRunnableMethod() to pass event data to the listener function.

https://reviewboard.mozilla.org/r/135380/#review138314

::: dom/media/MediaEventSource.h:255
(Diff revision 1)
>    ListenerImpl(Target* aTarget, const Function& aFunction)
> -    : mHelper(this, aTarget, aFunction) {}
> -  void Dispatch(const As&... aEvents) override {
> +    : mTarget(aTarget)
> +    , mFunction(aFunction)

It was there before, so this is an idea for another bug: Instead of passing the function by const-ref and copying it into mFunction here, it would be nice to pass a forwarding ref, so that rvalues (like lambdas) can just be moved.
Attachment #8863600 - Flags: review?(gsquelart) → review+
Comment on attachment 8863601 [details]
Bug 1361259. P3 - remove unused code.

https://reviewboard.mozilla.org/r/135382/#review138316
Attachment #8863601 - Flags: review?(gsquelart) → review+
Comment on attachment 8863602 [details]
Bug 1361259. P4 - enforce copy in NonExclusive mode for each listener must get a copy.

https://reviewboard.mozilla.org/r/135384/#review138318
Attachment #8863602 - Flags: review?(gsquelart) → review+
Comment on attachment 8863603 [details]
Bug 1361259. P5 - fix MediaEventSource::CopyEvent2 which is broken by P2.

https://reviewboard.mozilla.org/r/135386/#review138320
Attachment #8863603 - Flags: review?(gsquelart) → review+
Comment on attachment 8863599 [details]
Bug 1361259. P1 - let ListenerBase inherit RevocableToken.

https://reviewboard.mozilla.org/r/135378/#review138304

> It took me a while to understand the whole reasoning for making destructors virtual!
> As it's not obvious in this patch, could you please add a mention that Listeners will also be ref-counted through their RevocableToken base type when stored in ListenerHelper::mHelper.

Virtual destructors are requried when deleting an object through a base type pointer which is RevocableToken in this case. So in our Gecko code, all ref-counting types should either have a private destructor (checked by static-analysis) or a protected virtual destructor if they will be inherited.
Comment on attachment 8863600 [details]
Bug 1361259. P2 - use NewRunnableMethod() to pass event data to the listener function.

https://reviewboard.mozilla.org/r/135380/#review138314

> It was there before, so this is an idea for another bug: Instead of passing the function by const-ref and copying it into mFunction here, it would be nice to pass a forwarding ref, so that rvalues (like lambdas) can just be moved.

Sure. Will fix that in a new bug.
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8d5c205eeee
P1 - let ListenerBase inherit RevocableToken. r=gerald
https://hg.mozilla.org/integration/autoland/rev/b8fe34848ac8
P2 - use NewRunnableMethod() to pass event data to the listener function. r=gerald
https://hg.mozilla.org/integration/autoland/rev/cf5321dca675
P3 - remove unused code. r=gerald
https://hg.mozilla.org/integration/autoland/rev/d0d6b7a2b75d
P4 - enforce copy in NonExclusive mode for each listener must get a copy. r=gerald
https://hg.mozilla.org/integration/autoland/rev/f79e3e648100
P5 - fix MediaEventSource::CopyEvent2 which is broken by P2. r=gerald
See Also: → 1361305
Comment on attachment 8863599 [details]
Bug 1361259. P1 - let ListenerBase inherit RevocableToken.

https://reviewboard.mozilla.org/r/135378/#review138304

> Virtual destructors are requried when deleting an object through a base type pointer which is RevocableToken in this case. So in our Gecko code, all ref-counting types should either have a private destructor (checked by static-analysis) or a protected virtual destructor if they will be inherited.

I know, I was just saying that it's not obvious from the patch alone that Listener is now referenced through a RefPtr<RevocableToken>, and I was suggesting that you just mention it in the commit description.
You need to log in before you can comment on or make changes to this bug.