1188976 - Hoist MozPromise into xpcom/

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bobby Holley (:bholley)]

Getting a bug on file for this task specifically.

Comment 1

[Bobby Holley (:bholley)]

Nathan volunteered to have a look at dom/media/MozPromise.h in advance of the hoisting to see if there are any changes he'd like.

Comment 2

[Nathan Froyd [:froydnj]]

Couple of thoughts:

- Are API examples reasonable?  The comment here:

http://mxr.mozilla.org/mozilla-central/source/dom/media/MozPromise.h#71

does a decent job of explaining things, but there's a lot of template magic going on below that, and it's not obvious what pieces to use.  I remember seeing code examples at Whistler, and those helped me get my head around this brave new promise-laden world.

Or it might be reasonable to put documentation comments on key public methods: MozPromise::Then might benefit from that, to help cut through the template goo...maybe others.  It's also not obvious to me how subclassing MozPromise works--marking things as |protected| suggests that writing subclasses is a reasonable thing, yes?

- Is there a reason ProxyMediaCall (which probably needs to be renamed in the uplift) doesn't use variadic templates?  We now have mozilla::Tuple, which makes the MethodCallWith*Args classes superfluous.

- Over in bug 1179451 comment 32, it looks like there are some small refcounting inefficiencies with MozPromiseRequestHolder...and maybe other things that take raw pointers and store them in nsRefPtr.  Those sorts of things would be nice to clean up with nsRefPtr<>&& overloads or replacements...

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

We should probably have a page on MDN documenting how to use MozPromises, similar to other key APIs such as nsHashTable, XPCOM string guide, WebIDL, etc.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix up include. r=me

Not sure how this didn't get fixed before.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Rename ProxyMediaCall to InvokeAsync. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use mozilla::Tuple for InvokeAsync. v1

Note sure if there's a better way to do the ApplyAsArgs stuff.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Hoist MozPromise into xpcom. v1

Comment 8

[Bobby Holley (:bholley)]

ttps://treeherder.mozilla.org/#/jobs?repo=try&revision=fc4902297dc9

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8648829 [details] [diff] [review]
Part 3 - Use mozilla::Tuple for InvokeAsync. v1

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

I think this is mostly right, but I'm having a hard time seeing through the stuff below.

::: dom/media/MozPromise.h
@@ +869,2 @@
>  {
> +  return ((*aThisVal).*aMethod)(Get<0>(aArgs), Get<1>(aArgs), Get<2>(aArgs), Get<3>(aArgs));

This is not how you want to do things with Tuple.  Take a look at:

http://mxr.mozilla.org/mozilla-central/source/media/mtransport/runnable_utils.h#60

and the implementation of the WrapRunnable* classes below that.

You may also find:

http://mxr.mozilla.org/mozilla-central/source/mfbt/IndexSequence.h#11

enlightening.

@@ +897,5 @@
> +  typedef nsRefPtr<PromiseType>(ThisType::*MethodType)(ArgTypes...);
> +  MethodCall(MethodType aMethod, ThisType* aThisVal, Tuple<ArgTypes...> aArgs)
> +    : mMethod(aMethod)
> +    , mThisVal(aThisVal)
> +    , mArgs(aArgs)

I think you want to make this:

MethodCall(MethodType aMethod, ThisType* aThisVal, ArgTypes... aArgs)
  : mMethod(aMethod)
  , mThisVal(aThisVal)
  , mArgs(aArgs...)

possibly with a mozilla::Forward<> thrown in on mArgs.

@@ +945,5 @@
>  {
> +  typedef detail::MethodCall<PromiseType, ThisType, ArgTypes...> MethodCallType;
> +  typedef detail::ProxyRunnable<PromiseType, ThisType, ArgTypes...> ProxyRunnableType;
> +
> +  MethodCallType* methodCall = new MethodCallType(aMethod, aThisVal, Tuple<ArgTypes...>(aArgs...));

You can avoid leaking the internal implementation details here by just saying:

new MethodCallType(aMethod, aThisVal, aArgs...);

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use mozilla::Tuple for InvokeAsync. v2

Thanks for the quick feedback!

Comment 11

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6efea83ddcfe

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8648870 [details] [diff] [review]
Part 3 - Use mozilla::Tuple for InvokeAsync. v2

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

::: dom/media/MozPromise.h
@@ +865,5 @@
>  
>  namespace detail {
>  
> +template<typename ReturnType, typename ThisType, typename... ArgTypes, size_t... Indices>
> +ReturnType MethodCallInvokeHelper(ReturnType(ThisType::*aMethod)(ArgTypes...), ThisType* aThisVal,

Nit: please break this between |ReturnType| and |MethodCallInvokeHelper|.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7dacf8fabcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/da9889d86074
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd60a14316f
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4464cd4be8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7dacf8fabcc
https://hg.mozilla.org/mozilla-central/rev/da9889d86074
https://hg.mozilla.org/mozilla-central/rev/0cd60a14316f
https://hg.mozilla.org/mozilla-central/rev/bd4464cd4be8

Comment 15

[Pulsebot]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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