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 ed6450550e965282b8e7d18041a041ba0c8738ae..4072a5a0268f3294ccf05e9f6893f99aa5b4e047 100644
--- a/tool/input_sdk/private/debugger.dart
+++ b/tool/input_sdk/private/debugger.dart
@@ -170,10 +170,25 @@ class IterableSpan {
   }
 }
 
+class Library {
+  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 {
@@ -333,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.
     _formatters = [
+      new NamedConstructorFormatter(),
       new FunctionFormatter(),
       new MapFormatter(),
       new IterableFormatter(),
@@ -343,6 +359,8 @@ class DartFormatter {
       new IterableSpanFormatter(),
       new ClassMetadataFormatter(),
       new HeritageClauseFormatter(),
+      new ModuleLibraryFormatter(),
+      new LibraryFormatter(),
       new ObjectFormatter(),
     ];
   }
@@ -414,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
@@ -445,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));
       }
     }
@@ -464,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));
       }
     }
@@ -478,8 +478,100 @@ class ObjectFormatter extends Formatter {
   }
 
   addMetadataChildren(object, Set<NameValuePair> ret) {
-    ret.add(
-        new NameValuePair(name: '[[class]]', value: new ClassMetadata(object)));
+    var value = new ClassMetadata(object);
+    ret.add(new NameValuePair(name: value.name, value: value));
+  }
+
+  Object getPropertyValue(Object object, String name) {

[Alan Knight, 2016/07/21 17:44:48]

It's better to avoid "get" as a prefix (even though Java and JavaScript both do
it all the time), and "value" doesn't really tell us anything more. See
go/longnamesarelong

I'd probably call these property and protoChain. I could see getProperty or
propertyNamed, since property by itself isn't very specific. 

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

Jacob suggested safeGetProperty. Are you referring to the object being renamed
as protoChain?

[Alan Knight, 2016/07/22 20:28:41]

safeGetProperty seems reasonable.

I was referring to the getProtoChain method below, which could just be called
protoChain. But I think it's disappeared now anyway.

[bmilligan, 2016/07/22 20:47:40]

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.

[Alan Knight, 2016/07/21 17:44:49]

What does the comment mean about being careful to avoid properties that are Dart
methods? I don't see anything that looks like that. Or does it mean avoiding the
possibility of a Dart method named __proto__?

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

This function existed previously, I just moved it to the Object Formatter class.
I'm not sure who wrote that comment or what it means.

[Alan Knight, 2016/07/22 20:28:41]

I'm suspicious it got moved to the wrong place somewhere in the past, and
referred to one of those places where we had checks for particular properties to
avoid.

[bmilligan, 2016/07/22 20:47:40]

Done.

+  List<Object> getProtoChain(var current) {

[Alan Knight, 2016/07/21 17:44:49]

Don't put "var" on a method parameter, either leave it blank or put Object or
explicitly make it dynamic. Probably blank is fine in this case.

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

Done.

+    var protoChain = <Object>[];
+    while (current != null &&
+        !isNativeJavaScriptObject(current) &&
+        JS("bool", "# !== Object.prototype", current)) {
+      protoChain.add(current);

[Alan Knight, 2016/07/21 17:44:49]

Naming an input parameter "current" feels odd.

Also, perhaps as a hangover from ancient programming languages, assigning to an
input parameter makes me uncomfortable. So I'd probably give it a different name
and assign it to a separately declared current.

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

Done.

+      current = JSNative.getProperty(current, '__proto__');
+    }
+    return protoChain;
+  }
+}
+
+/// Formatter for module Dart Library objects.

[Alan Knight, 2016/07/21 17:44:49]

What's the difference between a module dart library and a dart library?

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

The dart library module is an object that contains several dart libraries. For
example, 'third_party/dart/async/' is a library module that contains dart
libraries such as async.dart, result.dart, src/async_memoizer.dart,
src/cancelable_operation.dart, etc.

[Alan Knight, 2016/07/22 20:28:41]

Acknowledged.

+class ModuleLibraryFormatter extends ObjectFormatter {
+  String libraryName;
+
+  accept(object) {
+    var current = object;
+    var protoChain = getProtoChain(current);
+    for (current in protoChain) {

[Alan Knight, 2016/07/21 17:44:49]

Using a for loop variable that's declared and initialized elsewhere makes me
even more uncomfortable. It works(I think) it just feels uncommon enough that
someone reading the code could easily miss it.

You could concatenate the collections, or make getProtoChain also return the
original object if this is its main use. Another approach would be to split out
a separate _acceptExactly(object) (or some better name meaning exactly that
object, not checking prototypes). Then this can be something like

   return _acceptExactly(object) || getProtoChain(current).any((proto) =>
_acceptExactly(proto));

which also avoids computing the proto chain if we accept the object directly.

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

Yeah, I switched this part to be identifiable by symbols, so now nothing I wrote
references the proto chain. Now its only use is in the Object Formatter in code
that I assume Jacob wrote by the TODO's. I could ask him about it to clean it
up.

+      for (var symbol in getOwnPropertySymbols(current)) {
+        if (symbolName(symbol) == 'dartLibraryName') {
+          libraryName = JSNative.getProperty(current, symbol);

[Alan Knight, 2016/07/21 17:44:49]

We now have a function for this, so we should use it.

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

Done.

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

[Alan Knight, 2016/07/21 17:44:49]

Better to just call this "libraryNames", makes the expressions a little more
readable, and equally obvious that it's a collection.

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

Done.

+    if (libraryNameArray.length > 1) {
+      libraryNameArray[libraryNameArray.length - 1] = '';
+    }
+    return 'Library Module: ${libraryNameArray.join('/')}';
+  }
+
+  List<NameValuePair> children(object) {
+    var properties = new LinkedHashSet<NameValuePair>();

[Alan Knight, 2016/07/21 17:44:49]

These are linked because we want to preserve the JS order?  I think devtools
normally displays things alphabetically, so maybe it would be better to use a
regular set and sort. Or maybe the JS order is already alphabetical? Not sure.

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

The JS order is already alphabetical. It's nice to keep it in alphabetical JS
order so that classes appear together, metadata appears first, functions are
together, etc.

[Alan Knight, 2016/07/22 20:28:41]

Acknowledged.

+    for (var name in getOwnPropertyNames(object)) {
+      var value = JSNative.getProperty(object, name);

[Alan Knight, 2016/07/21 17:44:49]

Shouldn't this, and similarly below be calling our wrapper function that catches
exceptions?

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

Done.

+      name = name.replaceAll('__', '/') + '.dart';

[Alan Knight, 2016/07/21 17:44:49]

A comment for the replacement would be good.

[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>();

[Alan Knight, 2016/07/21 17:44:49]

Should this just be called "children"?

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

Done.

+    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') {
+        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();
   }
 }
 
@@ -566,7 +658,7 @@ class IterableFormatter extends ObjectFormatter {
 }
 
 // This class does double duting displaying metadata for
-class ClassMetadataFormatter implements Formatter {
+class ClassMetadataFormatter extends ObjectFormatter {
   accept(object) => object is ClassMetadata;
 
   _getType(object) {
@@ -576,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);

[Alan Knight, 2016/07/21 17:44:48]

var typeName?

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

Done.

+    if (implements != null) {
+      var typeNames = implements().map((type) => getTypeName(type));

[Alan Knight, 2016/07/21 17:44:49]

If you're just calling a one argument function/method in a map, where the
argument is the loop variable, you can just use the function as a tear-off.

implements().map(getTypeName);

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

Done.

+      return ret + ' implements ${typeNames.join(", ")}';

[Alan Knight, 2016/07/21 17:44:49]

Really nitpicking it's slightly better to use an interpolation for the whole
business rather than a concatenation. So

"$ret implements ..."

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

Done.

+    } 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>[];

[Alan Knight, 2016/07/21 17:44:49]

Also better as "children"

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

Done.

-    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;

[Alan Knight, 2016/07/21 17:44:49]

This would be better in a separate list of properties we don't show.

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

Done.

+      var value = getPropertyValue(classObject, name);
+      for (var symbol in getOwnPropertySymbols(value)) {
+        if (symbolName(symbol) == 'isNamedConstructor') {
+          value = new NamedConstructor(value);
+          name = entry.name + '.' + name;
+        }
+      }
+      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' ||

[Alan Knight, 2016/07/21 17:44:49]

Also better in a separate list.

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

Done.

+            name == r'$identityHash') {
+          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');

[Alan Knight, 2016/07/21 17:44:49]

Also, I would think, call our wrapper for this. I presume we'd want to catch any
exceptions here.

There's probably a general issue there of ensuring robustness in our debugger if
we get an exception inside our own formatters, but that's more of a polish item
that we should note and deal with later.

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

Done.

+        for (var symbol in getOwnPropertySymbols(constructor)) {
+          if (symbolName(symbol) == 'sig') {
+            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';
+
+  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 {