Introduce library ids and info in the wire protocol

Introduce library ids and info in the wire protocol

- Library hierarchy (imports, prefixes)
- Global (top-level) variables
- Removing breakpoints
Committed: https://code.google.com/p/dart/source/detail?r=8075

[hausner, 2012-05-25 22:21:14]

cc'ing  devon and keerti

[hausner, 2012-05-29 16:26:41]

Friendly ping.

[siva, 2012-05-29 18:07:53]

lgtm

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc
File runtime/bin/dbg_connection.cc (right):

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:300: Dart_ListLength(lib_ids, &num_libs);
ASSERT return value is not an error?

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:312: (i == 0) ? "" : ", ", lib_id, chars);
maybe use GetStringChars(lib_url) here instead of 'chars' and remove the 3 lines
above msg.Printf...

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:386: intptr_t lib_id) {
indentation?

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:526: msg.Printf("{ \"id\": %d }", msg_id);
Shouldn't the reply have an Error reply so that the caller knows if the
breakpoint was deleted.
e.g: an invalid breakpoint id is passed in due to a typo or bug, the tool will
assume the breakpoint was deleted.

http://codereview.chromium.org/10447045/diff/1/runtime/include/dart_debugger_...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/10447045/diff/1/runtime/include/dart_debugger_...
runtime/include/dart_debugger_api.h:421: * in which case the respective library
is imported without a prefix.
which indicates the respective library has been imported without a prefix.

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

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:382: const Library& lib =
Library::Handle(Library::GetLibrary(library_id));
You could use Library::Handle(isolate, ....) here as you already have the
isolate.

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:438: const Library& lib =
Library::Handle(cls.library());
ditto comment about using Library::Handle(isolate,...)

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:523: ASSERT(!lib.IsNull());
ASSERT(Smi::IsValid(lib.index()));

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:552: ASSERT(!imported.IsNull());
ASSERT(Smi::IsValid(imported.index()));

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

http://codereview.chromium.org/10447045/diff/1/runtime/vm/object.cc#newcode5553
runtime/vm/object.cc:5553: set_index(libs.Length());
This is probably ok for now, but there is a proposal to use indirection after
the array has grown to a certain size. We should probably add a GetNextIndex or
some such method to GrowableObjectArray and use that here, so that we don't
forget about this implicit assumption of the length being the index here when
somebody goes and restructures GrowableObjectArray.

http://codereview.chromium.org/10447045/diff/1/tools/ddbg.dart
File tools/ddbg.dart (right):

http://codereview.chromium.org/10447045/diff/1/tools/ddbg.dart#newcode35
tools/ddbg.dart:35: sbp [<file>] <line> Set breakpoint
commands 'rbp' and 'pl' need to be added to this list.

[hausner, 2012-05-29 19:48:18]

Thanks.

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc
File runtime/bin/dbg_connection.cc (right):

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:300: Dart_ListLength(lib_ids, &num_libs);
On 2012/05/29 18:07:53, asiva wrote:
> ASSERT return value is not an error?

Done.

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:312: (i == 0) ? "" : ", ", lib_id, chars);
Yes. 
On 2012/05/29 18:07:53, asiva wrote:
> maybe use GetStringChars(lib_url) here instead of 'chars' and remove the 3
lines
> above msg.Printf...

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:386: intptr_t lib_id) {
On 2012/05/29 18:07:53, asiva wrote:
> indentation?

Done.

http://codereview.chromium.org/10447045/diff/1/runtime/bin/dbg_connection.cc#...
runtime/bin/dbg_connection.cc:526: msg.Printf("{ \"id\": %d }", msg_id);
On 2012/05/29 18:07:53, asiva wrote:
> Shouldn't the reply have an Error reply so that the caller knows if the
> breakpoint was deleted.
> e.g: an invalid breakpoint id is passed in due to a typo or bug, the tool will
> assume the breakpoint was deleted.

Done.

http://codereview.chromium.org/10447045/diff/1/runtime/include/dart_debugger_...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/10447045/diff/1/runtime/include/dart_debugger_...
runtime/include/dart_debugger_api.h:421: * in which case the respective library
is imported without a prefix.
On 2012/05/29 18:07:53, asiva wrote:
> which indicates the respective library has been imported without a prefix.

Done.

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

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:382: const Library& lib =
Library::Handle(Library::GetLibrary(library_id));
On 2012/05/29 18:07:53, asiva wrote:
> You could use Library::Handle(isolate, ....) here as you already have the
> isolate.

Done.

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:438: const Library& lib =
Library::Handle(cls.library());
On 2012/05/29 18:07:53, asiva wrote:
> ditto comment about using Library::Handle(isolate,...)

Done.

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:523: ASSERT(!lib.IsNull());
On 2012/05/29 18:07:53, asiva wrote:
> ASSERT(Smi::IsValid(lib.index()));

Done.

http://codereview.chromium.org/10447045/diff/1/runtime/vm/debugger_api_impl.c...
runtime/vm/debugger_api_impl.cc:552: ASSERT(!imported.IsNull());
On 2012/05/29 18:07:53, asiva wrote:
> ASSERT(Smi::IsValid(imported.index()));

Done.

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

http://codereview.chromium.org/10447045/diff/1/runtime/vm/object.cc#newcode5553
runtime/vm/object.cc:5553: set_index(libs.Length());
As we discussed offline, the code and its assumptions is fine.

On 2012/05/29 18:07:53, asiva wrote:
> This is probably ok for now, but there is a proposal to use indirection after
> the array has grown to a certain size. We should probably add a GetNextIndex
or
> some such method to GrowableObjectArray and use that here, so that we don't
> forget about this implicit assumption of the length being the index here when
> somebody goes and restructures GrowableObjectArray.

http://codereview.chromium.org/10447045/diff/1/tools/ddbg.dart
File tools/ddbg.dart (right):

http://codereview.chromium.org/10447045/diff/1/tools/ddbg.dart#newcode35
tools/ddbg.dart:35: sbp [<file>] <line> Set breakpoint
On 2012/05/29 18:07:53, asiva wrote:
> commands 'rbp' and 'pl' need to be added to this list.

Done.