Support Dart debugger API directly

Support Dart debugger API directly

BUG=
R=leafp@google.com, rmacnak@google.com

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

[Jacob, 2014-04-24 20:41:03]

See https://codereview.chromium.org/251383004
which contains tests for this code.

[Jacob, 2014-04-24 20:47:49]

Use the Mirrors API from Dart code to implement most Blink debugger API methods.

[vsm, 2014-04-24 23:48:44]

[+leafp]

Leaf is in the middle of switching this file over to dart:blink:
https://codereview.chromium.org/254463006/

Perhaps your changes could be added to a new file / part instead.  Otherwise,
this looks like a painful merge.

[Jacob, 2014-05-02 22:34:28]

Leaf's changes have landed and it looks like there weren't any merge conflicts.
I agree that this should be moved to a separate part file at some point as there
is no good reason other than convenience that these methods are in
native_DOMImplementation.dart Something like InspectorImplementation.dart or
DebuggerImplementation.dart would be better.

[Jacob, 2014-05-06 18:27:24]

Ping?

[rmacnak, 2014-05-06 21:14:43]

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:342: if (staticContext !=
declaration.isStatic) return;
Should do this check after considering constructors because generative
constructors are !isStatic but factory constructors are isStatic.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:343: if (declaration.isOperator)
return;
Because the completions assume dot notation?

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:358:
library.declarations.forEach((symbol, declaration) {
Add the prefixed imports (but not their declarations) ?

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:389: for (var interface in
classMirror.superinterfaces) {
Interfaces declared on superclasses?

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:445: var libraryUrl =
MirrorSystem.getName(libraryMirror.qualifiedName);
You're not comparing the url here. The qualified name might collide with user
code (though that is willful abuse on the user's part).

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:447: // getters for all dart
platform libraries rather than just dart.dom.
I wouldn't do this for all Dart platform libraries. It seems unlikely they would
all be fast and side-effect free. For instance DeclarationMirror.metadata might
trigger a compilation error if the metadata is being accessed for the first
time.

[Jacob, 2014-05-07 00:05:23]

Thanks for the careful review!
PTAL

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:342: if (staticContext !=
declaration.isStatic) return;
On 2014/05/06 21:14:43, Ryan Macnak wrote:
> Should do this check after considering constructors because generative
> constructors are !isStatic but factory constructors are isStatic.

Good catch! I added a test (see utils.dart in the related CL) and fixed this
special case.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:343: if (declaration.isOperator)
return;
On 2014/05/06 21:14:43, Ryan Macnak wrote:
> Because the completions assume dot notation?
Correct. Completions are after a dot.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:358:
library.declarations.forEach((symbol, declaration) {
On 2014/05/06 21:14:43, Ryan Macnak wrote:
> Add the prefixed imports (but not their declarations) ?

Correct. Interesting idea to show all completions. That could be what the user
wants in some cases. E.g. if i've imported the js library with prefix js, it
would be nice to see all the dart:js methods after I type j. For other cases it
would be less than useful though. For example, dart:html would have hundreds of
entries so you don't really want to show that huge list when I type h.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:389: for (var interface in
classMirror.superinterfaces) {
On 2014/05/06 21:14:43, Ryan Macnak wrote:
> Interfaces declared on superclasses?
Fixed.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:445: var libraryUrl =
MirrorSystem.getName(libraryMirror.qualifiedName);
On 2014/05/06 21:14:43, Ryan Macnak wrote:
> You're not comparing the url here. The qualified name might collide with user
> code (though that is willful abuse on the user's part).

Tweaked to actually use the URI instead.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:447: // getters for all dart
platform libraries rather than just dart.dom.
On 2014/05/06 21:14:43, Ryan Macnak wrote:
> I wouldn't do this for all Dart platform libraries. It seems unlikely they
would
> all be fast and side-effect free. For instance DeclarationMirror.metadata
might
> trigger a compilation error if the metadata is being accessed for the first
> time.
Makes sense.  I think the right solution is to provide an annotation that can be
used to indicate which getters should be shown automatically and whitelisting
all of dart.dom until then.  JS is already showing all DOM getters automatically
due to limitations in how native getters are shown in JS and I haven't heard of
anyone complain about issues debugging JS due to it.

[Jacob, 2014-05-07 00:13:29]

https://codereview.chromium.org/258503008/diff/10001/tools/dom/idl/dart/dart.idl
File tools/dom/idl/dart/dart.idl (right):

https://codereview.chromium.org/258503008/diff/10001/tools/dom/idl/dart/dart....
tools/dom/idl/dart/dart.idl:326: [Supplemental, NoInterfaceObject]
This is needed for the matching CL.  Basically it is the InjectedScriptHost
class with almost all the functionality disabled. inject is still supported.

[Leaf, 2014-05-07 01:47:27]

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:73: _MethodTrampoline(ObjectMirror
receiver, MethodMirror methodMirror, Symbol selector) :
line width?

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:88: _GetterTrampoline(ObjectMirror
receiver, MethodMirror methodMirror, Symbol selector) :
ditto

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:98: _SetterTrampoline(ObjectMirror
receiver, MethodMirror methodMirror, Symbol selector) :
also

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:440: libraryMirror = owner.owner;
Does this do the right thing if the class is produced by a mixin?  I think in
that case owner.owner might not be the library.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:495: ..value =
declarationMirror.reflectedType;
Is this idiomatic in Dart?  I'm presuming that this is parsed as (new
_Property(name))..setValue(etc.) but at least to new Dart eyes this is pretty
egregiously confusing (and requires knowledge of the relative precedence of ..
and =).   Maybe better to just have a constructor for _Property that takes a
value as well as a name?

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:509:
_addInstanceMirrors(objectMirror, classMirror.owner,
Same question as before about class mirror owners in the presence of mixins.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:521:
_addInstanceMirrors(objectMirror, classMirror.owner,
ditto

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:547: ..writable =
declaration.isFinal && !declaration.isConst;
I don't understand this line (possibly my fault).  Why is a non-final, non-const
variable not considered writable?

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:562: var property =
properties.putIfAbsent(name, () => new _Property(name));
I'm not sure I get why this is right.  If I understand the way things work (and
I may not), a static field x will induce a static getter method x and a static
setter method x=, both of which I think will get mapped to the name "x" by your
_getShortSymbolName function, along with the field itself (I think).  Won't
these all collide in the properties map?  And if so, I think you'll fill in the
property with different values depending on the order in which you hit them.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:615: if (declaration is ClassMirror
|| declaration is VariableMirror ||
declaration is known not to be ClassMirror here.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:628: } else if (declaration is
ClassMirror) {
Again, known to be false.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:630: } else if
(declaration.isRegularMethod) {
The section from here to the end is pretty close to identical to part of
_addStatics above.  Is there a useful way to factor this into one place?

[rmacnak, 2014-05-07 16:59:51]

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:440: libraryMirror = owner.owner;
On 2014/05/07 01:47:27, leafp wrote:
> Does this do the right thing if the class is produced by a mixin?  I think in
> that case owner.owner might not be the library.

A class's owner will always be a library.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:562: var property =
properties.putIfAbsent(name, () => new _Property(name));
On 2014/05/07 01:47:27, leafp wrote:
> I'm not sure I get why this is right.  If I understand the way things work
(and
> I may not), a static field x will induce a static getter method x and a static
> setter method x=, both of which I think will get mapped to the name "x" by
your
> _getShortSymbolName function, along with the field itself (I think).  Won't
> these all collide in the properties map?  And if so, I think you'll fill in
the
> property with different values depending on the order in which you hit them.

Implicit getters and setters are not part of ClassMirror.declarations.

[Jacob, 2014-05-07 20:57:08]

PTAL

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:73: _MethodTrampoline(ObjectMirror
receiver, MethodMirror methodMirror, Symbol selector) :
On 2014/05/07 01:47:27, leafp wrote:
> line width?

Done.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:88: _GetterTrampoline(ObjectMirror
receiver, MethodMirror methodMirror, Symbol selector) :
On 2014/05/07 01:47:27, leafp wrote:
> ditto

Done.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:98: _SetterTrampoline(ObjectMirror
receiver, MethodMirror methodMirror, Symbol selector) :
On 2014/05/07 01:47:27, leafp wrote:
> also

Done.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:440: libraryMirror = owner.owner;
On 2014/05/07 16:59:51, Ryan Macnak wrote:
> On 2014/05/07 01:47:27, leafp wrote:
> > Does this do the right thing if the class is produced by a mixin?  I think
in
> > that case owner.owner might not be the library.
> 
> A class's owner will always be a library.

yep.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:495: ..value =
declarationMirror.reflectedType;
On 2014/05/07 01:47:27, leafp wrote:
> Is this idiomatic in Dart?  I'm presuming that this is parsed as (new
> _Property(name))..setValue(etc.) but at least to new Dart eyes this is pretty
> egregiously confusing (and requires knowledge of the relative precedence of ..
> and =).   Maybe better to just have a constructor for _Property that takes a
> value as well as a name? 

This is the idiomatic way to use Dart cascades.  As you mention it is a little
bit ugly as it only works due to precedence rules. I think 60% of the Dart team
would say this is good idiomatic dart, 40% would say it is ugly.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:509:
_addInstanceMirrors(objectMirror, classMirror.owner,
On 2014/05/07 01:47:27, leafp wrote:
> Same question as before about class mirror owners in the presence of mixins.

Same answer as before. the owner is a library.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:521:
_addInstanceMirrors(objectMirror, classMirror.owner,
On 2014/05/07 01:47:27, leafp wrote:
> ditto

same answer as other case.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:547: ..writable =
declaration.isFinal && !declaration.isConst;
On 2014/05/07 01:47:27, leafp wrote:
> I don't understand this line (possibly my fault).  Why is a non-final,
non-const
> variable not considered writable?

I don't understand what I was thinking either. The only thing that makes sense
is
!declaration.isFinal && !declaration.isConst

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:562: var property =
properties.putIfAbsent(name, () => new _Property(name));
On 2014/05/07 16:59:51, Ryan Macnak wrote:
> On 2014/05/07 01:47:27, leafp wrote:
> > I'm not sure I get why this is right.  If I understand the way things work
> (and
> > I may not), a static field x will induce a static getter method x and a
static
> > setter method x=, both of which I think will get mapped to the name "x" by
> your
> > _getShortSymbolName function, along with the field itself (I think).  Won't
> > these all collide in the properties map?  And if so, I think you'll fill in
> the
> > property with different values depending on the order in which you hit them.
> 
> Implicit getters and setters are not part of ClassMirror.declarations.

Correct. That is why I have to add these cases.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:615: if (declaration is ClassMirror
|| declaration is VariableMirror ||
On 2014/05/07 01:47:27, leafp wrote:
> declaration is known not to be ClassMirror here.

Fixed. I originally tried to use this method for both libraries and classes but
the soup of special cases got ugly so I separated the two. The references to
ClassMirror here are left over from that.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:628: } else if (declaration is
ClassMirror) {
On 2014/05/07 01:47:27, leafp wrote:
> Again, known to be false.

Done.

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:630: } else if
(declaration.isRegularMethod) {
On 2014/05/07 01:47:27, leafp wrote:
> The section from here to the end is pretty close to identical to part of
> _addStatics above.  Is there a useful way to factor this into one place?

Done.

[Leaf, 2014-05-08 04:04:01]

Changes introduced a few more wide lines, but otherwise LGTM 

-leaf

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
File tools/dom/src/native_DOMImplementation.dart (right):

https://codereview.chromium.org/258503008/diff/1/tools/dom/src/native_DOMImpl...
tools/dom/src/native_DOMImplementation.dart:495: ..value =
declarationMirror.reflectedType;
Ok, I'll get used to it.

On 2014/05/07 20:57:08, Jacob wrote:
> On 2014/05/07 01:47:27, leafp wrote:
> > Is this idiomatic in Dart?  I'm presuming that this is parsed as (new
> > _Property(name))..setValue(etc.) but at least to new Dart eyes this is
pretty
> > egregiously confusing (and requires knowledge of the relative precedence of
..
> > and =).   Maybe better to just have a constructor for _Property that takes a
> > value as well as a name? 
> 
> This is the idiomatic way to use Dart cascades.  As you mention it is a little
> bit ugly as it only works due to precedence rules. I think 60% of the Dart
team
> would say this is good idiomatic dart, 40% would say it is ugly.

[rmacnak, 2014-05-08 18:21:03]

lgtm

[Jacob, 2014-06-04 01:14:07]

Committed patchset #6 manually as r36965 (presubmit successful).