[wrapper-tracing] Redesign dispatching on non-inheriting cases

[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remove CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG=chromium:701295
Review-Url: https://codereview.chromium.org/2748103002
Cr-Commit-Position: refs/heads/master@{#456714}
Committed: https://chromium.googlesource.com/chromium/src/+/96fd6f8611075233b0963e699dfd49305bbb1406

[Michael Lippautz, 2017-03-14 12:49:47]

Description was changed from

==========
[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remoe CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG=
==========

to

==========
[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remoe CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG=chromium:701295
==========

[Michael Lippautz, 2017-03-14 12:50:39]

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

[Michael Lippautz, 2017-03-14 12:51:59]

ptal: I think this way we can still unify the visitors as they do not know
anything about the special cases while at the same time not exposing any
GC-internal behavor on TraceWrapperBase and ScriptWrappable.

[Michael Lippautz, 2017-03-14 12:52:54]

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

[commit-bot: I haz the power, 2017-03-14 12:53:22]

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

[Michael Lippautz, 2017-03-14 12:56:13]

Description was changed from

==========
[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remoe CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG=chromium:701295
==========

to

==========
[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remove CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG=chromium:701295
==========

[haraken, 2017-03-14 13:00:12]

LGTM

https://codereview.chromium.org/2748103002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2748103002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:73: #define
DEFINE_TRACE_TRAIT_FOR_WRAPPER_WITHOUT_BASE(ClassName) \

DEFINE_TRAIT_FOR_TRACE_WRAPPERS (_WITHOUT_BASE) ?

Just to confirm: If a developer forgets to add DEFINE_TRAIT_FOR_TRACE_WRAPPERS
or TraceWrapperBase, she will hit a compile error, right?

https://codereview.chromium.org/2748103002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:149: void
markWrappersInAllWorlds(const TraceWrapperBase*) const {

Why is this needed?

[Michael Lippautz, 2017-03-14 13:43:43]

https://codereview.chromium.org/2748103002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/heap/WrapperVisitor.h (right):

https://codereview.chromium.org/2748103002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:73: #define
DEFINE_TRACE_TRAIT_FOR_WRAPPER_WITHOUT_BASE(ClassName) \
On 2017/03/14 13:00:11, haraken wrote:
> 
> DEFINE_TRAIT_FOR_TRACE_WRAPPERS (_WITHOUT_BASE) ?
> 

Done.

> Just to confirm: If a developer forgets to add DEFINE_TRAIT_FOR_TRACE_WRAPPERS
> or TraceWrapperBase, she will hit a compile error, right?

Yes, because the regular TraceTrait<T>::traceMarkedWrapper will not compile.

https://codereview.chromium.org/2748103002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/WrapperVisitor.h:149: void
markWrappersInAllWorlds(const TraceWrapperBase*) const {
On 2017/03/14 13:00:12, haraken wrote:
> 
> Why is this needed?

TraceTrait<T>::traceMarkedWrapper calls markWrappersInAllWorlds for all types,
even those that are TraceWrapperBase and not ScriptWrappable. This is the
pragmatic way of handling that case without further introducing further
specialization.

[Michael Lippautz, 2017-03-14 13:43:47]

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

[commit-bot: I haz the power, 2017-03-14 13:44:02]

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

[haraken, 2017-03-14 14:19:00]

LGTM

[adithyas, 2017-03-14 14:38:34]

lgtm

Thanks!

[commit-bot: I haz the power, 2017-03-14 15:51:19]

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

[commit-bot: I haz the power, 2017-03-14 15:51:20]

Dry run: This issue passed the CQ dry run.

[Michael Lippautz, 2017-03-14 15:51:38]

The CQ bit was checked by mlippautz@chromium.org

[commit-bot: I haz the power, 2017-03-14 15:52:14]

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, 2017-03-14 15:57:03]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489506698354740,
"parent_rev": "921a1a3db660c3086a41f381763d88d3a4ad2cf9", "commit_rev":
"96fd6f8611075233b0963e699dfd49305bbb1406"}

[commit-bot: I haz the power, 2017-03-14 15:57:48]

Description was changed from

==========
[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remove CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG=chromium:701295
==========

to

==========
[wrapper-tracing] Redesign dispatching on non-inheriting cases

Use a specialization of TraceTrait<>::traceMarkedWrapper for types that
do not inherit from TraceWrapperBase. This way we avoid exposing
non-virtual methods exposing GC details on managed objects.

Drive-by: Remove CanTraceWrappers macro as any misuse of the other macros
will yield in compile time errors.

BUG=chromium:701295

Review-Url: https://codereview.chromium.org/2748103002
Cr-Commit-Position: refs/heads/master@{#456714}
Committed:
https://chromium.googlesource.com/chromium/src/+/96fd6f8611075233b0963e699dfd...
==========

[commit-bot: I haz the power, 2017-03-14 15:57:54]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/96fd6f8611075233b0963e699dfd...