Closed Bug 1083950 Opened 5 years ago Closed 5 years ago

Add a way to get the promises that depend on a given promise via PromiseDebugging

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

So given a promise P1, that would be any promises returned by doing P1.then() and any return values from calling Promise.all/Promise.race and passing P1.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8506353 [details] [diff] [review]
Add a way to get the promises that depend on a given promise via PromiseDebugging

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

Very elegant!

::: dom/promise/PromiseCallback.h
@@ +56,5 @@
>  
>    void Call(JSContext* aCx,
>              JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;
>  
> +  virtual Promise* GetDependentPromise() MOZ_OVERRIDE

This 'virtual' can be removed. Same for the other overrides.

::: dom/promise/PromiseDebugging.cpp
@@ +59,5 @@
>  
> +/* static */ void
> +PromiseDebugging::GetDependentPromises(GlobalObject&, Promise& aPromise,
> +                                       nsTArray<nsRefPtr<Promise>>& aPromises)
> +{

Might be a good idea to add a NS_WARN_IF(aPromise.mStatus != Promise::Pending, "Calling GetDependentPromises() on resolved Promise!")

::: dom/promise/tests/test_dependentPromises.html
@@ +10,5 @@
> +  <link rel="stylesheet" type="text/css" href="chrome://global/skin"/>
> +  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"/>
> +  <script type="application/javascript">
> +
> +    alert(Promise);

huh?

@@ +38,5 @@
> +
> +  arraysEqual(getDependentNames(p), ["q", "r", "s"], "deps for p");
> +  arraysEqual(getDependentNames(q), ["s"], "deps for q");
> +  arraysEqual(getDependentNames(r), ["t"], "deps for r");
> +  arraysEqual(getDependentNames(r), ["t"], "deps for r");

s/r/s/g

::: dom/webidl/PromiseDebugging.webidl
@@ +45,5 @@
> +   * 3) Return values of Promise.race() if the given promise was passed in as
> +   *    one of the arguments.
> +   *
> +   * Once a promise is settled, it will generally notify its dependent promises
> +   * and forget about them, so this is most useful on unsettled promises.

Could you add a note that this function is not recursive.
Attachment #8506353 - Flags: review?(nsm.nikhil) → review+
> Might be a good idea to add a NS_WARN_IF(aPromise.mStatus != Promise::Pending, 
> "Calling GetDependentPromises() on resolved Promise!")

I expect devtools to do just that in some cases if opened after the page has been running for a bit, so I'd rather not warn about it....

> huh?

Oh, good catch!  I forgot that was in the test when I was trying it out...

> s/r/s/g

Also good catch.

> Could you add a note that this function is not recursive.

Absolutely.  How about:

   * Note that this function only returns the promises that directly depend on
   * p.  It does not recursively return promises that depend on promises that
   * depend on p.
(In reply to Boris Zbarsky [:bz] from comment #3)
> 
> > Could you add a note that this function is not recursive.
> 
> Absolutely.  How about:
> 
>    * Note that this function only returns the promises that directly depend
> on
>    * p.  It does not recursively return promises that depend on promises that
>    * depend on p.

+1
https://hg.mozilla.org/integration/mozilla-inbound/rev/652e8de626d2
Flags: in-testsuite+
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/652e8de626d2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.