Closed Bug 1086627 Opened 10 years ago Closed 9 years ago

Refactor Promise.cpp to match the current terminology in the Promises specification


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

Not set



Tracking Status
firefox43 --- fixed


(Reporter: Paolo, Assigned: alpha.abdoulaye)


(Blocks 1 open bug)


(Whiteboard: [good first bug])


(2 files, 2 obsolete files)

A few simple renames and refactorings in Promise.cpp would allow us to match the current terminology in the Promises specification, making the code easier to read.
Whiteboard: [good first bug]
Anybody working on this?  If not, I can pick it up.
I would like to be assigned to this bug if no one is working on it.
Flags: needinfo?(paolo.mozmail)
Infos provided by nsm, so problem solved.
Flags: needinfo?(paolo.mozmail)
I carefully read the specifications just as advised. It was a bit hard to grasp, as I am not familiar with Promises or JS, but I think I've got most of it, or at least the main idea behind it.
So now this is done, I came as agreed to have some new directions in order to solve this bug.
Flags: needinfo?(nsm.nikhil)
I asked Alpha to upload some preliminary renaming on IRC, so clearing ni?
Flags: needinfo?(nsm.nikhil)
Attached patch bug1086627.diff (obsolete) — Splinter Review
First attempt, not sure if the changes are exhaustive.
Attachment #8642492 - Flags: review?(nsm.nikhil)
Comment on attachment 8642492 [details] [diff] [review]

Review of attachment 8642492 [details] [diff] [review]:

::: dom/promise/Promise.cpp
@@ +178,1 @@
>                          JS::Handle<JSObject*> aThenable,

Nit: Please change the indentation of the other two arguments to line up.

@@ +283,1 @@
>                             PromiseCallback* aRejectCallback,

Nit: Same here.
Attachment #8642492 - Flags: review?(nsm.nikhil) → review+
Yep, sorry about that.
Attachment #8642492 - Attachment is obsolete: true
Some other things I can think of:
1) in dom/webidl/Promise.webidl remove the |// TODO Bug 875289...| comment.
2) AllResolveHandler can be renamed to AllResolveElementFunction. Update class comment to match spec as appropriate.
3) EnqueueCallbackTasks can be TriggerPromiseReactions.
4) PromiseCallbackTask -> PromiseReactionJob.

This can be done in a separate patch that can be put up for review on this same bug.
Attached patch Second patch (obsolete) — Splinter Review
Attachment #8644393 - Flags: review?(nsm.nikhil)
Comment on attachment 8644393 [details] [diff] [review]
Second patch

Review of attachment 8644393 [details] [diff] [review]:

Thanks for the patch! There is no need to ask for review again after you upload the new one with these changes.

If you have hg level 1 access, please push a build to try, otherwise needinfo? me and I'll push one for you. (I will be offline Friday-Monday).

::: dom/promise/Promise.cpp
@@ +48,3 @@
>  {
>  public:
> +  PromiseReactionJob(Promise* aPromise,

Nit: Indentation of the other two parameters.

@@ +904,5 @@
>    tmp->mValues = nullptr;
>  /**
> + * An AllResolveElementFunction is the per-promise part of the Promise.all() algorithm.

Nit: Wrap to 80 characters.

@@ +930,5 @@
>    void
>    RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override
>    {
>      // Should never be attached to Promise as a reject handler.
> +    MOZ_ASSERT(false, "AllResolveElementFunction should never be attached to a Promise's reject handler!");

Nit: While you are here, replace this with MOZ_CRASH("AllResolveElementFunction ...");

::: dom/promise/Promise.h
@@ +270,5 @@
>    // This method enqueues promise's resolve/reject callbacks with promise's
>    // result. It's executed when the resolver.resolve() or resolver.reject() is
>    // called or when the promise already has a result and new callbacks are
>    // appended by then(), catch() or done().

Nit: done() no longer exists in the spec, make this '... by then() or catch().'

::: dom/webidl/Promise.webidl
@@ +23,1 @@

Also remove this newline.
Attachment #8644393 - Flags: review?(nsm.nikhil) → review+
Here is the modified patch.
And no, I don't have commit access yet.
Attachment #8644393 - Attachment is obsolete: true
Flags: needinfo?(nsm.nikhil)
dom peer sign-off for webidl change -
Flags: needinfo?(nsm.nikhil)
changeset:  28b7e0f38d1c6b5ca87272cfacea05d3e5eb59a2
user:       Alpha A. <>
date:       Mon Aug 03 18:48:34 2015 +0200
Bug 1086627 - Rename ThenableResolverTask to PromiseResolveThenableJob to more closely match Promise spec. r=nsm

changeset:  3d086b6c82892f9d6bf2769153b86e65cb16cd79
user:       Alpha A. <>
date:       Thu Aug 06 17:18:30 2015 +0200
Bug 1086627 - Rename Promise constructs to more closely match the specification. r=nsm,jst
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.