bindings: Adds virtual ScriptWrappable::wrap method.

bindings: Adds virtual ScriptWrappable::wrap method.

Introduces a new virtual method ScriptWrappable::wrap and
uses it instead of blink::wrap(T*, ...).

Major design changes in this CL are:

* DEFINE_WRAPPERTYPEINFO defines virtual T::wrapperTypeInfo()
method in each script wrappable DOM classes.  With this macro,
ScriptWrappable::wrapperTypeInfo() returns the actual
WrapperTypeInfo of the instance.
Each script wrappable DOM classes must use this macro.

* New ScriptWrappable::wrap method plays the role of both of
old blink::wrap(T, ...) and V8T::createWrapper functions, and
the method is customizable in derived classes.

Follow-up CLs will remove the code which this CL make unnecessary
step by step.

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

[Yuki, 2014-08-29 04:46:51]

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

[Yuki, 2014-08-29 04:46:51]

Could you review this CL?

[haraken, 2014-08-29 05:24:04]

Just to confirm: If we add DEFINE_WRAPPERTYPEINFO to a class where it shouldn't
be added or miss adding DEFINE_WRAPPERTYPEINFO to a class where it should be
added, the error will be detected at compile time. Right?

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptWrappable.cpp (right):

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.cpp:36:
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(typeInfo()->derefObjectFunction ==
wrapperType->derefObjectFunction);

Do we need to check this? I guess we can drop this check now that the wrap
function is guaranteed to be a wrap function of the correct type (C++ compiler
guarantees it).

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.cpp:44:
V8DOMWrapper::associateObjectWithWrapperNonTemplate(this, wrapperType, wrapper,
isolate, WrapperConfiguration::Dependent);

It looks wrong to always use WrapperConfiguration::Dependent. Independent DOM
objects need to use WrapperConfiguration::Independent.

If you use WrapperConfiguration::Dependent for independent DOM objects, it won't
break anything but delay timing when the DOM objects are collected by V8 GC (and
thus regress performance).

I think we should add a test case to verify that independent DOM objects are
corrected in an expected GC round. A follow-up CL is fine.

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

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:153: // This method must be overridden
by using DEFINE_WRAPPERTYPEINFO macro.

by using => by

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:355: // that the class is
NoInterfaceObject.

Help me understand: What's the relationship between [NoInterfaceObject] and
DEFINE_WRAPPERTYPEINFO() ?

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:56: // class definition. Otherwise,
you have to write NotScriptWrappable in the IDL

NotScriptWrappable => [NotScriptWrappable]

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:59: // NotScriptWrappable is
inheritable.

Ditto.

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8SVGTestInterface.cpp (right):

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/tests/r...
Source/bindings/tests/results/V8SVGTestInterface.cpp:46: // NotScriptWrappable
is inheritable.

It's a bit redundant to generate this comment in all generated files. Can we
move the comment to ScriptWrappable? Then you can add one-line comment here:

// This should be defined in DEFINE_WRAPPERTYPEINFO in {{cpp_class}}.h.

[Yuki, 2014-09-01 06:06:40]

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptWrappable.cpp (right):

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.cpp:36:
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(typeInfo()->derefObjectFunction ==
wrapperType->derefObjectFunction);
On 2014/08/29 05:24:04, haraken wrote:
> 
> Do we need to check this? I guess we can drop this check now that the wrap
> function is guaranteed to be a wrap function of the correct type (C++ compiler
> guarantees it).

Done.

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.cpp:44:
V8DOMWrapper::associateObjectWithWrapperNonTemplate(this, wrapperType, wrapper,
isolate, WrapperConfiguration::Dependent);
On 2014/08/29 05:24:04, haraken wrote:
> 
> It looks wrong to always use WrapperConfiguration::Dependent. Independent DOM
> objects need to use WrapperConfiguration::Independent.
> 
> If you use WrapperConfiguration::Dependent for independent DOM objects, it
won't
> break anything but delay timing when the DOM objects are collected by V8 GC
(and
> thus regress performance).
> 
> I think we should add a test case to verify that independent DOM objects are
> corrected in an expected GC round. A follow-up CL is fine.

Nice catch!  Thanks for pointing this out.  As you already know, I'm now working
on this issue in a separate CL.

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

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:153: // This method must be overridden
by using DEFINE_WRAPPERTYPEINFO macro.
On 2014/08/29 05:24:04, haraken wrote:
> 
> by using => by

Done.

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:153: // This method must be overridden
by using DEFINE_WRAPPERTYPEINFO macro.
On 2014/08/29 05:24:04, haraken wrote:
> 
> by using => by

Done.

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:355: // that the class is
NoInterfaceObject.
On 2014/08/29 05:24:04, haraken wrote:
> 
> Help me understand: What's the relationship between [NoInterfaceObject] and
> DEFINE_WRAPPERTYPEINFO() ?

Added more comment.

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/templat...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:56: // class definition. Otherwise,
you have to write NotScriptWrappable in the IDL
On 2014/08/29 05:24:04, haraken wrote:
> 
> NotScriptWrappable => [NotScriptWrappable]

Done.

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/templat...
Source/bindings/templates/interface_base.cpp:59: // NotScriptWrappable is
inheritable.
On 2014/08/29 05:24:04, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/tests/r...
File Source/bindings/tests/results/V8SVGTestInterface.cpp (right):

https://codereview.chromium.org/478243002/diff/300001/Source/bindings/tests/r...
Source/bindings/tests/results/V8SVGTestInterface.cpp:46: // NotScriptWrappable
is inheritable.
On 2014/08/29 05:24:04, haraken wrote:
> 
> It's a bit redundant to generate this comment in all generated files. Can we
> move the comment to ScriptWrappable? Then you can add one-line comment here:
> 
> // This should be defined in DEFINE_WRAPPERTYPEINFO in {{cpp_class}}.h.

Done.

[haraken, 2014-09-01 06:15:14]

Do we need to land the other CL that addresses the Dependent/Independent issue
first? Or can we land this CL before fixing the Dependent/Independent issue?

[Yuki, 2014-09-01 07:02:45]

On 2014/09/01 06:15:14, haraken wrote:
> Do we need to land the other CL that addresses the Dependent/Independent issue
> first? Or can we land this CL before fixing the Dependent/Independent issue?

We need the other CL first.  Please hold on.  I'll ping you once I commit the
other CL.

[Yuki, 2014-09-01 13:54:50]

The Dependent/Independent lifetime issue was solved in a separate CL, and this
CL now uses the solution.  The hard coded "Dependent" lifetime was removed from
this CL.

For your first question (sorry, I forgot to answer):

If someone forgets to write DEFINE_WRAPPERTYPEINFO in the class definition, the
IDL compiler still generates T::s_wrapperTypeInfo and it causes undeclared
member error.

If someone writes DEFINE_WRAPPERTYPEINFO for the classes that specify
[NotScriptWrappable] or [NoInterfaceObject], or do not have *.idl file, then
T::s_wrapperTypeInfo is declared by DEFINE_WRAPPERTYPEINFO macro, but it's not
defined.  So it causes undefined reference error at link time (sorry, not
compile time).

If someone writes DEFINE_WRAPPERTYPEINFO for the classes that do not inherit
from ScriptWrappable and do not specify [NotScriptWrappable] either, you get a
compile error because wrapperTypeInfo() method is OVERRIDE, but any parent
classes declare it.

Could you take another look at this CL?

[haraken, 2014-09-02 01:10:11]

LGTM

What's your plan to remove ScriptWrappable::init()? Even if you plan to remove
ScriptWrappable::init() eventually, it's not a good idea to keep both
ScriptWrappable::init() and WRAPPERTYPEINFO() in the code base for a long time.
(We've already introduced too much binding complexity to the code base.)

If you can remove ScriptWrappable::init() by moving the contents into
WRAPPERTYPEINFO(), we want to do it in a follow-up CL. Once you no longer need
the contents, you can remove it from WRAPPERTYPEINFO().

https://codereview.chromium.org/478243002/diff/340001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptWrappable.cpp (right):

https://codereview.chromium.org/478243002/diff/340001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.cpp:31: const WrapperTypeInfo*
wrapperType = wrapperTypeInfo();

wrapperType => wrapperTypeInfo

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

https://codereview.chromium.org/478243002/diff/340001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:362: // that [NotScriptWrappable] is
inheritable.

It might be worth adding the summary of the comment like:

// A DOM class T must meet either of the following conditions:
//
// - T.idl inherits from [NotScriptWrappable] or has [NoInterfaceObject].
// - T inherits from ScriptWrappable and has DEFINE_WRAPPERTYPEINFO().

[Yuki, 2014-09-02 04:35:49]

I understand your concern and will remove ScriptWrappable::init soon, but I
think we need to retire createV8HTMLWrapper, etc. before removing
ScriptWrappable::init.  I'm also going to retire V8T::createWrapper, etc. which
depend on ScriptWrappable::init in order to make sure that we no longer need
ScriptWrappable::init.

Unfortunately, we cannot move ScriptWrappable::init into
DEFINE_WRAPPERTYPEINFO(), ecause those who depend on ScriptWrappable::init do
not necessarily call wrapperTypeInfo() method.

https://codereview.chromium.org/478243002/diff/340001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptWrappable.cpp (right):

https://codereview.chromium.org/478243002/diff/340001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.cpp:31: const WrapperTypeInfo*
wrapperType = wrapperTypeInfo();
On 2014/09/02 01:10:10, haraken wrote:
> 
> wrapperType => wrapperTypeInfo

We already have wrapperTypeInfo() method in the class, so I think it's better to
avoid the same name.

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

https://codereview.chromium.org/478243002/diff/340001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptWrappable.h:362: // that [NotScriptWrappable] is
inheritable.
On 2014/09/02 01:10:10, haraken wrote:
> 
> It might be worth adding the summary of the comment like:
> 
> // A DOM class T must meet either of the following conditions:
> //
> // - T.idl inherits from [NotScriptWrappable] or has [NoInterfaceObject].
> // - T inherits from ScriptWrappable and has DEFINE_WRAPPERTYPEINFO().

Done.

[Yuki, 2014-09-02 04:36:14]

I understand your concern and will remove ScriptWrappable::init soon, but I
think we need to retire createV8HTMLWrapper, etc. before removing
ScriptWrappable::init.  I'm also going to retire V8T::createWrapper, etc. which
depend on ScriptWrappable::init in order to make sure that we no longer need
ScriptWrappable::init.

Unfortunately, we cannot move ScriptWrappable::init into
DEFINE_WRAPPERTYPEINFO(), because those who depend on ScriptWrappable::init do
not necessarily call wrapperTypeInfo() method.

[haraken, 2014-09-02 04:45:23]

> Unfortunately, we cannot move ScriptWrappable::init into
> DEFINE_WRAPPERTYPEINFO(), because those who depend on ScriptWrappable::init do
> not necessarily call wrapperTypeInfo() method.

I'm just curious but why don't they match? I was expecting that a set of DOM
objects that need ScriptWrappable::init matches with a set of DOM objects that
need s_wrapperTypeInfo.

[Yuki, 2014-09-02 04:55:41]

On 2014/09/02 04:45:23, haraken wrote:
> > Unfortunately, we cannot move ScriptWrappable::init into
> > DEFINE_WRAPPERTYPEINFO(), because those who depend on ScriptWrappable::init
do
> > not necessarily call wrapperTypeInfo() method.
> 
> I'm just curious but why don't they match? I was expecting that a set of DOM
> objects that need ScriptWrappable::init matches with a set of DOM objects that
> need s_wrapperTypeInfo.

Yes, you're right regarding about a set of classes, but not call paths.  virtual
ScriptWrappable::wrap is called only at blink::wrap(T, ...).  There are other
paths to get directly to V8T::createWrapper, which depends on
ScriptWrappable::init.  In those cases, ScriptWrappable::wrap is not called.  We
need to eliminate such paths first.

[haraken, 2014-09-02 05:29:50]

On 2014/09/02 04:55:41, Yuki wrote:
> On 2014/09/02 04:45:23, haraken wrote:
> > > Unfortunately, we cannot move ScriptWrappable::init into
> > > DEFINE_WRAPPERTYPEINFO(), because those who depend on
ScriptWrappable::init
> do
> > > not necessarily call wrapperTypeInfo() method.
> > 
> > I'm just curious but why don't they match? I was expecting that a set of DOM
> > objects that need ScriptWrappable::init matches with a set of DOM objects
that
> > need s_wrapperTypeInfo.
> 
> Yes, you're right regarding about a set of classes, but not call paths. 
virtual
> ScriptWrappable::wrap is called only at blink::wrap(T, ...).  There are other
> paths to get directly to V8T::createWrapper, which depends on
> ScriptWrappable::init.  In those cases, ScriptWrappable::wrap is not called. 
We
> need to eliminate such paths first.

Thanks, I'm getting to understand the situation. It's crazy we have multiple
call paths to create a wrapper... (it's never your fault; you're fixing faults
we've made so far :-)

Would it be possible to change the current code base in a reasonable timeline so
that all wrapper creations are done through ScriptWrappable::wrap() (and then
remove V8T::createWrapper and ScriptWrappable::init)?

I'm a bit concerned about leaving fragile code in the code base for a long time.

[Yuki, 2014-09-02 05:56:20]

On 2014/09/02 05:29:50, haraken wrote:
> On 2014/09/02 04:55:41, Yuki wrote:
> > On 2014/09/02 04:45:23, haraken wrote:
> > > > Unfortunately, we cannot move ScriptWrappable::init into
> > > > DEFINE_WRAPPERTYPEINFO(), because those who depend on
> ScriptWrappable::init
> > do
> > > > not necessarily call wrapperTypeInfo() method.
> > > 
> > > I'm just curious but why don't they match? I was expecting that a set of
DOM
> > > objects that need ScriptWrappable::init matches with a set of DOM objects
> that
> > > need s_wrapperTypeInfo.
> > 
> > Yes, you're right regarding about a set of classes, but not call paths. 
> virtual
> > ScriptWrappable::wrap is called only at blink::wrap(T, ...).  There are
other
> > paths to get directly to V8T::createWrapper, which depends on
> > ScriptWrappable::init.  In those cases, ScriptWrappable::wrap is not called.

> We
> > need to eliminate such paths first.
> 
> Thanks, I'm getting to understand the situation. It's crazy we have multiple
> call paths to create a wrapper... (it's never your fault; you're fixing faults
> we've made so far :-)
> 
> Would it be possible to change the current code base in a reasonable timeline
so
> that all wrapper creations are done through ScriptWrappable::wrap() (and then
> remove V8T::createWrapper and ScriptWrappable::init)?
> 
> I'm a bit concerned about leaving fragile code in the code base for a long
time.

My plan is a little behind the original schedule, but I still think I can finish
this refactoring in this month.

[Yuki, 2014-09-02 06:03:37]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-09-02 06:04:23]

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

[commit-bot: I haz the power, 2014-09-02 06:18:55]

Committed patchset #20 (id:380001) as 181216