Refactor Observatory

Refactor Observatory

Introduce a VM class responsible for communicating with a VM and the isolates collection
Add two new base elements: IsolateElement and VMElement which are peered to an isolate or VM respectively.
Remove request manager (functionally replaced by VM)
Remove isolate manager (functionally replaced by VM)
Link creation and network requests delegated through Isolate class.

R=turnidge@google.com

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

[turnidge, 2014-03-05 21:02:47]

lgtm w/ comments.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/isolate.dart:133: //
TODO(johnmccutchan): Kill.
Maybe more verbose on the comment.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/isolate.dart:245: Future<Map>
getMap(String objectId) {
Future<ObservableMap>

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:50: ///
Returns the current object id.
I wonder if "object id" is the right terminology here, given that it may be
something like "stacktrace".

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:97: void
requestCurrentHash() {
When does requestCurrentHash get called?

It's kind of strange that it doesn't do the request if app.isolate == null. 
Does it end up dropping the ball?

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:104:
Logger.root.warning('Refreshing isolate list.');
To me the "isolate list" is a page.  How about "Refreshing isolates"?

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/service.dart:7: @reflectable
Needed?  Here are below.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/service.dart:51: //
ServiceMaps?
Maybe so?

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/vm.dart:13: Future<Map>
fetchMap(String path) {
Error checking?  Here or elsewhere?

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/vm.dart:15: print(response);
debug

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/vm.dart:21: Isolate
getIsolate(String id) => isolates[id];
Confirm that loading the stacktrace page directly (first page loaded) gets the
right isolate in the nav.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/breakpoint_list.dart:11:
// TODO(turnidge): Is a breakpoint list associated with a VM or an isolate?
An isolate.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/breakpoint_list.dart:19:
isolate.getMap('brekapoints').then((map) {
spelling "brekapoints"

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/breakpoint_list.html:10:
</isolate-nav-menu>
Move close tag up.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart:32:
print(map);
debug

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart:36:
print("ref is $ref");
debug

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_list.html:18:
<p>foo</p>
debug

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart:18:
return '${super.objectId}?line=$line';
Maybe put a TODO here to revisit how we are doing this.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_view.dart:29:
isolate.getMap('/coverage').then((Map coverage) {
slash here?

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/service_ref.dart:29:
String get objectId => ref['id'];
Should we check for ref == null?

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/vm_element.dart:11:
/// Base class for all custom elements which are peered to a VM.
maybe another word than "peered"

[Cutch, 2014-03-05 21:54:22]

Uploaded final changes (sans dart2js output)

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/isolate.dart:133: //
TODO(johnmccutchan): Kill.
On 2014/03/05 21:02:47, turnidge wrote:
> Maybe more verbose on the comment.

Done.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/isolate.dart:245: Future<Map>
getMap(String objectId) {
On 2014/03/05 21:02:47, turnidge wrote:
> Future<ObservableMap>

Done here and elsewhere.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:50: ///
Returns the current object id.
On 2014/03/05 21:02:47, turnidge wrote:
> I wonder if "object id" is the right terminology here, given that it may be
> something like "stacktrace".

I agree but will clean this up in a follow up CL.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:97: void
requestCurrentHash() {
On 2014/03/05 21:02:47, turnidge wrote:
> When does requestCurrentHash get called?
> 
> It's kind of strange that it doesn't do the request if app.isolate == null. 
> Does it end up dropping the ball?

requestCurrentHash gets called by the onHashChange listener registered in
init().

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/location_manager.dart:104:
Logger.root.warning('Refreshing isolate list.');
On 2014/03/05 21:02:47, turnidge wrote:
> To me the "isolate list" is a page.  How about "Refreshing isolates"?

Done.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/service.dart:7: @reflectable
On 2014/03/05 21:02:47, turnidge wrote:
> Needed?  Here are below.

Done.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/service.dart:51: //
ServiceMaps?
On 2014/03/05 21:02:47, turnidge wrote:
> Maybe so?

Elaborated on the comment.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/vm.dart:13: Future<Map>
fetchMap(String path) {
On 2014/03/05 21:02:47, turnidge wrote:
> Error checking?  Here or elsewhere?

Done.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/vm.dart:15: print(response);
On 2014/03/05 21:02:47, turnidge wrote:
> debug

Done.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory/vm.dart:21: Isolate
getIsolate(String id) => isolates[id];
On 2014/03/05 21:02:47, turnidge wrote:
> Confirm that loading the stacktrace page directly (first page loaded) gets the
> right isolate in the nav.

Done.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart:32:
print(map);
On 2014/03/05 21:02:47, turnidge wrote:
> debug

Done.

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/instance_ref.dart:36:
print("ref is $ref");
On 2014/03/05 21:02:47, turnidge wrote:
> debug

Done.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_list.html:18:
<p>foo</p>
On 2014/03/05 21:02:47, turnidge wrote:
> debug

Done.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_ref.dart:18:
return '${super.objectId}?line=$line';
On 2014/03/05 21:02:47, turnidge wrote:
> Maybe put a TODO here to revisit how we are doing this.

Done.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/script_view.dart:29:
isolate.getMap('/coverage').then((Map coverage) {
On 2014/03/05 21:02:47, turnidge wrote:
> slash here?

Done.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/service_ref.dart:29:
String get objectId => ref['id'];
On 2014/03/05 21:02:47, turnidge wrote:
> Should we check for ref == null?

Done.

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

https://codereview.chromium.org/184233007/diff/1/runtime/bin/vmservice/client...
runtime/bin/vmservice/client/lib/src/observatory_elements/vm_element.dart:11:
/// Base class for all custom elements which are peered to a VM.
On 2014/03/05 21:02:47, turnidge wrote:
> maybe another word than "peered"

Done here and elsewhere.

[Cutch, 2014-03-05 22:00:57]

Committed patchset #2 manually as r33347 (presubmit successful).