Add v8::Message::GetScriptOrigin() with tests

Added Message::GetScripOrigin.
Replaced Message::GetResourceName with GetScriptOrigin().ResourceName().

Now, GetScriptOrigin().ResourceName() function returns the resource name or sourceURL (from //# sourceURL=) for the script from where the function causing the error originates.

Method GetScriptResourceName() deprecated. Use GetScriptOrigin()->ResourceName() instead.

Function used in Blink: https://codereview.chromium.org/260513004/

R=yangguo@chromium.org, yurys@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21893

[vsevik, 2014-04-30 10:18:40]

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

https://codereview.chromium.org/265593002/diff/40001/include/v8.h#newcode1134
include/v8.h:1134: * Returns the resource name for the script from where the
function causing
Returns the resource name or sourceURL for the script from where the function
causing the error originates.

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

https://codereview.chromium.org/265593002/diff/40001/src/api.cc#newcode1946
src/api.cc:1946: // Return this.script.name.
This comment seems incorrect.

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

https://codereview.chromium.org/265593002/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17691: "//# sourceURL=foo2.js\";\n"
could you please break this string (e.g.
"//# source"
"URL=foo2.js\";\n"
To make sure we don't have any possible false sourceURL matches.

https://codereview.chromium.org/265593002/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17699: v8::String::Utf8Value
stack(try_catch.StackTrace());
StackTrace does not use your new method, so this test is not actually testing
it. (Try running it without your patch applied)
You should check that
 try_catch->Message()->GetScriptResourceNameOrSourceURL(); 
returns correct value instead.

Also could you please add a sourceURL comment (with different URL) to the whole
script as well so that we could clearly see that the value returned matches the
inner script.

https://codereview.chromium.org/265593002/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17719: v8::String::Utf8Value
stack(try_catch.StackTrace());
ditto

[kozyatinskiy1, 2014-04-30 15:46:42]

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

https://codereview.chromium.org/265593002/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17691: "//# sourceURL=foo2.js\";\n"
On 2014/04/30 10:18:40, vsevik wrote:
> could you please break this string (e.g.
> "//# source"
> "URL=foo2.js\";\n"
> To make sure we don't have any possible false sourceURL matches.

Done.

https://codereview.chromium.org/265593002/diff/40001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:17719: v8::String::Utf8Value
stack(try_catch.StackTrace());
On 2014/04/30 10:18:40, vsevik wrote:
> ditto

Done.

[vsevik, 2014-05-05 07:21:47]

Yury, could you please take a look?

[yurys, 2014-05-05 07:30:40]

https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h
File include/v8.h (right):

https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h#newc...
include/v8.h:1137: Handle<Value> GetScriptResourceNameOrSourceURL() const;
Would it make sense to expose GetUboundScript() method on Message object and
have script-specific methods right on UboundScript object instead of repeating
them in Message and other places in the API?

[kozyatinskiy1, 2014-05-15 08:24:58]

On 2014/05/05 07:30:40, yurys wrote:
> https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h
> File include/v8.h (right):
> 
>
https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h#newc...
> include/v8.h:1137: Handle<Value> GetScriptResourceNameOrSourceURL() const;
> Would it make sense to expose GetUboundScript() method on Message object and
> have script-specific methods right on UboundScript object instead of repeating
> them in Message and other places in the API?

UnboundScript is API wrapper under SharedFunctionInfo. There are cases where
it's impossible to get SharedFunctionInfo object in Message. For example:
message about SyntaxError.

[yurys, 2014-05-15 12:45:52]

On 2014/05/15 08:24:58, kozyatinskiy wrote:
> On 2014/05/05 07:30:40, yurys wrote:
> > https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h
> > File include/v8.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/265593002/diff/90001/include/v8.h#newc...
> > include/v8.h:1137: Handle<Value> GetScriptResourceNameOrSourceURL() const;
> > Would it make sense to expose GetUboundScript() method on Message object and
> > have script-specific methods right on UboundScript object instead of
repeating
> > them in Message and other places in the API?
> 
> UnboundScript is API wrapper under SharedFunctionInfo. There are cases where
> it's impossible to get SharedFunctionInfo object in Message. For example:
> message about SyntaxError.

Yeah, you are right. We discussed this with Vsevolod and it seems that what we
actually need in debugger is a public API to v8::internal::Script object that
contains all the data.

[vsevik, 2014-06-04 14:28:16]

> Yeah, you are right. We discussed this with Vsevolod and it seems that what we
> actually need in debugger is a public API to v8::internal::Script object that
> contains all the data.
After discussion with the V8 team it seems that there is no plans to change API
to support this case.
Do you think we could land this patch as is?

[yurys, 2014-06-05 08:53:03]

On 2014/06/04 14:28:16, vsevik wrote:
> > Yeah, you are right. We discussed this with Vsevolod and it seems that what
we
> > actually need in debugger is a public API to v8::internal::Script object
that
> > contains all the data.
> After discussion with the V8 team it seems that there is no plans to change
API
> to support this case.
> Do you think we could land this patch as is?

Yes, we should go ahead with this approach then. LGTM.

[yurys, 2014-06-05 09:11:20]

Yang, could you do OWNER review?

[Yang, 2014-06-05 10:40:43]

On 2014/06/05 09:11:20, yurys wrote:
> Yang, could you do OWNER review?

Now that I looked closer at this API, I see (if I'm not mistaken) two ways to
get the script name of where the exception was thrown:
- Through the script in the Message object, like exposed in this CL.
- Through the stack trace in the Message object. The top most function in the
stack trace can be used for Function::GetScriptOrigin, which gives you a
ScriptOrigin object, containing the script name or URL.

I wonder if we even need the first option. If not, we could even rip out the
script from the Message object altogether and just always use the second option
to get to the script.

[vsevik, 2014-06-05 11:44:03]

> Now that I looked closer at this API, I see (if I'm not mistaken) two ways to
> get the script name of where the exception was thrown:
> - Through the script in the Message object, like exposed in this CL.
> - Through the stack trace in the Message object. The top most function in the
> stack trace can be used for Function::GetScriptOrigin, which gives you a
> ScriptOrigin object, containing the script name or URL.
> 
> I wonder if we even need the first option. If not, we could even rip out the
> script from the Message object altogether and just always use the second
option
> to get to the script.

I agree that it would be great to remove redundant data from the Message object,
but I think there is a problem here.
As I mentioned in the e-mail thread previously we are working on adding better
syntax error handling here.
In case of SyntaxError the stack trace is either empty or has an eval() call on
its topmost frame.
The script where the original error happened and the position of the error in
this script is therefore not accessible from the stack trace which is why we
need to have it separately.

[Yang, 2014-06-05 11:52:37]

On 2014/06/05 11:44:03, vsevik wrote:
> > Now that I looked closer at this API, I see (if I'm not mistaken) two ways
to
> > get the script name of where the exception was thrown:
> > - Through the script in the Message object, like exposed in this CL.
> > - Through the stack trace in the Message object. The top most function in
the
> > stack trace can be used for Function::GetScriptOrigin, which gives you a
> > ScriptOrigin object, containing the script name or URL.
> > 
> > I wonder if we even need the first option. If not, we could even rip out the
> > script from the Message object altogether and just always use the second
> option
> > to get to the script.
> 
> I agree that it would be great to remove redundant data from the Message
object,
> but I think there is a problem here.
> As I mentioned in the e-mail thread previously we are working on adding better
> syntax error handling here.
> In case of SyntaxError the stack trace is either empty or has an eval() call
on
> its topmost frame.
> The script where the original error happened and the position of the error in
> this script is therefore not accessible from the stack trace which is why we
> need to have it separately.

Alright. Nevertheless, wouldn't it be nicer if the new method we expose here is
similar to Function::GetScriptOrigin, that gives a ScriptOrigin object that
contains all information including line numbers etc, so that we can phase out
the corresponding Message::Get* and Message::IsSharedCrossOrigin? I see that
ScriptOrigin doesn't have an equivalent to Message::Get{Start,End}Position and
Message::GetEndColumn, but the former is not used anywhere afaict, and the
latter can be added to ScriptOrigin. That would be more consistent IMO.

[kozyatinskiy1, 2014-06-11 16:19:11]

On 2014/06/05 11:52:37, Yang wrote:
> On 2014/06/05 11:44:03, vsevik wrote:
> > > Now that I looked closer at this API, I see (if I'm not mistaken) two ways
> to
> > > get the script name of where the exception was thrown:
> > > - Through the script in the Message object, like exposed in this CL.
> > > - Through the stack trace in the Message object. The top most function in
> the
> > > stack trace can be used for Function::GetScriptOrigin, which gives you a
> > > ScriptOrigin object, containing the script name or URL.
> > > 
> > > I wonder if we even need the first option. If not, we could even rip out
the
> > > script from the Message object altogether and just always use the second
> > option
> > > to get to the script.
> > 
> > I agree that it would be great to remove redundant data from the Message
> object,
> > but I think there is a problem here.
> > As I mentioned in the e-mail thread previously we are working on adding
better
> > syntax error handling here.
> > In case of SyntaxError the stack trace is either empty or has an eval() call
> on
> > its topmost frame.
> > The script where the original error happened and the position of the error
in
> > this script is therefore not accessible from the stack trace which is why we
> > need to have it separately.
> 
> Alright. Nevertheless, wouldn't it be nicer if the new method we expose here
is
> similar to Function::GetScriptOrigin, that gives a ScriptOrigin object that
> contains all information including line numbers etc, so that we can phase out
> the corresponding Message::Get* and Message::IsSharedCrossOrigin? I see that
> ScriptOrigin doesn't have an equivalent to Message::Get{Start,End}Position and
> Message::GetEndColumn, but the former is not used anywhere afaict, and the
> latter can be added to ScriptOrigin. That would be more consistent IMO.

We have exposed method Message::GetScriptOrigin, have replaced all
GetResourceName() in V8() to GetScriptOrigin().ResourceName() and have marked
GetResourceName as deprecated.
Yang, Could you have a look at last patch set?

[Yang, 2014-06-12 07:28:07]

On 2014/06/11 16:19:11, kozyatinskiy wrote:
> On 2014/06/05 11:52:37, Yang wrote:
> > On 2014/06/05 11:44:03, vsevik wrote:
> > > > Now that I looked closer at this API, I see (if I'm not mistaken) two
ways
> > to
> > > > get the script name of where the exception was thrown:
> > > > - Through the script in the Message object, like exposed in this CL.
> > > > - Through the stack trace in the Message object. The top most function
in
> > the
> > > > stack trace can be used for Function::GetScriptOrigin, which gives you a
> > > > ScriptOrigin object, containing the script name or URL.
> > > > 
> > > > I wonder if we even need the first option. If not, we could even rip out
> the
> > > > script from the Message object altogether and just always use the second
> > > option
> > > > to get to the script.
> > > 
> > > I agree that it would be great to remove redundant data from the Message
> > object,
> > > but I think there is a problem here.
> > > As I mentioned in the e-mail thread previously we are working on adding
> better
> > > syntax error handling here.
> > > In case of SyntaxError the stack trace is either empty or has an eval()
call
> > on
> > > its topmost frame.
> > > The script where the original error happened and the position of the error
> in
> > > this script is therefore not accessible from the stack trace which is why
we
> > > need to have it separately.
> > 
> > Alright. Nevertheless, wouldn't it be nicer if the new method we expose here
> is
> > similar to Function::GetScriptOrigin, that gives a ScriptOrigin object that
> > contains all information including line numbers etc, so that we can phase
out
> > the corresponding Message::Get* and Message::IsSharedCrossOrigin? I see that
> > ScriptOrigin doesn't have an equivalent to Message::Get{Start,End}Position
and
> > Message::GetEndColumn, but the former is not used anywhere afaict, and the
> > latter can be added to ScriptOrigin. That would be more consistent IMO.
> 
> We have exposed method Message::GetScriptOrigin, have replaced all
> GetResourceName() in V8() to GetScriptOrigin().ResourceName() and have marked
> GetResourceName as deprecated.
> Yang, Could you have a look at last patch set?

LGTM. Thanks.

[kozyatinskiy1, 2014-06-16 09:23:18]

Please, don't commit this patch without next:
1. https://codereview.chromium.org/260513004/ - replaced GetScriptResourceName
in blink.
2. https://codereview.chromium.org/334203002/ - replaced GetScriptResourceName
in chrome and extensions.
3. https://codereview.chromium.org/331133004/ - replaced GetScriptResourceName
in skia.

[kozyatinskiy1, 2014-06-16 11:26:13]

On 2014/06/16 09:23:18, kozyatinskiy wrote:
> Please, don't commit this patch without next:
> 1. https://codereview.chromium.org/260513004/ - replaced GetScriptResourceName
> in blink.
> 2. https://codereview.chromium.org/334203002/ - replaced GetScriptResourceName
> in chrome and extensions.
> 3. https://codereview.chromium.org/331133004/ - replaced GetScriptResourceName
> in skia.

I removed V8_DEPRECATED macro, otherwise chromium build fails while patches from
previous message are not commit.
I will remove method v8::Message::GetScriptResourceName later.

@yurys, Could you, please, land the last patch set?

[yurys, 2014-06-16 12:40:53]

lgtm

https://codereview.chromium.org/265593002/diff/180001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/265593002/diff/180001/src/api.cc#newcode1953
src/api.cc:1953: isolate));
style: wrong indentation.

https://codereview.chromium.org/265593002/diff/180001/src/api.cc#newcode1954
src/api.cc:1954: i::Handle<i::Script>
script(i::Script::cast(scriptValue->value()));
Why not i::Script::cast(message->script()) without intermediate cast to JSValue?

[kozyatinskiy1, 2014-06-16 16:10:08]

On 2014/06/16 12:40:53, yurys wrote:
> lgtm
> 
> https://codereview.chromium.org/265593002/diff/180001/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/265593002/diff/180001/src/api.cc#newcode1953
> src/api.cc:1953: isolate));
> style: wrong indentation.
Fixed.

> https://codereview.chromium.org/265593002/diff/180001/src/api.cc#newcode1954
> src/api.cc:1954: i::Handle<i::Script>
> script(i::Script::cast(scriptValue->value()));
> Why not i::Script::cast(message->script()) without intermediate cast to
JSValue?
Message object contains ScriptWrapper instead Script:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/objects.cc&....
ScriptWrapper is JSValue.

[yurys, 2014-06-20 07:44:15]

Committed patchset #9 manually as r21893 (presubmit successful).