Refactor VM service IDs

Refactor VM service IDs:

- IDs are strings which can be concatenated together like paths.
- Isolate ID is now "/isolates/XXX"
- Function, field, class, library, and script IDs are now stable.
- Introduce <type-ref> tags.

R=turnidge@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31358

[turnidge, 2013-12-19 20:39:29]

Ok, first pass done.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:57: }
Comment for why we are skipping these first 2 characters -- are they "#/"?  Then
something like "skip '#/'".

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:102:
return '#/$isolateId/$l';
Things are simpler now.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart:31:
setResponses(r);
So a request can generate a list of responses?  When does this happen?

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/class_ref.dart
(left):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/class_ref.dart:15:
d++;
What's this stuff?

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/class_view.html
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/class_view.html:18:
<library-ref app="{{ app }}" ref="{{ cls['library'] }}"></library-ref>
Yes, this is nicer now.  Easier to read.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/class_view.html:49:
<td><instance-ref app="{{ app }}" instance="{{ field['value']
}}"></instance-ref></td>
Should this instance= become a ref= here?  Looking at instance-ref, there is no
longer an instance observable.  Seems fishy.

Ditto the field= in the field ref?

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/field_ref.html
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/field_ref.html:15:
</template>
I think maybe we should make the above a <class-ref>

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart:16:
return super.name;
What's this doing?  Can this go away after class-view starts passing in a ref=?

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_view.html
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_view.html:31:
<td><instance-ref app="{{ app }}" instance="{{ field['value']
}}"></instance-ref></td>
instance= & field= seems suspicious here.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/library_view.html
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/library_view.html:21:
<script-ref app="{{ app }}"ref="{{ script }}"></script-ref>
Missing space before 'ref'.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart:27:
print('$app $ref');
Do you mean to have this print here?

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart:32: }
So sometimes we build a script-ref from a 'ref' and sometimes from a
'scriptURL'.  Is this just because a frame doesn't return a ref yet?  If this is
temporary can we add a TODO to remove it?

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/service_ref.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/service_ref.dart:17:
print('service ref changed $ref');
There are a variety of these debugging prints in the cl.  I didn't tag the
others.  Do we want these checked in?

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/running...
File runtime/bin/vmservice/running_isolates.dart (right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/running...
runtime/bin/vmservice/running_isolates.dart:36: });
We may eventually want to re-sync the list with the vm.  Trying to think about
the case where a machine is browsing and goes to sleep, misses updates, then
wakes up.

Just thinking out loud.  No action required here.

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

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1822
runtime/vm/object.cc:1822: String& needle_name = String::Handle();
String::Handle(isolate) maybe?  Avoids a call to Isolate::Current().

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1880
runtime/vm/object.cc:1880: }
Do you want to use the ReusableHandleScope here for consistency?

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode2462
runtime/vm/object.cc:2462: String& needle_name = String::Handle();
String::Handle(isolate)

Do we run into this a lot?  It would be possible to add a StringHandle2 accessor
(hand-written) to ReusableHandleScope.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode5701
runtime/vm/object.cc:5701: Function& func = Function::Handle(raw());
Instead of:

Function::Handle(raw())

Can we get away with:

*this

?

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode5705
runtime/vm/object.cc:5705: closure = true;
There is an IsClosureFunction predicate in Function.  Would that work to
distinguish these cases without hunting through the functions array?

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode5713
runtime/vm/object.cc:5713: jsobj.AddPropertyF("id", "classes/%" Pd "/closures/%"
Pd "", cid, id);
For thought: I'm wondering if these format strings (or paths or whatever) should
be gathered in a central place somehow.  My idea is that the same string
constant could be used in service dispatching and in link construction so they
don't get out of sync.

For now, this works.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode6046
runtime/vm/object.cc:6046: intptr_t id =
cls.FindFieldIndex(Field::Handle(raw()));
*this

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode9872
runtime/vm/object.cc:9872: jsobj.AddProperty("user_name", "regular code");
Too bad we can't get a nice name here.  I wonder...

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:14443: return String::null();
Is this case UNREACHABLE or UNIMPLEMENTED?

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:15117: }
Is there a way to avoid the code duplication by having a helper function that
operates on a String?  Would it help if we weren't putting the characters into
the destination string directly but instead used a temp buffer?

For example, String::EscapedString (in this file) is able to use a
CodePointIterator and works for all String types.

I would rather not have two versions of the Encode/Decode here.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object_test.cc#new...
runtime/vm/object_test.cc:360: 
Add heartbeat (simple) tests for Find{Function/Closure/Field}Index.  Success &
error cases.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode263
runtime/vm/service.cc:263: PrintCollectionErrorResponse(collection, js);        
                     \
Why not just use PrintError (accepts format string) here instead?  I can see a
helper if it is used a lot, but only uses are here in the macro.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode343
runtime/vm/service.cc:343: PrintGenericError(js);
Use PrintError and give a nice message here.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode348
runtime/vm/service.cc:348: Object::null_object().PrintToJSONStream(js, false);
So null is what we give for an invalid reference?  Seems ambiguous, would prefer
Error.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode355
runtime/vm/service.cc:355: } else if (js->num_arguments() == 4) {
Maybe just check that num_arguments is >= 3 and let the helper function (e.g.
HandleClassesClosures) handle the Error case.  This will open the door for more
interesting things in the future like:

class/55/function/33/deoptimize

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode367
runtime/vm/service.cc:367: PrintGenericError(js);
This goes away if you let helpers handle sub-errors.

I tend to think we should avoid PrintGenericError.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode369
runtime/vm/service.cc:369: }
Here and below...

You should have my changes now which support C++ unit tests for service
handlers.  I think we should write C++ tests for these handlers which cover the
success and error cases.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode394
runtime/vm/service.cc:394: lib.PrintToJSONStream(js, false);
So do we not yet support Fields/Functions for libraries?  Maybe a TODO?

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode405
runtime/vm/service.cc:405: Object::null_object().PrintToJSONStream(js, false);
Again, null object seems ambiguous.  I might prefer Error.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode531
runtime/vm/service.cc:531: { "library", HandleLibrary },
Is the plan to have both library and libraries?  I thought we were talking about
one or the other.

[Cutch, 2013-12-19 21:53:58]

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:57: }
On 2013/12/19 20:39:29, turnidge wrote:
> Comment for why we are skipping these first 2 characters -- are they "#/"? 
Then
> something like "skip '#/'".

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart:31:
setResponses(r);
On 2013/12/19 20:39:29, turnidge wrote:
> So a request can generate a list of responses?  When does this happen?

Presently it doesn't but I thought it would be interesting to be able to send
back a list of 'type': 'Class' objects. The response-viewer <template repeats
over all of the responses and instantiates a message viewer for each one.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/class_ref.dart
(left):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/class_ref.dart:15:
d++;
On 2013/12/19 20:39:29, turnidge wrote:
> What's this stuff?

git does weird things for new files.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/class_view.html
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/class_view.html:49:
<td><instance-ref app="{{ app }}" instance="{{ field['value']
}}"></instance-ref></td>
On 2013/12/19 20:39:29, turnidge wrote:
> Should this instance= become a ref= here?  Looking at instance-ref, there is
no
> longer an instance observable.  Seems fishy.
> 
> Ditto the field= in the field ref?

Yes but I left that out because I didn't want to disturb your -ref tags.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/field_ref.html
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/field_ref.html:15:
</template>
On 2013/12/19 20:39:29, turnidge wrote:
> I think maybe we should make the above a <class-ref>

I didn't want to mess with field-ref or instance-ref

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart:16:
return super.name;
On 2013/12/19 20:39:29, turnidge wrote:
> What's this doing?  Can this go away after class-view starts passing in a
ref=?

ServiceRefElement provides a default implementation of name which will return
either the 'name' or 'user_name' keys. InstanceRef wants to use the object
preview here. Perhaps "name" is the wrong property name on ServiceRefElement.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/library_view.html
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/library_view.html:21:
<script-ref app="{{ app }}"ref="{{ script }}"></script-ref>
On 2013/12/19 20:39:29, turnidge wrote:
> Missing space before 'ref'.

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart:27:
print('$app $ref');
On 2013/12/19 20:39:29, turnidge wrote:
> Do you mean to have this print here?

Removed.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart:32: }
On 2013/12/19 20:39:29, turnidge wrote:
> So sometimes we build a script-ref from a 'ref' and sometimes from a
> 'scriptURL'.  Is this just because a frame doesn't return a ref yet?  If this
is
> temporary can we add a TODO to remove it?

Yes, it's just because 'frame' outputs a 'url' property instead of a 'script': {
'type': '@Script', ... }.

Added TODO.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
File runtime/bin/vmservice/client/lib/src/observatory_elements/service_ref.dart
(right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/client/...
runtime/bin/vmservice/client/lib/src/observatory_elements/service_ref.dart:17:
print('service ref changed $ref');
On 2013/12/19 20:39:29, turnidge wrote:
> There are a variety of these debugging prints in the cl.  I didn't tag the
> others.  Do we want these checked in?

Removed.

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/running...
File runtime/bin/vmservice/running_isolates.dart (right):

https://codereview.chromium.org/98253009/diff/1/runtime/bin/vmservice/running...
runtime/bin/vmservice/running_isolates.dart:36: });
On 2013/12/19 20:39:29, turnidge wrote:
> We may eventually want to re-sync the list with the vm.  Trying to think about
> the case where a machine is browsing and goes to sleep, misses updates, then
> wakes up.
> 
> Just thinking out loud.  No action required here.

Agreed.

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

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1822
runtime/vm/object.cc:1822: String& needle_name = String::Handle();
On 2013/12/19 20:39:29, turnidge wrote:
> String::Handle(isolate) maybe?  Avoids a call to Isolate::Current().

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1880
runtime/vm/object.cc:1880: }
On 2013/12/19 20:39:29, turnidge wrote:
> Do you want to use the ReusableHandleScope here for consistency?

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode2462
runtime/vm/object.cc:2462: String& needle_name = String::Handle();
On 2013/12/19 20:39:29, turnidge wrote:
> String::Handle(isolate)
> 
> Do we run into this a lot?  It would be possible to add a StringHandle2
accessor
> (hand-written) to ReusableHandleScope.

Depends on user activity. We hit this whenever we need to generate an ID for a
field.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode5705
runtime/vm/object.cc:5705: closure = true;
On 2013/12/19 20:39:29, turnidge wrote:
> There is an IsClosureFunction predicate in Function.  Would that work to
> distinguish these cases without hunting through the functions array?

Possibly, let me test.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode5713
runtime/vm/object.cc:5713: jsobj.AddPropertyF("id", "classes/%" Pd "/closures/%"
Pd "", cid, id);
On 2013/12/19 20:39:29, turnidge wrote:
> For thought: I'm wondering if these format strings (or paths or whatever)
should
> be gathered in a central place somehow.  My idea is that the same string
> constant could be used in service dispatching and in link construction so they
> don't get out of sync.
> 
> For now, this works.

Agreed.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode6046
runtime/vm/object.cc:6046: intptr_t id =
cls.FindFieldIndex(Field::Handle(raw()));
On 2013/12/19 20:39:29, turnidge wrote:
> *this

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode9872
runtime/vm/object.cc:9872: jsobj.AddProperty("user_name", "regular code");
On 2013/12/19 20:39:29, turnidge wrote:
> Too bad we can't get a nice name here.  I wonder...

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:14443: return String::null();
On 2013/12/19 20:39:29, turnidge wrote:
> Is this case UNREACHABLE or UNIMPLEMENTED?

Added UNREACHABLE();

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:15117: }
On 2013/12/19 20:39:29, turnidge wrote:
> Is there a way to avoid the code duplication by having a helper function that
> operates on a String?  Would it help if we weren't putting the characters into
> the destination string directly but instead used a temp buffer?
> 
> For example, String::EscapedString (in this file) is able to use a
> CodePointIterator and works for all String types.
> 
> I would rather not have two versions of the Encode/Decode here.

Percent encoding is only defined on OneByteStrings, let's discuss.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object_test.cc#new...
runtime/vm/object_test.cc:360: 
On 2013/12/19 20:39:29, turnidge wrote:
> Add heartbeat (simple) tests for Find{Function/Closure/Field}Index.  Success &
> error cases.

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode263
runtime/vm/service.cc:263: PrintCollectionErrorResponse(collection, js);        
                     \
On 2013/12/19 20:39:29, turnidge wrote:
> Why not just use PrintError (accepts format string) here instead?  I can see a
> helper if it is used a lot, but only uses are here in the macro.

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode343
runtime/vm/service.cc:343: PrintGenericError(js);
On 2013/12/19 20:39:29, turnidge wrote:
> Use PrintError and give a nice message here.

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode348
runtime/vm/service.cc:348: Object::null_object().PrintToJSONStream(js, false);
On 2013/12/19 20:39:29, turnidge wrote:
> So null is what we give for an invalid reference?  Seems ambiguous, would
prefer
> Error.

Legacy code, returning Error now.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode355
runtime/vm/service.cc:355: } else if (js->num_arguments() == 4) {
On 2013/12/19 20:39:29, turnidge wrote:
> Maybe just check that num_arguments is >= 3 and let the helper function (e.g.
> HandleClassesClosures) handle the Error case.  This will open the door for
more
> interesting things in the future like:
> 
> class/55/function/33/deoptimize
> 

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode367
runtime/vm/service.cc:367: PrintGenericError(js);
On 2013/12/19 20:39:29, turnidge wrote:
> This goes away if you let helpers handle sub-errors.
> 
> I tend to think we should avoid PrintGenericError.

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode369
runtime/vm/service.cc:369: }
On 2013/12/19 20:39:29, turnidge wrote:
> Here and below...
> 
> You should have my changes now which support C++ unit tests for service
> handlers.  I think we should write C++ tests for these handlers which cover
the
> success and error cases.

Done.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode394
runtime/vm/service.cc:394: lib.PrintToJSONStream(js, false);
On 2013/12/19 20:39:29, turnidge wrote:
> So do we not yet support Fields/Functions for libraries?  Maybe a TODO?

Good point! TODO added.

https://codereview.chromium.org/98253009/diff/1/runtime/vm/service.cc#newcode531
runtime/vm/service.cc:531: { "library", HandleLibrary },
On 2013/12/19 20:39:29, turnidge wrote:
> Is the plan to have both library and libraries?  I thought we were talking
about
> one or the other.

library is how you get to the root library. libraries is the collection. We
could maybe rename it to "root_library" or something. Or will the root library
always be library id 0?

[turnidge, 2013-12-20 17:44:45]

A couple of minor comments on service.cc & service_test.cc

We should talk in person about the Encode/Decode stuff.

Looking good.

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

https://codereview.chromium.org/98253009/diff/1/runtime/vm/object.cc#newcode2462
runtime/vm/object.cc:2462: String& needle_name = String::Handle();
On 2013/12/19 21:53:58, Cutch wrote:
> On 2013/12/19 20:39:29, turnidge wrote:
> > String::Handle(isolate)
> > 
> > Do we run into this a lot?  It would be possible to add a StringHandle2
> accessor
> > (hand-written) to ReusableHandleScope.
> 
> Depends on user activity. We hit this whenever we need to generate an ID for a
> field. 

I meant do we run into needing two String handles (as opposed to just the one)
in a leaf function often while coding.  Doesn't matter.  We can optimize that
case if it shows on profiles.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc#new...
runtime/vm/object.cc:5704: intptr_t id = cls.FindFunctionIndex(func);
Did you end up trying the isClosure thing?

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc#new...
runtime/vm/object.cc:15069: RawOneByteString* OneByteString::EncodeURI(const
String& str) {
Let's talk in person about Encode/Decode.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service.cc#ne...
runtime/vm/service.cc:249: length);        \
For discussion only - this message exposes that we use dense, ordered collection
ids, which is sort of none of the user's business.  They should view it as
opaque.  We could says something like "blah is not a valid blah id".  But the
bounds do help us a bit during development.

Fine as is for now.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service.cc#ne...
runtime/vm/service.cc:281: id, js);
Add check that js->num_arguments() == 4 with nice error.  Here and in the two
functions below.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service_test....
runtime/vm/service_test.cc:268: handler.msg());
Add test for invalid class.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service_test....
runtime/vm/service_test.cc:316: "\"option_keys\":[],\"option_values\":[]}}",
handler.msg());
Add invalid subcommand access for field/function

[Cutch, 2013-12-20 18:23:46]

PTAL

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc#new...
runtime/vm/object.cc:5704: intptr_t id = cls.FindFunctionIndex(func);
On 2013/12/20 17:44:45, turnidge wrote:
> Did you end up trying the isClosure thing?

Yes, it doesn't work.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc#new...
runtime/vm/object.cc:15069: RawOneByteString* OneByteString::EncodeURI(const
String& str) {
On 2013/12/20 17:44:45, turnidge wrote:
> Let's talk in person about Encode/Decode.

No need. It's fixed.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service.cc#ne...
runtime/vm/service.cc:281: id, js);
On 2013/12/20 17:44:45, turnidge wrote:
> Add check that js->num_arguments() == 4 with nice error.  Here and in the two
> functions below.

Done.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service_test....
runtime/vm/service_test.cc:268: handler.msg());
On 2013/12/20 17:44:45, turnidge wrote:
> Add test for invalid class.

Done.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/service_test....
runtime/vm/service_test.cc:316: "\"option_keys\":[],\"option_values\":[]}}",
handler.msg());
On 2013/12/20 17:44:45, turnidge wrote:
> Add invalid subcommand access for field/function

Done.

[turnidge, 2013-12-20 21:00:43]

LGTM with one small comment.

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/98253009/diff/230004/runtime/vm/object.cc#new...
runtime/vm/object.cc:5704: intptr_t id = cls.FindFunctionIndex(func);
On 2013/12/20 18:23:46, Cutch wrote:
> On 2013/12/20 17:44:45, turnidge wrote:
> > Did you end up trying the isClosure thing?
> 
> Yes, it doesn't work.

Can you try IsNonImplicitClosureFunction?  Looking at the call to
AddClosureFunciton flow_graph_builder.cc, I have a hunch that the closures may
be of this variety.

[Cutch, 2013-12-20 22:36:23]

Committed patchset #8 manually as r31358 (presubmit successful).

[siva, 2013-12-21 00:32:36]

https://codereview.chromium.org/98253009/diff/1270006/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/98253009/diff/1270006/runtime/vm/object.cc#ne...
runtime/vm/object.cc:1878: if (raw_ptr()->closure_functions_ ==
GrowableObjectArray::null()) {
should be if (closures() == GrowableObjectArray::null()) {

https://codereview.chromium.org/98253009/diff/1270006/runtime/vm/object.cc#ne...
runtime/vm/object.cc:1884: GrowableObjectArray::Handle(isolate,
raw_ptr()->closure_functions_);
ditto please use closures()

https://codereview.chromium.org/98253009/diff/1270006/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2475: // No field found found.
found found?

https://codereview.chromium.org/98253009/diff/1270006/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5717: const Function& func = *this;
This seems very weird, why do you have to do this and not just have
FindFunctionIndex be a method in class Function.