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

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Paolo, Assigned: alpha.abdoulaye)

Tracking

(Blocks: 1 bug)

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.

Updated

4 years ago
Whiteboard: [good first bug]

Comment 1

4 years ago
Anybody working on this?  If not, I can pick it up.
(Assignee)

Comment 2

4 years ago
I would like to be assigned to this bug if no one is working on it.
Assignee: nobody → alpha
(Assignee)

Updated

3 years ago
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 3

3 years ago
Infos provided by nsm, so problem solved.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 4

3 years ago
Hey,
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)
(Assignee)

Comment 6

3 years ago
Created attachment 8642492 [details] [diff] [review]
bug1086627.diff

First attempt, not sure if the changes are exhaustive.
Attachment #8642492 - Flags: review?(nsm.nikhil)
Comment on attachment 8642492 [details] [diff] [review]
bug1086627.diff

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+
(Assignee)

Comment 8

3 years ago
Created attachment 8642559 [details] [diff] [review]
rev2 - Previous patch, with indentation fixes

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.
(Assignee)

Comment 10

3 years ago
Created attachment 8644393 [details] [diff] [review]
Second patch
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;
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>  
>  /**
> + * 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+
(Assignee)

Comment 12

3 years ago
Created attachment 8644678 [details] [diff] [review]
rev2 - Second patch

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 - http://logs.glob.uno/?c=content#c314262
Flags: needinfo?(nsm.nikhil)
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/28b7e0f38d1c6b5ca87272cfacea05d3e5eb59a2
changeset:  28b7e0f38d1c6b5ca87272cfacea05d3e5eb59a2
user:       Alpha A. <alpha@a-alpha.net>
date:       Mon Aug 03 18:48:34 2015 +0200
description:
Bug 1086627 - Rename ThenableResolverTask to PromiseResolveThenableJob to more closely match Promise spec. r=nsm

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/3d086b6c82892f9d6bf2769153b86e65cb16cd79
changeset:  3d086b6c82892f9d6bf2769153b86e65cb16cd79
user:       Alpha A. <alpha@a-alpha.net>
date:       Thu Aug 06 17:18:30 2015 +0200
description:
Bug 1086627 - Rename Promise constructs to more closely match the specification. r=nsm,jst
https://hg.mozilla.org/mozilla-central/rev/28b7e0f38d1c
https://hg.mozilla.org/mozilla-central/rev/3d086b6c8289
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.