[ABANDONED] Implement non-static methods of Promise and PromiseResolver.

Implement non-static methods of Promise and PromiseResolver.

We are implementing DOM/Promises.
The first CL is https://src.chromium.org/viewvc/blink?revision=152943&view=revision .
As the second step, this CL implements all non-static methods of Promise and PromiseResolver.
Again, all methods are marked as Custom and there are many boilerplate. we will fix them in a future CL.

R=abarth
BUG=243345

[yhirano, 2013-06-27 02:51:09]

abarth@, can you take a look at this CL?

[abarth-chromium, 2013-06-27 03:25:16]

This CL is too big.  Can you break it into pieces?  Maybe post a separate CL for
each function?  I reviewed a bunch of it.

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

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:72: return
handleScope.Close(templ)->GetFunction();
Don't you mean:

return handleScope.Close(templ->GetFunction());    ?

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:78: : m_callback(callback),
m_receiver(receiver), m_result(result)
Please split these up on to separate lines.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:91: ScriptValue m_result;
You can use ScopedPersistent rather than ScriptValue here.  ScriptValue is for
code outside the bindings.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:97: ScriptState* state =
mainWorldScriptState(static_cast<Document*>(context)->frame());
We should ask the Document whether ActiveDOMObjects are stopped.  If they are,
we should drop this task on the floor.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:102: }
Can this occur?  If not we shouldn't handle this case.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:107: }
Can this occur?  If not we shouldn't handle this case.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:115: v8::Local<v8::Object>
receiver =m_receiver.v8Value().As<v8::Object>();
You're missing a = before m_receiver.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:117: v8::Local<v8::Value> v =
V8ScriptRunner::callFunction(callback, context, receiver, 1, &result);
You don't need |v| here.  You can just ignore the return value.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:124: return
setDOMException(INVALID_STATE_ERR, isolate);
Can this occur?  If not, we shouldn't handle this case.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:127: return
setDOMException(INVALID_STATE_ERR, isolate);
This cannot occur.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:129: return v8::Undefined();
v8::Undefined(isolate)   ?

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:135: v8SetReturnValue(args,
v8::Undefined());
v8::Undefined(isolate)   ?

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:137: v8::Local<v8::Value> result =
v8::Undefined();
v8::Undefined(isolate)    ?

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:142:
ASSERT(!environment.IsEmpty());
Why is this ASSERT valid?  If we don't execute line 139, then this ASSERT will
be false.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:176: v8::Local<v8::ObjectTemplate>
tmpl = v8::ObjectTemplate::New();
tmpl  -> template (please use complete words in variable names)

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:178: v8::Local<v8::Object> env =
tmpl->NewInstance();
env ->  ???  (please use complete words in variable names)

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:187: v8::Local<v8::Value> value =
V8ScriptRunner::compileAndRunInternalScript(script, isolate);
There's got to be a better way to do this.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:195: v8::Local<v8::Object> global
= isolate->GetCurrentContext()->Global();
Why do you need to get the global?  Doesn't callFunction default to using the
global object of the current context?

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:212: ASSERT(!resolver.IsEmpty());
Again, why do we have this ASSERT and the branch on line 208?

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PromiseCustom.h (right):

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.h:65: static void
fulfillResolver(v8::Handle<v8::Object> resolver, v8::Handle<v8::Value> result,
bool synchronous, v8::Isolate*);
Should we use an enum rather than a bool for |synchronous| ?

[yhirano, 2013-06-27 03:31:29]

On 2013/06/27 03:25:16, abarth wrote:
> This CL is too big.  Can you break it into pieces?  Maybe post a separate CL
for
> each function?  I reviewed a bunch of it.
> 
I cannot test each method individually.
Can I upload the code without tests?

[abarth-chromium, 2013-06-27 03:34:02]

On 2013/06/27 03:31:29, yhirano wrote:
> I cannot test each method individually.
> Can I upload the code without tests?

Yeah.  Having seen this CL, I know that you've written the tests.  :)

[yhirano, 2013-06-27 05:40:34]

I fixed the issues which are easy to fix.
Two issues are remaining.

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

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:72: return
handleScope.Close(templ)->GetFunction();
On 2013/06/27 03:25:16, abarth wrote:
> Don't you mean:
> 
> return handleScope.Close(templ->GetFunction());    ?

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:78: : m_callback(callback),
m_receiver(receiver), m_result(result)
On 2013/06/27 03:25:16, abarth wrote:
> Please split these up on to separate lines.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:91: ScriptValue m_result;
On 2013/06/27 03:25:16, abarth wrote:
> You can use ScopedPersistent rather than ScriptValue here.  ScriptValue is for
> code outside the bindings.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:97: ScriptState* state =
mainWorldScriptState(static_cast<Document*>(context)->frame());
On 2013/06/27 03:25:16, abarth wrote:
> We should ask the Document whether ActiveDOMObjects are stopped.  If they are,
> we should drop this task on the floor.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:102: }
On 2013/06/27 03:25:16, abarth wrote:
> Can this occur?  If not we shouldn't handle this case.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:107: }
On 2013/06/27 03:25:16, abarth wrote:
> Can this occur?  If not we shouldn't handle this case.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:115: v8::Local<v8::Object>
receiver =m_receiver.v8Value().As<v8::Object>();
On 2013/06/27 03:25:16, abarth wrote:
> You're missing a = before m_receiver.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:117: v8::Local<v8::Value> v =
V8ScriptRunner::callFunction(callback, context, receiver, 1, &result);
On 2013/06/27 03:25:16, abarth wrote:
> You don't need |v| here.  You can just ignore the return value.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:124: return
setDOMException(INVALID_STATE_ERR, isolate);
On 2013/06/27 03:25:16, abarth wrote:
> Can this occur?  If not, we shouldn't handle this case.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:127: return
setDOMException(INVALID_STATE_ERR, isolate);
On 2013/06/27 03:25:16, abarth wrote:
> This cannot occur.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:129: return v8::Undefined();
On 2013/06/27 03:25:16, abarth wrote:
> v8::Undefined(isolate)   ?

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:135: v8SetReturnValue(args,
v8::Undefined());
On 2013/06/27 03:25:16, abarth wrote:
> v8::Undefined(isolate)   ?

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:137: v8::Local<v8::Value> result =
v8::Undefined();
On 2013/06/27 03:25:16, abarth wrote:
> v8::Undefined(isolate)    ?

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:142:
ASSERT(!environment.IsEmpty());
On 2013/06/27 03:25:16, abarth wrote:
> Why is this ASSERT valid?  If we don't execute line 139, then this ASSERT will
> be false.

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:176: v8::Local<v8::ObjectTemplate>
tmpl = v8::ObjectTemplate::New();
On 2013/06/27 03:25:16, abarth wrote:
> tmpl  -> template (please use complete words in variable names)

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:178: v8::Local<v8::Object> env =
tmpl->NewInstance();
On 2013/06/27 03:25:16, abarth wrote:
> env ->  ???  (please use complete words in variable names)

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.cpp:212: ASSERT(!resolver.IsEmpty());
On 2013/06/27 03:25:16, abarth wrote:
> Again, why do we have this ASSERT and the branch on line 208?

Done.

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PromiseCustom.h (right):

https://codereview.chromium.org/17874002/diff/10001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PromiseCustom.h:65: static void
fulfillResolver(v8::Handle<v8::Object> resolver, v8::Handle<v8::Value> result,
bool synchronous, v8::Isolate*);
On 2013/06/27 03:25:16, abarth wrote:
> Should we use an enum rather than a bool for |synchronous| ?

Done.