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

Status: RESOLVED FIXED

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

Description

[:Paolo Amadini]

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.

Comment 1

[ckotowski]

Anybody working on this?  If not, I can pick it up.

Comment 2

[Alpha Abdoulaye]

I would like to be assigned to this bug if no one is working on it.

Comment 3

[Alpha Abdoulaye]

Infos provided by nsm, so problem solved.

Comment 4

[Alpha Abdoulaye]

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.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I asked Alpha to upload some preliminary renaming on IRC, so clearing ni?

Comment 6

[Alpha Abdoulaye]

Attachment: bug1086627.diff

First attempt, not sure if the changes are exhaustive.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 8

[Alpha Abdoulaye]

Attachment: rev2 - Previous patch, with indentation fixes

Yep, sorry about that.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 10

[Alpha Abdoulaye]

Attachment: Second patch

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 12

[Alpha Abdoulaye]

Attachment: rev2 - Second patch

Here is the modified patch.
And no, I don't have commit access yet.

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

dom peer sign-off for webidl change - http://logs.glob.uno/?c=content#c314262

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/28b7e0f38d1c6b5ca87272cfacea05d3e5eb59a2
changeset:  28b7e0f38d1c6b5ca87272cfacea05d3e5eb59a2
user:       Alpha A. <[e-mail]>
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. <[e-mail]>
date:       Thu Aug 06 17:18:30 2015 +0200
description:
Bug 1086627 - Rename Promise constructs to more closely match the specification. r=nsm,jst

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28b7e0f38d1c
https://hg.mozilla.org/mozilla-central/rev/3d086b6c8289