Library custom formatters

tool/input_sdk/private/debugger.dart

Index: tool/input_sdk/private/debugger.dart
diff --git a/tool/input_sdk/private/debugger.dart b/tool/input_sdk/private/debugger.dart
index 4c29b9e1bd8bd7ec86c03646c9243abfe96dd8e4..4072a5a0268f3294ccf05e9f6893f99aa5b4e047 100644
--- a/tool/input_sdk/private/debugger.dart
+++ b/tool/input_sdk/private/debugger.dart
@@ -126,17 +126,69 @@ class MapEntry {
 }
 
 class IterableSpan {
-  IterableSpan({this.start, this.end, this.iterable});
+  IterableSpan(this.start, this.end, this.iterable);
 
   final int start;
   final int end;
   final Iterable iterable;
+  int get length => end - start;
+
+  /// Using length - .5, a list of length 10000 results in a
+  /// maxPowerOfSubsetSize of 1, so the list will be broken up into 100,
+  /// 100-length subsets. A list of length 10001 results in a
+  /// maxPowerOfSubsetSize of 2, so the list will be broken up into 1
+  /// 10000-length subset and 1 1-length subset.
+  int get maxPowerOfSubsetSize =>
+      (log(length - .5) / log(_maxSpanLength)).truncate();
+  int get subsetSize => pow(_maxSpanLength, maxPowerOfSubsetSize);
+
+  Map<int, dynamic> asMap() =>
+      iterable.skip(start).take(length).toList().asMap();

[Jacob, 2016/07/21 02:10:31]

nice!

[bmilligan, 2016/07/22 18:25:18]

Acknowledged.

+
+  List<NameValuePair> children() {
+    var ret = <NameValuePair>[];
+    if (length <= _maxSpanLength) {
+      asMap().forEach((i, element) {
+        ret.add(
+            new NameValuePair(name: (i + start).toString(), value: element));
+      });
+    } else {
+      for (var i = start; i < end; i += subsetSize) {
+        var subSpan = new IterableSpan(i, min(end, subsetSize + i), iterable);
+        if (subSpan.length == 1) {
+          ret.add(new NameValuePair(
+              name: i.toString(), value: iterable.elementAt(i)));
+        } else {
+          ret.add(new NameValuePair(
+              name: '[${i}...${subSpan.end - 1}]',
+              value: subSpan,
+              hideName: true));
+        }
+      }
+    }
+    return ret;
+  }
+}
+
+class Library {

[Jacob, 2016/07/21 02:10:32]

do you need to wrap the libraries? I would expect the symbols added will let you
just tell immediately that an object is a library or a named constructor?

[bmilligan, 2016/07/22 18:25:18]

The symbol for the libraries is on the library module, not the libraries
themselves. The library module is identified that way, but there's no tag on its
children libraries.

[Jacob, 2016/07/22 19:17:50]

Acknowledged.

+  Library(this.name, this.object);
+
+  final String name;
+  final Object object;
+}
+
+class NamedConstructor {
+  NamedConstructor(this.object);
+
+  final Object object;
 }
 
 class ClassMetadata {
   ClassMetadata(this.object);
 
   final Object object;
+  String get name =>
+      getTypeName(object is Type ? object : dart.getReifiedType(object));
 }
 
 class HeritageClause {
@@ -218,6 +270,8 @@ class JsonMLFormatter {
   // DartFormatter.
   DartFormatter _simpleFormatter;
 
+  bool customFormattersOn = false;
+
   JsonMLFormatter(this._simpleFormatter);
 
   void setMaxSpanLengthForTestingOnly(int spanLength) {
@@ -225,6 +279,7 @@ class JsonMLFormatter {
   }
 
   header(object, config) {
+    customFormattersOn = true;
     if (config == JsonMLConfig.skipDart || isNativeJavaScriptObject(object)) {
       return null;
     }
@@ -293,9 +348,10 @@ class DartFormatter {
   List<Formatter> _formatters;
 
   DartFormatter() {
-    // The order of formatters matters as formatters later in the list take
-    // precidence.
+    // The order of formatters matters as formatters earlier in the list take
+    // precedence.

[Jacob, 2016/07/21 02:10:31]

good catch :)

[bmilligan, 2016/07/22 18:25:17]

Acknowledged.

     _formatters = [
+      new NamedConstructorFormatter(),
       new FunctionFormatter(),
       new MapFormatter(),
       new IterableFormatter(),
@@ -303,6 +359,8 @@ class DartFormatter {
       new IterableSpanFormatter(),
       new ClassMetadataFormatter(),
       new HeritageClauseFormatter(),
+      new ModuleLibraryFormatter(),
+      new LibraryFormatter(),
       new ObjectFormatter(),
     ];
   }
@@ -374,16 +432,8 @@ class ObjectFormatter extends Formatter {
     // Set of property names used to avoid duplicates.
     addMetadataChildren(object, properties);
 
-    /// Helper to add members walking up the prototype chain being careful
-    /// to avoid properties that are Dart methods.
-    var protoChain = <Object>[];
     var current = object;
-    while (current != null &&
-        !isNativeJavaScriptObject(current) &&
-        JS("bool", "# !== Object.prototype", current)) {
-      protoChain.add(current);
-      current = JSNative.getProperty(current, '__proto__');
-    }
+    var protoChain = getProtoChain(current);
 
     // We walk the prototype chain for symbol properties because they take
     // priority and are accessed instead of Dart properties according to Dart
@@ -405,12 +455,7 @@ class ObjectFormatter extends Formatter {
           // start with an _
           continue;
         }
-        var value;
-        try {
-          value = JSNative.getProperty(object, symbol);
-        } catch (e) {
-          value = '<Exception thrown> $e';
-        }
+        var value = getPropertyValue(object, symbol);
         properties.add(new NameValuePair(name: dartName, value: value));
       }
     }
@@ -424,12 +469,7 @@ class ObjectFormatter extends Formatter {
         if (hasMethod(object, name)) {
           continue;
         }
-        var value;
-        try {
-          value = JSNative.getProperty(object, name);
-        } catch (e) {
-          value = '<Exception thrown> $e';
-        }
+        var value = getPropertyValue(object, name);
         properties.add(new NameValuePair(name: name, value: value));
       }
     }
@@ -438,8 +478,100 @@ class ObjectFormatter extends Formatter {
   }
 
   addMetadataChildren(object, Set<NameValuePair> ret) {
-    ret.add(
-        new NameValuePair(name: '[[class]]', value: new ClassMetadata(object)));

[Jacob, 2016/07/21 02:10:31]

btw the reason I have to create a ClassMetadata object here is that 
object is an actual instance object and doesn't have some symbol on the object
indicating it is a ClassMetadata object.

[bmilligan, 2016/07/22 18:25:17]

Acknowledged.

+    var value = new ClassMetadata(object);
+    ret.add(new NameValuePair(name: value.name, value: value));
+  }
+
+  Object getPropertyValue(Object object, String name) {

[Jacob, 2016/07/21 02:10:31]

call this 
safeGetProperty
and make it a top level member.

[bmilligan, 2016/07/22 18:25:17]

Done.

+    var value;
+    try {
+      value = JSNative.getProperty(object, name);
+    } catch (e) {
+      value = '<Exception thrown> $e';
+    }
+    return value;
+  }
+
+  /// Helper to add members walking up the prototype chain being careful
+  /// to avoid properties that are Dart methods.
+  List<Object> getProtoChain(var current) {
+    var protoChain = <Object>[];
+    while (current != null &&
+        !isNativeJavaScriptObject(current) &&
+        JS("bool", "# !== Object.prototype", current)) {
+      protoChain.add(current);
+      current = JSNative.getProperty(current, '__proto__');
+    }
+    return protoChain;
+  }
+}
+
+/// Formatter for module Dart Library objects.
+class ModuleLibraryFormatter extends ObjectFormatter {
+  String libraryName;
+
+  accept(object) {
+    var current = object;
+    var protoChain = getProtoChain(current);
+    for (current in protoChain) {
+      for (var symbol in getOwnPropertySymbols(current)) {
+        if (symbolName(symbol) == 'dartLibraryName') {

[Jacob, 2016/07/21 02:10:31]

you will be able to match the actual symbol instead of just the symbols name now
that you have the unique symbol instance stored in the rtti library.

You will also be able to simplify and not have to walk the proto chain as a
single getProperty call will be fine.

[bmilligan, 2016/07/22 18:25:18]

Done.

+          libraryName = JSNative.getProperty(current, symbol);
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
+  bool hasChildren(object) => true;
+
+  String preview(object) {
+    var libraryNameArray = libraryName.split('/');

[Jacob, 2016/07/21 02:10:31]

I think 
libraryNameParts would be cleaner

[bmilligan, 2016/07/22 18:25:17]

Done.

+    if (libraryNameArray.length > 1) {

[Jacob, 2016/07/21 02:10:31]

comment why you are stripping the last entry

[bmilligan, 2016/07/22 18:25:18]

Done.

+      libraryNameArray[libraryNameArray.length - 1] = '';

[Jacob, 2016/07/21 02:10:31]

libraryNameParts.last = ''

[bmilligan, 2016/07/22 18:25:18]

'.last' cannot be used as a setter, which is why I used the indexing. The API
says .last is read-only. :/

[Jacob, 2016/07/22 19:17:50]

Acknowledged.

+    }
+    return 'Library Module: ${libraryNameArray.join('/')}';
+  }
+
+  List<NameValuePair> children(object) {
+    var properties = new LinkedHashSet<NameValuePair>();
+    for (var name in getOwnPropertyNames(object)) {
+      var value = JSNative.getProperty(object, name);
+      name = name.replaceAll('__', '/') + '.dart';

[Jacob, 2016/07/21 02:10:31]

prefer string interoplation. So
name = "${name.replaceAll('__', '/')}.dart";

[bmilligan, 2016/07/22 18:25:18]

Done.

+      properties.add(new NameValuePair(
+          name: name, value: new Library(name, value), hideName: true));
+    }
+    return properties.toList();
+  }
+}
+
+/// Formatter for Dart Library objects.
+class LibraryFormatter extends ObjectFormatter {
+  accept(object) => object is Library;
+
+  bool hasChildren(object) => true;
+
+  String preview(object) => object.name;
+
+  List<NameValuePair> children(object) {
+    var properties = new LinkedHashSet<NameValuePair>();
+    var entry = object.object;
+    for (var name in getOwnPropertyNames(entry)) {
+      var value = getPropertyValue(entry, name);
+      // TODO(bmilligan): Make a note on the corresponding class object that it
+      // has a generic type.
+      if (JSNative.getProperty(value, 'name') == 'makeGenericType') {

[Jacob, 2016/07/21 16:19:00]

this is fragile. is there not an existing symbol that indicates that a type is
generic? If not, we should add one. Check with John on this.

[Jennifer Messerly, 2016/07/21 16:20:25]

yeah there should be, I know we track type args on there. Probably can look at
how it's printed by the typeName code.

[bmilligan, 2016/07/22 18:25:17]

Done.

[bmilligan, 2016/07/22 18:25:17]

Done.

+        continue;
+      } else if (value is Type) {
+        var classMetadata = new ClassMetadata(value);
+        properties.add(
+            new NameValuePair(name: classMetadata.name, value: classMetadata));
+      } else {
+        properties.add(new NameValuePair(name: name, value: value));
+      }
+    }
+    return properties.toList();
   }
 }
 
@@ -517,8 +649,7 @@ class IterableFormatter extends ObjectFormatter {
     // TODO(jacobr): handle large Iterables better.
     // TODO(jacobr): consider only using numeric indices
     var ret = new LinkedHashSet<NameValuePair>();
-    ret.addAll(childrenHelper(
-        new IterableSpan(start: 0, end: object.length, iterable: object)));
+    ret.addAll((new IterableSpan(0, object.length, object)).children());

[Jacob, 2016/07/21 16:18:59]

remove unneeded extra set of parens.

[bmilligan, 2016/07/22 18:25:18]

Done.

     // TODO(jacobr): provide a link to show regular class properties here.
     // required for subclasses of iterable, etc.
     addMetadataChildren(object, ret);
@@ -527,7 +658,7 @@ class IterableFormatter extends ObjectFormatter {
 }
 
 // This class does double duting displaying metadata for
-class ClassMetadataFormatter implements Formatter {
+class ClassMetadataFormatter extends ObjectFormatter {

[Jacob, 2016/07/21 16:18:59]

why is this now an extends instead of implements relationship?
generally prefer implements instead of extends unless there is a strong reason
to use inheritance.

[bmilligan, 2016/07/22 18:25:18]

There was a function I had in ObjectFormatter that I used here. That function is
now top level, which I should have done originally.

   accept(object) => object is ClassMetadata;
 
   _getType(object) {

[Jacob, 2016/07/21 16:18:58]

is the return type of this method 
Type?

[bmilligan, 2016/07/22 18:25:18]

rtti.dart says getReifiedType "returns the runtime representation of the type of
obj. This is different from the user-visible Type object returned by calling
'runtimeType' on some Dart object". There are no return types defined in rtti,
so the best I can assume is that it returns an Object?

[Jacob, 2016/07/22 19:17:50]

Acknowledged.

@@ -537,47 +668,96 @@ class ClassMetadataFormatter implements Formatter {
 
   String preview(object) {
     ClassMetadata entry = object;
-    return getTypeName(_getType(entry.object));
+    var type =
+        entry.object is Type ? entry.object : dart.getReifiedType(entry.object);
+    var implements = dart.getImplements(type);
+    var ret = getTypeName(type);
+    if (implements != null) {
+      var typeNames = implements().map((type) => getTypeName(type));
+      return ret + ' implements ${typeNames.join(", ")}';
+    } else {
+      return ret;
+    }
   }
 
   bool hasChildren(object) => true;
 
   List<NameValuePair> children(object) {
     ClassMetadata entry = object;
+    var classObject = entry.object;
     // TODO(jacobr): add other entries describing the class such as
     // links to the superclass, mixins, implemented interfaces, and methods.
-    var type = _getType(entry.object);
+    var type = _getType(classObject);
     var ret = <NameValuePair>[];
-    var implements = dart.getImplements(type);
-    if (implements != null) {
-      ret.add(new NameValuePair(
-          name: '[[Implements]]',
-          value: new HeritageClause('implements', implements())));
-    }
+
     var mixins = dart.getMixins(type);
     if (mixins != null && mixins.isNotEmpty) {
       ret.add(new NameValuePair(
           name: '[[Mixins]]', value: new HeritageClause('mixins', mixins)));
     }
-    ret.add(new NameValuePair(
-        name: '[[JavaScript View]]',
-        value: entry.object,
-        config: JsonMLConfig.skipDart));
 
-    // TODO(jacobr): provide a link to the base class or perhaps the entire
-    // base class hierarchy as a flat list.
+    // Addition of NameValuePairs for static variables and named constructors.
+    for (var name in getOwnPropertyNames(classObject)) {
+      if (name == 'length' || name == 'name' || name == 'prototype') continue;

[Jacob, 2016/07/21 16:19:00]

why are we removing length and name?
I would add a TODO to perform more principled checks to filter out spurious
members.

[bmilligan, 2016/07/22 18:25:17]

Length is always = 0, and name is redundant as it's already displayed in the
parent node. I thought they were unnecessary properties to display, but could
delete those filters.

I should give you credit for that TODO. It's spurious of me to pretend I use the
word spurious.

+      var value = getPropertyValue(classObject, name);
+      for (var symbol in getOwnPropertySymbols(value)) {
+        if (symbolName(symbol) == 'isNamedConstructor') {
+          value = new NamedConstructor(value);
+          name = entry.name + '.' + name;

[Jacob, 2016/07/21 16:18:58]

I think it is cleaner to write
name = '${entry.name}.${name}';

[bmilligan, 2016/07/22 18:25:18]

Done.

+        }
+      }
+      ret.add(new NameValuePair(name: name, value: value));
+    }
 
-    if (entry.object is! Type) {
-      ret.add(new NameValuePair(
-          name: '[[JavaScript Constructor]]',
-          value: JSNative.getProperty(entry.object, 'constructor'),
-          config: JsonMLConfig.skipDart));
-      // TODO(jacobr): add constructors, methods, extended class, and static
+    // Addition of class methods.
+    var prototype = JS('var', '#["prototype"]', classObject);
+    if (prototype != null) {
+      for (var name in getOwnPropertyNames(prototype)) {
+        if (name == 'constructor' ||
+            name == 'new' ||
+            name == r'$identityHash') {

[Jacob, 2016/07/21 16:18:59]

$identityHash is a little scary. Add a TODO to not hard code that one.

[bmilligan, 2016/07/22 18:25:18]

Done. Is it because the name is fragile?

[Jacob, 2016/07/22 19:17:50]

yeah. someone might well change it to $identity or a symbol instead of naming
$identityHash.

[bmilligan, 2016/07/22 20:10:24]

Acknowledged.

+          continue;
+        }
+        // Simulate dart.bind by using dart.tag and tear off the function
+        // so it will be recognized by the FunctionFormatter.
+        var function = getPropertyValue(prototype, name);
+        var constructor = getPropertyValue(prototype, 'constructor');
+        for (var symbol in getOwnPropertySymbols(constructor)) {
+          if (symbolName(symbol) == 'sig') {

[Jacob, 2016/07/21 16:18:59]

should not be using symbolName. You should be able to get the actual 'sig'
symbol.
That will avoid the enumeration of the symbols and be much more efficient.

[bmilligan, 2016/07/22 18:25:18]

Done.

+            var sigObj = getPropertyValue(constructor, symbol);
+            var value = getPropertyValue(sigObj, name);
+            if (getTypeName(dart.getReifiedType(value)) != 'Null') {
+              dart.tag(function, value);
+              ret.add(new NameValuePair(name: name, value: function));
+            }
+          }
+        }
+      }
     }
+    // TODO(jacobr): provide a link to the base class or perhaps the entire
+    // base class hierarchy as a flat list.
+    // TODO(jacobr): add constructors, methods, extended class, and static
     return ret;
   }
 }
 
+class NamedConstructorFormatter implements Formatter {
+  accept(object) => object is NamedConstructor;
+
+  // TODO(bmilligan): Display the signature of the named constructor as the
+  // preview.
+  String preview(object) => 'Named Constructor';

[bmilligan, 2016/07/21 01:59:27]

I'm working on getting this to display the signature of the constructor, but
that process is more in depth. I thought I would submit the custom formatters
now to get the ball rolling and add that feature later.

[Jacob, 2016/07/21 16:18:59]

Sounds good.

[bmilligan, 2016/07/22 18:25:18]

Acknowledged.

+
+  bool hasChildren(object) => true;
+
+  List<NameValuePair> children(object) => <NameValuePair>[
+        new NameValuePair(
+            name: 'JavaScript Function',
+            value: object,
+            config: JsonMLConfig.skipDart)
+      ];
+}
+
 /// Formatter for synthetic MapEntry objects used to display contents of a Map
 /// cleanly.
 class MapEntryFormatter implements Formatter {
@@ -625,50 +805,12 @@ class IterableSpanFormatter implements Formatter {
   accept(object) => object is IterableSpan;
 
   String preview(object) {
-    IterableSpan entry = object;
     return '[${object.start}...${object.end-1}]';
   }
 
   bool hasChildren(object) => true;
 
-  List<NameValuePair> children(object) => childrenHelper(object);
-}
-
-List<NameValuePair> childrenHelper(IterableSpan span) {
-  var length = span.end - span.start;
-  var ret = new List<NameValuePair>();
-  if (length <= _maxSpanLength) {
-    for (var i = span.start; i < span.end; i++) {
-      /// TODO(bmilligan): Stop using elementAt if it becomes a performance
-      /// bottleneck in the future.
-      ret.add(new NameValuePair(
-          name: i.toString(), value: span.iterable.elementAt(i)));
-    }
-  } else {
-    /// Using length - .5, a list of length 10000 results in a
-    /// maxPowerOfSubsetSize of 1, so the list will be broken up into 100,
-    /// 100-length subsets. A list of length 10001 results in a
-    /// maxPowerOfSubsetSize of 2, so the list will be broken up into 1
-    /// 10000-length subset and 1 1-length subset.
-    var maxPowerOfSubsetSize =
-        (log(length - .5) / log(_maxSpanLength)).truncate();
-    var subsetSize = pow(_maxSpanLength, maxPowerOfSubsetSize);
-    for (var i = span.start; i < span.end; i += subsetSize) {
-      var endIndex = min(span.end, subsetSize + i);
-      if (endIndex - i == 1)
-        ret.add(new NameValuePair(
-            name: i.toString(), value: span.iterable.elementAt(i)));
-      else {
-        var entryWrapper =
-            new IterableSpan(start: i, end: endIndex, iterable: span.iterable);
-        ret.add(new NameValuePair(
-            name: '[${i}...${endIndex - 1}]',
-            value: entryWrapper,
-            hideName: true));
-      }
-    }
-  }
-  return ret;
+  List<NameValuePair> children(object) => object.children();
 }
 
 /// This entry point is automatically invoked by the code generated by