Optional binary payloads in VM service events.

Optional binary payloads in VM service events.

Allow an event to send a binary blob in addition to its JSON message.
For testing, add _Echo event type that is triggered by an _echo/event request.

R=johnmccutchan@google.com

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

[Cutch, 2014-08-15 21:33:58]

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
File runtime/bin/vmservice/observatory/lib/service_common.dart (right):

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
runtime/bin/vmservice/observatory/lib/service_common.dart:171: var meta = new
Utf8Decoder().convert(new Uint8List.view(
Can we reuse the Utf8Decoder?

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
File runtime/bin/vmservice/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
runtime/bin/vmservice/observatory/lib/src/service/object.dart:256: void
postEventMessage(dynamic eventMessage, [dynamic data]) {
Is eventMessage actually dynamic now?

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

https://codereview.chromium.org/474633002/diff/100001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2454: const intptr_t total_bytes = sizeof(uint64_t) +
meta_bytes + size;
Do we really need 64-bits to encode the meta-data size? I guess it's future
proof but seems big.

https://codereview.chromium.org/474633002/diff/120001/runtime/bin/vmservice/o...
File runtime/bin/vmservice/observatory/lib/service_html.dart (right):

https://codereview.chromium.org/474633002/diff/120001/runtime/bin/vmservice/o...
runtime/bin/vmservice/observatory/lib/service_html.dart:59: class PostMessageVM
extends VM {
Dartium needs to support binary messages too, right?

[koda, 2014-08-15 22:20:54]

PTAL

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
File runtime/bin/vmservice/observatory/lib/service_common.dart (right):

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
runtime/bin/vmservice/observatory/lib/service_common.dart:171: var meta = new
Utf8Decoder().convert(new Uint8List.view(
On 2014/08/15 21:33:58, Cutch wrote:
> Can we reuse the Utf8Decoder?

Done.

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
File runtime/bin/vmservice/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/474633002/diff/100001/runtime/bin/vmservice/o...
runtime/bin/vmservice/observatory/lib/src/service/object.dart:256: void
postEventMessage(dynamic eventMessage, [dynamic data]) {
On 2014/08/15 21:33:58, Cutch wrote:
> Is eventMessage actually dynamic now?

Already fixed in the later patch set.

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

https://codereview.chromium.org/474633002/diff/100001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2454: const intptr_t total_bytes = sizeof(uint64_t) +
meta_bytes + size;
On 2014/08/15 21:33:58, Cutch wrote:
> Do we really need 64-bits to encode the meta-data size? I guess it's future
> proof but seems big.

Do we really need more than 32 bits for IP addresses? ;)

https://codereview.chromium.org/474633002/diff/120001/runtime/bin/vmservice/o...
File runtime/bin/vmservice/observatory/lib/service_html.dart (right):

https://codereview.chromium.org/474633002/diff/120001/runtime/bin/vmservice/o...
runtime/bin/vmservice/observatory/lib/service_html.dart:59: class PostMessageVM
extends VM {
On 2014/08/15 21:33:58, Cutch wrote:
> Dartium needs to support binary messages too, right?

Per our discussion offline, Dartium doesn't support any async events, and adding
this will be the more significant, and separate, change.

[Cutch, 2014-08-15 23:47:42]

LGTM after changing the "id" strings for both messages. There are two dispatch
tables, one is isolate specific and all id's are implicitly prefixed with
"/isolates/xxx/" and one that is VM-global and all id's are implicitly prefixed
with "/vm". The client is responsible for knowing who they requested from (an
isolate or the VM).

Note, that I think there are some tests in standalone/vmservice that exercise
_echo - you could probably kill those now.

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

https://codereview.chromium.org/474633002/diff/120001/runtime/vm/service.cc#n...
runtime/vm/service.cc:818: jsobj.AddProperty("id", "_echo/isolate");
similarly here, the request id is:

/isolates/xxx/_echo. Adding "/isolate" doesn't add anything.

https://codereview.chromium.org/474633002/diff/120001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2306: jsobj.AddProperty("id", "_echo/root");
the /root isn't necessary. The request id's are relative to their owner:

"vm/_echo" is mapped to the 'root' echo handler.

so we don't gain anything by having "vm/_echo/root".

[koda, 2014-08-15 23:53:28]

I mainly wanted to distinguish them for the unit test
otherwise the events look exactly the same
but I guess I can do that some other way.

So to clarify, you are suggesting to simply use "echo" as the id for both?

On 2014/08/15 23:47:42, Cutch wrote:
> LGTM after changing the "id" strings for both messages. There are two dispatch
> tables, one is isolate specific and all id's are implicitly prefixed with
> "/isolates/xxx/" and one that is VM-global and all id's are implicitly
prefixed
> with "/vm". The client is responsible for knowing who they requested from (an
> isolate or the VM).
> 
> Note, that I think there are some tests in standalone/vmservice that exercise
> _echo - you could probably kill those now.
> 
> https://codereview.chromium.org/474633002/diff/120001/runtime/vm/service.cc
> File runtime/vm/service.cc (right):
> 
>
https://codereview.chromium.org/474633002/diff/120001/runtime/vm/service.cc#n...
> runtime/vm/service.cc:818: jsobj.AddProperty("id", "_echo/isolate");
> similarly here, the request id is:
> 
> /isolates/xxx/_echo. Adding "/isolate" doesn't add anything.
> 
>
https://codereview.chromium.org/474633002/diff/120001/runtime/vm/service.cc#n...
> runtime/vm/service.cc:2306: jsobj.AddProperty("id", "_echo/root");
> the /root isn't necessary. The request id's are relative to their owner:
> 
> "vm/_echo" is mapped to the 'root' echo handler.
> 
> so we don't gain anything by having "vm/_echo/root".

[koda, 2014-08-18 15:35:12]

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