Closed Bug 1361305 Opened 4 years ago Closed 4 years ago

Use forwarding reference for the ListenerImpl constructor in MediaEventSource.h

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

Motivated by https://reviewboard.mozilla.org/r/135380/#review138314. We want to enable move whenever possible.
Assignee: nobody → jwwang
Priority: -- → P3
See Also: → 1361259
Attachment #8863975 - Flags: review?(gsquelart)
Attachment #8863976 - Flags: review?(gsquelart)
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.)
Attachment #8863975 - Flags: review?(gsquelart) → review+
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
Attachment #8863976 - Flags: review?(gsquelart) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a02b3b88ec61
https://hg.mozilla.org/mozilla-central/rev/4cbfb9e89d93
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.