VM Service isolate listing

VM Service isolate listing

R=asiva@google.com

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

[Cutch, 2013-07-18 22:15:14]

PTAL. 

I've added more tests and reworked the test suite so that vm service port
assignment can be dynamic (needed to avoid port ownership issues.)

[siva, 2013-07-19 17:41:16]

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/cons...
File runtime/bin/vmservice/constants.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/cons...
runtime/bin/vmservice/constants.dart:9: static const int
isolateShutdownMessageId = 2;
Can these just be constants, eg:

static const int ISOLATE_STARTUP_MESSAGE_ID = 1;


The style guide link for naming constants:
http://www.dartlang.org/articles/style-guide/#do-name-constants-using-allcaps...

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
File runtime/bin/vmservice/resources.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
runtime/bin/vmservice/resources.dart:8: String extension = new
Path(name).extension;
var extension:

Style guide link:

http://www.dartlang.org/articles/style-guide/#prefer-using-var-without-a-type...

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
runtime/bin/vmservice/resources.dart:36: Resource(this.name, this.mimeType,
this.isText, this.data);
I don't see a field isText in the class, where does it get resolved from?

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
runtime/bin/vmservice/resources.dart:43: Resource resource = new Resource(name,
detectMimeType(name), data);
The constructor above has 4 params but it is being invoked with only 3.

Also mimeType variable is not used at all?

Ditto comment regarding style guide recommending use of 'var' for local
variables.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
File runtime/bin/vmservice/running_isolate.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolate.dart:14: 
extraa blank lines here and above?

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolate.dart:42: Map response;
ditto comment regarding style guide preference for using 'var'.
Here and in other places too...

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
File runtime/bin/vmservice/running_isolates.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolates.dart:12: 
extra blank line?

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolates.dart:62: request.setErrorResponse('Could
not parse isolate id: $e');
Not sure what the guide line is with messages in code, Should these be in a
table for easy internalization and accessed using an error message code?

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
runtime/bin/vmservice/server.dart:54: bool display_message = port == 0;
slightly unreadable, maybe (port == 0);

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
File runtime/bin/vmservice/service_request.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
runtime/bin/vmservice/service_request.dart:11: String get response => _response;
The style guide prefers to avoid wrapping fields in getter/setters:

http://www.dartlang.org/articles/style-guide/#avoid-wrapping-fields-in-getter...

Not sure if this case requires it to be wrapped, you could still have a 
setErrorResponse which just sets response to the output from JSON.stringify

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/vmse...
File runtime/bin/vmservice/vmservice.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/vmse...
runtime/bin/vmservice/vmservice.dart:9: // TODO(johnmccutchan): Factor 'dart:io'
dependency into separate library.
Can we open an issue and add that issue number here instead of the ldap.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl.cc
File runtime/bin/vmservice_impl.cc (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.cc:23: #include "vm/message.h"
We usually sort the include headers alphabetically....

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.cc:390: ASSERT(sp_id_obj.IsSmi() ||
sp_id_obj.IsMint());
Why do you need the assert? the call AsInt64Value() will barf out with an
UNREACHABLE if the value turns out to be a BIG INT.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.cc:448: }
Do you think there will be more than 1 native entry to be resolved here?

We could just simplify this to just check for "SendServideMessage"
and return SendServiceMessage without the for loop.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl.h
File runtime/bin/vmservice_impl.h (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.h:51: void VmServiceShutdownCallback(void*
callback_data);
Can this be a static method inside of VmService?

https://codereview.chromium.org/19622003/diff/8001/runtime/vm/message.h
File runtime/vm/message.h (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/vm/message.h#newco...
runtime/vm/message.h:28: kServiceType = 1,    // Service isolate message.
I am not sure about this message type encoding here.

Another possible model is we have OOB messages and then
we have the Service handler taking a shot at handling the message
and if it does not recognize it then we move on to Mirror message
handler and then onto any other such handlers that we have.

Let us discuss this.

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

https://codereview.chromium.org/19622003/diff/8001/runtime/vm/service.cc#newc...
runtime/vm/service.cc:10: #include "vm/port.h"
include headers in alphabetic ordering...

https://codereview.chromium.org/19622003/diff/8001/runtime/vm/service.cc#newc...
runtime/vm/service.cc:52: }
Why not put this in an AllStatic class

class Service : public AllStatic {
   ....
   ....
}

https://codereview.chromium.org/19622003/diff/8001/tests/standalone/vmservice...
File tests/standalone/vmservice/multiple_isolate_list_test.dart (right):

https://codereview.chromium.org/19622003/diff/8001/tests/standalone/vmservice...
tests/standalone/vmservice/multiple_isolate_list_test.dart:10: //   *) Spawn a
second isolate and run test again.
How does the VM start with the VM service turned on for these tests?

You don't seem to pass the command line option here....

[Cutch, 2013-07-19 18:15:02]

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/cons...
File runtime/bin/vmservice/constants.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/cons...
runtime/bin/vmservice/constants.dart:9: static const int
isolateShutdownMessageId = 2;
On 2013/07/19 17:41:16, siva wrote:
> Can these just be constants, eg:
> 
> static const int ISOLATE_STARTUP_MESSAGE_ID = 1;
> 
> 
> The style guide link for naming constants:
>
http://www.dartlang.org/articles/style-guide/#do-name-constants-using-allcaps...

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
File runtime/bin/vmservice/resources.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
runtime/bin/vmservice/resources.dart:8: String extension = new
Path(name).extension;
On 2013/07/19 17:41:16, siva wrote:
> var extension:
> 
> Style guide link:
> 
>
http://www.dartlang.org/articles/style-guide/#prefer-using-var-without-a-type...

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
runtime/bin/vmservice/resources.dart:36: Resource(this.name, this.mimeType,
this.isText, this.data);
On 2013/07/19 17:41:16, siva wrote:
> I don't see a field isText in the class, where does it get resolved from?

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/reso...
runtime/bin/vmservice/resources.dart:43: Resource resource = new Resource(name,
detectMimeType(name), data);
On 2013/07/19 17:41:16, siva wrote:
> The constructor above has 4 params but it is being invoked with only 3.
> 
> Also mimeType variable is not used at all?
> 
> Ditto comment regarding style guide recommending use of 'var' for local
> variables.

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
File runtime/bin/vmservice/running_isolate.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolate.dart:14: 
On 2013/07/19 17:41:16, siva wrote:
> extraa blank lines here and above?

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolate.dart:42: Map response;
On 2013/07/19 17:41:16, siva wrote:
> ditto comment regarding style guide preference for using 'var'.
> Here and in other places too...

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
File runtime/bin/vmservice/running_isolates.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolates.dart:12: 
On 2013/07/19 17:41:16, siva wrote:
> extra blank line?

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/runn...
runtime/bin/vmservice/running_isolates.dart:62: request.setErrorResponse('Could
not parse isolate id: $e');
On 2013/07/19 17:41:16, siva wrote:
> Not sure what the guide line is with messages in code, Should these be in a
> table for easy internalization and accessed using an error message code?

Eventually, yes. But, I don't think any other error messages are
internationalized throughout Dart.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
runtime/bin/vmservice/server.dart:54: bool display_message = port == 0;
On 2013/07/19 17:41:16, siva wrote:
> slightly unreadable, maybe (port == 0);

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
File runtime/bin/vmservice/service_request.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/serv...
runtime/bin/vmservice/service_request.dart:11: String get response => _response;
On 2013/07/19 17:41:16, siva wrote:
> The style guide prefers to avoid wrapping fields in getter/setters:
> 
>
http://www.dartlang.org/articles/style-guide/#avoid-wrapping-fields-in-getter...
> 
> Not sure if this case requires it to be wrapped, you could still have a 
> setErrorResponse which just sets response to the output from JSON.stringify

I'm protecting _response from being set without going through setErrorResponse
or setResponse.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/vmse...
File runtime/bin/vmservice/vmservice.dart (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice/vmse...
runtime/bin/vmservice/vmservice.dart:9: // TODO(johnmccutchan): Factor 'dart:io'
dependency into separate library.
On 2013/07/19 17:41:16, siva wrote:
> Can we open an issue and add that issue number here instead of the ldap.

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl.cc
File runtime/bin/vmservice_impl.cc (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.cc:23: #include "vm/message.h"
On 2013/07/19 17:41:16, siva wrote:
> We usually sort the include headers alphabetically....
> 

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.cc:390: ASSERT(sp_id_obj.IsSmi() ||
sp_id_obj.IsMint());
On 2013/07/19 17:41:16, siva wrote:
> Why do you need the assert? the call AsInt64Value() will barf out with an
> UNREACHABLE if the value turns out to be a BIG INT.

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.cc:448: }
On 2013/07/19 17:41:16, siva wrote:
> Do you think there will be more than 1 native entry to be resolved here?
> 

It's possible.

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl.h
File runtime/bin/vmservice_impl.h (right):

https://codereview.chromium.org/19622003/diff/8001/runtime/bin/vmservice_impl...
runtime/bin/vmservice_impl.h:51: void VmServiceShutdownCallback(void*
callback_data);
On 2013/07/19 17:41:16, siva wrote:
> Can this be a static method inside of VmService?

Done.

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

https://codereview.chromium.org/19622003/diff/8001/runtime/vm/service.cc#newc...
runtime/vm/service.cc:10: #include "vm/port.h"
On 2013/07/19 17:41:16, siva wrote:
> include headers in alphabetic ordering...

Done.

https://codereview.chromium.org/19622003/diff/8001/runtime/vm/service.cc#newc...
runtime/vm/service.cc:52: }
On 2013/07/19 17:41:16, siva wrote:
> Why not put this in an AllStatic class
> 
> class Service : public AllStatic {
>    ....
>    ....
> }

Done.

https://codereview.chromium.org/19622003/diff/8001/tests/standalone/vmservice...
File tests/standalone/vmservice/multiple_isolate_list_test.dart (right):

https://codereview.chromium.org/19622003/diff/8001/tests/standalone/vmservice...
tests/standalone/vmservice/multiple_isolate_list_test.dart:10: //   *) Spawn a
second isolate and run test again.
On 2013/07/19 17:41:16, siva wrote:
> How does the VM start with the VM service turned on for these tests?
> 
> You don't seem to pass the command line option here....

This test launches a new Dart instance with the --enable-vm-service command line
flag (See TestLauncher).

[Cutch, 2013-07-19 20:17:36]

Message type is gone. PTAL.

[siva, 2013-07-19 21:13:24]

lgtm

[Cutch, 2013-07-22 15:39:17]

Committed patchset #7 manually as r25279 (presubmit successful).