gin: Use V8 Maybe APIs

gin: Use V8 Maybe APIs

TEST=gin_unittests
BUG=479439
Committed: https://crrev.com/7a6acf6c7d45e24f07781c69249d90967013bbe4
Cr-Commit-Position: refs/heads/master@{#331923}

[bashi, 2015-04-28 02:25:11]

bashi@chromium.org changed reviewers:
+ jochen@chromium.org

[bashi, 2015-04-28 02:25:11]

PTAL? Please add reviewers if you are not the right person to ask reviews for
gin/.

[jochen (gone - plz use gerrit), 2015-04-28 11:14:20]

https://codereview.chromium.org/1106393002/diff/1/gin/converter.h
File gin/converter.h (right):

https://codereview.chromium.org/1106393002/diff/1/gin/converter.h#newcode166
gin/converter.h:166: return v8::Handle<v8::Value>();
we shouldn't return empty handles but MaybeLocal<> values

https://codereview.chromium.org/1106393002/diff/1/gin/try_catch.cc
File gin/try_catch.cc (right):

https://codereview.chromium.org/1106393002/diff/1/gin/try_catch.cc#newcode24
gin/try_catch.cc:24: TryCatch::TryCatch() :
try_catch_(v8::Isolate::GetCurrent()) {
no calls to GetCurrent please

[bashi, 2015-05-01 03:50:35]

https://codereview.chromium.org/1106393002/diff/1/gin/converter.h
File gin/converter.h (right):

https://codereview.chromium.org/1106393002/diff/1/gin/converter.h#newcode166
gin/converter.h:166: return v8::Handle<v8::Value>();
On 2015/04/28 11:14:20, jochen wrote:
> we shouldn't return empty handles but MaybeLocal<> values

Changing return type requires 3-way CLs.
1. Add Maybe version of Converter::ToV8()
2. Use them in other components which depend on gin
3. Remove the current ToV8()

Does this sound reasonable? Then I'll do that.

Also, should we change all Converter::ToV8() to return MaybeLocal for
consistency?

https://codereview.chromium.org/1106393002/diff/1/gin/try_catch.cc
File gin/try_catch.cc (right):

https://codereview.chromium.org/1106393002/diff/1/gin/try_catch.cc#newcode24
gin/try_catch.cc:24: TryCatch::TryCatch() :
try_catch_(v8::Isolate::GetCurrent()) {
On 2015/04/28 11:14:20, jochen wrote:
> no calls to GetCurrent please

Done.

[jochen (gone - plz use gerrit), 2015-05-04 13:25:40]

hum, maybe the set API should CHECK() and we'll document in the API that you can
only use it if you're sure it'll work?

Is it possible to not use GetCurrentContext() easily?

[bashi, 2015-05-08 03:53:30]

On 2015/05/04 13:25:40, jochen wrote:
> hum, maybe the set API should CHECK() and we'll document in the API that you
can
> only use it if you're sure it'll work?

Added CHECK() in ToV8(). (Let me know if I don't understand your points)

> 
> Is it possible to not use GetCurrentContext() easily?

I don't think it's easy because ToV8()s are exposed APIs. If we want to change
them to take v8::Context we need to rewrite callsites (outside of gin) first.

[jochen (gone - plz use gerrit), 2015-05-15 04:43:17]

hum, I'm really not sure about this change.

We're just pushing the problem one level down. If we actually try to terminate
execution, this will still just crash.

I agree that changing the APIs will require changing the callsites as well, but
I think that's what we should do - otherwise, the actual goal of not crashing
will not be achieved.

[bashi, 2015-05-15 05:02:56]

On 2015/05/15 04:43:17, jochen (traveling) wrote:
> hum, I'm really not sure about this change.
> 
> We're just pushing the problem one level down. If we actually try to terminate
> execution, this will still just crash.
> 
> I agree that changing the APIs will require changing the callsites as well,
but
> I think that's what we should do - otherwise, the actual goal of not crashing
> will not be achieved.

Yes. That's what I suggested in
https://codereview.chromium.org/1106393002/#msg4. Will revise the CL just to add
Maybe version.

[bashi, 2015-05-15 05:56:18]

Revised CL. PTAL?

[jochen (gone - plz use gerrit), 2015-05-15 15:40:39]

looks better, thx!

https://codereview.chromium.org/1106393002/diff/60001/gin/converter.h
File gin/converter.h (right):

https://codereview.chromium.org/1106393002/diff/60001/gin/converter.h#newcode158
gin/converter.h:158: // DEPRECATED. Use Maybe version.
how many callsites are there?

https://codereview.chromium.org/1106393002/diff/60001/gin/try_catch.h
File gin/try_catch.h (right):

https://codereview.chromium.org/1106393002/diff/60001/gin/try_catch.h#newcode19
gin/try_catch.h:19: TryCatch(v8::Isolate* isolate);
explicit

[bashi, 2015-05-19 02:09:08]

Sorry for being late. I work three days per week...

https://codereview.chromium.org/1106393002/diff/60001/gin/converter.h
File gin/converter.h (right):

https://codereview.chromium.org/1106393002/diff/60001/gin/converter.h#newcode158
gin/converter.h:158: // DEPRECATED. Use Maybe version.
On 2015/05/15 15:40:39, jochen (traveling) wrote:
> how many callsites are there?

- Some testing code use this ToV8()
- Mojo uses this ToV8() via gin::Dictionary::Set<T>().

I noticed that the latter causes a problem. Set() is a template function and it
assumes that there is non-maybe version of Converter<T>::ToV8(). If we remove
non-maybe version of ToV8() from Converter<std::vector<T>>,
gin::Dictionary::Set<T>() won't work.

There would be some workaround to make Set() work without non-maybe version of
ToV8<std::vector<T>>():

1. Add maybe version of ToV8() to all types. All Converter<T> would have
ToV8(v8::Context, T val). Deprecate non-maybe version of ToV8(). Make
Dictionary::Set() always use maybe version of ToV8().
2. Add another Set() to Dictionary. It takes v8::Context like V8 APIs which
returns MaybeLocal. The Set() uses maybe version of ToV8() internally.
3. Use template meta-programming to detect proper ToV8() to be used.

I'd prefer (3). because
- (1) requires many additional work not only inside gin but also callsites which
extends Converter for specific types (e.g. Converter<mojo::Handle>).
- (2) would be very confusing for callsites. If we take this approach, callsites
need to know which ToV8() (maybe or non-maybe) are provided to the given type T.
For example:
    dictionary.Set("int_value", 1);
    dictionary.Set(context, "array_value", array);
  I don't think this is a good API.

WDYT?

https://codereview.chromium.org/1106393002/diff/60001/gin/try_catch.h
File gin/try_catch.h (right):

https://codereview.chromium.org/1106393002/diff/60001/gin/try_catch.h#newcode19
gin/try_catch.h:19: TryCatch(v8::Isolate* isolate);
On 2015/05/15 15:40:39, jochen (traveling) wrote:
> explicit

Will do.

[jochen (gone - plz use gerrit), 2015-05-21 07:32:19]

since you prefer (3) let's do that

[bashi, 2015-05-22 03:17:33]

Patchset #5 (id:80001) has been deleted

[bashi, 2015-05-22 03:50:25]

Patchset #5 (id:100001) has been deleted

[bashi, 2015-05-22 03:52:03]

On 2015/05/21 07:32:19, jochen wrote:
> since you prefer (3) let's do that

Done. PTAL?

[jochen (gone - plz use gerrit), 2015-05-27 14:43:26]

lgtm with nits

https://codereview.chromium.org/1106393002/diff/140001/gin/dictionary.h
File gin/dictionary.h (right):

https://codereview.chromium.org/1106393002/diff/140001/gin/dictionary.h#newco...
gin/dictionary.h:37: .ToLocal(&val))
nit add {}

https://codereview.chromium.org/1106393002/diff/140001/gin/try_catch.cc
File gin/try_catch.cc (right):

https://codereview.chromium.org/1106393002/diff/140001/gin/try_catch.cc#newco...
gin/try_catch.cc:14: v8::Isolate* isolate = v8::Isolate::GetCurrent();
please pass in the Isolate* as parameter

[bashi, 2015-05-29 02:04:02]

https://codereview.chromium.org/1106393002/diff/140001/gin/dictionary.h
File gin/dictionary.h (right):

https://codereview.chromium.org/1106393002/diff/140001/gin/dictionary.h#newco...
gin/dictionary.h:37: .ToLocal(&val))
On 2015/05/27 14:43:26, jochen wrote:
> nit add {}

Done.

https://codereview.chromium.org/1106393002/diff/140001/gin/try_catch.cc
File gin/try_catch.cc (right):

https://codereview.chromium.org/1106393002/diff/140001/gin/try_catch.cc#newco...
gin/try_catch.cc:14: v8::Isolate* isolate = v8::Isolate::GetCurrent();
On 2015/05/27 14:43:26, jochen wrote:
> please pass in the Isolate* as parameter

Done.

[bashi, 2015-05-29 02:04:14]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-05-29 02:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1106393002/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-05-29 02:04:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1106393002/160001

[commit-bot: I haz the power, 2015-05-29 04:06:16]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-05-29 04:07:13]

Patchset 7 (id:??) landed as
https://crrev.com/7a6acf6c7d45e24f07781c69249d90967013bbe4
Cr-Commit-Position: refs/heads/master@{#331923}

[bashi, 2015-05-29 06:52:55]

A revert of this CL (patchset #7 id:160001) has been created in
https://codereview.chromium.org/1161053002/ by bashi@chromium.org.

The reason for reverting is: Broke layout tests.