DevTools: CPUProfiler: provide url for scripts that have sourceURL property.

DevTools: CPUProfiler: provide url for scripts that have sourceURL property.


BUG=none
TEST=SourceURLSupportForNewFunctions, LogExistingFunctionSourceURLCheck
R=jkummerow@chromium.org, yurys@chromium.org

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

[yurys, 2013-06-11 10:14:39]

Please provide a test for this.

[loislo, 2013-06-11 11:03:54]

On 2013/06/11 10:14:39, Yury Semikhatsky wrote:
> Please provide a test for this.

done

[yurys, 2013-06-11 11:23:06]

https://codereview.chromium.org/16035027/diff/7001/src/handles.cc
File src/handles.cc (right):

https://codereview.chromium.org/16035027/diff/7001/src/handles.cc#newcode369
src/handles.cc:369: if (!isolate->IsInitialized()) {
Can we move this check to Logger::LogExistingFunction or to
GetScriptNameOrSourceURL? Logger::LogExistingFunction should be the only place
where isolate is not initialized and we are trying to create a script wrapper.

BTW, how can it happen that we have Script but no initialized Isolate?

https://codereview.chromium.org/16035027/diff/7001/src/handles.cc#newcode614
src/handles.cc:614: if (property->IsJSFunction()) {
How can it happen? We should be checking for is_bootstrapping or something
instead I think.

https://codereview.chromium.org/16035027/diff/7001/test/cctest/test-cpu-profi...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/16035027/diff/7001/test/cctest/test-cpu-profi...
test/cctest/test-cpu-profiler.cc:563:
CHECK_EQ(v8::String::New("cpu_profiler_test_source.js"),
I'd rather have it tested in a separate test.

[loislo, 2013-06-11 12:02:33]

On 2013/06/11 11:23:06, Yury Semikhatsky wrote:
> https://codereview.chromium.org/16035027/diff/7001/src/handles.cc
> File src/handles.cc (right):
> 
> https://codereview.chromium.org/16035027/diff/7001/src/handles.cc#newcode369
> src/handles.cc:369: if (!isolate->IsInitialized()) {
> Can we move this check to Logger::LogExistingFunction or to
> GetScriptNameOrSourceURL? Logger::LogExistingFunction should be the only place
> where isolate is not initialized and we are trying to create a script wrapper.
> 
> BTW, how can it happen that we have Script but no initialized Isolate?
> 
> https://codereview.chromium.org/16035027/diff/7001/src/handles.cc#newcode614
> src/handles.cc:614: if (property->IsJSFunction()) {
> How can it happen? We should be checking for is_bootstrapping or something
> instead I think.

The only thing we have to check is the isolate->IsInitialized()
We can do that in GetScriptNameOrSourceURL.
> 
>
https://codereview.chromium.org/16035027/diff/7001/test/cctest/test-cpu-profi...
> File test/cctest/test-cpu-profiler.cc (right):
> 
>
https://codereview.chromium.org/16035027/diff/7001/test/cctest/test-cpu-profi...
> test/cctest/test-cpu-profiler.cc:563:
> CHECK_EQ(v8::String::New("cpu_profiler_test_source.js"),
> I'd rather have it tested in a separate test.

extracted

[loislo, 2013-06-11 14:07:32]

On 2013/06/11 12:02:33, loislo wrote:
> On 2013/06/11 11:23:06, Yury Semikhatsky wrote:
> > https://codereview.chromium.org/16035027/diff/7001/src/handles.cc
> > File src/handles.cc (right):
> > 
> > https://codereview.chromium.org/16035027/diff/7001/src/handles.cc#newcode369
> > src/handles.cc:369: if (!isolate->IsInitialized()) {
> > Can we move this check to Logger::LogExistingFunction or to
> > GetScriptNameOrSourceURL? Logger::LogExistingFunction should be the only
place
> > where isolate is not initialized and we are trying to create a script
wrapper.
> > 
> > BTW, how can it happen that we have Script but no initialized Isolate?
> > 
> > https://codereview.chromium.org/16035027/diff/7001/src/handles.cc#newcode614
> > src/handles.cc:614: if (property->IsJSFunction()) {
> > How can it happen? We should be checking for is_bootstrapping or something
> > instead I think.
> 
> The only thing we have to check is the isolate->IsInitialized()
> We can do that in GetScriptNameOrSourceURL.
> > 
> >
>
https://codereview.chromium.org/16035027/diff/7001/test/cctest/test-cpu-profi...
> > File test/cctest/test-cpu-profiler.cc (right):
> > 
> >
>
https://codereview.chromium.org/16035027/diff/7001/test/cctest/test-cpu-profi...
> > test/cctest/test-cpu-profiler.cc:563:
> > CHECK_EQ(v8::String::New("cpu_profiler_test_source.js"),
> > I'd rather have it tested in a separate test.
> 
> extracted

sourceURL support also added to the compiler

[vsevik, 2013-06-11 14:26:01]

https://codereview.chromium.org/16035027/diff/17001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/16035027/diff/17001/src/compiler.cc#newcode1188
src/compiler.cc:1188: Handle<Object> name = GetScriptNameOrSourceURL(script);
Can you just do GetScriptNameOrSourceURL here?
What if we support sourceURLs for inline scripts at some point?

[yurys, 2013-06-11 14:40:38]

lgtm

https://codereview.chromium.org/16035027/diff/17001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/16035027/diff/17001/src/log.cc#newcode1742
src/log.cc:1742: if (line_num >= 0) {
Can you change this to line_num != -1 ?

[loislo, 2013-06-11 14:58:30]

On 2013/06/11 14:40:38, Yury Semikhatsky wrote:
> lgtm
> 
> https://codereview.chromium.org/16035027/diff/17001/src/log.cc
> File src/log.cc (right):
> 
> https://codereview.chromium.org/16035027/diff/17001/src/log.cc#newcode1742
> src/log.cc:1742: if (line_num >= 0) {
> Can you change this to line_num != -1 ?

done

[Jakob Kummerow, 2013-06-11 15:07:31]

lgtm

[loislo, 2013-06-12 08:27:34]

Committed patchset #6 manually as r15074 (presubmit successful).