Support new style JS interop

Support new style JS interop

BUG=
R=terry@google.com, vsm@google.com

Committed: https://src.chromium.org/viewvc/blink/branches/dart/dartium?view=rev&revision=198008

[Jacob, 2015-06-18 22:22:43]

jacobr@google.com changed reviewers:
+ terry@google.com, vsm@google.com

[Jacob, 2015-06-18 22:22:44]

https://codereview.chromium.org/1177953010/diff/1/Source/bindings/core/dart/D...
File Source/bindings/core/dart/DartInjectedScript.cpp (right):

https://codereview.chromium.org/1177953010/diff/1/Source/bindings/core/dart/D...
Source/bindings/core/dart/DartInjectedScript.cpp:976: V8Scope v8scope(domData);
Not sure why this ever worked without a V8Scope

https://codereview.chromium.org/1177953010/diff/1/Source/bindings/core/dart/D...
File Source/bindings/core/dart/DartJsInteropData.h (right):

https://codereview.chromium.org/1177953010/diff/1/Source/bindings/core/dart/D...
Source/bindings/core/dart/DartJsInteropData.h:48: bool jsObjectImplDefined() {
return m_jsObjectImplDefined; }
Needed so that we can determine whether it is too late to define a JSObjectImpl
class that implements all of the JavaScript interfaces we are interested in.

[terry, 2015-06-25 14:11:46]

lgtm just a minor comment.

https://codereview.chromium.org/1177953010/diff/40001/Source/bindings/core/da...
File Source/bindings/core/dart/DartJsInterop.cpp (right):

https://codereview.chromium.org/1177953010/diff/40001/Source/bindings/core/da...
Source/bindings/core/dart/DartJsInterop.cpp:54: 
Jacob can you make a note that:

v8::String::NewFromUtf8 usages should be cached for constant strings (Siva
mentioned this in some stuff I was doing).  I'll take a look at fixing this with
my checkin.  Waiting on Siva to send me where/how it should be cached based on
the isolate - I think?

[Jacob, 2015-06-25 16:49:50]

https://codereview.chromium.org/1177953010/diff/40001/Source/bindings/core/da...
File Source/bindings/core/dart/DartJsInterop.cpp (right):

https://codereview.chromium.org/1177953010/diff/40001/Source/bindings/core/da...
Source/bindings/core/dart/DartJsInterop.cpp:54: 
On 2015/06/25 14:11:46, terry wrote:
> Jacob can you make a note that:
> 
> v8::String::NewFromUtf8 usages should be cached for constant strings (Siva
> mentioned this in some stuff I was doing).  I'll take a look at fixing this
with
> my checkin.  Waiting on Siva to send me where/how it should be cached based on
> the isolate - I think?

Done.

[vsm, 2015-06-26 14:46:34]

lgtm

https://codereview.chromium.org/1177953010/diff/60001/Source/bindings/core/da...
File Source/bindings/core/dart/DartJsInterop.cpp (right):

https://codereview.chromium.org/1177953010/diff/60001/Source/bindings/core/da...
Source/bindings/core/dart/DartJsInterop.cpp:1320: // just returning.
Throw an exception?  Should be able to steal the boilerplate for that.

[Jacob, 2015-06-26 20:53:31]

PTAL. I've also added extra tests for the new behavior added and fixed a bug I
noticed in push handling variable #s of elements.
https://codereview.chromium.org/1213873004

https://codereview.chromium.org/1177953010/diff/60001/Source/bindings/core/da...
File Source/bindings/core/dart/DartJsInterop.cpp (right):

https://codereview.chromium.org/1177953010/diff/60001/Source/bindings/core/da...
Source/bindings/core/dart/DartJsInterop.cpp:1320: // just returning.
On 2015/06/26 14:46:33, vsm wrote:
> Throw an exception?  Should be able to steal the boilerplate for that.

I shouldn't throw an exception here. Added some somewhat ugly code to actually
call the JS array method in this case which happens to actually behave like it
should far more often than I expected.

[vsm, 2015-06-29 13:49:26]

https://codereview.chromium.org/1177953010/diff/80001/Source/bindings/core/da...
File Source/bindings/core/dart/DartJsInterop.cpp (right):

https://codereview.chromium.org/1177953010/diff/80001/Source/bindings/core/da...
Source/bindings/core/dart/DartJsInterop.cpp:1311: v8::Isolate* v8Isolate =
v8::Isolate::GetCurrent();
Do we actually ever get down this path?  And if so, why not throw an exception?

E.g., If I tear off a DOM method and invoke it with a different this, I get a
TypeError.

[Jacob, 2015-06-29 17:56:08]

https://codereview.chromium.org/1177953010/diff/80001/Source/bindings/core/da...
File Source/bindings/core/dart/DartJsInterop.cpp (right):

https://codereview.chromium.org/1177953010/diff/80001/Source/bindings/core/da...
Source/bindings/core/dart/DartJsInterop.cpp:1311: v8::Isolate* v8Isolate =
v8::Isolate::GetCurrent();
On 2015/06/29 13:49:26, vsm wrote:
> Do we actually ever get down this path?  And if so, why not throw an
exception?
> 
> E.g., If I tear off a DOM method and invoke it with a different this, I get a
> TypeError.

Yeah it is a bit strange but I'm trying to make this object behave like Array
which keeps on trucking... successfully when called on a non Array object. Added
comment
/// The chrome devtools
// appear to go down exactly this code path as I got crashes in the devtools
// due to info.Holder not being a DartHandleProxy. I haven't investigated
// whether devtools would still function properly if we threw an exception
// which would match the behavior of DOM classes when a method is called on the
// wrong object. The current behavior while strange matches the behavior
// observed when an method from Array is called on an arbitrary JS object.

[vsm, 2015-06-29 18:07:07]

On 2015/06/29 17:56:08, Jacob wrote:
>
https://codereview.chromium.org/1177953010/diff/80001/Source/bindings/core/da...
> File Source/bindings/core/dart/DartJsInterop.cpp (right):
> 
>
https://codereview.chromium.org/1177953010/diff/80001/Source/bindings/core/da...
> Source/bindings/core/dart/DartJsInterop.cpp:1311: v8::Isolate* v8Isolate =
> v8::Isolate::GetCurrent();
> On 2015/06/29 13:49:26, vsm wrote:
> > Do we actually ever get down this path?  And if so, why not throw an
> exception?
> > 
> > E.g., If I tear off a DOM method and invoke it with a different this, I get
a
> > TypeError.
> 
> Yeah it is a bit strange but I'm trying to make this object behave like Array
> which keeps on trucking... successfully when called on a non Array object.
Added
> comment
> /// The chrome devtools
> // appear to go down exactly this code path as I got crashes in the devtools
> // due to info.Holder not being a DartHandleProxy. I haven't investigated
> // whether devtools would still function properly if we threw an exception
> // which would match the behavior of DOM classes when a method is called on
the
> // wrong object. The current behavior while strange matches the behavior
> // observed when an method from Array is called on an arbitrary JS object.

lgtm

I guess the key point here is that for a Dart proxy to masquerade as a JS array,
the proxy's tear-offs must be able to masquerade as array tear-offs as well. 
And devtools might actually depend on that....

[Jacob, 2015-06-29 18:21:48]

Committed patchset #5 (id:80001) manually as 198008 (presubmit successful).