Closed
Bug 1109954
Opened 9 years ago
Closed 9 years ago
Make resolve/reject values optional in MediaPromise callback signatures
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file, 1 obsolete file)
8.27 KB,
patch
|
bholley
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8534685 -
Flags: review?(cpearce)
Assignee | ||
Comment 2•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=26dffafad634
Updated•9 years ago
|
Attachment #8534685 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 3•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c9d42ef78e
Assignee | ||
Comment 4•9 years ago
|
||
Backed out for colliding with bug 1108701. I'll look at this later. https://hg.mozilla.org/integration/mozilla-inbound/rev/c90dea32c778
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 5•9 years ago
|
||
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?
Attachment #8534685 -
Attachment is obsolete: true
Attachment #8535311 -
Flags: review+
Attachment #8535311 -
Flags: feedback?(botond)
Comment 6•9 years ago
|
||
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".
Attachment #8535311 -
Flags: feedback?(botond)
Assignee | ||
Comment 7•9 years ago
|
||
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?
Flags: needinfo?(bobbyholley)
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7f336bd431cd
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4815448f7c
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ac4815448f7c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 12•9 years ago
|
||
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.
Attachment #8535311 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8535311 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•