Improve Dart Debugger performance and robustness by creating Dart wrappers using the standard SetNa…

Improve Dart Debugger performance and robustness by creating Dart wrappers using the standard SetNamedPropertyHandler and SetIndexedPropertyHandler logic rather than performing a lot of thunking back and forth between JS, C++, and Dart.
This lets us provide better error messages when invoking invalid properties, handle NoSuchMethod better, and display Map and List keys and values.

As part of this fix, enable additional missing dev tools functionality such as proper Node highlighting when logging nodes.
 
BUG=
R=podivilov@chromium.org, vsm@google.com

Committed: https://src.chromium.org/viewvc/multivm?view=rev&revision=1301

[Jacob, 2013-07-11 18:52:27]

This CL isn't sent out for review at the moment.  Sending out with commends now
to provide context for our meeting today on Dart API pain points.
In general I've found the Dart VM API easy to use but there are a few pieces of
missing functionality that result in extra complexity on my side and many more
round trips between my code and the VM than needed.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
File Source/bindings/dart/DartHandleProxy.cpp (right):

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:80: bool libraryHasMember(Dart_Handle
library, Dart_Handle name)
Requested Dart API:
"LibraryHasMember" or something similar.

Basically we need an internal and external version depending on whether the
question you are asking is:
"Does library Foo have member Bar when used externally and does it have member
Bar when you are within library Bar itself.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:88: {
Requested Dart Api:
"ClassHasMember"
would be nice if this was one line.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:163: if (isConstructor)
Nit: why are constructor names prefixed with the class name? Seems like this
just adds extra work for api users?

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:245: ret = Dart_Invoke(handle,
V8Converter::stringToDart(v8::String::Concat(v8::String::New("get:"), name)), 0,
0);
For classes, I can just call 
Dart_Invoke with the prefix "get:" added to get a closure to a member, invoke a
getter, or get a field.
This is very convenient and it would be nice if I could do the same with
Libraries but currently I cannot.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:282: // FIXME: this seems like an
overly complex way to have to invoke a constructor.
nit: this seems more complex than needed.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:311: void
getImportedLibrariesMatchingPrefix(int32_t libraryId, Dart_Handle prefix,
Vector<std::pair<Dart_Handle, intptr_t> >* libraries)
It would be nice if I didn't have to do all this work to find the libraries
matching a prefix.
With the right debugger APIs, I should even have to worry about exactly what
libraries match a prefix. 
Ideal solution for me:
Make all the existing library APIs just let me specify a prefix.
For example: I would be golden if I could just write (psuedo code)
Dart_Invoke(someLibraryHandle,  "someLibraryPrefix.someMemberName");

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:341: {
would be nice if I could just call Dart_Invoke instead of calling all three
options

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:365: {
This is a mess due to handling prefixes.
It would be nice if the API was prefix aware

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:449: {
It would be nice if Dart libraries provided a method to enumerate all members so
I didn't have to write all this code.
In particular, going back and forth between libraryIds and LibraryHandles is
slow and annoying.
This was enough of a bottleneck that I cache the list of library members for
performance.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:506: bool isDynamic = false;
What should we do about the class "dynamic".
It exists but then behaves badly so I have to hack around it.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:509: if (!Dart_ClassIsTypedef(handle)
&& !Dart_ClassIsFunctionType(handle) && !isDynamic) {
Similarly to the case of Library, it would be nice if there was just an API to
get all the string names of Class/Type members

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:547: ret = Dart_Invoke(handle,
V8Converter::stringToDart(v8::String::Concat(v8::String::New("set:"),
property)), 1, &dartValue);
TODO(jacobr): similar to the case of Libraries, this probably doesn't work for
static fields.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:578: addFunctionNames(handle,
properties, &count, false, false);
it would be nice if there was just an Dart API method to enumerate all the
members of a Type/Library/Class instance.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:606: {
It would be nice if the local frame was just exposed as a Dart object with
fields that could be returned and set rather than a list of field names and
values.
It would be nice if the local frame also included closures defined in the local
frame (don't think it does).

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:623: {
This is the one case where we can't manipulate Data in the Dart context.  It
would be nice to fix for completeness.
Users will be confused that all other data is modifiable but not this.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:654: // Truncate everything after the
'@'.
it would be nice if there was an option to get names without the @ both for the
frame api and elsewhere.

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:1023: {
why do I have to keep around both a handle to a library and a libraryId?

[Jacob, 2013-07-15 18:03:26]

Ready for review

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
File Source/bindings/dart/DartHandleProxy.cpp (right):

https://codereview.chromium.org/18169002/diff/2001/Source/bindings/dart/DartH...
Source/bindings/dart/DartHandleProxy.cpp:356: if (!Dart_IsNull(ret) &&
!Dart_IsError(ret)) {
why do i have a null check here?

https://codereview.chromium.org/18169002/diff/10001/LayoutTests/dart/dom/Cons...
File LayoutTests/dart/dom/Console-expected.txt (right):

https://codereview.chromium.org/18169002/diff/10001/LayoutTests/dart/dom/Cons...
LayoutTests/dart/dom/Console-expected.txt:3: CONSOLE MESSAGE:
shows a blank line as as an interactive object is shown in the console

https://codereview.chromium.org/18169002/diff/10001/LayoutTests/dart/dom/Cons...
LayoutTests/dart/dom/Console-expected.txt:3: CONSOLE MESSAGE:
displays a blank line as an interactive object is shown in the console rather
than calling toString.  I'll admit this test is now a bit ugly

[Jacob, 2013-07-15 18:38:18]

Anton, can you comment on the hack I had to add to
Source/bindings/v8/V8Binding.cpp
?
I'm unclear of the right way to emulate the trampoline logic you previously had
with the new setup of implementing the native object hooks.
This line of code is required to make window.console work when it is triggered
from a command in the debugger console.

[vsm, 2013-07-15 18:40:29]

First set of quick comments.

https://chromiumcodereview.appspot.com/18169002/diff/14002/LayoutTests/dart/i...
File LayoutTests/dart/inspector/evaluate-in-console-expected.txt (right):

https://chromiumcodereview.appspot.com/18169002/diff/14002/LayoutTests/dart/i...
LayoutTests/dart/inspector/evaluate-in-console-expected.txt:18: Receiver:
Closure: () => String from Function 'toString':.
It looks brittle to depend on the actual error string here.  Is there anyway to
wrap it up, so we're checking for an error but not reliant on the exact message.

https://chromiumcodereview.appspot.com/18169002/diff/14002/LayoutTests/dart/i...
File LayoutTests/dart/inspector/scope-variables-expected.txt (right):

https://chromiumcodereview.appspot.com/18169002/diff/14002/LayoutTests/dart/i...
LayoutTests/dart/inspector/scope-variables-expected.txt:34: #0     
throwingGetter
(file:///usr/local/google/home/jacobr/src/dartium6/src/third_party/WebKit/LayoutTests/dart/inspector/scope-variables.dart:42:3)]
Note: stack trace has 'jacobr' in here.

https://chromiumcodereview.appspot.com/18169002/diff/14002/Source/bindings/v8...
File Source/bindings/v8/V8Binding.cpp (right):

https://chromiumcodereview.appspot.com/18169002/diff/14002/Source/bindings/v8...
Source/bindings/v8/V8Binding.cpp:314: if (window.IsEmpty()) {
Is this something to upstream sooner rather than later?  I'm worried about
losing these changes on Blink rolls.

[Jacob, 2013-07-15 22:23:54]

PTAL
Btw, this CL depends on 
https://codereview.chromium.org/18125007/

https://codereview.chromium.org/18169002/diff/14002/LayoutTests/dart/inspecto...
File LayoutTests/dart/inspector/evaluate-in-console-expected.txt (right):

https://codereview.chromium.org/18169002/diff/14002/LayoutTests/dart/inspecto...
LayoutTests/dart/inspector/evaluate-in-console-expected.txt:18: Receiver:
Closure: () => String from Function 'toString':.
On 2013/07/15 18:40:29, vsm wrote:
> It looks brittle to depend on the actual error string here.  Is there anyway
to
> wrap it up, so we're checking for an error but not reliant on the exact
message.

Done.

https://codereview.chromium.org/18169002/diff/14002/LayoutTests/dart/inspecto...
File LayoutTests/dart/inspector/scope-variables-expected.txt (right):

https://codereview.chromium.org/18169002/diff/14002/LayoutTests/dart/inspecto...
LayoutTests/dart/inspector/scope-variables-expected.txt:34: #0     
throwingGetter
(file:///usr/local/google/home/jacobr/src/dartium6/src/third_party/WebKit/LayoutTests/dart/inspector/scope-variables.dart:42:3)]
On 2013/07/15 18:40:29, vsm wrote:
> Note: stack trace has 'jacobr' in here.
Added logic to scope-variables.html to strip source locations.
It looks like the layout-test code doesn't provide generic wild card support and
that instead each test that needs to strip paths does their own normalization in
the test file instead of the expectation file.

https://codereview.chromium.org/18169002/diff/14002/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/18169002/diff/14002/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:314: if (window.IsEmpty()) {
On 2013/07/15 18:40:29, vsm wrote:
> Is this something to upstream sooner rather than later?  I'm worried about
> losing these changes on Blink rolls.
The inspector tests will fail without this change as console.log won't work so
we won't lose it with Blink rolls.
That said, I don't think this change is right.  Hopefully Anton can point me to
a better long term solution to get the V8 isolate we are talking to in the
correct state.

[Anton Muhin, 2013-07-16 13:27:40]

+Pavel who knows a lot about this part.

Jacob, overall patch is very big and hard to understand.

May you, please, tell me what's the problem you face with V8 part?

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/dart/Dart...
Source/bindings/dart/DartDOMWrapper.h:191: if (Dart_IsError(result))
btw, that would lead to crashes as we don't want to verify the pointer read from
receiver for performance reasons.

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:314: if (window.IsEmpty()) {
when do you trigger this path?

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:71:
v8::Handle<v8::Object> handle =
v8::Handle<v8::Object>::Cast(value.v8ValueRaw());
it might be not a restriction now, but we tried really hard not to mess up with
v8 bindings.

[Anton Muhin, 2013-07-16 13:28:16]

Oops, now indeed +Pavel.

[Jacob, 2013-07-16 16:44:59]

Sorry for the size of the CL. Porting the proxies over from the old style to
this style made sense to do as a single batch instead of trying to leave some
objects (e.g. libraries) with js driven proxies while implementing some proxies
with C++ proxies.

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/dart/Dart...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/dart/Dart...
Source/bindings/dart/DartDOMWrapper.h:191: if (Dart_IsError(result))
On 2013/07/16 13:27:40, Anton Muhin wrote:
> btw, that would lead to crashes as we don't want to verify the pointer read
from
> receiver for performance reasons.

Would you like me to add a separate method
maybeReadNativePointer method that can return NULL?
The problem is there is no other way currently to verify that an object really
has a native pointer versus just implements an interface for a native object
(e.g. Node).

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:314: if (window.IsEmpty()) {
On 2013/07/16 13:27:40, Anton Muhin wrote:
> when do you trigger this path?

This path is triggered if you invoke a Dart method that logs to the console as
logging to the console in Dart triggers this path.

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:71:
v8::Handle<v8::Object> handle =
v8::Handle<v8::Object>::Cast(value.v8ValueRaw());
On 2013/07/16 13:27:40, Anton Muhin wrote:
> it might be not a restriction now, but we tried really hard not to mess up
with
> v8 bindings.

My impression is that restriction has been lifted a bit now and we should try to
implement things the right way as much as possible versus leaving the v8
bindings completely frozen.

[dgrove, 2013-07-16 16:47:21]

On 2013/07/16 16:44:59, Jacob wrote:
> Sorry for the size of the CL. Porting the proxies over from the old style to
> this style made sense to do as a single batch instead of trying to leave some
> objects (e.g. libraries) with js driven proxies while implementing some
proxies
> with C++ proxies.
> 
>
https://codereview.chromium.org/18169002/diff/25001/Source/bindings/dart/Dart...
> File Source/bindings/dart/DartDOMWrapper.h (right):
> 
>
https://codereview.chromium.org/18169002/diff/25001/Source/bindings/dart/Dart...
> Source/bindings/dart/DartDOMWrapper.h:191: if (Dart_IsError(result))
> On 2013/07/16 13:27:40, Anton Muhin wrote:
> > btw, that would lead to crashes as we don't want to verify the pointer read
> from
> > receiver for performance reasons.
> 
> Would you like me to add a separate method
> maybeReadNativePointer method that can return NULL?
> The problem is there is no other way currently to verify that an object really
> has a native pointer versus just implements an interface for a native object
> (e.g. Node).
> 
>
https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
> File Source/bindings/v8/V8Binding.cpp (right):
> 
>
https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
> Source/bindings/v8/V8Binding.cpp:314: if (window.IsEmpty()) {
> On 2013/07/16 13:27:40, Anton Muhin wrote:
> > when do you trigger this path?
> 
> This path is triggered if you invoke a Dart method that logs to the console as
> logging to the console in Dart triggers this path.
> 
>
https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/custom...
> File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):
> 
>
https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/custom...
> Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:71:
> v8::Handle<v8::Object> handle =
> v8::Handle<v8::Object>::Cast(value.v8ValueRaw());
> On 2013/07/16 13:27:40, Anton Muhin wrote:
> > it might be not a restriction now, but we tried really hard not to mess up
> with
> > v8 bindings.
> 
> My impression is that restriction has been lifted a bit now and we should try
to
> implement things the right way as much as possible versus leaving the v8
> bindings completely frozen.

This is correct.

[Jacob, 2013-07-17 05:58:16]

PTAL
also added a stub for @staticFields and @classInfo data used by the Dart Editor

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/18169002/diff/25001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:314: if (window.IsEmpty()) {
On 2013/07/16 16:44:59, Jacob wrote:
> On 2013/07/16 13:27:40, Anton Muhin wrote:
> > when do you trigger this path?
> 
> This path is triggered if you invoke a Dart method that logs to the console as
> logging to the console in Dart triggers this path.
It took far longer than it should but figured out a reasonable change to
DartDebugServer that removes the need for this hack.
Fixed the issue by running Dart snippets in the Debug context and making sure
that the evaluate method itself ran in the page context.

[Jacob, 2013-07-18 03:53:10]

Please let me know if there is anything I can do to make this code easier to
review. Debugging is painfully slow without this CL so I want to get it landed
as soon as safely possible.

[podivilov, 2013-07-18 10:35:24]

On 2013/07/18 03:53:10, Jacob wrote:
> Please let me know if there is anything I can do to make this code easier to
> review. Debugging is painfully slow without this CL so I want to get it landed
> as soon as safely possible.

LGTM for proxy changes.

I would recommend to send Node highlighting support as a separate cl, because
current implementation might have performance implications.

[Jacob, 2013-07-18 18:29:44]

On 2013/07/18 10:35:24, podivilov wrote:
> On 2013/07/18 03:53:10, Jacob wrote:
> > Please let me know if there is anything I can do to make this code easier to
> > review. Debugging is painfully slow without this CL so I want to get it
landed
> > as soon as safely possible.
> 
> LGTM for proxy changes.
> 
> I would recommend to send Node highlighting support as a separate cl, because
> current implementation might have performance implications.

I've moved the hookup for the Node highlighting support to a separate cl. 
Notice that the scope-variables-expected.txt file changes as DivElement is now
not detected as a Node.
I like that fix as even though I don't think there are debugger performance
concerns due to hotspots in the JS side of node type detection (exceptions are
intentionally thrown and caught) I like keeping the dart and v8 code in separate
CLs.

[vsm, 2013-07-18 18:33:23]

lgtm

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
File Source/bindings/dart/DartHandleProxy.cpp (right):

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
Source/bindings/dart/DartHandleProxy.cpp:130: && DartNode::toNative(type,
exception);
Nit: I think this would be clearer as:

if (!DartNode::toNative(type, exception)) {
  ...
  return false;
}

would be easier to read.

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
Source/bindings/dart/DartHandleProxy.cpp:940: v8::Local<v8::Object> proxy;
How do you want to handle Dart_Handles to DOM types?

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
Source/bindings/dart/DartHandleProxy.cpp:973: const char*
DartHandleProxy::getType(v8::Handle<v8::Value> value)
Is this supposed to return the JS type as a string?

[Jacob, 2013-07-18 18:49:28]

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
File Source/bindings/dart/DartHandleProxy.cpp (right):

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
Source/bindings/dart/DartHandleProxy.cpp:130: && DartNode::toNative(type,
exception);
On 2013/07/18 18:33:23, vsm wrote:
> Nit: I think this would be clearer as:
> 
> if (!DartNode::toNative(type, exception)) {
>   ...
>   return false;
> }
> 
> would be easier to read.

Done.

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
Source/bindings/dart/DartHandleProxy.cpp:940: v8::Local<v8::Object> proxy;
On 2013/07/18 18:33:23, vsm wrote:
> How do you want to handle Dart_Handles to DOM types?

Added a comment explaining why I currently don't want to.
It is for the same reason that we don't want to convert Dart lists to Arrays
anymore.

    // We could unwrap Dart DOM types to native types and then rewrap them as
    // JS DOM types but currently we choose not to instead returning JS
    // proxies for the Dart DOM types so that the Dart DOM APIs for DOM types
    // are exposed in the debugger instead of the JS APIs.
    // We attempt to get the best of both worlds by providing the method
    // getJavaScriptType to let debugger APIs treat these Dart proxies for
    // DOM types like native DOM types for cases such as visual Node
    // highlighting.

https://chromiumcodereview.appspot.com/18169002/diff/42001/Source/bindings/da...
Source/bindings/dart/DartHandleProxy.cpp:973: const char*
DartHandleProxy::getType(v8::Handle<v8::Value> value)
On 2013/07/18 18:33:23, vsm wrote:
> Is this supposed to return the JS type as a string?

Bad method name.
I've changed it to the more descriptive
getJavaScriptType

[vsm, 2013-07-18 18:50:48]

Still LGTM

[Jacob, 2013-07-18 18:51:23]

Committed patchset #10 manually as r1301 (presubmit successful).

[siva, 2013-10-03 22:41:31]

https://chromiumcodereview.appspot.com/18169002/diff/34001/Source/bindings/da...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://chromiumcodereview.appspot.com/18169002/diff/34001/Source/bindings/da...
Source/bindings/dart/DartDOMWrapper.h:194: // Node objects rather than objects
that implement the Node interface.
Could we open an issue and add a TODO(issue number) here so that we
remember and track this.