[inspector] migrate scriptParsed and getCompiledScripts to native

[inspector] migrate scriptParsed and getCompiledScripts to native

* Introduced DebugInterface::Script - native wrapper on internal::Script.
* Migrated getCompildScripts and scriptParsed to native wrapper.

BUG=chromium:652939, v8:5510
R=yangguo@chromium.org,dgozman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/f7f591ab6b0061abc302120ad3040e51e2c43852
Cr-Commit-Position: refs/heads/master@{#40660}

[kozy, 2016-10-25 20:08:50]

Dmitry and Yang, please take a look.

[kozy, 2016-10-25 20:08:55]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 20:09:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 22:09:11]

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

[commit-bot: I haz the power, 2016-10-25 22:09:12]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[kozy, 2016-10-25 23:21:15]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 23:21:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 00:14:29]

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

[commit-bot: I haz the power, 2016-10-26 00:14:30]

Dry run: This issue passed the CQ dry run.

[kozy, 2016-10-27 21:18:28]

Ping.

I think we can go even further and replace V8DebuggerScript with
v8::DebugInterface::Script.
Advantages:
* adding new api into script object for agent will be easier,
* inspector will hold all scripts when DevTools is opened.

I'll make it in followup if there is no objection.

[kozy, 2016-10-28 01:11:57]

Please take another look!

[kozy, 2016-10-28 01:20:37]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 01:20:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 02:17:06]

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

[commit-bot: I haz the power, 2016-10-28 02:17:06]

Dry run: This issue passed the CQ dry run.

[Yang, 2016-10-28 05:39:25]

Sorry for the delay. I have one comment.

The script object wrapper in V8 that wraps v8::internal::Script only exists to
expose getters to script properties such as script name to Javascript. We are
keen to remove these getters, so let's not use wrappers anymore.

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

https://codereview.chromium.org/2449213002/diff/40001/src/api.cc#newcode8937
src/api.cc:8937: i::Handle<i::JSObject> wrapper = i::Script::GetWrapper(script);
If would be awesome if we could get rid of script wrapper objects. We only need
them to add JS functionality to it that the debugger uses. Since we are moving
away from that, it would be best not to use script wrappers at all anymore.

How about the caller passes in a PersistentValueVector that gets appended here
with Local<DebugInterface::Script> objects to return?

[kozy, 2016-10-28 05:50:44]

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

https://codereview.chromium.org/2449213002/diff/40001/src/api.cc#newcode8937
src/api.cc:8937: i::Handle<i::JSObject> wrapper = i::Script::GetWrapper(script);
On 2016/10/28 05:39:24, Yang wrote:
> If would be awesome if we could get rid of script wrapper objects. We only
need
> them to add JS functionality to it that the debugger uses. Since we are moving
> away from that, it would be best not to use script wrappers at all anymore.
> 
> How about the caller passes in a PersistentValueVector that gets appended here
> with Local<DebugInterface::Script> objects to return?

We still pass wrapped script as argument of event listener for after compile
events. I can change argument for these events in this patch and pass just
handle to Script object but it potentially can break another clients. WDYT?
And I'll change function to use persistent vector tomorrow.

[Yang, 2016-10-28 05:51:50]

On 2016/10/28 05:50:44, kozyatinskiy wrote:
> https://codereview.chromium.org/2449213002/diff/40001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/2449213002/diff/40001/src/api.cc#newcode8937
> src/api.cc:8937: i::Handle<i::JSObject> wrapper =
i::Script::GetWrapper(script);
> On 2016/10/28 05:39:24, Yang wrote:
> > If would be awesome if we could get rid of script wrapper objects. We only
> need
> > them to add JS functionality to it that the debugger uses. Since we are
moving
> > away from that, it would be best not to use script wrappers at all anymore.
> > 
> > How about the caller passes in a PersistentValueVector that gets appended
here
> > with Local<DebugInterface::Script> objects to return?
> 
> We still pass wrapped script as argument of event listener for after compile
> events. I can change argument for these events in this patch and pass just
> handle to Script object but it potentially can break another clients. WDYT?
> And I'll change function to use persistent vector tomorrow.

LGTM. I'm fine with following this up in separate CLs.

[kozy, 2016-10-28 18:15:50]

Addressed comment about PersistentValueVector, and will address other in follow
up.
Dmitry, please take a look.

[kozy, 2016-10-28 18:16:25]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 18:16:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 18:51:26]

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

[commit-bot: I haz the power, 2016-10-28 18:51:27]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-10-28 19:57:15]

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
src/debug/debug-interface.h:143: class Script {
What about comments? Do we drop the altogether or keep adding good explanations?

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
src/debug/debug-interface.h:150: MaybeLocal<Array>
LineEnds(v8::Local<v8::Context>) const;
Let's go with vector<int>, and remove all contexts here.

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
src/debug/debug-interface.h:157: static MaybeLocal<Script>
Wrap(v8::Local<v8::Context>,
We definitely need a comment for this one.

https://codereview.chromium.org/2449213002/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2449213002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-script.cc:114: size_t secondComma =
contextData.find(",", firstComma + 1);
Let's not call when firstComma is kNotFound.

[kozy, 2016-10-28 20:59:37]

All done. Dmitry, please take another look.

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
src/debug/debug-interface.h:143: class Script {
On 2016/10/28 19:57:15, dgozman wrote:
> What about comments? Do we drop the altogether or keep adding good
explanations?

Done.

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
src/debug/debug-interface.h:150: MaybeLocal<Array>
LineEnds(v8::Local<v8::Context>) const;
On 2016/10/28 19:57:15, dgozman wrote:
> Let's go with vector<int>, and remove all contexts here.

Done.

https://codereview.chromium.org/2449213002/diff/60001/src/debug/debug-interfa...
src/debug/debug-interface.h:157: static MaybeLocal<Script>
Wrap(v8::Local<v8::Context>,
On 2016/10/28 19:57:15, dgozman wrote:
> We definitely need a comment for this one.

Done.

https://codereview.chromium.org/2449213002/diff/60001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2449213002/diff/60001/src/inspector/v8-debugg...
src/inspector/v8-debugger-script.cc:114: size_t secondComma =
contextData.find(",", firstComma + 1);
On 2016/10/28 19:57:15, dgozman wrote:
> Let's not call when firstComma is kNotFound.

Done.

[kozy, 2016-10-28 21:43:27]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 21:43:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 21:48:11]

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

[commit-bot: I haz the power, 2016-10-28 21:48:12]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11686)

[kozy, 2016-10-28 22:04:09]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 22:04:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 22:09:05]

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

[commit-bot: I haz the power, 2016-10-28 22:09:06]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11687)

[dgozman, 2016-10-28 22:52:52]

lgtm

https://codereview.chromium.org/2449213002/diff/100001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2449213002/diff/100001/src/debug/debug-interf...
src/debug/debug-interface.h:167: * Return empty local if called with not a
wrapper over
Returns empty local if not called with a valid wrapper of ...

https://codereview.chromium.org/2449213002/diff/100001/src/inspector/v8-debug...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2449213002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger-script.cc:85: if (lineEnds.size()) {
CHECK(!lineEnds.empty());
int source_length = lineEnds[lineEnds.size() - 1];

[kozy, 2016-10-29 01:26:06]

thanks! all done.

https://codereview.chromium.org/2449213002/diff/100001/src/debug/debug-interf...
File src/debug/debug-interface.h (right):

https://codereview.chromium.org/2449213002/diff/100001/src/debug/debug-interf...
src/debug/debug-interface.h:167: * Return empty local if called with not a
wrapper over
On 2016/10/28 22:52:51, dgozman wrote:
> Returns empty local if not called with a valid wrapper of ...

Done.

https://codereview.chromium.org/2449213002/diff/100001/src/inspector/v8-debug...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2449213002/diff/100001/src/inspector/v8-debug...
src/inspector/v8-debugger-script.cc:85: if (lineEnds.size()) {
On 2016/10/28 22:52:51, dgozman wrote:
> CHECK(!lineEnds.empty());
> int source_length = lineEnds[lineEnds.size() - 1];

Done.

[kozy, 2016-10-29 01:30:14]

The CQ bit was checked by kozyatinskiy@chromium.org

[kozy, 2016-10-29 01:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2449213002/#ps120001
(title: "addressed comments")

[commit-bot: I haz the power, 2016-10-29 01:30:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 02:17:46]

Committed patchset #7 (id:120001)

[jgruber, 2016-11-02 08:39:06]

jgruber@chromium.org changed reviewers:
+ jgruber@chromium.org

[jgruber, 2016-11-02 08:39:07]

A nit drive-by-comment below. LGTM otherwise, nice!

https://codereview.chromium.org/2449213002/diff/120001/src/inspector/v8-debug...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2449213002/diff/120001/src/inspector/v8-debug...
src/inspector/v8-debugger-script.cc:94: } else {
Sorry for the late comment; I've been reading a couple of older CLs on the
train, hope you don't mind. 

Isn't this dead code since you CHECK(lineEnds.size()) above? Also, we could pass
the vector around with move semantics instead of copying it.

[kozy, 2016-11-03 00:18:36]

https://codereview.chromium.org/2449213002/diff/120001/src/inspector/v8-debug...
File src/inspector/v8-debugger-script.cc (right):

https://codereview.chromium.org/2449213002/diff/120001/src/inspector/v8-debug...
src/inspector/v8-debugger-script.cc:94: } else {
On 2016/11/02 08:39:07, jgruber wrote:
> Sorry for the late comment; I've been reading a couple of older CLs on the
> train, hope you don't mind. 
> 
> Isn't this dead code since you CHECK(lineEnds.size()) above? Also, we could
pass
> the vector around with move semantics instead of copying it.

Yes, it's dead code, thanks!
For vector - I think that copy elision optimization will help to avoid
additional copying.

[commit-bot: I haz the power, 2016-11-17 22:17:30]

Description was changed from

==========
[inspector] migrate scriptParsed and getCompiledScripts to native

* Introduced DebugInterface::Script - native wrapper on internal::Script.
* Migrated getCompildScripts and scriptParsed to native wrapper.

BUG=chromium:652939, v8:5510
R=yangguo@chromium.org,dgozman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
[inspector] migrate scriptParsed and getCompiledScripts to native

* Introduced DebugInterface::Script - native wrapper on internal::Script.
* Migrated getCompildScripts and scriptParsed to native wrapper.

BUG=chromium:652939, v8:5510
R=yangguo@chromium.org,dgozman@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/f7f591ab6b0061abc302120ad3040e51e2c43852
Cr-Commit-Position: refs/heads/master@{#40660}
==========

[commit-bot: I haz the power, 2016-11-17 22:17:32]

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