Miscellaneous MediaPromise improvements

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla37
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

5 years ago
I ran into a couple of things while implementing bug 1108707.
Assignee

Comment 2

5 years ago
We need this so that MediaTaskQueue can use promises.
Attachment #8533336 - Flags: review?(cpearce)
Assignee

Comment 4

5 years ago
It turns out that there are use-cases where this restriction is clunky.
Attachment #8533338 - Flags: review?(cpearce)
Attachment #8533335 - Flags: review?(cpearce) → review+
Comment on attachment 8533336 [details] [diff] [review]
Part 2 - Stop including MediaTaskQueue.h from MediaPromise.h. v1

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

::: dom/media/MediaPromise.cpp
@@ +8,5 @@
> +#include "MediaTaskQueue.h"
> +#include "nsThreadUtils.h"
> +
> +namespace mozilla {
> +namespace detail {

We're not supposed to use nested namespaces according to the style guide. I don't in particular mind, but you could make it a static member of the class instead?

::: dom/media/MediaPromise.h
@@ +35,5 @@
> +class MediaTaskQueue;
> +namespace detail {
> +
> +nsresult DispatchMediaPromise(MediaTaskQueue* aQueue, nsIRunnable* aRunnable);
> +nsresult DispatchMediaPromise(nsIEventTarget* aTarget, nsIRunnable* aRunnable);

Maybe that should be called DispatchResolveRunnable, since it's not really "dispatching a promise", it dispatches an nsIRunnable.
Attachment #8533336 - Flags: review?(cpearce) → review+
Assignee

Comment 8

5 years ago
(In reply to Chris Pearce (:cpearce) from comment #7)
> We're not supposed to use nested namespaces according to the style guide.

I think the |namespace detail| pattern is generally considered to be an exception to that.

> I
> don't in particular mind, but you could make it a static member of the class
> instead?

Not easily, because the class is templated.

> Maybe that should be called DispatchResolveRunnable, since it's not really
> "dispatching a promise", it dispatches an nsIRunnable.

Ok, I'll do "DispatchPromiseRunnable", since it's either a ResolveRunnable _or_ a RejectRunnable.
Comment on attachment 8533338 [details] [diff] [review]
Part 4 - Allow promises to be resolved without a MediaPromiseHolder. v1

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

::: dom/media/MediaPromise.h
@@ +183,5 @@
>                    runnable.get(), aPromise, this);
>        DebugOnly<nsresult> rv = detail::DispatchMediaPromise(mResponseTarget, runnable);
>        MOZ_ASSERT(NS_SUCCEEDED(rv));
>      }
>  

DoResolve() shouldn't need to be exposed, only the Resolve()/Reject() functions should need to be, right?
Attachment #8533338 - Flags: review?(cpearce) → review+
Comment on attachment 8533337 [details] [diff] [review]
Part 3 - Add MediaPromise::{Resolve,Reject}IfExists. v1

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

::: dom/media/MediaPromise.h
@@ +316,5 @@
>      mPromise = nullptr;
>    }
>  
> +  void ResolveIfExists(typename PromiseType::ResolveValueType aResolveValue,
> +               const char* aMethodName)

Nit: Indentation of "const char* aMethodName"
Attachment #8533337 - Flags: review?(cpearce) → review+
Comment on attachment 8533339 [details] [diff] [review]
Part 5 - Add the ability to chain same-type promises.

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

::: dom/media/MediaPromise.h
@@ +283,5 @@
>    ~MediaPromise()
>    {
>      MOZ_COUNT_DTOR(MediaPromise);
>      PROMISE_LOG("MediaPromise::~MediaPromise [this=%p]", this);
>      MOZ_ASSERT(!IsPending());

Maybe assert that mChainedPromises is empty in the dtor too, to ensure we've at least rejected everything on shutdown.
Attachment #8533339 - Flags: review?(cpearce) → review+
Comment on attachment 8533335 [details] [diff] [review]
Part 1 - Separate ThenValueRunnable so that the constructors don't collide when the resolve value is the same as the reject value. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve Flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This does affect non-MSE playback, but is well tested at this point. Risk is low.
[String/UUID change made/needed]: None.

This request applies to all patches in this bug.
Attachment #8533335 - Flags: approval-mozilla-aurora?
Attachment #8533335 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.