Introduce Promises.

Introduce Promises.

We would like to introduce DOM/Promises[1].
Unlike ordinary DOM objects, the internal state is held as a JavaScript object to avoid leaks.
To achieve this, WebCore::Promise class is empty (it is added to make the IDL processor happy) and all methods are marked as CUSTOM.
Perhaps we will make the IDL processor support this style in a future CL.

As a first step, This CL contains Promise constructor only.
The remaining part will be implemented in another CL.

[1] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/9q5kP0eMQc8 
BUG=243345
R=abarth@chromium.org
TESTS=fast/js/Promise-init.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152943

[yhirano, 2013-06-21 01:56:15]

Hi, abarth@, can you take a look at it?

[abarth-chromium, 2013-06-21 05:38:45]

This looks like a great start.  LGTM.  Thanks for being willing to try out a few
designs.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:45:
args.GetReturnValue().Set(v8::Local<v8::Value>());
There isn't a SetReturnValue function?

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:52: v8::Local<v8::ObjectTemplate>
associatedTemplate = v8::ObjectTemplate::New();
Normally we create the object template in V8PerIsolateData.  Can we store the
template there and just clone it for each call to the constructor?  That's much
more efficient because it helps V8 understand that the object's structure is
shared among all its instances.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:58:
associated->SetInternalField(V8PromiseCustom::AssociatedStatusIndex,
v8::NumberObject::New(V8PromiseCustom::Pending));
These plain numbers we can keep on the CPP side.  It's only the references to
other V8 objects that need to be in the V8 side, strictly speaking.  However, I
can see the appeal of keeping all the data on the V8 side, especially if that
means there's no CPP object at all!

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:64:
promiseResolver->SetInternalField(v8DOMWrapperObjectIndex, associated);
These two object share the same associated storage object?  /me doesn't fully
understand how promises work...

Is there a more descriptive name we can use than "associated" ?  I guess this is
the state for the promise?

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:70: if (init->Call(promise,
sizeof(argv) / sizeof(argv[0]), argv).IsEmpty()) {
WTF_ARRAY_LENGTH(argv)

[haraken, 2013-06-21 05:47:58]

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:47: if (args.Length() != 1 ||
args[0].IsEmpty() || !args[0]->IsFunction()) {

This should be 'args.Length() < 1'.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:56: v8::Local<v8::Object>
promiseResolver = V8DOMWrapper::createWrapper(args.Holder(),
&V8PromiseResolver::info, 0, isolate);

Don't you need to store a C++ object pointer to the wrapper? (I don't fully
understand how Promise works.)

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:70: if (init->Call(promise,
sizeof(argv) / sizeof(argv[0]), argv).IsEmpty()) {

You can use V8ScriptRunner::callFunction().

[abarth-chromium, 2013-06-21 05:58:52]

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:56: v8::Local<v8::Object>
promiseResolver = V8DOMWrapper::createWrapper(args.Holder(),
&V8PromiseResolver::info, 0, isolate);
On 2013/06/21 05:47:58, haraken wrote:
> Don't you need to store a C++ object pointer to the wrapper? (I don't fully
> understand how Promise works.)

The "associated" object isn't a wrapper.  It's just a V8 object with a number of
internal fields.  I how that won't confuse other code in the bindings...  We
might need to reserve the first two lots for zeros.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:70: if (init->Call(promise,
sizeof(argv) / sizeof(argv[0]), argv).IsEmpty()) {
On 2013/06/21 05:47:58, haraken wrote:
> You can use V8ScriptRunner::callFunction().

Ah, sorry I missed that!

[yhirano, 2013-06-21 09:36:52]

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:45:
args.GetReturnValue().Set(v8::Local<v8::Value>());
On 2013/06/21 05:38:46, abarth wrote:
> There isn't a SetReturnValue function?

Thanks, done.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:47: if (args.Length() != 1 ||
args[0].IsEmpty() || !args[0]->IsFunction()) {
On 2013/06/21 05:47:58, haraken wrote:
> 
> This should be 'args.Length() < 1'.

Done.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:52: v8::Local<v8::ObjectTemplate>
associatedTemplate = v8::ObjectTemplate::New();
On 2013/06/21 05:38:46, abarth wrote:
> Normally we create the object template in V8PerIsolateData.  Can we store the
> template there and just clone it for each call to the constructor?  That's
much
> more efficient because it helps V8 understand that the object's structure is
> shared among all its instances.

We can attach FunctionTemplate data to V8PerIsolateData, but cannot attach
ObjectTemplate.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:56: v8::Local<v8::Object>
promiseResolver = V8DOMWrapper::createWrapper(args.Holder(),
&V8PromiseResolver::info, 0, isolate);
On 2013/06/21 05:47:58, haraken wrote:
> 
> Don't you need to store a C++ object pointer to the wrapper? (I don't fully
> understand how Promise works.)
No, instead, we set a JavaScript object as an internal field to it.

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:64:
promiseResolver->SetInternalField(v8DOMWrapperObjectIndex, associated);
On 2013/06/21 05:38:46, abarth wrote:
> These two object share the same associated storage object?  /me doesn't fully
> understand how promises work...
> 
> Is there a more descriptive name we can use than "associated" ?  I guess this
is
> the state for the promise?
Hmm...
There is "state" property in the spec (I called it as "status" in PS1, but it
was wrong).
How about "internal"?

https://codereview.chromium.org/17505004/diff/1/Source/bindings/v8/custom/V8P...
Source/bindings/v8/custom/V8PromiseCustom.cpp:70: if (init->Call(promise,
sizeof(argv) / sizeof(argv[0]), argv).IsEmpty()) {
On 2013/06/21 05:58:53, abarth wrote:
> On 2013/06/21 05:47:58, haraken wrote:
> > You can use V8ScriptRunner::callFunction().
> 
> Ah, sorry I missed that!

Done.

[do-not-use, 2013-06-21 15:04:58]

https://codereview.chromium.org/17505004/diff/6001/LayoutTests/fast/js/Promis...
File LayoutTests/fast/js/Promise-init.html (right):

https://codereview.chromium.org/17505004/diff/6001/LayoutTests/fast/js/Promis...
LayoutTests/fast/js/Promise-init.html:31: } catch (e) {
Any reason we are now using shouldThrow()?

https://codereview.chromium.org/17505004/diff/6001/LayoutTests/fast/js/Promis...
LayoutTests/fast/js/Promise-init.html:50: } catch (e) {
Ditto.

https://codereview.chromium.org/17505004/diff/6001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8PromiseResolverCustom.cpp (right):

https://codereview.chromium.org/17505004/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:33: 
Why this empty file? Do you anticipate to add things here soon?

https://codereview.chromium.org/17505004/diff/6001/Source/core/dom/PromiseRes...
File Source/core/dom/PromiseResolver.idl (right):

https://codereview.chromium.org/17505004/diff/6001/Source/core/dom/PromiseRes...
Source/core/dom/PromiseResolver.idl:32: EnabledAtRuntime=promise
Although it does not impact the generated code, Promise should technically use a
capital P here I believe.

[abarth-chromium, 2013-06-21 16:47:50]

On 2013/06/21 09:36:52, yhirano wrote:
> We can attach FunctionTemplate data to V8PerIsolateData, but cannot attach
> ObjectTemplate.

Ok.  I'm not sure I understand why completely, but this is something we can
iterate on.

> > Is there a more descriptive name we can use than "associated" ?  I guess
this
> > is the state for the promise?
> Hmm...
> There is "state" property in the spec (I called it as "status" in PS1, but it
> was wrong).
> How about "internal"?

Internal is ok.

[yhirano, 2013-06-24 03:33:34]

https://codereview.chromium.org/17505004/diff/6001/LayoutTests/fast/js/Promis...
File LayoutTests/fast/js/Promise-init.html (right):

https://codereview.chromium.org/17505004/diff/6001/LayoutTests/fast/js/Promis...
LayoutTests/fast/js/Promise-init.html:31: } catch (e) {
On 2013/06/21 15:04:58, Christophe Dumez wrote:
> Any reason we are now using shouldThrow()?

Done.

https://codereview.chromium.org/17505004/diff/6001/LayoutTests/fast/js/Promis...
LayoutTests/fast/js/Promise-init.html:50: } catch (e) {
On 2013/06/21 15:04:58, Christophe Dumez wrote:
> Ditto.

Done.

https://codereview.chromium.org/17505004/diff/6001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8PromiseResolverCustom.cpp (right):

https://codereview.chromium.org/17505004/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8PromiseResolverCustom.cpp:33: 
On 2013/06/21 15:04:58, Christophe Dumez wrote:
> Why this empty file? Do you anticipate to add things here soon?

No. As in the issue description, We define the class to make the IDL processor
happy. We might fix the IDL processor in the future, not in this CL.

https://codereview.chromium.org/17505004/diff/6001/Source/core/dom/PromiseRes...
File Source/core/dom/PromiseResolver.idl (right):

https://codereview.chromium.org/17505004/diff/6001/Source/core/dom/PromiseRes...
Source/core/dom/PromiseResolver.idl:32: EnabledAtRuntime=promise
On 2013/06/21 15:04:58, Christophe Dumez wrote:
> Although it does not impact the generated code, Promise should technically use
a
> capital P here I believe.

Really? I saw IDL files having EnabledAtRuntime in Source/core/dom. The flag
name starts with a lower letter, in most cases.

[yhirano, 2013-06-24 04:28:44]

> name starts with a lower letter, in most cases.
lower case letter

[do-not-use, 2013-06-24 07:04:39]

On 2013/06/24 04:28:44, yhirano wrote:
> > name starts with a lower letter, in most cases.
> lower case letter

Thanks for checking. Please disregard my comment then. As I said, it does not
affect the generated code anyway.

[yhirano, 2013-06-24 07:25:30]

Kentaro, Christophe, I think I responded to all your comments.
Is there any other comments?
If not, I would like to land this CL.

[do-not-use, 2013-06-24 07:36:23]

On 2013/06/24 07:25:30, yhirano wrote:
> Kentaro, Christophe, I think I responded to all your comments.
> Is there any other comments?
> If not, I would like to land this CL.

No more comment from me.

[haraken, 2013-06-24 07:39:17]

LGTM

[yhirano, 2013-06-24 07:43:03]

Thank you very much.

[commit-bot: I haz the power, 2013-06-24 07:43:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/17505004/19001

[commit-bot: I haz the power, 2013-06-24 08:33:19]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-06-24 08:35:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/17505004/19001

[commit-bot: I haz the power, 2013-06-24 09:16:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/17505004/24002

[commit-bot: I haz the power, 2013-06-24 10:40:12]

Change committed as 152943

[arv (Not doing code reviews), 2013-06-24 14:32:53]

https://chromiumcodereview.appspot.com/17505004/diff/24002/Source/bindings/v8...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://chromiumcodereview.appspot.com/17505004/diff/24002/Source/bindings/v8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:58:
internal->SetInternalField(V8PromiseCustom::InternalStateIndex,
v8::NumberObject::New(V8PromiseCustom::Pending));
Why NumberObject and not Number?

[yhirano, 2013-06-25 01:02:09]

https://chromiumcodereview.appspot.com/17505004/diff/24002/Source/bindings/v8...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://chromiumcodereview.appspot.com/17505004/diff/24002/Source/bindings/v8...
Source/bindings/v8/custom/V8PromiseCustom.cpp:58:
internal->SetInternalField(V8PromiseCustom::InternalStateIndex,
v8::NumberObject::New(V8PromiseCustom::Pending));
On 2013/06/24 14:32:54, arv wrote:
> Why NumberObject and not Number?

Thanks, will do in the next CL.