[dartium] Use ScriptWrappable when there are multiple Dart wrappers.

[dartium] Use ScriptWrappable when there are multiple Dart wrappers.

BUG=
R=asiva@google.com

Committed: https://src.chromium.org/viewvc/multivm/branches/1650/blink?view=rev&revision=175710

[rmacnak, 2014-06-05 01:26:20]

https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartScriptWrappable.h (right):

https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:79: wrapperInfo->wrapper = wrapper;
This seems to actually come from a path related to cross-frame wrappers rather
than custom elements. Maybe reaching here should be considered a bug?

https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:98: ASSERT_NOT_REACHED();
Don't seem to hit this path, even with the new test.

[rmacnak, 2014-06-05 18:01:10]

Cross frame Location and History seem to have been to only places replacing
wrappers without first removing the old one. Now passing with asserts against
trying to set a wrapper when we already have one.

[siva, 2014-06-05 18:05:49]

Adding Pete to the cc list so he can suggest a test case that would exercise the
custom element upgrade path.

https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartScriptWrappable.h (right):

https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:98: ASSERT_NOT_REACHED();
Pete might be able to help with a test case that would exercise this path.

On 2014/06/05 01:26:21, rmacnak wrote:
> Don't seem to hit this path, even with the new test.

https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:155: wrapperInfo->wrappers.remove(i);
Add a comment here and above too that when the last Dart Wrapper is removed we
transition back to a v8WrapperOrTypeInfo.

[blois, 2014-06-05 18:17:30]

On 2014/06/05 18:05:49, siva wrote:
> Adding Pete to the cc list so he can suggest a test case that would exercise
the
> custom element upgrade path.
> 
>
https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
> File Source/bindings/dart/DartScriptWrappable.h (right):
> 
>
https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
> Source/bindings/dart/DartScriptWrappable.h:98: ASSERT_NOT_REACHED();
> Pete might be able to help with a test case that would exercise this path.
> 
> On 2014/06/05 01:26:21, rmacnak wrote:
> > Don't seem to hit this path, even with the new test.
> 
>
https://codereview.chromium.org/313183003/diff/20001/Source/bindings/dart/Dar...
> Source/bindings/dart/DartScriptWrappable.h:155:
wrapperInfo->wrappers.remove(i);
> Add a comment here and above too that when the last Dart Wrapper is removed we
> transition back to a v8WrapperOrTypeInfo.

Easiest way to explore custom elements is probably
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/test....
After this returns an upgraded wrapper then both wrappers can be used (though
Vijay has been talking about neutering the first).

Note that I just realized this test was not enabled for Dartium (I recently
fixed an issue on the blink side), will turn it on with
https://codereview.chromium.org/313363002/

[rmacnak, 2014-06-06 01:23:10]

html/custom/created_callback_test seems to be the only test that tries to set a
wrapper for a wrappable that already has one.

Still haven't found a way to trigger this with multiple isolates.

[vsm, 2014-06-06 13:39:46]

Looking nice.  One other case to test is when there are multiple frames.  E.g.,
parent page and child frame where:

1. Parent and child have wrappers on child's window, history, location.

2. Parent and child have wrappers on child's document or other nodes.  This case
requires JS interop as we currently don't provide access via Dart.

https://chromiumcodereview.appspot.com/313183003/diff/60001/LayoutTests/dart/...
File LayoutTests/dart/multi-wrapper-test.html (right):

https://chromiumcodereview.appspot.com/313183003/diff/60001/LayoutTests/dart/...
LayoutTests/dart/multi-wrapper-test.html:58: var custom =
querySelector('#custom');
Perhaps query this also before you launch the dom isolates and make sure it's
the same object before and after?

https://chromiumcodereview.appspot.com/313183003/diff/60001/Source/bindings/d...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://chromiumcodereview.appspot.com/313183003/diff/60001/Source/bindings/d...
Source/bindings/dart/DartDOMWrapper.h:69: #ifdef DEBUG
Isn't this redundant with the ASSERT?

https://chromiumcodereview.appspot.com/313183003/diff/60001/Source/bindings/d...
Source/bindings/dart/DartDOMWrapper.h:306: #ifdef DEBUG
ditto

https://chromiumcodereview.appspot.com/313183003/diff/60001/Source/bindings/d...
File Source/bindings/dart/DartScriptWrappable.h (right):

https://chromiumcodereview.appspot.com/313183003/diff/60001/Source/bindings/d...
Source/bindings/dart/DartScriptWrappable.h:98: ASSERT_NOT_REACHED();
This is still not reached by your test?  (ASSERT is debug only.)

[rmacnak, 2014-06-06 21:26:31]

Finally hit that path. The secret is to use a custom element whose constructor
throws.

https://codereview.chromium.org/313183003/diff/60001/LayoutTests/dart/multi-w...
File LayoutTests/dart/multi-wrapper-test.html (right):

https://codereview.chromium.org/313183003/diff/60001/LayoutTests/dart/multi-w...
LayoutTests/dart/multi-wrapper-test.html:58: var custom =
querySelector('#custom');
On 2014/06/06 13:39:45, vsm wrote:
> Perhaps query this also before you launch the dom isolates and make sure it's
> the same object before and after?

Added.

https://codereview.chromium.org/313183003/diff/60001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartDOMWrapper.h (right):

https://codereview.chromium.org/313183003/diff/60001/Source/bindings/dart/Dar...
Source/bindings/dart/DartDOMWrapper.h:69: #ifdef DEBUG
On 2014/06/06 13:39:45, vsm wrote:
> Isn't this redundant with the ASSERT?

Leftover from copy-paste from asserts that needed an api scope. Removed.

[rmacnak, 2014-06-06 21:30:09]

On 2014/06/06 13:39:46, vsm wrote:
> Looking nice.  One other case to test is when there are multiple frames. 
E.g.,
> parent page and child frame where:
> 
> 1. Parent and child have wrappers on child's window, history, location.

Added.

> 2. Parent and child have wrappers on child's document or other nodes.  This
case
> requires JS interop as we currently don't provide access via Dart.

Added, but this isn't very interesting since the parent has Dart wrappers on JS
wrappers of the child's nodes rather than Dart wrappers directly on the nodes.

[siva, 2014-06-06 21:35:35]

lgtm

https://codereview.chromium.org/313183003/diff/80001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartScriptWrappable.h (right):

https://codereview.chromium.org/313183003/diff/80001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:81: // Inflate to a multiwrapper,
unimplemented.
You can remove the 'unimplemented' part now.

https://codereview.chromium.org/313183003/diff/80001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:155: if (len == 1) {
Add a comment here that we are deflating back to a v8WrapperOrTypeInfo as all
dart wrappers have been deleted.

[rmacnak, 2014-06-06 22:22:29]

https://codereview.chromium.org/313183003/diff/80001/Source/bindings/dart/Dar...
File Source/bindings/dart/DartScriptWrappable.h (right):

https://codereview.chromium.org/313183003/diff/80001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:81: // Inflate to a multiwrapper,
unimplemented.
On 2014/06/06 21:35:34, siva wrote:
> You can remove the 'unimplemented' part now.

Done.

https://codereview.chromium.org/313183003/diff/80001/Source/bindings/dart/Dar...
Source/bindings/dart/DartScriptWrappable.h:155: if (len == 1) {
On 2014/06/06 21:35:35, siva wrote:
> Add a comment here that we are deflating back to a v8WrapperOrTypeInfo as all
> dart wrappers have been deleted.

Done.

[rmacnak, 2014-06-06 22:30:45]

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