[wrapper-tracing] Bring remoteplayback up to date

[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org

Committed: https://crrev.com/424ba959fb61163ddb49388932259fb34e5a7036
Cr-Commit-Position: refs/heads/master@{#426449}

[Michael Lippautz, 2016-10-19 12:14:33]

Description was changed from

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes the test:

BUG=chromium:468240
==========

to

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes the test:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
==========

[Michael Lippautz, 2016-10-19 12:17:07]

Patchset #1 (id:1) has been deleted

[Michael Lippautz, 2016-10-19 12:18:34]

Description was changed from

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes the test:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
==========

to

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
==========

[Michael Lippautz, 2016-10-19 12:19:15]

Description was changed from

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
==========

to

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org
==========

[Michael Lippautz, 2016-10-19 12:19:15]

mlippautz@chromium.org changed reviewers:
+ haraken@chromium.org, hlopko@chromium.org

[Michael Lippautz, 2016-10-19 12:20:21]

Please take a careful look at this one.

I will rebaseline the bindings tests if we take this approach.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:43: class
CORE_EXPORT TraceWrapperBase {
We could use this super class in cases where the extra vtable pointer doesn't
matter or we cannot properly include because of component builds.

A future CL could also make it a base class for ScriptWrappable, getting rid of
the special visitor code.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl:20: public
TraceWrapperBase {
If you think that the extra vtable ptr is too much, we would need to have
special handling in RemotePlayback::traceWrappers.

[haraken, 2016-10-19 12:32:39]

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl:20: public
TraceWrapperBase {
On 2016/10/19 12:20:21, Michael Lippautz wrote:
> If you think that the extra vtable ptr is too much, we would need to have
> special handling in RemotePlayback::traceWrappers.

I don't think sizeof(callback function) really matters, so I think the vtable
ptr is fine.

However, would you help me understand why the callback function needs to inherit
from TraceWrapperBase? For example, we're tracing ElementRareData without making
ElementRareData TraceWrapperBase.

[Michael Lippautz, 2016-10-19 13:13:14]

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl:20: public
TraceWrapperBase {
On 2016/10/19 12:32:38, haraken wrote:
> On 2016/10/19 12:20:21, Michael Lippautz wrote:
> > If you think that the extra vtable ptr is too much, we would need to have
> > special handling in RemotePlayback::traceWrappers.
> 
> I don't think sizeof(callback function) really matters, so I think the vtable
> ptr is fine.
> 
> However, would you help me understand why the callback function needs to
inherit
> from TraceWrapperBase? For example, we're tracing ElementRareData without
making
> ElementRareData TraceWrapperBase.

Sure; ElementRareData and similar classes are part of the "special" cases in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/...

This macro generates the appropriate dispatchers. Ideally, all special cases
would just inherit from this TraceWrapperBase and we wouldn't need them.
However, I think for ElementRareData we want to save the vtable ptr. We could
revisit some of the cases and check whether they already have a vtable ptr and
if yes just inherit from TraceWrapperBase.

Additionally, we cannot use the special case macro for the callback in this CL
as the callback is part of modules, which ScriptWrappableVisitor and friends
(platform) cannot  see (linker).

[Marcel Hlopko, 2016-10-19 15:08:17]

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:43: class
CORE_EXPORT TraceWrapperBase {
On 2016/10/19 at 12:20:21, Michael Lippautz wrote:
> We could use this super class in cases where the extra vtable pointer doesn't
matter or we cannot properly include because of component builds.
> 
> A future CL could also make it a base class for ScriptWrappable, getting rid
of the special visitor code.

Nit: TraceWrappersBase?

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:27:
visitor->traceWrappers(&reinterpret_cast<ScopedPersistent<v8::Object>&>(const_cast<ScopedPersistent<v8::Function>&>(m_callback)));
Kentaro, just to be sure, should really each blink callback object keep its
wrapper alive?

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl:20: public
TraceWrapperBase {
On 2016/10/19 at 13:13:14, Michael Lippautz wrote:
> On 2016/10/19 12:32:38, haraken wrote:
> > On 2016/10/19 12:20:21, Michael Lippautz wrote:
> > > If you think that the extra vtable ptr is too much, we would need to have
> > > special handling in RemotePlayback::traceWrappers.
> > 
> > I don't think sizeof(callback function) really matters, so I think the
vtable
> > ptr is fine.
> > 
> > However, would you help me understand why the callback function needs to
inherit
> > from TraceWrapperBase? For example, we're tracing ElementRareData without
making
> > ElementRareData TraceWrapperBase.
> 
> Sure; ElementRareData and similar classes are part of the "special" cases in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/...
> 
> This macro generates the appropriate dispatchers. Ideally, all special cases
would just inherit from this TraceWrapperBase and we wouldn't need them.
However, I think for ElementRareData we want to save the vtable ptr. We could
revisit some of the cases and check whether they already have a vtable ptr and
if yes just inherit from TraceWrapperBase.
> 
> Additionally, we cannot use the special case macro for the callback in this CL
as the callback is part of modules, which ScriptWrappableVisitor and friends
(platform) cannot  see (linker).

To reiterate what Michael is saying, if a class is not inheriting from
ScriptWrappable, we need to tell somehow WrapperVisitor to trace it. Before we
only had WRAPPER_VISITOR_SPECIAL_CLASSES, where you had to add your class.
TraceWrapperBase solves the same problem, it marks the class so WrapperVisitor
traces it, but in a nicer way, because you do the change in your class, not in
some arcane WrapperVisitor.h file.

[Michael Lippautz, 2016-10-19 15:13:41]

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:27:
visitor->traceWrappers(&reinterpret_cast<ScopedPersistent<v8::Object>&>(const_cast<ScopedPersistent<v8::Function>&>(m_callback)));
On 2016/10/19 15:08:17, Marcel Hlopko wrote:
> Kentaro, just to be sure, should really each blink callback object keep its
> wrapper alive?

This infra merely adds the capability of blink callbacks keeping the wrapper
alive. You still require the parent object to call traceWrappers on the
callback, which only RemotePlayback currently does.

[haraken, 2016-10-19 15:31:43]

LGTM

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:43: class
CORE_EXPORT TraceWrapperBase {
On 2016/10/19 15:08:17, Marcel Hlopko wrote:
> On 2016/10/19 at 12:20:21, Michael Lippautz wrote:
> > We could use this super class in cases where the extra vtable pointer
doesn't
> matter or we cannot properly include because of component builds.
> > 
> > A future CL could also make it a base class for ScriptWrappable, getting rid
> of the special visitor code.
> 
> Nit: TraceWrappersBase?

Maybe we should unify TraceWrappers and TraceWrapper into one of the two. We
have TraceWrapperMember. We have TRACE_WRAPPERS.

I'd slightly prefer TraceWrapper but anyway a follow-up CL is fine.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:27:
visitor->traceWrappers(&reinterpret_cast<ScopedPersistent<v8::Object>&>(const_cast<ScopedPersistent<v8::Function>&>(m_callback)));
On 2016/10/19 15:13:41, Michael Lippautz wrote:
> On 2016/10/19 15:08:17, Marcel Hlopko wrote:
> > Kentaro, just to be sure, should really each blink callback object keep its
> > wrapper alive?
> 
> This infra merely adds the capability of blink callbacks keeping the wrapper
> alive. You still require the parent object to call traceWrappers on the
> callback, which only RemotePlayback currently does.

In almost all cases, the spec requires to add a strong reference from the
wrapper to the callback functions.

In any case, making the parent object explicitly call traceWrappers() sounds
like a good programming pattern. We can represent any reachability with it.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:27:
visitor->traceWrappers(&reinterpret_cast<ScopedPersistent<v8::Object>&>(const_cast<ScopedPersistent<v8::Function>&>(m_callback)));

Can we add an overloaded function to traceWrappers() so that we don't need to
write complicated reinterpret_cast on the caller side?

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl:20: public
TraceWrapperBase {
On 2016/10/19 15:08:17, Marcel Hlopko wrote:
> On 2016/10/19 at 13:13:14, Michael Lippautz wrote:
> > On 2016/10/19 12:32:38, haraken wrote:
> > > On 2016/10/19 12:20:21, Michael Lippautz wrote:
> > > > If you think that the extra vtable ptr is too much, we would need to
have
> > > > special handling in RemotePlayback::traceWrappers.
> > > 
> > > I don't think sizeof(callback function) really matters, so I think the
> vtable
> > > ptr is fine.
> > > 
> > > However, would you help me understand why the callback function needs to
> inherit
> > > from TraceWrapperBase? For example, we're tracing ElementRareData without
> making
> > > ElementRareData TraceWrapperBase.
> > 
> > Sure; ElementRareData and similar classes are part of the "special" cases in
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/...
> > 
> > This macro generates the appropriate dispatchers. Ideally, all special cases
> would just inherit from this TraceWrapperBase and we wouldn't need them.
> However, I think for ElementRareData we want to save the vtable ptr. We could
> revisit some of the cases and check whether they already have a vtable ptr and
> if yes just inherit from TraceWrapperBase.
> > 
> > Additionally, we cannot use the special case macro for the callback in this
CL
> as the callback is part of modules, which ScriptWrappableVisitor and friends
> (platform) cannot  see (linker).
> 
> To reiterate what Michael is saying, if a class is not inheriting from
> ScriptWrappable, we need to tell somehow WrapperVisitor to trace it. Before we
> only had WRAPPER_VISITOR_SPECIAL_CLASSES, where you had to add your class.
> TraceWrapperBase solves the same problem, it marks the class so WrapperVisitor
> traces it, but in a nicer way, because you do the change in your class, not in
> some arcane WrapperVisitor.h file.

Thanks for the clarification -- yes, TraceWrappersBase definitely looks nicer.

I'm okay with handling ElementRareData (and a couple of classes where the object
size really matters) specially; e.g., keep using
WRAPPER_VISITOR_SPECIAL_CLASSES.

Also making ScriptWrappable inherit from TraceWrappersBase totally makes sense.

[Michael Lippautz, 2016-10-19 15:44:35]

Thanks for the comments and reviews.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:43: class
CORE_EXPORT TraceWrapperBase {
On 2016/10/19 15:31:43, haraken wrote:
> On 2016/10/19 15:08:17, Marcel Hlopko wrote:
> > On 2016/10/19 at 12:20:21, Michael Lippautz wrote:
> > > We could use this super class in cases where the extra vtable pointer
> doesn't
> > matter or we cannot properly include because of component builds.
> > > 
> > > A future CL could also make it a base class for ScriptWrappable, getting
rid
> > of the special visitor code.
> > 
> > Nit: TraceWrappersBase?
> 
> Maybe we should unify TraceWrappers and TraceWrapper into one of the two. We
> have TraceWrapperMember. We have TRACE_WRAPPERS.
> 
> I'd slightly prefer TraceWrapper but anyway a follow-up CL is fine.

Will do renaming, and inheriting changes to ScriptWrappable in a followup CL.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:27:
visitor->traceWrappers(&reinterpret_cast<ScopedPersistent<v8::Object>&>(const_cast<ScopedPersistent<v8::Function>&>(m_callback)));
On 2016/10/19 15:31:43, haraken wrote:
> On 2016/10/19 15:13:41, Michael Lippautz wrote:
> > On 2016/10/19 15:08:17, Marcel Hlopko wrote:
> > > Kentaro, just to be sure, should really each blink callback object keep
its
> > > wrapper alive?
> > 
> > This infra merely adds the capability of blink callbacks keeping the wrapper
> > alive. You still require the parent object to call traceWrappers on the
> > callback, which only RemotePlayback currently does.
> 
> In almost all cases, the spec requires to add a strong reference from the
> wrapper to the callback functions.
> 
> In any case, making the parent object explicitly call traceWrappers() sounds
> like a good programming pattern. We can represent any reachability with it.

Acknowledged.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.cpp.tmpl:27:
visitor->traceWrappers(&reinterpret_cast<ScopedPersistent<v8::Object>&>(const_cast<ScopedPersistent<v8::Function>&>(m_callback)));
On 2016/10/19 15:31:43, haraken wrote:
> 
> Can we add an overloaded function to traceWrappers() so that we don't need to
> write complicated reinterpret_cast on the caller side?

traceWrappers is overloaded with the two basic v8 type v8::Object and v8::Value.


I'd rather add a 
  template<typename S>
  const ScopedPersistent<S>& ScopedPersistent<T>::cast() const;
and use that one here.

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl
(right):

https://chromiumcodereview.appspot.com/2434763002/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/callback_function.h.tmpl:20: public
TraceWrapperBase {
On 2016/10/19 15:31:43, haraken wrote:
> On 2016/10/19 15:08:17, Marcel Hlopko wrote:
> > On 2016/10/19 at 13:13:14, Michael Lippautz wrote:
> > > On 2016/10/19 12:32:38, haraken wrote:
> > > > On 2016/10/19 12:20:21, Michael Lippautz wrote:
> > > > > If you think that the extra vtable ptr is too much, we would need to
> have
> > > > > special handling in RemotePlayback::traceWrappers.
> > > > 
> > > > I don't think sizeof(callback function) really matters, so I think the
> > vtable
> > > > ptr is fine.
> > > > 
> > > > However, would you help me understand why the callback function needs to
> > inherit
> > > > from TraceWrapperBase? For example, we're tracing ElementRareData
without
> > making
> > > > ElementRareData TraceWrapperBase.
> > > 
> > > Sure; ElementRareData and similar classes are part of the "special" cases
in
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/...
> > > 
> > > This macro generates the appropriate dispatchers. Ideally, all special
cases
> > would just inherit from this TraceWrapperBase and we wouldn't need them.
> > However, I think for ElementRareData we want to save the vtable ptr. We
could
> > revisit some of the cases and check whether they already have a vtable ptr
and
> > if yes just inherit from TraceWrapperBase.
> > > 
> > > Additionally, we cannot use the special case macro for the callback in
this
> CL
> > as the callback is part of modules, which ScriptWrappableVisitor and friends
> > (platform) cannot  see (linker).
> > 
> > To reiterate what Michael is saying, if a class is not inheriting from
> > ScriptWrappable, we need to tell somehow WrapperVisitor to trace it. Before
we
> > only had WRAPPER_VISITOR_SPECIAL_CLASSES, where you had to add your class.
> > TraceWrapperBase solves the same problem, it marks the class so
WrapperVisitor
> > traces it, but in a nicer way, because you do the change in your class, not
in
> > some arcane WrapperVisitor.h file.
> 
> Thanks for the clarification -- yes, TraceWrappersBase definitely looks nicer.
> 
> I'm okay with handling ElementRareData (and a couple of classes where the
object
> size really matters) specially; e.g., keep using
> WRAPPER_VISITOR_SPECIAL_CLASSES.
> 
> Also making ScriptWrappable inherit from TraceWrappersBase totally makes
sense.

Acknowledged.

[Michael Lippautz, 2016-10-20 09:43:52]

The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 09:44:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 11:05:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 11:05:34]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2016-10-20 11:13:10]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-10-20 11:13:11]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2434763002/#ps60001
(title: "Reset bingings results")

[commit-bot: I haz the power, 2016-10-20 11:13:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-20 11:18:24]

Description was changed from

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org
==========

to

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org
==========

[commit-bot: I haz the power, 2016-10-20 11:18:25]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:17:19]

Description was changed from

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org
==========

to

==========
[wrapper-tracing] Bring remoteplayback up to date

Fixes:
  media/remoteplayback/availability-callback-gc.html

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org

Committed: https://crrev.com/424ba959fb61163ddb49388932259fb34e5a7036
Cr-Commit-Position: refs/heads/master@{#426449}
==========

[commit-bot: I haz the power, 2016-10-21 13:17:21]

Patchset 3 (id:??) landed as
https://crrev.com/424ba959fb61163ddb49388932259fb34e5a7036
Cr-Commit-Position: refs/heads/master@{#426449}