[V8] Added Script::is_debugger_script flag for embedders

[V8] Added Script::is_debugger_script flag for embedders

In DevTools we need one more flag for script origin - is debugger script. We already have "is shared origin" flag. The new flag added by analogy with the old but new has accessor in script object.

R=yurys@chromium.org

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

[kozy, 2015-01-26 15:17:02]

Yury, ptal.

[yurys, 2015-01-26 15:41:06]

lgtm

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

https://codereview.chromium.org/879553002/diff/1/include/v8.h#newcode980
include/v8.h:980: Handle<Boolean> resource_is_internal_script =
Handle<Boolean>())
The "internal" term is somewhat confusing. Can we rename to something like
is_debugger_script?

https://codereview.chromium.org/879553002/diff/1/include/v8.h#newcode990
include/v8.h:990: V8_INLINE Handle<Boolean> ResourceIsInternalScript() const;
Can you add a comment to the method describing meaning of the flag?

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

https://codereview.chromium.org/879553002/diff/1/src/api.cc#newcode1573
src/api.cc:1573: source->resource_is_internal_script == v8::True(v8_isolate);
source->resource_is_internal_script->IsTrue()

https://codereview.chromium.org/879553002/diff/1/src/api.cc#newcode1661
src/api.cc:1661:
script->set_is_internal_script(origin.ResourceIsInternalScript() ==
ditto

[yurys, 2015-01-26 15:46:43]

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

https://codereview.chromium.org/879553002/diff/1/include/v8.h#newcode984
include/v8.h:984: resource_is_internal_script_(resource_is_internal_script),
Would it make sense to have something like Script::SetDebugData(Handle<Value>)
and keep flags like that in such  object?

[kozy, 2015-01-26 16:42:13]

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

https://codereview.chromium.org/879553002/diff/1/include/v8.h#newcode980
include/v8.h:980: Handle<Boolean> resource_is_internal_script =
Handle<Boolean>())
On 2015/01/26 15:41:06, yurys wrote:
> The "internal" term is somewhat confusing. Can we rename to something like
> is_debugger_script?

Done.

https://codereview.chromium.org/879553002/diff/1/include/v8.h#newcode984
include/v8.h:984: resource_is_internal_script_(resource_is_internal_script),
On 2015/01/26 15:46:43, yurys wrote:
> Would it make sense to have something like Script::SetDebugData(Handle<Value>)
> and keep flags like that in such  object?

Acknowledged.

https://codereview.chromium.org/879553002/diff/1/include/v8.h#newcode990
include/v8.h:990: V8_INLINE Handle<Boolean> ResourceIsInternalScript() const;
On 2015/01/26 15:41:05, yurys wrote:
> Can you add a comment to the method describing meaning of the flag?

Done.

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

https://codereview.chromium.org/879553002/diff/1/src/api.cc#newcode1573
src/api.cc:1573: source->resource_is_internal_script == v8::True(v8_isolate);
On 2015/01/26 15:41:06, yurys wrote:
> source->resource_is_internal_script->IsTrue()

Done.

[kozy, 2015-01-26 16:42:19]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2015-01-26 16:42:50]

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

[commit-bot: I haz the power, 2015-01-26 16:45:32]

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

[commit-bot: I haz the power, 2015-01-26 16:45:33]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/201)

[kozy, 2015-01-26 17:09:26]

kozyatinskiy@chromium.org changed reviewers:
+ yangguo@chromium.org

[kozy, 2015-01-26 17:09:40]

On 2015/01/26 16:45:33, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   v8_presubmit on tryserver.v8
> (http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/201)

Yang, please take a look.

[noordhuis, 2015-01-26 23:51:16]

info@bnoordhuis.nl changed reviewers:
+ info@bnoordhuis.nl

[noordhuis, 2015-01-26 23:51:17]

https://codereview.chromium.org/879553002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/879553002/diff/40001/include/v8.h#newcode991
include/v8.h:991: * Returns true for embdedder's debugger scripts
Typo: s/embdedder/embedder/

From the diff, it's not clear to me why it's called something something
debugger, it doesn't seem debug related at all.  Maybe 'builtin' or 'native' is
a better name?

[kozy, 2015-01-27 10:02:15]

https://codereview.chromium.org/879553002/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/879553002/diff/40001/include/v8.h#newcode991
include/v8.h:991: * Returns true for embdedder's debugger scripts
On 2015/01/26 23:51:17, info1 wrote:
> Typo: s/embdedder/embedder/
> 
> From the diff, it's not clear to me why it's called something something
> debugger, it doesn't seem debug related at all.  Maybe 'builtin' or 'native'
is
> a better name?

Done.

I suppose that bultin and native is not better name. This flag added for
DevTools, we need to mark some scripts as internal. From DevTools side this flag
can be called as isDebuggerScript. I understood that from V8 side this name is
not correct. Maybe add embedder to name: isEmbedderInternalScript is better.

[kozy, 2015-01-27 15:16:43]

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

[kozy, 2015-01-27 15:16:44]

Jochen or Yang, please take a look.

[Yang, 2015-01-28 09:47:30]

I will need some more convincing.

Why is this change necessary? The way I see it, you need a way to attach a
boolean metadata to script objects, and retrieve that piece of metadata both via
API and in Javascript. The content of that metadata is not interesting to V8, as
it only passes that through.

There are better ways to do this. Script objects are wrapped in JSValue objects,
which are JSObjects. As such, you can just manipulate them as normal Javascript
objects. You could
1) Attach a property to the script object to contain that metadata. If that
property has to be hidden, you can simply use a Symbol as property name. (It
seems to me that this is not a requirement, given that you implemented the
accessor).
2) Use a WeakMap to map script object to the metadata. This is virtually
equivalent to (1).

Either way, there is no need to make this intrusive change to V8.

Btw. this should actually be done for is_shared_cross_origin as well.

https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc#newcode794
src/accessors.cc:794: void Accessors::ScriptIsDebuggerScriptGetter(
Why is this necessary? I don't see it for the is_shared_cross_origin flag.

[Yang, 2015-01-28 10:52:56]

On 2015/01/28 09:47:30, Yang wrote:
> I will need some more convincing.
> 
> Why is this change necessary? The way I see it, you need a way to attach a
> boolean metadata to script objects, and retrieve that piece of metadata both
via
> API and in Javascript. The content of that metadata is not interesting to V8,
as
> it only passes that through.
> 
> There are better ways to do this. Script objects are wrapped in JSValue
objects,
> which are JSObjects. As such, you can just manipulate them as normal
Javascript
> objects. You could
> 1) Attach a property to the script object to contain that metadata. If that
> property has to be hidden, you can simply use a Symbol as property name. (It
> seems to me that this is not a requirement, given that you implemented the
> accessor).
> 2) Use a WeakMap to map script object to the metadata. This is virtually
> equivalent to (1).
> 
> Either way, there is no need to make this intrusive change to V8.
> 
> Btw. this should actually be done for is_shared_cross_origin as well.
> 
> https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc
> File src/accessors.cc (right):
> 
>
https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc#newcode794
> src/accessors.cc:794: void Accessors::ScriptIsDebuggerScriptGetter(
> Why is this necessary? I don't see it for the is_shared_cross_origin flag.

Upon closer inspection it seems to me that you would need to get the script
object from the message to be able to use the property. I propose exposing that
and deprecate ScriptOrigin. I found some bugs in the way we return ScriptOrigin
values and it just seems to me that there is a lot of redundancy. ScriptOrigin
was intended to pass a set of values to V8, not return from it.

[Yang, 2015-01-28 11:28:30]

On 2015/01/28 10:52:56, Yang wrote:
> On 2015/01/28 09:47:30, Yang wrote:
> > I will need some more convincing.
> > 
> > Why is this change necessary? The way I see it, you need a way to attach a
> > boolean metadata to script objects, and retrieve that piece of metadata both
> via
> > API and in Javascript. The content of that metadata is not interesting to
V8,
> as
> > it only passes that through.
> > 
> > There are better ways to do this. Script objects are wrapped in JSValue
> objects,
> > which are JSObjects. As such, you can just manipulate them as normal
> Javascript
> > objects. You could
> > 1) Attach a property to the script object to contain that metadata. If that
> > property has to be hidden, you can simply use a Symbol as property name. (It
> > seems to me that this is not a requirement, given that you implemented the
> > accessor).
> > 2) Use a WeakMap to map script object to the metadata. This is virtually
> > equivalent to (1).
> > 
> > Either way, there is no need to make this intrusive change to V8.
> > 
> > Btw. this should actually be done for is_shared_cross_origin as well.
> > 
> > https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc
> > File src/accessors.cc (right):
> > 
> >
>
https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc#newcode794
> > src/accessors.cc:794: void Accessors::ScriptIsDebuggerScriptGetter(
> > Why is this necessary? I don't see it for the is_shared_cross_origin flag.
> 
> Upon closer inspection it seems to me that you would need to get the script
> object from the message to be able to use the property. I propose exposing
that
> and deprecate ScriptOrigin. I found some bugs in the way we return
ScriptOrigin
> values and it just seems to me that there is a lot of redundancy. ScriptOrigin
> was intended to pass a set of values to V8, not return from it.

Also there is no test case in this CL that's supposed to test how you intend to
use this new API.

[yurys, 2015-01-28 12:14:10]

On 2015/01/28 10:52:56, Yang wrote:
> On 2015/01/28 09:47:30, Yang wrote:
> > I will need some more convincing.
> > 
> > Why is this change necessary? The way I see it, you need a way to attach a
> > boolean metadata to script objects, and retrieve that piece of metadata both
> via
> > API and in Javascript. The content of that metadata is not interesting to
V8,
> as
> > it only passes that through.
> > 
> > There are better ways to do this. Script objects are wrapped in JSValue
> objects,
> > which are JSObjects. As such, you can just manipulate them as normal
> Javascript
> > objects. You could
> > 1) Attach a property to the script object to contain that metadata. If that
> > property has to be hidden, you can simply use a Symbol as property name. (It
> > seems to me that this is not a requirement, given that you implemented the
> > accessor).
> > 2) Use a WeakMap to map script object to the metadata. This is virtually
> > equivalent to (1).
> > 
> > Either way, there is no need to make this intrusive change to V8.
> > 
> > Btw. this should actually be done for is_shared_cross_origin as well.
> > 
> > https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc
> > File src/accessors.cc (right):
> > 
> >
>
https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc#newcode794
> > src/accessors.cc:794: void Accessors::ScriptIsDebuggerScriptGetter(
> > Why is this necessary? I don't see it for the is_shared_cross_origin flag.
> 
> Upon closer inspection it seems to me that you would need to get the script
> object from the message to be able to use the property. I propose exposing
that
> and deprecate ScriptOrigin. I found some bugs in the way we return
ScriptOrigin
> values and it just seems to me that there is a lot of redundancy. ScriptOrigin
> was intended to pass a set of values to V8, not return from it.

The way we use script metadata from Script/ScriptOrigin in DevTools debugger
doesn't require it to be stored in a JSObject and to be accessible from
javascript. Access that data in DebuggerScript.js [1] just to read the fields
later and serialize them as a C++ structure [2]. I don't see why we cannot pass
v8::Script or something that would expose accessors to the data stored in Script
object internally. That would break debug-debugger.js but if we want to get away
from it towards a better defined C++ API that would be right direction.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Yang, 2015-01-28 14:02:05]

We should rework this API, but this is not something for today, and I don't want
to block your progress. I have some comments. And there need to be test cases
for this (just add a couple of CHECKs to test-api.cc).

https://codereview.chromium.org/879553002/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/879553002/diff/60001/include/v8.h#newcode980
include/v8.h:980: Handle<Boolean> resource_is_debugger_script =
Handle<Boolean>())
Can we call this (and elsewhere) something more generic like
resource_is_embedder_script?

https://codereview.chromium.org/879553002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/879553002/diff/60001/src/api.cc#newcode1924
src/api.cc:1924: v8::Boolean::New(v8_isolate, script->is_shared_cross_origin()),
This is great. I noticed that this is missing and would have cause a lot of head
scratching. Can you add a check to check_message_3 in test-api.cc?

  CHECK(message->GetScriptOrigin().ResourceIsSharedCrossOrigin());

https://codereview.chromium.org/879553002/diff/60001/src/api.cc#newcode4096
src/api.cc:4096: v8::Integer::New(isolate, script->column_offset()->value()));
Can we fix this place as well?

[kozy, 2015-01-29 10:52:56]

Thanks!

https://codereview.chromium.org/879553002/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/879553002/diff/60001/include/v8.h#newcode980
include/v8.h:980: Handle<Boolean> resource_is_debugger_script =
Handle<Boolean>())
On 2015/01/28 14:02:04, Yang wrote:
> Can we call this (and elsewhere) something more generic like
> resource_is_embedder_script?

Renamed to is_embedder_debug_script.

https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc#newcode794
src/accessors.cc:794: void Accessors::ScriptIsDebuggerScriptGetter(
On 2015/01/28 09:47:30, Yang wrote:
> Why is this necessary? I don't see it for the is_shared_cross_origin flag.

We use this variable in our DebuggerScript.js in blink. We need to have this
accessor for receiving script information after compile.

https://codereview.chromium.org/879553002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/879553002/diff/60001/src/api.cc#newcode1924
src/api.cc:1924: v8::Boolean::New(v8_isolate, script->is_shared_cross_origin()),
On 2015/01/28 14:02:04, Yang wrote:
> This is great. I noticed that this is missing and would have cause a lot of
head
> scratching. Can you add a check to check_message_3 in test-api.cc?
> 
>   CHECK(message->GetScriptOrigin().ResourceIsSharedCrossOrigin());

Done.

https://codereview.chromium.org/879553002/diff/60001/src/api.cc#newcode4096
src/api.cc:4096: v8::Integer::New(isolate, script->column_offset()->value()));
On 2015/01/28 14:02:04, Yang wrote:
> Can we fix this place as well?

Done.
And checks added to test-api.cc

[Yang, 2015-01-29 13:58:04]

On 2015/01/29 10:52:56, kozyatinskiy wrote:
> Thanks!
> 
> https://codereview.chromium.org/879553002/diff/60001/include/v8.h
> File include/v8.h (right):
> 
> https://codereview.chromium.org/879553002/diff/60001/include/v8.h#newcode980
> include/v8.h:980: Handle<Boolean> resource_is_debugger_script =
> Handle<Boolean>())
> On 2015/01/28 14:02:04, Yang wrote:
> > Can we call this (and elsewhere) something more generic like
> > resource_is_embedder_script?
> 
> Renamed to is_embedder_debug_script.
> 
> https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc
> File src/accessors.cc (right):
> 
>
https://codereview.chromium.org/879553002/diff/60001/src/accessors.cc#newcode794
> src/accessors.cc:794: void Accessors::ScriptIsDebuggerScriptGetter(
> On 2015/01/28 09:47:30, Yang wrote:
> > Why is this necessary? I don't see it for the is_shared_cross_origin flag.
> 
> We use this variable in our DebuggerScript.js in blink. We need to have this
> accessor for receiving script information after compile.
> 
> https://codereview.chromium.org/879553002/diff/60001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/879553002/diff/60001/src/api.cc#newcode1924
> src/api.cc:1924: v8::Boolean::New(v8_isolate,
script->is_shared_cross_origin()),
> On 2015/01/28 14:02:04, Yang wrote:
> > This is great. I noticed that this is missing and would have cause a lot of
> head
> > scratching. Can you add a check to check_message_3 in test-api.cc?
> > 
> >   CHECK(message->GetScriptOrigin().ResourceIsSharedCrossOrigin());
> 
> Done.
> 
> https://codereview.chromium.org/879553002/diff/60001/src/api.cc#newcode4096
> src/api.cc:4096: v8::Integer::New(isolate, script->column_offset()->value()));
> On 2015/01/28 14:02:04, Yang wrote:
> > Can we fix this place as well?
> 
> Done.
> And checks added to test-api.cc

lgtm.

[kozy, 2015-01-29 13:58:27]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2015-01-29 13:59:27]

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

[commit-bot: I haz the power, 2015-01-29 14:01:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-29 14:01:44]

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