Closed Bug 1161742 Opened 9 years ago Closed 9 years ago

Make MediaPromises work with lambdas

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

It's time. This shouldn't be too hard.
This cleanup was made possibly by the introduction of AbstractThread. :-)
Attachment #8601820 - Flags: review?(jwwang)
Attachment #8601825 - Flags: review?(jwwang)
Attachment #8601826 - Flags: review?(jwwang)
Attachment #8601820 - Flags: review?(jwwang) → review+
Comment on attachment 8601821 [details] [diff] [review]
Part 2 - Introduce ResolveOrRejectValue to make MediaPromise implementation less verbose. v1

Review of attachment 8601821 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaPromise.h
@@ +53,5 @@
> +  {
> +  public:
> +    void SetResolve(ResolveValueType& aResolveValue)
> +    {
> +      MOZ_ASSERT(!mRejectValue.isSome());

Should we assert IsNothing() if we only want to resolve or reject once?
Attachment #8601821 - Flags: review?(jwwang) → review+
Comment on attachment 8601822 [details] [diff] [review]
Part 3 - Hoist part of ResolveOrReject into ThenValueBase. v1

Review of attachment 8601822 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaPromise.h
@@ -257,4 @@
>      {
> -      Consumer::mComplete = true;
> -      if (Consumer::mDisconnected) {
> -        MOZ_ASSERT(!mThisVal);

This assertion is missing after extraction. Is that your intention?
Attachment #8601822 - Flags: review?(jwwang) → review+
Attachment #8601823 - Flags: review?(jwwang) → review+
Comment on attachment 8601824 [details] [diff] [review]
Part 5 - Add support for lambdas in MediaPromise. v1

Review of attachment 8601824 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaPromise.h
@@ +386,5 @@
> +  void Then(AbstractThread* aResponseThread, const char* aCallSite,
> +            ResolveFunction&& aResolveFunction, RejectFunction&& aRejectFunction)
> +  {
> +    nsRefPtr<Consumer> c =
> +      RefableThen(aResponseThread, aCallSite, Move(aResolveFunction), Move(aRejectFunction));

It should be Forward(aResolveFunction) in case we pass a non-temp object.
Attachment #8601824 - Flags: review?(jwwang) → review+
Attachment #8601825 - Flags: review?(jwwang) → review+
Comment on attachment 8601826 [details] [diff] [review]
Part 7 - Use lambdas for Seek. v1

Review of attachment 8601826 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ -2384,5 @@
>  void
> -MediaDecoderStateMachine::OnSeekCompleted(int64_t aTime)
> -{
> -  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> -  MOZ_ASSERT(OnTaskQueue());

The assertion is missing after moving.
Attachment #8601826 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #8)
> Should we assert IsNothing() if we only want to resolve or reject once?

We assert that implicitly in the .emplace call, but I agree that just asserting IsNothing() is clearer.

(In reply to JW Wang [:jwwang] from comment #9)
> This assertion is missing after extraction. Is that your intention?

Yeah. There's not an easy way to carry it over and I didn't think it was important enough to design around.

(In reply to JW Wang [:jwwang] from comment #10)
> It should be Forward(aResolveFunction) in case we pass a non-temp object.

Good point.

(In reply to JW Wang [:jwwang] from comment #11)
> > -  MOZ_ASSERT(OnTaskQueue());
> 
> The assertion is missing after moving.

That's intentional. In the interest of keeping lambdas short and readable, I decided on a convention where MediaPromise callbacks don't need to assert that they're running on the intended task queue, because you can always see the target thread a few lines above and the MediaPromise machinery guarantees that we'll be called back on the target thread.
(In reply to Bobby Holley (:bholley) from comment #12)
> That's intentional. In the interest of keeping lambdas short and readable, I
> decided on a convention where MediaPromise callbacks don't need to assert
> that they're running on the intended task queue, because you can always see
> the target thread a few lines above and the MediaPromise machinery
> guarantees that we'll be called back on the target thread.

Now I see the beauty of lambdas. :)
(In reply to JW Wang [:jwwang] from comment #10)

> It should be Forward(aResolveFunction) in case we pass a non-temp object.

No.

You get there because the argument is a rvalue. It is always a temp object. You have to use Move.
(In reply to Jean-Yves Avenard [:jya] from comment #14)
> (In reply to JW Wang [:jwwang] from comment #10)
> 
> > It should be Forward(aResolveFunction) in case we pass a non-temp object.
> 
> No.
> 
> You get there because the argument is a rvalue. It is always a temp object.
> You have to use Move.

T&& is a forwarding reference instead of a rvalue reference. Even if it is a rvalue reference, Forward() will still turn out to be a rvalue reference.
You need to log in before you can comment on or make changes to this bug.