[ABANDONED] Introduce Promise example implementation written in JavaScript.

Introduce Promise example implementation written in JavaScript.

We would like to introduce DOM/Promises[1] written in JavaScript.
Since it is experimental, we would like to land the utilities code first.
This CL introduces Promise, but it has only example methods.

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

[yhirano, 2013-06-14 06:57:26]

Split from https://codereview.chromium.org/16838012/ .

[yhirano, 2013-06-14 07:13:31]

abarth@'s comment at https://codereview.chromium.org/16838012/ . I will fix them
soon.


I need to study this CL more carefully to understand it...  Should we start with
a simplified version?

https://codereview.chromium.org/16838012/diff/15001/Source/core/core_derived_...
File Source/core/core_derived_sources.gyp (right):

https://codereview.chromium.org/16838012/diff/15001/Source/core/core_derived_...
Source/core/core_derived_sources.gyp:632: 'action_name':
'generatePromiseCustomScript',
Can we do this with a rule rather than having a separate named action for every
JS file?  We might need to invent a new file extension like ".blinkjs" to
trigger the rule.

https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:35: postTask(fulfillCallback.bind(undefined,
this.associated.result));
We need to be careful about calling things like "bind" that can be modified by
the page.

https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:137: var promises =
Array.prototype.slice.call(arguments, 0);
Similarly, we don't want to depend on any statics.

https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:252: return Object.freeze($Promise);
Why Object.freeze?

https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/PromiseRe...
File Source/core/dom/PromiseResolver.js (right):

https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/PromiseRe...
Source/core/dom/PromiseResolver.js:34: if (this.promise === undefined ||
this.promise.state !== 'pending') {
Do you mean

"promise" in this

?

https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/PromiseRe...
Source/core/dom/PromiseResolver.js:43: this.promise = undefined;
Should we call "delete this.promise" instead?

https://codereview.chromium.org/16838012/diff/15001/Source/core/page/RuntimeE...
File Source/core/page/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/16838012/diff/15001/Source/core/page/RuntimeE...
Source/core/page/RuntimeEnabledFeatures.in:61: Promise status=experimental
Let's start off with stat

[esprehn_google.com, 2013-06-14 07:27:56]

On Fri, Jun 14, 2013 at 12:13 AM, <yhirano@chromium.org> wrote:

> ...
>
> https://codereview.chromium.**org/16838012/diff/15001/**
>
Source/core/dom/PromiseRe.<https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/PromiseRe.>
> ..
> Source/core/dom/**PromiseResolver.js:43: this.promise = undefined;
> Should we call "delete this.promise" instead?
>

Last I checked using delete causes deoptimization of the object. You don't
want to ever modify which properties exist after the constructor runs.

[yhirano, 2013-06-17 05:49:22]

On 2013/06/14 07:13:31, yhirano wrote:
> abarth@'s comment at https://codereview.chromium.org/16838012/ . I will fix
them
> soon.
> 
> 
> I need to study this CL more carefully to understand it...  Should we start
with
> a simplified version?
> 
>
https://codereview.chromium.org/16838012/diff/15001/Source/core/core_derived_...
> File Source/core/core_derived_sources.gyp (right):
> 
>
https://codereview.chromium.org/16838012/diff/15001/Source/core/core_derived_...
> Source/core/core_derived_sources.gyp:632: 'action_name':
> 'generatePromiseCustomScript',
> Can we do this with a rule rather than having a separate named action for
every
> JS file?  We might need to invent a new file extension like ".blinkjs" to
> trigger the rule.
> 
Done.

> https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.js
> File Source/core/dom/Promise.js (right):
> 
>
https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.j...
> Source/core/dom/Promise.js:35: postTask(fulfillCallback.bind(undefined,
> this.associated.result));
> We need to be careful about calling things like "bind" that can be modified by
> the page.
> 
Will do in another CL.

>
https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.j...
> Source/core/dom/Promise.js:137: var promises =
> Array.prototype.slice.call(arguments, 0);
> Similarly, we don't want to depend on any statics.
> 
>
https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/Promise.j...
> Source/core/dom/Promise.js:252: return Object.freeze($Promise);
> Why Object.freeze?
> 
Done.

>
https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/PromiseRe...
> File Source/core/dom/PromiseResolver.js (right):
> 
>
https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/PromiseRe...
> Source/core/dom/PromiseResolver.js:34: if (this.promise === undefined ||
> this.promise.state !== 'pending') {
> Do you mean
> 
> "promise" in this
> 
> ?
> 
Will do in another CL.

>
https://codereview.chromium.org/16838012/diff/15001/Source/core/dom/PromiseRe...
> Source/core/dom/PromiseResolver.js:43: this.promise = undefined;
> Should we call "delete this.promise" instead?
> 
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#delete
says
this.foo = null;
is more preferable than
delete this;
. I will use this.promise = null; in the future CL.

>
https://codereview.chromium.org/16838012/diff/15001/Source/core/page/RuntimeE...
> File Source/core/page/RuntimeEnabledFeatures.in (right):
> 
>
https://codereview.chromium.org/16838012/diff/15001/Source/core/page/RuntimeE...
> Source/core/page/RuntimeEnabledFeatures.in:61: Promise status=experimental
> Let's start off with stat
Done.

[yhirano, 2013-06-17 05:56:01]

marja, 
> Please also make sure that your patch builds if you remove "#define
V8_USE_UNSAFE_HANDLES" from src/v8/include/v8.h.

I tried it with this CL.
Though the chromium build was failed, the cause was not this CL.

[tyoshino (SeeGerritForStatus), 2013-06-17 07:51:57]

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
File Source/bindings/v8/V8PromiseUtilities.cpp (right):

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:72:
placeholder->SetHiddenValue(V8HiddenPropertyName::jsConstructor(), value);
check return value?

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:95: v8::Handle<v8::Value> that =
thisObject->GetInternalField(v8DOMWrapperObjectIndex);
that -> wrapper?

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:100: return
V8ThrowException::throwError(v8TypeError, "The prototypex is not an object.",
isolate);
prototypex -> prototype

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:114: return
V8ThrowException::throwError(v8TypeError, (std::string("The property ") + name +
" is not a function.").c_str(), args.GetIsolate());
I still don't see any use of std::string in Blink source except for unittests.
How about using WTFString?

There's some discussion about this though it's stopped.
https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/blink-dev/_...

[yhirano, 2013-06-17 08:39:22]

I fixed some error messages and remove unnecessary code.

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
File Source/bindings/v8/V8PromiseUtilities.cpp (right):

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:72:
placeholder->SetHiddenValue(V8HiddenPropertyName::jsConstructor(), value);
On 2013/06/17 07:51:58, tyoshino wrote:
> check return value?

Done.

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:95: v8::Handle<v8::Value> that =
thisObject->GetInternalField(v8DOMWrapperObjectIndex);
On 2013/06/17 07:51:58, tyoshino wrote:
> that -> wrapper?
Not wrapper, but wrapped, or unwrapped.
In fact, thisObject is a wrapper, but I think thisObject is a better name than
wrapper here.

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:100: return
V8ThrowException::throwError(v8TypeError, "The prototypex is not an object.",
isolate);
On 2013/06/17 07:51:58, tyoshino wrote:
> prototypex -> prototype
Thanks, done.

https://codereview.chromium.org/17035004/diff/17001/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:114: return
V8ThrowException::throwError(v8TypeError, (std::string("The property ") + name +
" is not a function.").c_str(), args.GetIsolate());
On 2013/06/17 07:51:58, tyoshino wrote:
> I still don't see any use of std::string in Blink source except for unittests.
> How about using WTFString?
> 
> There's some discussion about this though it's stopped.
>
https://groups.google.com/a/chromium.org/forum/?fromgroups#%21topic/blink-dev...

Done.

[abarth-chromium, 2013-06-17 18:05:06]

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Hidd...
File Source/bindings/v8/V8HiddenPropertyName.h (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Hidd...
Source/bindings/v8/V8HiddenPropertyName.h:44: V(jsConstructor) \
Why not just "constructor" ?

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
File Source/bindings/v8/V8PromiseUtilities.cpp (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:49: v8::Local<v8::Object> global =
isolate->GetCurrentContext()->Global();
Perhaps we should pass in the context instead of assuming we always want the
current context?

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:50: v8::Local<v8::Value>
placeholderValue = global->Get(v8::String::NewSymbol(name));
We don't want to read from the global object.  The global object might have been
manipulated by the web site.  Instead, we want to store this information in
V8PerContextData.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:60: value =
V8ScriptRunner::compileAndRunInternalScript(v8String(String(Promise_js,
sizeof(Promise_js)), isolate), isolate);
Most of the code from here down should be abstracted into a function in
V8ScriptRunner for calling blinkjs scripts.  It's likely we'll want to pass in a
number of arguments so that the script has functions it can call that haven't
been manipulated by the page.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:85: return
constructor->Get(v8::String::NewSymbol("prototype"));
This isn't right.  The prototype property might have been manipulated by the
page.  Instead, we need to store the prototype property in V8PerContextData.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
File Source/bindings/v8/V8PromiseUtilities.h (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.h:45: static v8::Handle<v8::Value>
callStatic(const char* name, const v8::FunctionCallbackInfo<v8::Value>& args,
v8::Handle<v8::Value> constructor);
Should these last three be part of V8ScriptRunner?  They don't seem specific to
Promises?

Also, V8PromiseUtilities isn't a great name for this sort of class.  "Utility"
classes accumulate lots of unrelated functionality because they're not very
focused.

Rather, I'd call this header V8PromiseCustom.h and rename:

promiseConstructor -> v8PromiseConstructor
promisePrototype -> v8PromisePrototype

I'd also skip the V8PromiseUtilities class and just put these functions in the
global WebCore namespace.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:38: namespace WebCore {
Please add a blank line after the namespace declaration.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:63: }
We should teach the code generator how to generate this code.  We'll want to add
an IDL attribute for [ImplementedInJavaScript] or something of that nature.

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.h
File Source/core/dom/Promise.h (right):

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.h...
Source/core/dom/Promise.h:40: };
We should change the code generator not to need it.  This will likely need to
key off an IDL attribute indicating that a given interface is implemented in
JavaScript.

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.idl
File Source/core/dom/Promise.idl (right):

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.i...
Source/core/dom/Promise.idl:37: [Custom] static any getStatic(any value);
These shouldn't need to be custom.  We should teach the code generator how to
deal with interfaces that are implemented in JavaScript.

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:44: function $Promise(value)
Why $Promise?  Should we just call this function Promise?

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:46: this.value = value;
I don't see a "value" property on Promise instances in the spec:

http://dom.spec.whatwg.org/#promise

Where is that defined?  I don't see getStatic or get either.  Am I looking at
the wrong version of the spec?

[abarth-chromium, 2013-06-17 18:06:41]

Rather than having a "placeholder" value in the global object, we should make
the Promise property on Window generate a callback into C++ when accessed (e.g.,
the same way window.screen causes us to eventually call Screen::create).  We can
then use that callback to execute Promise.js.

[yhirano, 2013-06-18 06:01:26]

On 2013/06/17 18:06:41, abarth wrote:
> Rather than having a "placeholder" value in the global object, we should make
> the Promise property on Window generate a callback into C++ when accessed
(e.g.,
> the same way window.screen causes us to eventually call Screen::create).  We
can
> then use that callback to execute Promise.js.

Oh, I didn't think of that.
If we choose the way, we don't need Promise.idl and IDL processor work, right?

[abarth-chromium, 2013-06-18 07:56:29]

On 2013/06/18 06:01:26, yhirano wrote:
> If we choose the way, we don't need Promise.idl and IDL processor work, right?

We can still have Promise.idl, but we'll change what the IDL processor does with
it.  We can start out with something custom for now and make it autogenerated in
a later iteration.

[yhirano, 2013-06-18 08:44:49]

On 2013/06/18 07:56:29, abarth wrote:
> On 2013/06/18 06:01:26, yhirano wrote:
> > If we choose the way, we don't need Promise.idl and IDL processor work,
right?
> 
> We can still have Promise.idl, but we'll change what the IDL processor does
with
> it.  We can start out with something custom for now and make it autogenerated
in
> a later iteration.

Of course we can have, but I think if we create "Promise" function on DOMWindow,
not having Promise.idl and moving all works into the JavaScript implementation
is a simpler  and cleaner way.
I will create an experimental CL for that.

[yhirano, 2013-06-18 08:56:15]

For the IDL processor:

I would like to write custom C++ implementation in this and few subsequent CLs,
and make it to be generated later.

For Promise.js implementation:

This is a mere "example" implementation and it is not a valid implementation of
"DOM Promises".
I would like to concentrate infrastructure of "DOM object written in
JavaScript".
After that, I will implement Promise.js as specified in the WHATWG spec draft.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Hidd...
File Source/bindings/v8/V8HiddenPropertyName.h (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Hidd...
Source/bindings/v8/V8HiddenPropertyName.h:44: V(jsConstructor) \
On 2013/06/17 18:05:06, abarth wrote:
> Why not just "constructor" ?

Done.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
File Source/bindings/v8/V8PromiseUtilities.cpp (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:49: v8::Local<v8::Object> global =
isolate->GetCurrentContext()->Global();
On 2013/06/17 18:05:06, abarth wrote:
> Perhaps we should pass in the context instead of assuming we always want the
> current context?

Done (in ScriptRunner::blinkJSConstructor).

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:50: v8::Local<v8::Value>
placeholderValue = global->Get(v8::String::NewSymbol(name));
On 2013/06/17 18:05:06, abarth wrote:
> We don't want to read from the global object.  The global object might have
been
> manipulated by the web site.  Instead, we want to store this information in
> V8PerContextData.

Done (in ScriptRunner::blinkJSConstructor).

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:60: value =
V8ScriptRunner::compileAndRunInternalScript(v8String(String(Promise_js,
sizeof(Promise_js)), isolate), isolate);
On 2013/06/17 18:05:06, abarth wrote:
> Most of the code from here down should be abstracted into a function in
> V8ScriptRunner for calling blinkjs scripts.  It's likely we'll want to pass in
a
> number of arguments so that the script has functions it can call that haven't
> been manipulated by the page.

Done.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.cpp:85: return
constructor->Get(v8::String::NewSymbol("prototype"));
On 2013/06/17 18:05:06, abarth wrote:
> This isn't right.  The prototype property might have been manipulated by the
> page.  Instead, we need to store the prototype property in V8PerContextData.

But Web developers cannot access the object. They access a wrapper object. So
there is no problem, I think.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
File Source/bindings/v8/V8PromiseUtilities.h (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
Source/bindings/v8/V8PromiseUtilities.h:45: static v8::Handle<v8::Value>
callStatic(const char* name, const v8::FunctionCallbackInfo<v8::Value>& args,
v8::Handle<v8::Value> constructor);
On 2013/06/17 18:05:06, abarth wrote:
> Should these last three be part of V8ScriptRunner?  They don't seem specific
to
> Promises?
> 
> Also, V8PromiseUtilities isn't a great name for this sort of class.  "Utility"
> classes accumulate lots of unrelated functionality because they're not very
> focused.
> 
> Rather, I'd call this header V8PromiseCustom.h and rename:
> 
> promiseConstructor -> v8PromiseConstructor
> promisePrototype -> v8PromisePrototype
> 
> I'd also skip the V8PromiseUtilities class and just put these functions in the
> global WebCore namespace.

Done.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PromiseCustom.cpp (right):

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:38: namespace WebCore {
On 2013/06/17 18:05:06, abarth wrote:
> Please add a blank line after the namespace declaration.

Done.

https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:63: }
On 2013/06/17 18:05:06, abarth wrote:
> We should teach the code generator how to generate this code.  We'll want to
add
> an IDL attribute for [ImplementedInJavaScript] or something of that nature.

Will do in another CL.

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.h
File Source/core/dom/Promise.h (right):

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.h...
Source/core/dom/Promise.h:40: };
On 2013/06/17 18:05:06, abarth wrote:
> We should change the code generator not to need it.  This will likely need to
> key off an IDL attribute indicating that a given interface is implemented in
> JavaScript.

Will do in another CL.

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.idl
File Source/core/dom/Promise.idl (right):

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.i...
Source/core/dom/Promise.idl:37: [Custom] static any getStatic(any value);
On 2013/06/17 18:05:06, abarth wrote:
> These shouldn't need to be custom.  We should teach the code generator how to
> deal with interfaces that are implemented in JavaScript.
Will do in another CL.

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:44: function $Promise(value)
On 2013/06/17 18:05:06, abarth wrote:
> Why $Promise?  Should we just call this function Promise?

In fear of confusing with window.Promise. We use window.Promise, like in
getStatic. Notice window.Promise is different from $Promise.

https://codereview.chromium.org/17035004/diff/32003/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:46: this.value = value;
On 2013/06/17 18:05:06, abarth wrote:
> I don't see a "value" property on Promise instances in the spec:
> 
> http://dom.spec.whatwg.org/#promise
> 
> Where is that defined?  I don't see getStatic or get either.  Am I looking at
> the wrong version of the spec?

As noted above, this is a mere example.

[abarth-chromium, 2013-06-18 09:04:08]

On 2013/06/18 08:56:15, yhirano wrote:
>
https://codereview.chromium.org/17035004/diff/32003/Source/bindings/v8/V8Prom...
> Source/bindings/v8/V8PromiseUtilities.cpp:85: return
> constructor->Get(v8::String::NewSymbol("prototype"));
> On 2013/06/17 18:05:06, abarth wrote:
> > This isn't right.  The prototype property might have been manipulated by the
> > page.  Instead, we need to store the prototype property in V8PerContextData.
> 
> But Web developers cannot access the object. They access a wrapper object. So
> there is no problem, I think.

I see.  That's good.  :)

[abarth-chromium, 2013-06-18 09:13:15]

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:46: this.value = value;
I'm worried that this will call setters on Object.prototype.  Consider if the
page does the following:

Object.prototype.__defineSetter__("value", function() { alert("hacked") });
var p = new Promise();

Won't that trigger the alert?

[abarth-chromium, 2013-06-18 09:17:22]

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:41: return new Promise(value);
This calls whatever the web page has mutated window.Promise to be.  Instead, we
need to call $Promise here.

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:53: return $Promise;
What does this print:

console.log((new Promise(42)).toString())

?

[jsbell, 2013-06-18 18:23:12]

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:49: $Promise.prototype = {
Possibly use $Promise.prototype = Object.create(null, descriptor); here - you're
probably going to want to configure the properties anyway.

[abarth-chromium, 2013-06-18 18:41:35]

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:49: $Promise.prototype = {
On 2013/06/18 18:23:13, jsbell wrote:
> Possibly use $Promise.prototype = Object.create(null, descriptor); here -
you're
> probably going to want to configure the properties anyway.

Both Object and create would be external dependencies...

I'm starting to think we need to investigate a different approach.  It's too
hard to avoid all dependencies on the page's JS environment.

[jsbell, 2013-06-18 19:00:07]

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:49: $Promise.prototype = {
On 2013/06/18 18:41:35, abarth wrote:
> On 2013/06/18 18:23:13, jsbell wrote:
> > Possibly use $Promise.prototype = Object.create(null, descriptor); here -
> you're
> > probably going to want to configure the properties anyway.
> 
> Both Object and create would be external dependencies...
> 
> I'm starting to think we need to investigate a different approach.  It's too
> hard to avoid all dependencies on the page's JS environment.

If this is run *after* user scripts can touch the page, yes. The SES folks would
probably say that if you aren't the first script on the page you there is very
little you can assert about the behavior of your script.

(That may be what you mean by "different approach".)

[esprehn, 2013-06-18 19:08:33]

On 2013/06/18 19:00:07, jsbell wrote:
> ...
> 
> If this is run *after* user scripts can touch the page, yes. The SES folks
would
> probably say that if you aren't the first script on the page you there is very
> little you can assert about the behavior of your script.
> 
> (That may be what you mean by "different approach".)

Always running first to be secure sounds scary. Can we just use the global
objects from the Regex context I added?

[abarth-chromium, 2013-06-18 19:47:00]

On 2013/06/18 19:08:33, esprehn wrote:
> Always running first to be secure sounds scary. Can we just use the global
> objects from the Regex context I added?

We'd then need to build a marshalling layer to make sure we don't leak objects
from that static context into content pages.  That path leads to building
Gecko-style wrappers...

I wonder if we should try an approach whereby we store the object graph in V8
objects but keep all the control logic in CPP.  We could use internal fields to
make that fast.  Storing the object graph in V8 would mean the V8 garbage
collector can trace it and we avoid reference cyles.

[abarth-chromium, 2013-06-18 19:47:09]

On 2013/06/18 19:47:00, abarth wrote:
> On 2013/06/18 19:08:33, esprehn wrote:
> > Always running first to be secure sounds scary. Can we just use the global
> > objects from the Regex context I added?
> 
> We'd then need to build a marshalling layer to make sure we don't leak objects
> from that static context into content pages.  That path leads to building
> Gecko-style wrappers...
> 
> I wonder if we should try an approach whereby we store the object graph in V8
> objects but keep all the control logic in CPP.  We could use internal fields
to
> make that fast.  Storing the object graph in V8 would mean the V8 garbage
> collector can trace it and we avoid reference cyles.

cycles

[esprehn, 2013-06-18 19:53:39]

On 2013/06/18 19:47:00, abarth wrote:
> On 2013/06/18 19:08:33, esprehn wrote:
> > Always running first to be secure sounds scary. Can we just use the global
> > objects from the Regex context I added?
> 
> We'd then need to build a marshalling layer to make sure we don't leak objects
> from that static context into content pages.  That path leads to building
> Gecko-style wrappers...
> 
> I wonder if we should try an approach whereby we store the object graph in V8
> objects but keep all the control logic in CPP.  We could use internal fields
to
> make that fast.  Storing the object graph in V8 would mean the V8 garbage
> collector can trace it and we avoid reference cyles.

I meant exposing the regexContext as a property so we'd have pristine objects to
call methods on like: window.pristineContext.Object.hasOwnProperty.call(x, ...).
That'd be fine if we wrap the pristineContext method usage in an API and only
allow primitive values across it. It's still somewhat error prone I guess since
we need to audit all blinkjs code that lands.

I think keeping the logic in CPP and storing the graph in JS sounds a lot
better.

[abarth-chromium, 2013-06-18 20:20:18]

On 2013/06/18 19:53:39, esprehn wrote:
> I meant exposing the regexContext as a property so we'd have pristine objects
to
> call methods on like: window.pristineContext.Object.hasOwnProperty.call(x,
...).
> That'd be fine if we wrap the pristineContext method usage in an API and only
> allow primitive values across it. It's still somewhat error prone I guess
since
> we need to audit all blinkjs code that lands.

Yeah, you'd have to be very careful not to leak references to objects from that
context back to the page.  It's possible, but would probably require a linting
tool to get right.

> I think keeping the logic in CPP and storing the graph in JS sounds a lot
> better.

Yeah...

[yhirano, 2013-06-19 01:09:51]

On 2013/06/18 20:20:18, abarth wrote:
> On 2013/06/18 19:53:39, esprehn wrote:
> > I meant exposing the regexContext as a property so we'd have pristine
objects
> to
> > call methods on like: window.pristineContext.Object.hasOwnProperty.call(x,
> ...).
> > That'd be fine if we wrap the pristineContext method usage in an API and
only
> > allow primitive values across it. It's still somewhat error prone I guess
> since
> > we need to audit all blinkjs code that lands.
> 
> Yeah, you'd have to be very careful not to leak references to objects from
that
> context back to the page.  It's possible, but would probably require a linting
> tool to get right.
> 
> > I think keeping the logic in CPP and storing the graph in JS sounds a lot
> > better.
> 
> Yeah...

I am trying to introduce another Context so that all blinkjs lives on the
context.
Once we enter the Promise methods we enter the context.
Is esprehn's proposal identical with it?

[yhirano, 2013-06-19 12:55:55]

I introduced another v8::Context for BlinkJS implementation.
The context is shared among all BlinkJS implementation.
This update solves external dependency problems which are already reported. I
added some tests and all of them pass now.

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.js
File Source/core/dom/Promise.js (right):

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:41: return new Promise(value);
On 2013/06/18 09:17:22, abarth wrote:
> This calls whatever the web page has mutated window.Promise to be.  Instead,
we
> need to call $Promise here.
Done. Please see Promise-overwrite-constructor.html.

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:46: this.value = value;
On 2013/06/18 09:13:16, abarth wrote:
> I'm worried that this will call setters on Object.prototype.  Consider if the
> page does the following:
> 
> Object.prototype.__defineSetter__("value", function() { alert("hacked") });
> var p = new Promise();
> 
> Won't that trigger the alert?

Done. Please see Promise-define-setter.html.

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:49: $Promise.prototype = {
On 2013/06/18 19:00:08, jsbell wrote:
> On 2013/06/18 18:41:35, abarth wrote:
> > On 2013/06/18 18:23:13, jsbell wrote:
> > > Possibly use $Promise.prototype = Object.create(null, descriptor); here -
> > you're
> > > probably going to want to configure the properties anyway.
> > 
> > Both Object and create would be external dependencies...
> > 
> > I'm starting to think we need to investigate a different approach.  It's too
> > hard to avoid all dependencies on the page's JS environment.
> 
> If this is run *after* user scripts can touch the page, yes. The SES folks
would
> probably say that if you aren't the first script on the page you there is very
> little you can assert about the behavior of your script.
> 
> (That may be what you mean by "different approach".)

There is no external dependencies now.
No property customize is needed, since Web developers can't access this object
directly.

https://codereview.chromium.org/17035004/diff/52001/Source/core/dom/Promise.j...
Source/core/dom/Promise.js:53: return $Promise;
On 2013/06/18 09:17:22, abarth wrote:
> What does this print:
> 
> console.log((new Promise(42)).toString())
> 
> ?
[object Promise]

[abarth-chromium, 2013-06-19 17:41:16]

On 2013/06/19 01:09:51, yhirano wrote:
> I am trying to introduce another Context so that all blinkjs lives on the
> context.
> Once we enter the Promise methods we enter the context.
> Is esprehn's proposal identical with it?

The problem with that approach is that we need to be extremely careful about
loading references to objects in that context.  Because that context is shared
between all pages, it's a vector for cross-site scripting.

[abarth-chromium, 2013-06-19 17:48:49]

I don't think a JavaScript-based implementation is feasible.  I'm sorry I wasn't
able to foresee all the problems before you spent time working on the code.

We should keep the data structure you're using in this CL, however, and just
change the code from JS to CPP.  Specifically, instead of writing:

v8SetReturnValue(args, V8ScriptRunner::callStaticMethod("getStatic", args,
constructor(args.GetIsolate())))

We should just implement the getStatic function in CPP.  We should continue to
store all the data in the V8 object, however, probably using SetInternalField
and GetInternalField.  That's both fast and doesn't conflict with
author-supplied names.  In particular, we can add the additional internal fields
to the Promise wrapper object itself so that we don't even need to introduce an
extra object.

Does that make sense?  I can write up a doc with some diagrams if that would be
helpful.

[yhirano, 2013-06-20 01:37:31]

On 2013/06/19 17:41:16, abarth wrote:
> On 2013/06/19 01:09:51, yhirano wrote:
> > I am trying to introduce another Context so that all blinkjs lives on the
> > context.
> > Once we enter the Promise methods we enter the context.
> > Is esprehn's proposal identical with it?
> 
> The problem with that approach is that we need to be extremely careful about
> loading references to objects in that context.  Because that context is shared
> between all pages, it's a vector for cross-site scripting.

In the implementation, the context is one-to-one with DOMWindow and not shared
between all pages, right?

[abarth-chromium, 2013-06-20 01:45:17]

Creating an extra context per DOMWindow will use more resources than necessary. 
Plus, leaking objects from that context will still be a correctness bug.

[yhirano, 2013-06-20 01:49:20]

On 2013/06/20 01:45:17, abarth wrote:
> Creating an extra context per DOMWindow will use more resources than
necessary. 
> Plus, leaking objects from that context will still be a correctness bug.

I see. I will write another CL for Promise implementation in C++.

[abarth-chromium, 2013-06-20 03:17:16]

On 2013/06/20 01:49:20, yhirano wrote:
> I see. I will write another CL for Promise implementation in C++.

The key is that we want to store the state in the V8 heap so that it can be
garbage collected properly.

[yhirano, 2013-06-20 13:04:31]

I have a question. I want to introduce a class that has utility functions (e.g.
postTask) and variables (e.g. const int promisePendingStatus = 1). Do you have
an idea of the class name?
In the above comment, you recommended V8PromiseCustom. Did it mean that the
class name is V8PromiseCustom, declared in V8PromiseCustom.h and implemented in
V8PromiseCustom.cpp with the implementation of V8Promise?

[abarth-chromium, 2013-06-20 16:04:22]

On 2013/06/20 13:04:31, yhirano wrote:
> I have a question. I want to introduce a class that has utility functions
(e.g.
> postTask) and variables (e.g. const int promisePendingStatus = 1). Do you have
> an idea of the class name?
> In the above comment, you recommended V8PromiseCustom. Did it mean that the
> class name is V8PromiseCustom, declared in V8PromiseCustom.h and implemented
in
> V8PromiseCustom.cpp with the implementation of V8Promise?

Yeah, we can try that.  We don't usually have a V8FooCustom class, but we can
have one in this case if it's helpful.