1109954 - Make resolve/reject values optional in MediaPromise callback signatures

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

This addresses bug 1108707 comment 3. I realized this was straightforward to do, and went ahead and did it. It turns out this is useful because it lets us chain pre-existing argument-less runnable methods off of promises, which we want for bug 1109216.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Make resolve/reject values optional in callback signatures. v1

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=26dffafad634

Comment 3

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89c9d42ef78e

Comment 4

[Bobby Holley (:bholley)]

Backed out for colliding with bug 1108701. I'll look at this later.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c90dea32c778

Comment 5

[Bobby Holley (:bholley)]

Attachment: Make resolve/reject values optional in callback signatures. v2 r=cpearce

This fixes the bustage by adding an explicit specialization for nsRefPtr<T>
Resolve/Reject values.

Botond, can you have a brief look at the changes in MediaPromise.h and make
sure there isn't a more elegant solution?

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8535311 [details] [diff] [review]
Make resolve/reject values optional in callback signatures. v2 r=cpearce

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

(In reply to Bobby Holley (:bholley) from comment #5)
> This fixes the bustage by adding an explicit specialization for nsRefPtr<T>
> Resolve/Reject values.

(Nit: it's an overload, not an explicit specialization. An explicit specialization would look like:
  
   template <>  // no parameters here
   void Function(/* signature */)

It wouldn't be very useful in this case because explicit specializations don't have any template parameters, so they only apply to a set of concrete parameter types. A *partial* specialization would be useful, but C++ does not allow those for functions, only for classes. However, functions can be overloaded, and that serves the purpose well enough.)

> Botond, can you have a brief look at the changes in MediaPromise.h and make
> sure there isn't a more elegant solution?

There is a trick you can employ to avoid special-casing nsRefPtr.

Presumably, when you didn't have the overload that's catering to nsRefPtr, and then tried to call a method that took an nsRefPtr<T> with a T* argument, the problem you ran into was that the compiler tried to match this:

    template<typename ThisType, typename ValueType>
    void InvokeCallbackMethod(ThisType* aThisVal, void(ThisType::*aMethod)(ValueType), ValueType aValue)

but failed, because it deduced conflicting template argument values for 'ValueType':

  from 'void(ThisType::*aMethod)(ValueType)', it deduced 'ValueType = nsRefPtr<T>'.
  from 'ValueType aValue', it deduced 'ValueType = T*'.

You can get around this by disabling template argument deduction for the second use of ValueType.

To do this, we first need a helper template:

  template <typename T>
  struct Nondeduced
  {
    typedef T type;
  };

(MFBT would probably be a good place for this.)

Next, we wrap the use of ValueType for which we don't want template argument deduction, with this:

    template<typename ThisType, typename ValueType>
    void InvokeCallbackMethod(ThisType* aThisVal, 
                              void(ThisType::*aMethod)(ValueType), 
                              typename Nondeduced<ValueType>::type aValue)

Now, 'typename Nondeduced<ValueType>::type' is a "non-deduced context", so the compiler does not try to deduce a value for 'ValueType' from it.

Therefore, there is only one place in the signature where 'ValueType' is deduced:

  from 'void(ThisType::*aMethod)(ValueType)', deduce 'ValueType = nsRefPtr<T>'

This results in the following instantiated signature:

    void InvokeCallbackMethod(YourClassType* aThisVal, 
                              void(YourClassType::*aMethod)(nsRefPtr<T>), 
                              nsRefPtr<T> aValue)

which can be called with a T* as the third argument.

I'll leave it up to you whether or not you'd like to employ this trick. I think the code as-is is also reasonable.

::: dom/media/MediaPromise.h
@@ +155,5 @@
>      const char* mCallSite;
>    };
>  
> +  /*
> +   * We create two specializations for invoking Resolve/Reject Methods so as to

s/specializations/overloads

@@ +156,5 @@
>    };
>  
> +  /*
> +   * We create two specializations for invoking Resolve/Reject Methods so as to
> +   * make the resolve/reject value argument "optional" via SFINAE. The downside

Nit: I don't think SFINAE comes into the picture here. I'd just omit "via SFINAE".

Comment 7

[Bobby Holley (:bholley)]

Thanks Botond! That's super helpful.

(In reply to Botond Ballo [:botond] from comment #6)
> This results in the following instantiated signature:
> 
>     void InvokeCallbackMethod(YourClassType* aThisVal, 
>                               void(YourClassType::*aMethod)(nsRefPtr<T>), 
>                               nsRefPtr<T> aValue)
> 
> which can be called with a T* as the third argument.

I don't quite follow this part. It seems like it would result in:

>     void InvokeCallbackMethod(YourClassType* aThisVal, 
>                               void(YourClassType::*aMethod)(T*), 
>                               T* aValue)

Which can be called with nsRefPtr<T> as the third argument. Is that what you meant?

Comment 8

[Botond Ballo [:botond]]

(In reply to Bobby Holley (:bholley) from comment #7)
> Thanks Botond! That's super helpful.
> 
> (In reply to Botond Ballo [:botond] from comment #6)
> > This results in the following instantiated signature:
> > 
> >     void InvokeCallbackMethod(YourClassType* aThisVal, 
> >                               void(YourClassType::*aMethod)(nsRefPtr<T>), 
> >                               nsRefPtr<T> aValue)
> > 
> > which can be called with a T* as the third argument.
> 
> I don't quite follow this part. It seems like it would result in:
> 
> >     void InvokeCallbackMethod(YourClassType* aThisVal, 
> >                               void(YourClassType::*aMethod)(T*), 
> >                               T* aValue)
> 
> Which can be called with nsRefPtr<T> as the third argument. Is that what you
> meant?

It works in either direction. 

If the method's parameter type is T*, you'll get:

     void InvokeCallbackMethod(YourClassType* aThisVal, 
                               void(YourClassType::*aMethod)(T*), 
                               T* aValue)

If the method's parameter type is nsRefPtr<T>, you'll get:

     void InvokeCallbackMethod(YourClassType* aThisVal, 
                               void(YourClassType::*aMethod)(nsRefPtr<T>), 
                               nsRefPtr<T> aValue)

Basically, by making the second use of ValueType nondeduced, you're saying that the first use of ValueType (the one in 'void(ThisType::*aMethod)(ValueType)') determines what value will be deduced for ValueType.

Comment 9

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7f336bd431cd

Comment 10

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4815448f7c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac4815448f7c

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8535311 [details] [diff] [review]
Make resolve/reject values optional in callback signatures. v2 r=cpearce

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, shipped on Nightly.
[Risks and why]: Low. 
[String/UUID change made/needed]: None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a024fc4f0bd2