Allow Observatory to run as a hosted web service

[WIP]

Allow Observatory to run as a hosted web service

- Add vm-connect pane for connecting to a different VM.
- Can connect with Dartium VMs
- Can connect with content_shell VMs.
- Can connect with standalone VMs.
- Remember previously connected to standalone VMs.
- Auto refresh list available Dartium VMs.
- Provide simple server to tunnel Dartium / Content shell Chrome Remote Debugging Protocol requests.
- Improved WebSocket VM.
- Removed support for HTTP VM.
- Add support for polling updates to ObservatoryElement.

R=turnidge@google.com

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

[turnidge, 2014-07-01 00:27:47]

I'm not sure if it is "okay" for us to use the cloud icon for our standalone vm.
 Also seems a bit misleading.  Maybe we should use the dart "flight" icon?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/README.txt (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/README.txt:11: Connect to Observatory on
localhost:9090
Maybe this file should be a README_content_shell?  Or have some more general
observatory info in it too?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/bin/server.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:72: }
blank line here

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:94: return pubRequest.close();
What is this doing?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:131:
request.response.headers.add('Access-Control-Allow-Origin', '*');
I don't know my browser security.  Is "*" appropriate?  Is there anything
tighter that would work?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:153: new
ObservatoryServer(args)..start();
I would normally parse args in main and then pass the parsed values into a
constructor, rather than making a class be responsible for arg parsing.  Your
call.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/service_html.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:21: bool get everConnected =>
lastConnectionTime > 0;
Maybe "hasConnected" or "hasEverConnected"?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:62: 
Maybe a class-level comment about NetworkVM.

Also, why the name NetworkVM and not WebSocketVM?  Just curious.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:126:
_sendAllPendingRequests();
I'm wondering if we should move the call to _sendAllPendingRequests before the
call to _notifyConnect?

The scenario I'm worried about is that:

1. somebody uses vm.onConnect to wait for the connection.

2. _notifyConnect is called, _connected.future is completed and they then
immediately call getString(ID1).

3. The request for ID1 is then added to the pending requests AND immediately
sent,  since the websocket is in the OPEN state.

Another option would be to not give _pendingRequests double duty.  Instead you
could have two data structures:

1. _delayedRequests - a list of (id, completer) pairs for messages which have
not yet been sent
2. _pendingRequests - a map from seq -> completer for all outstanding requests
(no id needed anymore).

Your call.

4. We *then* call _sendAllPendingRequests and send the request for ID1 a second
time.

In short, I'm worried about the fact that sometime the pending requests have
been sent already and sometimes they haven't.  Reversing the calls above seems
to make this better.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:148: response =
map['response'];
Should we consider changing our naming to be consistent with crdp?  (id, data) 
Probably doesn't matter, but curious what you think.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:216: Future<String>
_makeRequest(String id) {
I would move this function closer to getString -- it is basically the send half
of getString.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/app/application.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/application.dart:44:
_vm.exceptions.stream.listen(_onException);
Are the two above lines necessary now that these happen as a side-effect of
setting the "vm" field?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/application.dart:95:
Logger.root.info('Uninstalling pane: $_currentPane');
Are we keeping this logging in long-term?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/app/settings.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/settings.dart:12: static void
set(String key, dynamic o) {
Maybe replace o -> obj or maybe o -> value?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/app/target_manager.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/target_manager.dart:25: //
Fill in this comment?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/target_manager.dart:44: return
'ws://localhost:8181/ws';
This code seems familiar.  Is it cloned elsewhere?  Or did it move here?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/elements/observatory_element.dart
(right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/elements/observatory_element.dart:44: Timer
_pollTimer;
I wonder if there is a way to make this simpler.  Is it useful to have this
available to all ObservatoryElements?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/elements/vm_connect.html (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/elements/vm_connect.html:37: width: 20ex;
Maybe 20em?  em is a measure of the width of an "m".  ex is the measure of the
height of an "x".

[turnidge, 2014-07-01 00:30:02]

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/service_html.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:126:
_sendAllPendingRequests();
This comment got garbled during editing -- it should read like this:

I'm wondering if we should move the call to _sendAllPendingRequests before the
call to _notifyConnect?

The scenario I'm worried about is that:

1. somebody uses vm.onConnect to wait for the connection.

2. _notifyConnect is called, _connected.future is completed and they then
immediately call getString(ID1).

3. The request for ID1 is then added to the pending requests AND immediately
sent,  since the websocket is in the OPEN state.

4. We *then* call _sendAllPendingRequests and send the request for ID1 a second
time.

In short, I'm worried about the fact that sometime the pending requests have
been sent already and sometimes they haven't.  Reversing the calls above seems
to make this better.

Another option would be to not give _pendingRequests double duty.  Instead you
could have two data structures:

1. _delayedRequests - a list of (id, completer) pairs for messages which have
not yet been sent
2. _pendingRequests - a map from seq -> completer for all outstanding requests
(no id needed anymore).

Your call.

On 2014/07/01 00:27:47, turnidge wrote:
> I'm wondering if we should move the call to _sendAllPendingRequests before the
> call to _notifyConnect?
> 
> The scenario I'm worried about is that:
> 
> 1. somebody uses vm.onConnect to wait for the connection.
> 
> 2. _notifyConnect is called, _connected.future is completed and they then
> immediately call getString(ID1).
> 
> 3. The request for ID1 is then added to the pending requests AND immediately
> sent,  since the websocket is in the OPEN state.
> 
> Another option would be to not give _pendingRequests double duty.  Instead you
> could have two data structures:
> 
> 1. _delayedRequests - a list of (id, completer) pairs for messages which have
> not yet been sent
> 2. _pendingRequests - a map from seq -> completer for all outstanding requests
> (no id needed anymore).
> 
> Your call.
> 
> 4. We *then* call _sendAllPendingRequests and send the request for ID1 a
second
> time.
> 
> In short, I'm worried about the fact that sometime the pending requests have
> been sent already and sometimes they haven't.  Reversing the calls above seems
> to make this better.

[Cutch, 2014-07-01 17:14:52]

All comments addressed. Replaced cloud icon with Dart logo.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/README.txt (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/README.txt:11: Connect to Observatory on
localhost:9090
On 2014/07/01 00:27:46, turnidge wrote:
> Maybe this file should be a README_content_shell?  Or have some more general
> observatory info in it too?

Done. Renamed to README_android

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/bin/server.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:72: }
On 2014/07/01 00:27:46, turnidge wrote:
> blank line here

Done.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:94: return pubRequest.close();
On 2014/07/01 00:27:47, turnidge wrote:
> What is this doing?

The close method on an HttpClientRequest means it's ready to be sent to the
server, obvious isn't it? The future returned by close() completes to an
HttpClientResponse.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:131:
request.response.headers.add('Access-Control-Allow-Origin', '*');
On 2014/07/01 00:27:46, turnidge wrote:
> I don't know my browser security.  Is "*" appropriate?  Is there anything
> tighter that would work?

It's either * or a list of domain names. We can't be sure where the request will
be coming from.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:153: new
ObservatoryServer(args)..start();
On 2014/07/01 00:27:46, turnidge wrote:
> I would normally parse args in main and then pass the parsed values into a
> constructor, rather than making a class be responsible for arg parsing.  Your
> call.

I'm going to keep it as is.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/service_html.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:21: bool get everConnected =>
lastConnectionTime > 0;
On 2014/07/01 00:27:47, turnidge wrote:
> Maybe "hasConnected" or "hasEverConnected"?

Done.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:62: 
On 2014/07/01 00:27:47, turnidge wrote:
> Maybe a class-level comment about NetworkVM.
> 

Done.

> Also, why the name NetworkVM and not WebSocketVM?  Just curious.

Renamed back to WebSocketVM. Mid-way through the work I was thinking of making
this support HTTP requests as well but abandoned that.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:126:
_sendAllPendingRequests();
On 2014/07/01 00:30:02, turnidge wrote:
> This comment got garbled during editing -- it should read like this:
> 
> I'm wondering if we should move the call to _sendAllPendingRequests before the
> call to _notifyConnect?
> 
> The scenario I'm worried about is that:
> 
> 1. somebody uses vm.onConnect to wait for the connection.
> 
> 2. _notifyConnect is called, _connected.future is completed and they then
> immediately call getString(ID1).
> 
> 3. The request for ID1 is then added to the pending requests AND immediately
> sent,  since the websocket is in the OPEN state.
> 
> 4. We *then* call _sendAllPendingRequests and send the request for ID1 a
second
> time.
> 
> In short, I'm worried about the fact that sometime the pending requests have
> been sent already and sometimes they haven't.  Reversing the calls above seems
> to make this better.
> 
> Another option would be to not give _pendingRequests double duty.  Instead you
> could have two data structures:
> 
> 1. _delayedRequests - a list of (id, completer) pairs for messages which have
> not yet been sent
> 2. _pendingRequests - a map from seq -> completer for all outstanding requests
> (no id needed anymore).
> 
> Your call.
> 
> On 2014/07/01 00:27:47, turnidge wrote:
> > I'm wondering if we should move the call to _sendAllPendingRequests before
the
> > call to _notifyConnect?
> > 
> > The scenario I'm worried about is that:
> > 
> > 1. somebody uses vm.onConnect to wait for the connection.
> > 
> > 2. _notifyConnect is called, _connected.future is completed and they then
> > immediately call getString(ID1).
> > 
> > 3. The request for ID1 is then added to the pending requests AND immediately
> > sent,  since the websocket is in the OPEN state.
> > 
> > Another option would be to not give _pendingRequests double duty.  Instead
you
> > could have two data structures:
> > 
> > 1. _delayedRequests - a list of (id, completer) pairs for messages which
have
> > not yet been sent
> > 2. _pendingRequests - a map from seq -> completer for all outstanding
requests
> > (no id needed anymore).
> > 
> > Your call.
> > 
> > 4. We *then* call _sendAllPendingRequests and send the request for ID1 a
> second
> > time.
> > 
> > In short, I'm worried about the fact that sometime the pending requests have
> > been sent already and sometimes they haven't.  Reversing the calls above
seems
> > to make this better.
> 

Good catch. I've gone ahead and split pendingRequests and delayedRequests into
two sets. Now _sendRequest adds the request to pendingRequests and when the
connection goes up, all delayedRequests are sent.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:148: response =
map['response'];
On 2014/07/01 00:27:47, turnidge wrote:
> Should we consider changing our naming to be consistent with crdp?  (id, data)

> Probably doesn't matter, but curious what you think.

The ['params']['id'] and ['params']['data'] are dictated by crdp. I don't think
we should change our response to match, it doesn't buy us much.

I am thinking of changing the 'Dart.observatoryData' command to be
'Dart.serviceResponse' in a future round of Dartium cleanups.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:216: Future<String>
_makeRequest(String id) {
On 2014/07/01 00:27:47, turnidge wrote:
> I would move this function closer to getString -- it is basically the send
half
> of getString.

Done.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/app/application.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/application.dart:44:
_vm.exceptions.stream.listen(_onException);
On 2014/07/01 00:27:47, turnidge wrote:
> Are the two above lines necessary now that these happen as a side-effect of
> setting the "vm" field?

Done.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/application.dart:95:
Logger.root.info('Uninstalling pane: $_currentPane');
On 2014/07/01 00:27:47, turnidge wrote:
> Are we keeping this logging in long-term?

Not sure. I like having info on pane switches. Shouldn't happen very often. If
it becomes noisy, we should remove it.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/app/settings.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/settings.dart:12: static void
set(String key, dynamic o) {
On 2014/07/01 00:27:47, turnidge wrote:
> Maybe replace o -> obj or maybe o -> value?

Done. value.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/app/target_manager.dart (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/target_manager.dart:25: //
On 2014/07/01 00:27:47, turnidge wrote:
> Fill in this comment?

Done.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/app/target_manager.dart:44: return
'ws://localhost:8181/ws';
On 2014/07/01 00:27:47, turnidge wrote:
> This code seems familiar.  Is it cloned elsewhere?  Or did it move here?

It moved here.

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/elements/observatory_element.dart
(right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/elements/observatory_element.dart:44: Timer
_pollTimer;
On 2014/07/01 00:27:47, turnidge wrote:
> I wonder if there is a way to make this simpler.  Is it useful to have this
> available to all ObservatoryElements?

Perhaps it makes more sense on a pane-level. That being said, almost all dart:io
related elements auto-refresh. I could factor this out into an
PollingObservatoryElement if we don't want it on the base element itself.
Thoughts?

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/elements/vm_connect.html (right):

https://codereview.chromium.org/335463008/diff/120001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/elements/vm_connect.html:37: width: 20ex;
On 2014/07/01 00:27:47, turnidge wrote:
> Maybe 20em?  em is a measure of the width of an "m".  ex is the measure of the
> height of an "x".

Done.

TIL:

"ex" -> height
"em" -> width

[turnidge, 2014-07-01 19:38:16]

LGTM.  Nice change.

Two smallish comments below.

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/bin/server.dart (right):

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:95: return pubRequest.close();
Maybe add a comment?

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/service_html.dart (right):

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:147:
_sendAllDelayedRequests();
It's still worth reversing these two lines, so that any delayed requests go out
before all post-connect requests.

[Cutch, 2014-07-01 21:42:55]

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/bin/server.dart (right):

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/bin/server.dart:95: return pubRequest.close();
On 2014/07/01 19:38:15, turnidge wrote:
> Maybe add a comment?

Done.

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/service_html.dart (right):

https://codereview.chromium.org/335463008/diff/140001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/service_html.dart:147:
_sendAllDelayedRequests();
On 2014/07/01 19:38:16, turnidge wrote:
> It's still worth reversing these two lines, so that any delayed requests go
out
> before all post-connect requests.

Done.

[Cutch, 2014-07-01 21:53:49]

Committed patchset #9 manually as r37910 (presubmit successful).