1361305 - Use forwarding reference for the ListenerImpl constructor in MediaEventSource.h

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

Motivated by https://reviewboard.mozilla.org/r/135380/#review138314. We want to enable move whenever possible.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1361305. P1 - use forwarding reference for the ListenerImpl constructor to enable move whenever possible.

Review commit: https://reviewboard.mozilla.org/r/135700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135700/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1361305. P2 - add a gtest to test an rvalue lambda is moved instead of copied when adding a listener.

Review commit: https://reviewboard.mozilla.org/r/135702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135702/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8863975 [details]
Bug 1361305. P1 - use forwarding reference for the ListenerImpl constructor to enable move whenever possible.

https://reviewboard.mozilla.org/r/135700/#review138724

Thank you for implementing this.

r+ with optional nits:

::: dom/media/MediaEventSource.h:171
(Diff revision 1)
>   */
>  template <typename Target, typename Function, typename... As>
>  class ListenerImpl : public Listener<As...>
>  {
> +  // Strip CV and reference from Function.
> +  using StorageType = typename Decay<Function>::Type;

I don't think "Type" is useful in this particular type name (Because I believe you mean it to be a C++ type, as opposed to some domain-specific type, e.g.: media type).
I'd prefer `StoredFunction` or `FunctionStorage` -- We know it's a C++ type, and the name tells us it's there to store a function object.

::: dom/media/MediaEventSource.h:353
(Diff revision 1)
>      auto f = [=] (ArgType<Es>&&... aEvents) {
>        (thiz.get()->*aMethod)(Move(aEvents)...);
>      };
> -    return ConnectInternal(aTarget, f);
> +    return ConnectInternal(aTarget, Move(f));

It was there before, and it's quite possible that the compiler will optimize this just fine, but while we're here:

I would think `auto f = <lambda>; foo(Move(f));` could be rewritten as just `foo(<lambda>);`.
I.e., just give the lambda prvalue directly to ConnectInternal, instead of storing it in a local `auto` variable only to move from it.

(Same in the next function.)

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8863976 [details]
Bug 1361305. P2 - add a gtest to test an rvalue lambda is moved instead of copied when adding a listener.

https://reviewboard.mozilla.org/r/135702/#review138726

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8863975 [details]
Bug 1361305. P1 - use forwarding reference for the ListenerImpl constructor to enable move whenever possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135700/diff/1-2/

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8863976 [details]
Bug 1361305. P2 - add a gtest to test an rvalue lambda is moved instead of copied when adding a listener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135702/diff/1-2/

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8863975 [details]
Bug 1361305. P1 - use forwarding reference for the ListenerImpl constructor to enable move whenever possible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135700/diff/2-3/

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8863976 [details]
Bug 1361305. P2 - add a gtest to test an rvalue lambda is moved instead of copied when adding a listener.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135702/diff/2-3/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a02b3b88ec61
P1 - use forwarding reference for the ListenerImpl constructor to enable move whenever possible. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4cbfb9e89d93
P2 - add a gtest to test an rvalue lambda is moved instead of copied when adding a listener. r=gerald

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/a02b3b88ec61
https://hg.mozilla.org/mozilla-central/rev/4cbfb9e89d93