StackTrace should provide access to //@ sourceURL=... value

StackTrace should provide access to //@ sourceURL=... value

Committed: http://code.google.com/p/v8/source/detail?r=5586

[mnaganov (inactive), 2010-10-05 08:00:58]

LGTM

http://codereview.chromium.org/3602013/diff/5001/6004
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/3602013/diff/5001/6004#newcode10576
test/cctest/test-api.cc:10576: v8::Handle<v8::String> origin =
v8::String::New("capture-stack-trace-test");
nit: This also could be v8_str

[yurys, 2010-10-05 08:31:30]

http://codereview.chromium.org/3602013/diff/5001/6004
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/3602013/diff/5001/6004#newcode10576
test/cctest/test-api.cc:10576: v8::Handle<v8::String> origin =
v8::String::New("capture-stack-trace-test");
On 2010/10/05 08:00:58, Michail Naganov wrote:
> nit: This also could be v8_str

Removed the line. origin isn't used in this test anyway.

[Mads Ager (chromium), 2010-10-05 08:37:19]

LGTM

http://codereview.chromium.org/3602013/diff/13001/14001
File include/v8.h (right):

http://codereview.chromium.org/3602013/diff/13001/14001#newcode825
include/v8.h:825: * is undefined and it source ends with //@ sourceURL=...
string.
it -> its

http://codereview.chromium.org/3602013/diff/13001/14003
File src/top.cc (right):

http://codereview.chromium.org/3602013/diff/13001/14003#newcode399
src/top.cc:399: if (caught_exception)
Please make this a one-liner or wrap in braces.

[yurys, 2010-10-05 08:41:17]

http://codereview.chromium.org/3602013/diff/13001/14001
File include/v8.h (right):

http://codereview.chromium.org/3602013/diff/13001/14001#newcode825
include/v8.h:825: * is undefined and it source ends with //@ sourceURL=...
string.
On 2010/10/05 08:37:20, Mads Ager wrote:
> it -> its

Done.

http://codereview.chromium.org/3602013/diff/13001/14003
File src/top.cc (right):

http://codereview.chromium.org/3602013/diff/13001/14003#newcode399
src/top.cc:399: if (caught_exception)
On 2010/10/05 08:37:20, Mads Ager wrote:
> Please make this a one-liner or wrap in braces.

Done.

[Vitaly Repeshko, 2010-10-05 10:21:43]

LGTM

http://codereview.chromium.org/3602013/diff/21001/22003
File src/top.cc (right):

http://codereview.chromium.org/3602013/diff/21001/22003#newcode390
src/top.cc:390: Handle<String> method_name =
To be consistent with existing column_key, line_key, etc. you might want to
extract the symbol lookup out of the loop.

http://codereview.chromium.org/3602013/diff/21001/22003#newcode392
src/top.cc:392: Handle<JSValue> script_wrapper =
GetScriptWrapper(Handle<Script>(script));
Hmm, the script handle should have been created much earlier. Most other locals
are handles, so we risk using an old raw pointer here.

[yurys, 2010-10-05 12:35:26]

The patch is already landed. I'll address these in a separate change.

On 2010/10/05 10:21:43, Vitaly wrote:
> LGTM
> 
> http://codereview.chromium.org/3602013/diff/21001/22003
> File src/top.cc (right):
> 
> http://codereview.chromium.org/3602013/diff/21001/22003#newcode390
> src/top.cc:390: Handle<String> method_name =
> To be consistent with existing column_key, line_key, etc. you might want to
> extract the symbol lookup out of the loop.
> 
> http://codereview.chromium.org/3602013/diff/21001/22003#newcode392
> src/top.cc:392: Handle<JSValue> script_wrapper =
> GetScriptWrapper(Handle<Script>(script));
> Hmm, the script handle should have been created much earlier. Most other
locals
> are handles, so we risk using an old raw pointer here.