Expose JSON stringifier through V8 API

Expose JSON stringifier through V8 API

BUG=602659
LOG=N

Committed: https://crrev.com/0ba934d7bff00298511e38139c7f1dda076fc179
Cr-Commit-Position: refs/heads/master@{#35543}

[haavardm, 2016-04-15 10:43:29]

Description was changed from

==========
Expose JSON stringifier through V8 API

BUG=602659
LOG=N
==========

to

==========
Expose JSON stringifier through V8 API

BUG=602659
LOG=N
==========

[haavardm, 2016-04-15 10:43:29]

haavardm@opera.com changed reviewers:
+ jochen@chromium.org, rouslan@chromium.org, verwaest@chromium.org,
yangguo@chromium.org

[haavardm, 2016-04-15 10:44:22]

On 2016/04/15 10:43:29, haavardm wrote:
> mailto:haavardm@opera.com changed reviewers:
> + mailto:jochen@chromium.org, mailto:rouslan@chromium.org,
mailto:verwaest@chromium.org,
> mailto:yangguo@chromium.org

I took over this from https://codereview.chromium.org/1847823004/ after request
from Rouslan

[haavardm, 2016-04-15 10:44:49]

PTAL

https://codereview.chromium.org/1891203002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1891203002/diff/1/include/v8.h#newcode1682
include/v8.h:1682: static V8_WARN_UNUSED_RESULT MaybeLocal<Value> Parse(
Should this be deprecated?

https://codereview.chromium.org/1891203002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1891203002/diff/1/src/api.cc#newcode2757
src/api.cc:2757: ? i::JsonParser<true>::Parse(source)
Does |context| end up in source? or is a new empty context set and used in
JsonParser?

[jochen (gone - plz use gerrit), 2016-04-15 10:50:11]

https://codereview.chromium.org/1891203002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1891203002/diff/1/include/v8.h#newcode1682
include/v8.h:1682: static V8_WARN_UNUSED_RESULT MaybeLocal<Value> Parse(
On 2016/04/15 at 10:44:49, haavardm wrote:
> Should this be deprecated?

yes, please mark it as V8_DEPRECATE_SOON - that means you'll also have to update
the v8 tests to use the new api

https://codereview.chromium.org/1891203002/diff/1/include/v8.h#newcode1695
include/v8.h:1695: Isolate* isolate, Local<Object> json_object);
should also take a Local<Context> instead of Isolate*

https://codereview.chromium.org/1891203002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1891203002/diff/1/src/api.cc#newcode2757
src/api.cc:2757: ? i::JsonParser<true>::Parse(source)
On 2016/04/15 at 10:44:49, haavardm wrote:
> Does |context| end up in source? or is a new empty context set and used in
JsonParser?

the PREPARE_FOR_EXECUTION macro will create a potential exception in |context|

[haavardm, 2016-04-15 11:45:23]

ptal

https://codereview.chromium.org/1891203002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/1891203002/diff/1/include/v8.h#newcode1682
include/v8.h:1682: static V8_WARN_UNUSED_RESULT MaybeLocal<Value> Parse(
On 2016/04/15 10:50:11, jochen - slow wrote:
> On 2016/04/15 at 10:44:49, haavardm wrote:
> > Should this be deprecated?
> 
> yes, please mark it as V8_DEPRECATE_SOON - that means you'll also have to
update
> the v8 tests to use the new api

Done.

https://codereview.chromium.org/1891203002/diff/1/include/v8.h#newcode1695
include/v8.h:1695: Isolate* isolate, Local<Object> json_object);
On 2016/04/15 10:50:11, jochen - slow wrote:
> should also take a Local<Context> instead of Isolate*

Done.

https://codereview.chromium.org/1891203002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1891203002/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21959: 
"git cl format" wants only one empty line here between the tests, while the rest
of the file have 2 empty lines. I let cl format have it's way so it's currently
inconsistent.  But I'll change to whatever is preferred.

[jochen (gone - plz use gerrit), 2016-04-15 11:53:27]

lgtm

https://codereview.chromium.org/1891203002/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/1891203002/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:21959: 
On 2016/04/15 at 11:45:23, haavardm wrote:
> "git cl format" wants only one empty line here between the tests, while the
rest of the file have 2 empty lines. I let cl format have it's way so it's
currently inconsistent.  But I'll change to whatever is preferred.

yeah, we decided to not mass-update but change those two to one one at a time.

[haavardm, 2016-04-15 11:58:20]

The CQ bit was checked by haavardm@opera.com

[commit-bot: I haz the power, 2016-04-15 11:58:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891203002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891203002/20001

[commit-bot: I haz the power, 2016-04-15 12:22:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-15 12:22:27]

Try jobs failed on following builders:
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)

[haavardm, 2016-04-15 12:31:58]

Fixed compile error. PTAL

https://codereview.chromium.org/1891203002/diff/40001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/1891203002/diff/40001/src/api.cc#newcode2766
src/api.cc:2766: RETURN_TO_LOCAL_UNCHECKED(Parse(Local<Context>(), json_string),
Value);
Is using an empty context correct here? (alternatively I could get the current
context from the isolate retrieved from |json_string|)

[please use gerrit instead, 2016-04-15 17:20:47]

lgtm

Thank you for picking this up!

[haavardm, 2016-04-15 17:42:22]

The CQ bit was checked by haavardm@opera.com

[haavardm, 2016-04-15 17:42:22]

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/1891203002/#ps40001
(title: "Fixed compile error on linux")

[commit-bot: I haz the power, 2016-04-15 17:42:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1891203002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1891203002/40001

[commit-bot: I haz the power, 2016-04-15 18:23:56]

Description was changed from

==========
Expose JSON stringifier through V8 API

BUG=602659
LOG=N
==========

to

==========
Expose JSON stringifier through V8 API

BUG=602659
LOG=N
==========

[commit-bot: I haz the power, 2016-04-15 18:23:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-15 18:24:33]

Description was changed from

==========
Expose JSON stringifier through V8 API

BUG=602659
LOG=N
==========

to

==========
Expose JSON stringifier through V8 API

BUG=602659
LOG=N

Committed: https://crrev.com/0ba934d7bff00298511e38139c7f1dda076fc179
Cr-Commit-Position: refs/heads/master@{#35543}
==========

[commit-bot: I haz the power, 2016-04-15 18:24:34]

Patchset 3 (id:??) landed as
https://crrev.com/0ba934d7bff00298511e38139c7f1dda076fc179
Cr-Commit-Position: refs/heads/master@{#35543}