bindings: Introduces ScriptWrappable::associateWithWrapper in addition to wrap.

bindings: Introduces ScriptWrappable::associateWithWrapper in addition to wrap.

generate_constructor_wrapper macro in bindings/templates/methods.cpp
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/templates/methods.cpp&q=%22macro%20generate_constructor_wrapper%22%20file:methods.cpp&sq=package:chromium&l=611
would like to reuse the existing wrapper object, and in that case,
it needs to associate the impl with the existing wrapper.
(Note that wrap method creates a new wrapper.)

This CL introduces ScriptWrappable::associateWithWrapper to allow it.
This method does not need to be virtual because it must be called from
only a) wrap method, or b) V8T::constructorCallback, where we already
know the right WrapperTypeInfo.

BUG=235436
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181548

[Yuki, 2014-09-05 12:04:34]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2014-09-05 12:04:34]

Could you review this CL?

[haraken, 2014-09-05 16:41:39]

> This CL introduces ScriptWrappable::associateWithWrapper to allow it.
> This method does not need to be virtual because it must be called from
> only a) wrap method, or b) V8T::constructorCallback, where we already
> know the right WrapperTypeInfo.

Even if it's currently guaranteed that T::associateWithWrapper is called only
from T::wrap, it's fragile to define non-virtual T::associateWithWrapper methods
in a subclass of ScriptWrappable. It can easily lead to a wrong dispatch.

As I commented below, can we completely remove T::associateWithWrapper from
subclasses of ScriptWrappable?

One more question: Will this CL remove all [Custom=Wrap]s from the code base?

https://codereview.chromium.org/542113003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/542113003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:5788:
frame()->script().windowProxy(world)->updateDocumentWrapper(wrapper);

The line 5876 - 5788 should be put in Document::wrap (after the
Document::associateWithWrapper call).

T::associateWithWrapper should not call anything but refObject and
associateObjectWithWrapperNonTemplate.

BTW, if T::associateWithWrapper always calls refObject and
associateObjectWithWrapperNonTemplate, can we remove all
T::associateWithWrapper's from all subclasses of ScriptWrappable?

https://codereview.chromium.org/542113003/diff/1/Source/core/events/MessageEv...
File Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/542113003/diff/1/Source/core/events/MessageEv...
Source/core/events/MessageEvent.cpp:207: return
associateWithWrapperInternal(wrapper, isolate);

I guess that associateWithWrapperInternal should be executed in wrap, but should
not be executed in associateWithWrapper.

https://codereview.chromium.org/542113003/diff/1/Source/core/html/ImageData.cpp
File Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/542113003/diff/1/Source/core/html/ImageData.c...
Source/core/html/ImageData.cpp:137: return associateWithWrapperInternal(wrapper,
wrapper, isolate);

Ditto. I guess associateWithWrapperInternal should be called in wrap but should
not be called in associateWithWrapper.

[Yuki, 2014-09-08 07:56:05]

As discussed offline, I made associateWithWrapper a virtual method.

We still have several [Custom=Wrap]: CSSRule, CSSValue, ArrayBufferView and
DataView.  All of them except for DataView are not ScriptWrappable.  DataView
itself is ScriptWrappable, but its wrap method delegates to
V8TypedArray<DataView>::wrap, and we cannot simply remove the custom wrap.

https://codereview.chromium.org/542113003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/542113003/diff/1/Source/core/dom/Document.cpp...
Source/core/dom/Document.cpp:5788:
frame()->script().windowProxy(world)->updateDocumentWrapper(wrapper);
On 2014/09/05 16:41:39, haraken wrote:
> 
> The line 5876 - 5788 should be put in Document::wrap (after the
> Document::associateWithWrapper call).
> 
> T::associateWithWrapper should not call anything but refObject and
> associateObjectWithWrapperNonTemplate.
> 
> BTW, if T::associateWithWrapper always calls refObject and
> associateObjectWithWrapperNonTemplate, can we remove all
> T::associateWithWrapper's from all subclasses of ScriptWrappable?

As we talked offline, we need two entry points for the case a) we create a new
wrapper from the impl, and case b) JS already created a wrapper object and we'd
like to re-use that wrapper in callback constructors.  The method
T::associateWithWrapper is the entry point for the case b).

I believe that line 5786 - 5788 are (potentially) necessary for the case b),
too, although Document doesn't have a callback constructor.

https://codereview.chromium.org/542113003/diff/1/Source/core/events/MessageEv...
File Source/core/events/MessageEvent.cpp (right):

https://codereview.chromium.org/542113003/diff/1/Source/core/events/MessageEv...
Source/core/events/MessageEvent.cpp:207: return
associateWithWrapperInternal(wrapper, isolate);
On 2014/09/05 16:41:39, haraken wrote:
> 
> I guess that associateWithWrapperInternal should be executed in wrap, but
should
> not be executed in associateWithWrapper.

We need associateWithWrapperInternal for callback constructor's case, too. 
That's why MessageEvent needed Custom=Wrap.

https://codereview.chromium.org/542113003/diff/1/Source/core/html/ImageData.cpp
File Source/core/html/ImageData.cpp (right):

https://codereview.chromium.org/542113003/diff/1/Source/core/html/ImageData.c...
Source/core/html/ImageData.cpp:137: return associateWithWrapperInternal(wrapper,
wrapper, isolate);
On 2014/09/05 16:41:39, haraken wrote:
> 
> Ditto. I guess associateWithWrapperInternal should be called in wrap but
should
> not be called in associateWithWrapper.

Done.

[haraken, 2014-09-08 08:48:45]

LGTM

https://codereview.chromium.org/542113003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/542113003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptWrappable.h:123: // Associates the instance with
the existing wrapper. Returns |wrapper|.

Why does this need to return a wrapper? The returned wrapper should be the same
as the wrapper passed to the method.

https://codereview.chromium.org/542113003/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/542113003/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5774:
wrapperType->installConditionallyEnabledProperties(wrapper, isolate);

Probably this needs to be in associateWithWrapper, doesn't it?

[Yuki, 2014-09-08 09:00:22]

https://codereview.chromium.org/542113003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/542113003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptWrappable.h:123: // Associates the instance with
the existing wrapper. Returns |wrapper|.
On 2014/09/08 08:48:45, haraken wrote:
> 
> Why does this need to return a wrapper? The returned wrapper should be the
same
> as the wrapper passed to the method.

Not necessary, it's just convenient that we can
    return associateWithWrapper(...);
instead of
    associateWithWrapper(...);
    return wrapper;

If you don't like this style, I'll change the return type to void.

https://codereview.chromium.org/542113003/diff/60001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/542113003/diff/60001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5774:
wrapperType->installConditionallyEnabledProperties(wrapper, isolate);
On 2014/09/08 08:48:45, haraken wrote:
> 
> Probably this needs to be in associateWithWrapper, doesn't it?

Reading bindings/templates/methods.cpp, the original code doesn't call it. 
Though, I'm not 100% sure...

[haraken, 2014-09-08 09:07:23]

https://codereview.chromium.org/542113003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/542113003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptWrappable.h:123: // Associates the instance with
the existing wrapper. Returns |wrapper|.
On 2014/09/08 09:00:22, Yuki wrote:
> On 2014/09/08 08:48:45, haraken wrote:
> > 
> > Why does this need to return a wrapper? The returned wrapper should be the
> same
> > as the wrapper passed to the method.
> 
> Not necessary, it's just convenient that we can
>     return associateWithWrapper(...);
> instead of
>     associateWithWrapper(...);
>     return wrapper;
> 
> If you don't like this style, I'll change the return type to void.

Either is fine with me :)

[Yuki, 2014-09-08 09:12:22]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-09-08 09:12:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yukishiino@chromium.org/542113003/60001

[commit-bot: I haz the power, 2014-09-08 09:17:48]

Committed patchset #4 (id:60001) as 181548