[promises] Refactor CreatePromise

[promises] Refactor CreatePromise

BUG=v8:5343
Committed: https://crrev.com/2886e3917f8c6f06707ed130b041d6acf2e95782
Cr-Commit-Position: refs/heads/master@{#41660}

[gsathya, 2016-12-12 23:33:29]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 23:33:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2016-12-12 23:34:54]

Description was changed from

==========
[promises] Refactor CreatePromise

BUG=v8:5343
==========

to

==========
[promises] Refactor CreatePromise

BUG=v8:5343
==========

[gsathya, 2016-12-12 23:34:54]

gsathya@chromium.org changed reviewers:
+ caitp@igalia.com, littledan@chromium.org

[caitp, 2016-12-12 23:41:58]

https://codereview.chromium.org/2571663002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2571663002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise.cc:62: compiler::Node*
CreatePromise(CodeStubAssembler* a, compiler::Node* context) {
There are _a lot_ of builtins that need to be able to allocate and initialize
JSPromises. Can we move these helpers to CodeStubAssembler please?

For less-widely needed helpers, I suggest subclassing CodeStubAssembler with
PromiseBuiltinAssembler or something, and installing the helpers directly on it.
Then, you can use the TF_BUILTIN() thing to generate builtins with the
subclassed assembler, and avoid passing a pointer to the assembler everywhere.
IMO it's much easier to read.

Other than those nits, refactoring looks fine to me

[commit-bot: I haz the power, 2016-12-12 23:59:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 23:59:31]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-13 00:05:34]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 00:05:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Ehrenberg, 2016-12-13 00:10:36]

lgtm

No opinion on Caitlin's style advice, but correctness-wise, this looks good to
me.

[gsathya, 2016-12-13 00:21:49]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 00:21:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2016-12-13 00:44:19]

On 2016/12/12 23:41:58, caitp wrote:
>
https://codereview.chromium.org/2571663002/diff/1/src/builtins/builtins-promi...
> File src/builtins/builtins-promise.cc (right):
> 
>
https://codereview.chromium.org/2571663002/diff/1/src/builtins/builtins-promi...
> src/builtins/builtins-promise.cc:62: compiler::Node*
> CreatePromise(CodeStubAssembler* a, compiler::Node* context) {
> There are _a lot_ of builtins that need to be able to allocate and initialize
> JSPromises. Can we move these helpers to CodeStubAssembler please?

Agreed. I've moved them.

> For less-widely needed helpers, I suggest subclassing CodeStubAssembler with
> PromiseBuiltinAssembler or something, and installing the helpers directly on
it.
> Then, you can use the TF_BUILTIN() thing to generate builtins with the
> subclassed assembler, and avoid passing a pointer to the assembler everywhere.
> IMO it's much easier to read.
> 
> Other than those nits, refactoring looks fine to me

Yes, I do want to do this, but I want to finish the porting first.

[commit-bot: I haz the power, 2016-12-13 00:46:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 00:47:00]

Dry run: This issue passed the CQ dry run.

[caitp, 2016-12-13 01:04:58]

Lgtm

https://codereview.chromium.org/2571663002/diff/60001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2571663002/diff/60001/src/code-stub-assembler...
src/code-stub-assembler.h:1056: Node* CreatePromise(Node* context);
I would suggest using AllocateJSPromise rather than CreatePromise, for
consistency

[gsathya, 2016-12-13 01:46:30]

The CQ bit was checked by gsathya@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 01:46:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gsathya, 2016-12-13 01:47:17]

On 2016/12/13 01:04:58, caitp wrote:
> Lgtm
> 
>
https://codereview.chromium.org/2571663002/diff/60001/src/code-stub-assembler.h
> File src/code-stub-assembler.h (right):
> 
>
https://codereview.chromium.org/2571663002/diff/60001/src/code-stub-assembler...
> src/code-stub-assembler.h:1056: Node* CreatePromise(Node* context);
> I would suggest using AllocateJSPromise rather than CreatePromise, for
> consistency

I see other methods with Create*, but shrug. Changed

[caitp, 2016-12-13 02:09:07]

On 2016/12/13 01:47:17, gsathya wrote:
> On 2016/12/13 01:04:58, caitp wrote:
> > Lgtm
> > 
> >
>
https://codereview.chromium.org/2571663002/diff/60001/src/code-stub-assembler.h
> > File src/code-stub-assembler.h (right):
> > 
> >
>
https://codereview.chromium.org/2571663002/diff/60001/src/code-stub-assembler...
> > src/code-stub-assembler.h:1056: Node* CreatePromise(Node* context);
> > I would suggest using AllocateJSPromise rather than CreatePromise, for
> > consistency
> 
> I see other methods with Create*, but shrug. Changed

You're right, but other than the one named after the spec op, there are only 2
like that, vs ~20 of the other style (and there are some named New<thing>, too,
heh).

[commit-bot: I haz the power, 2016-12-13 02:11:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 02:11:42]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-13 02:12:04]

The CQ bit was checked by gsathya@chromium.org

[gsathya, 2016-12-13 02:12:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org, caitp@igalia.com
Link to the patchset: https://codereview.chromium.org/2571663002/#ps80001
(title: "rename to allocatejspromise")

[commit-bot: I haz the power, 2016-12-13 02:12:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 02:14:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481595124298060,
"parent_rev": "4ee177ad5e8bdee303cf6494dfcf10831f296d82", "commit_rev":
"947bf5645edfe3edbf12e937b19e1f65d765ed68"}

[commit-bot: I haz the power, 2016-12-13 02:14:15]

Description was changed from

==========
[promises] Refactor CreatePromise

BUG=v8:5343
==========

to

==========
[promises] Refactor CreatePromise

BUG=v8:5343

Review-Url: https://codereview.chromium.org/2571663002
==========

[commit-bot: I haz the power, 2016-12-13 02:14:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-13 02:15:29]

Description was changed from

==========
[promises] Refactor CreatePromise

BUG=v8:5343

Review-Url: https://codereview.chromium.org/2571663002
==========

to

==========
[promises] Refactor CreatePromise

BUG=v8:5343

Committed: https://crrev.com/2886e3917f8c6f06707ed130b041d6acf2e95782
Cr-Commit-Position: refs/heads/master@{#41660}
==========

[commit-bot: I haz the power, 2016-12-13 02:15:30]

Patchset 5 (id:??) landed as
https://crrev.com/2886e3917f8c6f06707ed130b041d6acf2e95782
Cr-Commit-Position: refs/heads/master@{#41660}