:Paolo Amadini Reporter
	Description • 10 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.

	ckotowski
	Comment 1 • 9 years ago

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

	Alpha Abdoulaye Assignee
	Comment 2 • 9 years ago

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

	Alpha Abdoulaye Assignee
	Comment 3 • 9 years ago

Infos provided by nsm, so problem solved.

	Alpha Abdoulaye Assignee
	Comment 4 • 9 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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 5 • 9 years ago

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

	Alpha Abdoulaye Assignee
	Comment 6 • 9 years ago

First attempt, not sure if the changes are exhaustive.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 7 • 9 years ago

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.

	Alpha Abdoulaye Assignee
	Comment 8 • 9 years ago

Yep, sorry about that.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 9 • 9 years ago

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.

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 11 • 9 years ago

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.

	Alpha Abdoulaye Assignee
	Comment 12 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 13 • 9 years ago

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

	Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)
	Comment 14 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 9 years ago

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