Complete the switch to ServiceObject

runtime/bin/vmservice/client/lib/src/service/service.dart

+// Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+part of service;
+
+/// A [ServiceObject] is an object known to the VM service and is tied
+/// to an owning [Isolate].
+abstract class ServiceObject extends Observable {
+  Isolate _isolate;

[turnidge, 2014/03/10 21:03:28]

consider blank lines between decls.

[Cutch, 2014/03/11 03:17:48]

Done.

+  /// Owning isolate.
+  Isolate get isolate => _isolate;
+  /// Owning vm.
+  VM get vm => _isolate.vm;
+  /// The complete service url of this object.
+  String get link => isolate.relativeLink(_id);
+  // The complete service url of this object with a '#/' prefix.
+  String get hashLink => isolate.relativeHashLink(_id);
+  String _id;
+  /// The id of this object.
+  String get id => _id;
+  String _serviceType;
+  /// The service type of this object.
+  String get serviceType => _serviceType;
+  bool _ref;
+
+  @observable String name;
+  @observable String vmName;
+
+  /// Refresh [this]. Returns a future which completes to [this] or
+  /// a [ServiceError].
+  Future<ServiceObject> refresh() {
+    assert(isolate != null);
+    if (id == '') {
+      // Errors don't have ids.
+      assert(serviceType == 'Error');

[turnidge, 2014/03/10 21:03:28]

We want to consider whether we want to assert on bad data from the vm.

[Cutch, 2014/03/11 03:17:48]

I'm convinced that it's a bad idea. But, we need a uniform way of handling this
internally and bubbling the error back up to the application. Let's discuss
tomorrow morning.

+      return new Future.value(this);
+    }
+    return isolate.vm.fetchMap(link).then(update);
+  }
+
+  /// Update [this] using [m] as a source. [m] can be a reference.
+  ServiceObject update(ObservableMap m) {
+    // Assert that m is a service map.
+    assert(ServiceObject.isServiceMap(m));
+    if ((m['type'] == 'Error') && (_serviceType != 'Error')) {
+      // Got an unexpected error. Don't update the object.
+      return _upgradeToServiceObject(vm, isolate, m);
+    }
+    // Assert that the id hasn't changed.
+    assert(m['id'] == _id);
+    // Assert that the type hasn't changed.
+    assert(ServiceObject.unreffedType(m['type']) == _serviceType);
+    _update(m);
+    return this;
+  }
+
+  // update internal state from [map]. [map] can be a reference.
+  void _update(ObservableMap map);
+
+  /// Returns true if [this] has only been partially initialized via
+  /// a reference. See [deref].
+  bool isRef() => _ref;
+
+  /// If [this] was created from a reference, request the full object
+  /// from the service by calling [refresh]. Else, return [this].
+  Future<ServiceObject> deref() {

[turnidge, 2014/03/10 21:03:28]

I suggested this elsewhere but will write it here again...

If we rename:

deref -> load
refresh -> reload

it may make their relationship more clear.

[Cutch, 2014/03/11 03:17:48]

Done.

+    if (!_ref) {
+      // Not a reference.
+      return new Future.value(this);
+    }
+    // Call refresh which will fill in the entire object.
+    return new Future(refresh);

[turnidge, 2014/03/10 21:03:28]

Can this be just

return refresh();

[Cutch, 2014/03/11 03:17:48]

Done.

+  }
+
+  void _created() {
+    var refNotice = _ref ? ' Created from reference.' : '';
+    Logger.root.info('Created ServiceObject for \'${_id}\' with type '
+                     '\'${_serviceType}\'.' + refNotice);
+  }
+
+  ServiceObject(this._isolate, this._id, this._serviceType) {
+    _ref = isRefType(_serviceType);
+    _serviceType = unreffedType(_serviceType);
+    _created();
+  }
+
+  ServiceObject.fromMap(this._isolate, ObservableMap m) {
+    assert(isServiceMap(m));
+    _id = m['id'];
+    _ref = isRefType(m['type']);
+    _serviceType = unreffedType(m['type']);
+    _created();
+    update(m);
+  }
+
+  /// Returns true if [map] is a service map. i.e. it has the following keys:
+  /// 'id' and a 'type'.
+  static bool isServiceMap(ObservableMap m) {
+    return (m != null) && (m['id'] != null) && (m['type'] != null);
+  }
+
+  /// Returns true if [type] is a reference type. i.e. it begins with an
+  /// '@' character.
+  static bool isRefType(String type) {

[turnidge, 2014/03/10 21:03:28]

Could change to "isRef" instead of "isRefType".  Not sure.

[Cutch, 2014/03/11 03:17:48]

Disagree. "isRef" is an instance method on the ServiceObject. We could probably
make these static functions private.

+    return type.startsWith('@');
+  }
+
+  /// Returns the unreffed version of [type].
+  static String unreffedType(String type) {

[turnidge, 2014/03/10 21:03:28]

Consider renaming to "stripRef"

[Cutch, 2014/03/11 03:17:48]

Done.

+    if (!isRefType(type)) {
+      return type;
+    }
+    // Strip off the '@' character.
+    return type.substring(1);
+  }
+}
+
+
+/// Upgrades response ([m]) from [vm] and [isolate] to a [ServiceObject].
+ServiceObject _upgradeToServiceObject(VM vm, Isolate isolate, ObservableMap m) {
+  assert(ServiceObject.isServiceMap(m));
+  var type = ServiceObject.unreffedType(m['type']);
+  if (type == 'Error') {
+      return new ServiceError.fromMap(isolate, m);
+  }
+  if (isolate == null) {
+    // Only an isolate list should have a null isolate.
+    assert(type == 'IsolateList');
+    return new IsolateList.fromMap(vm, m);
+  }
+  if (isolate.scripts.cachesType(type)) {

[turnidge, 2014/03/10 21:03:28]

The data-driven thing here doesn't buy much.  I would prefer a switch until
being data-driven buys us something.

[Cutch, 2014/03/11 03:17:48]

I've reworked this.

+    return isolate.scripts.cachedOrUpgradeAndAdd(m);
+  }
+  if (isolate.codes.cachesType(type)) {
+    return isolate.codes.cachedOrUpgradeAndAdd(m);
+  }
+  if (isolate.classes.cachesType(type)) {
+    return isolate.classes.cachedOrUpgradeAndAdd(m);
+  }
+  return new ServiceMap.fromMap(isolate, m);
+}