Implement ScriptPromise and ScriptFunction

Implement ScriptPromise and ScriptFunction

These two classes allow C++ code to hook into a Promise's resolution chain.
Documentation provided in ScriptPromise.h

BUG=

[alecflett, 2013-08-09 20:57:52]

abarth@ - Here is an updated version of what we worked on. I passed the isolate
through a few places that we didn't have before. LMK if that's overkill, I'm
kind of working on the assumption that using the current global isolate was
bad..

That said, I hit a wall, that there is no ScriptExecutionContext in scope for
these tests, and getScriptExecutionContext() (also global, etc etc) bails
because there is no active DOMWindow.

To begin to address this, I added a ScriptExecutionContext parameter to then()
hoping I could create one in the test but I wasn't sure how to do that and
couldn't find any tests that didn't create entire frames to hold the documents
to be the ScriptExecutionContext.

 I was mostly cribbing off of PromiseResolverTest.

So the paths I see are:
1) figure out a way to get a ScriptExecutionContext "active" before running
tests to let getScriptExecutionContext() work
or
2) Create a test-safe ScriptExecutionContext and pass it in to then(), getting
rid of most calls to getScriptExecutionContext() in the process.

The last one seems like a more sustainable approach - either one already exists,
or writing one would be beneficial to folks testing this layer. I'm going to
attempt the latter while I await feedback.

[abarth-chromium, 2013-08-09 21:46:52]

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
File Source/bindings/v8/ScriptFunction.h (right):

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:35: #include "V8Binding.h"
Please use paths from the Source directory

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:37: 
No need for this blank line.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:42: // to use this class,
?

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:54: ref();
Rather than calling ref() and deref(), maybe we should abstract our a base class
called GarbageCollected that does this work?

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:64: self->deref();
Don't we need to clear out m_function in this function?

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:71: ScriptFunction* function =
static_cast<ScriptFunction*>(args.Data().As<v8::External>()->Value());
We need to hold a RefPtr<ScriptFunction> here in case the garbage collector
decides to destroy function during this function.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:74: value = args[0];
Can we use the ? : operator to avoid calling v8::Undefined when it's not needed?

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:53: //     m_value =
This code same seems incomplete...

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:62: //   m_resultCallback = adoptRef(new
MyCallback(this));
Doesn't this example code create a reference cycle?  MyClass holds a RefPtr to
MyCallback, which holds a RefPtr to MyClass...  You don't need to hold a RefPtr
to ScriptFunction in MyClass.  The lifetime of the ScriptFunction will be
managed by the GC.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:71: ScriptPromise() : m_isolate(0) { }
Can we remove this constructor?  ScriptPromise should be a short-lived object
that you only construct when you have a v8::Handle<v8::Value> that you want to
interact with.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:72: explicit
ScriptPromise(v8::Handle<v8::Value> promise, v8::Isolate* isolate)
I would have expected this object to take a ScriptValue rather than a
v8::Handle<v8::Value>.  The code using this class is going to be outside the
bindings directory, so it's likely to be dealing in ScriptValue rather than in
the lower-level V8 handles.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:80: }
I'd skip this function.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:86: }
This one too.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:87: bool then(ScriptFunction* function,
ScriptExecutionContext* context) const
I'd probably take a PassRefPtr<ScriptFunction> function because most callers
will want to give us their last reference.

There's no need for the |ScriptExecutionContext| parameter.  We should get that
from the static via getScriptExecutionContext().  If we let the caller supply a
|ScriptExecutionContext|, they could give us the wrong one.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptProm...
Source/bindings/v8/ScriptPromise.h:92: }
No need for { }.

Also, don't we need to check promise.IsEmpty() before using operator->  ?

[abarth-chromium, 2013-08-09 21:49:23]

On 2013/08/09 20:57:52, alecflett wrote:
> abarth@ - Here is an updated version of what we worked on. I passed the
isolate
> through a few places that we didn't have before. LMK if that's overkill, I'm
> kind of working on the assumption that using the current global isolate was
> bad..

We like to pass the isolate around.  That's good.  Generally, we don't make the
code outside of the bindings directory worry too much about the isolate though,
so you can leave it off of APIs you expect to be used outside of the bindings.

> That said, I hit a wall, that there is no ScriptExecutionContext in scope for
> these tests, and getScriptExecutionContext() (also global, etc etc) bails
> because there is no active DOMWindow.

I think trying to write unit tests for this code is leading you down a bad path.
 Rather than writing unit tests, we should write real code that uses these
functions.  In real code, you won't have any of these problems.

> So the paths I see are:
> 1) figure out a way to get a ScriptExecutionContext "active" before running
> tests to let getScriptExecutionContext() work
> or
> 2) Create a test-safe ScriptExecutionContext and pass it in to then(), getting
> rid of most calls to getScriptExecutionContext() in the process.

I think we'd be better off not writing unit tests.  It's torturing the
production code.  Why not write real code?

[abarth-chromium, 2013-08-09 21:50:29]

If this is only needed in the NavigationController branch, I'm happy to review
CLs for that branch.

[alecflett, 2013-08-12 23:26:40]

abarth@ - ok, this is now using Internals. I just stuck a dummy method that adds
one to the value from C++. I welcome other approaches.

[abarth-chromium, 2013-08-12 23:32:40]

Good idea~

[abarth-chromium, 2013-08-12 23:35:32]

https://codereview.chromium.org/22470008/diff/14001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptFunction.h (right):

https://codereview.chromium.org/22470008/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptFunction.h:41: // to use this class,
I find this comment sort of confusing.  Can we unify the two pieces into one
comment somewhere?

https://codereview.chromium.org/22470008/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptFunction.h:53: ref();
Last time I looked at this CL, I asked about factoring out a GarbageCollected
base class...  What do you think about that?

https://codereview.chromium.org/22470008/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptFunction.h:72: v8::Local<v8::Value> value =
v8::Undefined(isolate);
It looks like you didn't address a number of comments from my last review...

[alecflett, 2013-08-13 20:37:10]

ok, new patch uploaded - sorry I had completely forgotten about a few of your
comments. Got 'em all now.

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
File Source/bindings/v8/ScriptFunction.h (right):

https://codereview.chromium.org/22470008/diff/1/Source/bindings/v8/ScriptFunc...
Source/bindings/v8/ScriptFunction.h:74: value = args[0];
On 2013/08/09 21:46:52, abarth wrote:
> Can we use the ? : operator to avoid calling v8::Undefined when it's not
needed?

Doing that now - I see why it was done the other way elsewhere though - note the
explicit casting on v8::Undefined.

https://codereview.chromium.org/22470008/diff/14001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptFunction.h (right):

https://codereview.chromium.org/22470008/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptFunction.h:53: ref();
On 2013/08/12 23:35:32, abarth wrote:
> Last time I looked at this CL, I asked about factoring out a GarbageCollected
> base class...  What do you think about that?

Sorry, I forgot about that.

https://codereview.chromium.org/22470008/diff/14001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptFunction.h:72: v8::Local<v8::Value> value =
v8::Undefined(isolate);
On 2013/08/12 23:35:32, abarth wrote:
> It looks like you didn't address a number of comments from my last review...

oops, not sure how the new version of this file didn't get uploaded.

[alecflett, 2013-08-15 00:01:24]

abarth@ - ping?

[abarth-chromium, 2013-08-15 00:22:10]

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Garbag...
File Source/bindings/v8/GarbageCollected.h (right):

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Garbag...
Source/bindings/v8/GarbageCollected.h:40: class GarbageCollected {
Oh, sorry.  I meant that GarbageCollected would handle all the v8::External
work.  The idea is that if you wanted your C++ object to be garbage collected,
you'd subclass GarbageCollected.

class MyClass : public GarbageCollected<MyClass> {
public:
  void foo();
....
};

v8::Handle<v8::External> bar = MyClass::create(..);
MyClass::from(bar)->foo();

The idea is that GarbageCollected is an alternative to RefCounted as a base
class that defines how the class's memory is managed.

[abarth-chromium, 2013-08-15 00:23:54]

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptFunction.h (right):

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptFunction.h:59: ref();
You should be able to encapsulate all this nastiness in GarbageCollected.  In
fact, you should be able to avoid using RefCounted as a base class entirely. 
When the v8 object dies, you can just delete |this| because it's been collected
by the GC.

[abarth-chromium, 2013-08-15 00:35:14]

I'm not sure I've done a good job explaining the issues in this CL.  Maybe we
should talk in person with a white board?

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptFunction.h (right):

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptFunction.h:80: GarbageCollected<v8::Function>
m_function;
ScriptFunction need to hold a ScopedPeristent<v8::Function>.  That v8::Function
will then hold the v8::External.  The GarbageCollected base class then holds a
weak handle to the v8::External to detect when it's time to delete the void*
stored in the v8::External.

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
File Source/bindings/v8/ScriptPromise.h (right):

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromise.h:52: //    
m_receiver->HaveValue(scriptValue.v8Value().As<v8::Integer>()->Value())
This example code is no good.  Why do you assume that the scriptValue actually
is an integer?  I'm worried that people will copy this code, but we have no way
to test it or even mark sure it compiles...

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromise.h:62: //   promise->then(adoptRef(new
MyCallback(this)));
adoptRef(new MyCallback(this))  should be   MyCallback::create

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromise.h:95: ScriptValue ResultPromise() { return
ScriptValue(m_result.newLocal(v8::Isolate::GetCurrent())); }
ResultPromise -> resultPromise

https://codereview.chromium.org/22470008/diff/22001/Source/bindings/v8/Script...
Source/bindings/v8/ScriptPromise.h:99: ScopedPersistent<v8::Value> m_result;
Rather than storing m_result as a member, why not make it an out parameter of
|then| ?

[abarth-chromium, 2013-08-19 22:00:04]

On 2013/08/15 00:35:14, abarth wrote:
> I'm not sure I've done a good job explaining the issues in this CL.  Maybe we
> should talk in person with a white board?

I took a swing at updating the CL:

https://codereview.chromium.org/23254004/

Let me know what you think.