Add functions to list library and script urls in an isolate.

Add functions to list library and script urls in an isolate.
Committed: https://code.google.com/p/dart/source/detail?r=3571

[siva, 2012-01-24 18:37:13]

LGTM with some comments.

http://codereview.chromium.org/9117015/diff/2010/runtime/include/dart_debugge...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:44: * Returns a a string containing the
source code of the given script
a a

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl.cc
File runtime/vm/debugger_api_impl.cc (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:266: intptr_t num_scripts =
loaded_scripts.Length();
Move this line up and using num_scripts for creating the list?

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:270: const String& url =
String::Handle(script.url());
This handle could be allocated outside the loop.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:271: Dart_ListSetAt(script_list, i,
Api::NewLocalHandle(url));
Do we have to create the List using the API functions?
We could just create the list as Array::New(...)
set all the elements using SetAt(...) would avoid the need of doing a
Api::NewLocalHandle for each url.

At the end of the loop we could return as
return Api::NewLocalHandle(...);

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:296: const String& lib_url =
String::Handle(lib.url());
This handle could be created outside the loop?

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:297: Dart_ListSetAt(library_list, i,
Api::NewLocalHandle(lib_url));
Ditto comment about creating the list using API functions instead of using
Array::New(...).

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:206: printf("Lib %d: %s\n", i, chars);
instead of printf would it make sense to do an EXPECT_STREQ(..)

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:217: printf("  script %d: '%s'\n", i + 1,
chars);
Ditto.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4051: }
Should this be an assertion that loaded_scripts should always be null over here?

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4064: // cached in loaded_scripts_.
Should we assert that this should be called only after class finalization is
done.

[hausner, 2012-01-24 21:34:42]

Thank you.

http://codereview.chromium.org/9117015/diff/2010/runtime/include/dart_debugge...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:44: * Returns a a string containing the
source code of the given script
On 2012/01/24 18:37:14, asiva wrote:
> a a

Done.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl.cc
File runtime/vm/debugger_api_impl.cc (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:266: intptr_t num_scripts =
loaded_scripts.Length();
On 2012/01/24 18:37:14, asiva wrote:
> Move this line up and using num_scripts for creating the list?

Done.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:270: const String& url =
String::Handle(script.url());
On 2012/01/24 18:37:14, asiva wrote:
> This handle could be allocated outside the loop.
Ah yes.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:271: Dart_ListSetAt(script_list, i,
Api::NewLocalHandle(url));
On 2012/01/24 18:37:14, asiva wrote:
> Do we have to create the List using the API functions?
> We could just create the list as Array::New(...)
> set all the elements using SetAt(...) would avoid the need of doing a
> Api::NewLocalHandle for each url.
> 
> At the end of the loop we could return as
> return Api::NewLocalHandle(...);
Seems to work. Good tip.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:296: const String& lib_url =
String::Handle(lib.url());
On 2012/01/24 18:37:14, asiva wrote:
> This handle could be created outside the loop?

Done.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl.cc:297: Dart_ListSetAt(library_list, i,
Api::NewLocalHandle(lib_url));
On 2012/01/24 18:37:14, asiva wrote:
> Ditto comment about creating the list using API functions instead of using
> Array::New(...).

Done.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:206: printf("Lib %d: %s\n", i, chars);
On 2012/01/24 18:37:14, asiva wrote:
> instead of printf would it make sense to do an EXPECT_STREQ(..)

Yes, if I knew what the expected library names are, and in which order they get
enumerated. I think the test would become too fragile if I baked in these
details.  As it is now, I can use this test as a sanity check by running it
manually and checking the output.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/debugger_api_impl...
runtime/vm/debugger_api_impl_test.cc:217: printf("  script %d: '%s'\n", i + 1,
chars);
On 2012/01/24 18:37:14, asiva wrote:
> Ditto.

Ditto regarding script urls.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4051: }
On 2012/01/24 18:37:14, asiva wrote:
> Should this be an assertion that loaded_scripts should always be null over
here?
See comment below.

http://codereview.chromium.org/9117015/diff/2010/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4064: // cached in loaded_scripts_.
On 2012/01/24 18:37:14, asiva wrote:
> Should we assert that this should be called only after class finalization is
> done.

I had an assert at first that made sure this is only called when the library is
in load_state_ == RawLibrary::kLoaded. The assert fired for the core libraries,
though.

Rather than figuring out why the core libs are not considered kLoaded, I made
this function more flexible by invalidating the cache whenever anything is
loaded. In all practical sense, I never expect this cache to be invalidated
though.