Closed
Bug 1161742
Opened 9 years ago
Closed 9 years ago
Make MediaPromises work with lambdas
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
(Blocks 1 open bug)
Details
Attachments
(7 files)
5.59 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
10.65 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
2.32 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
6.25 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
3.00 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
4.04 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
It's time. This shouldn't be too hard.
Assignee | ||
Comment 1•9 years ago
|
||
This cleanup was made possibly by the introduction of AbstractThread. :-)
Attachment #8601820 -
Flags: review?(jwwang)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8601821 -
Flags: review?(jwwang)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8601822 -
Flags: review?(jwwang)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8601823 -
Flags: review?(jwwang)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8601824 -
Flags: review?(jwwang)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8601825 -
Flags: review?(jwwang)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8601826 -
Flags: review?(jwwang)
Updated•9 years ago
|
Attachment #8601820 -
Flags: review?(jwwang) → review+
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8601823 -
Flags: review?(jwwang) → review+
Comment 10•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8601825 -
Flags: review?(jwwang) → review+
Comment 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
(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.
Comment 13•9 years ago
|
||
(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. :)
Comment 14•9 years ago
|
||
(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.
Assignee | ||
Comment 15•9 years ago
|
||
Per IRC discussion, I'm going to leave it as-is for now and we change change it if we ever have a use for non-rvalues. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e84d753525f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7636ab3485da remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b6d0d1adea remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/39a90ea44a69 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc31573bde8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4663c0cc2e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/04a3f1ef4fbb
Comment 16•9 years ago
|
||
(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.
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9e84d753525f https://hg.mozilla.org/mozilla-central/rev/7636ab3485da https://hg.mozilla.org/mozilla-central/rev/e3b6d0d1adea https://hg.mozilla.org/mozilla-central/rev/39a90ea44a69 https://hg.mozilla.org/mozilla-central/rev/9fc31573bde8 https://hg.mozilla.org/mozilla-central/rev/7d4663c0cc2e https://hg.mozilla.org/mozilla-central/rev/04a3f1ef4fbb
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•