[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

BUG=chromium:468240
Committed: https://crrev.com/a22545438325b77d8926b8e6adefd0dfe34ee1a0
Cr-Commit-Position: refs/heads/master@{#440389}

[Michael Lippautz, 2016-12-21 17:21:22]

Description was changed from

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

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

to

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

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

[Michael Lippautz, 2016-12-21 17:21:23]

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

[Michael Lippautz, 2016-12-21 17:22:15]

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

https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/TraceTraits.h:206:
static_assert(!NeedsAdjustAndMark<T>::value,
This static_assert makes sure we don't trace T for T being a mixin.

[Michael Lippautz, 2016-12-21 17:23:16]

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

[commit-bot: I haz the power, 2016-12-21 17:23:53]

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

[sof, 2016-12-21 18:57:28]

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

https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/TraceTraits.h:206:
static_assert(!NeedsAdjustAndMark<T>::value,
On 2016/12/21 17:22:14, Michael Lippautz wrote:
> This static_assert makes sure we don't trace T for T being a mixin.

IsGarbageCollectedMixin<T>::value is the more readable alternative.

[commit-bot: I haz the power, 2016-12-21 19:16:46]

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

[commit-bot: I haz the power, 2016-12-21 19:16:46]

Dry run: This issue passed the CQ dry run.

[Marcel Hlopko, 2016-12-21 19:46:27]

I like the simplification, lgtm to land this

[haraken, 2016-12-22 00:48:55]

LGTM

[haraken, 2016-12-22 00:50:07]

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

https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/TraceTraits.h:208: return
reinterpret_cast<const T*>(t);

Also we can add DCHECK(t->checkHeader()).

[Michael Lippautz, 2016-12-22 08:45:23]

Thanks!

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

https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/TraceTraits.h:206:
static_assert(!NeedsAdjustAndMark<T>::value,
On 2016/12/21 18:57:28, sof wrote:
> On 2016/12/21 17:22:14, Michael Lippautz wrote:
> > This static_assert makes sure we don't trace T for T being a mixin.
> 
> IsGarbageCollectedMixin<T>::value is the more readable alternative.

Done.

https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/heap/TraceTraits.h:208: return
reinterpret_cast<const T*>(t);
On 2016/12/22 00:50:07, haraken wrote:
> 
> Also we can add DCHECK(t->checkHeader()).

Done.

[Michael Lippautz, 2016-12-22 08:45:28]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-12-22 08:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, hlopko@chromium.org
Link to the patchset: https://codereview.chromium.org/2595053002/#ps20001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-12-22 08:46:01]

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-12-22 08:59:01]

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

[commit-bot: I haz the power, 2016-12-22 08:59:02]

Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Michael Lippautz, 2016-12-22 09:06:44]

On 2016/12/21 18:57:28, sof wrote:
>
https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):
> 
>
https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/heap/TraceTraits.h:206:
> static_assert(!NeedsAdjustAndMark<T>::value,
> On 2016/12/21 17:22:14, Michael Lippautz wrote:
> > This static_assert makes sure we don't trace T for T being a mixin.
> 
> IsGarbageCollectedMixin<T>::value is the more readable alternative.

Actually, IsGarbageCollectedMixin seems to be true also for the mixin
application, which is not the intended assertion behavior. E.g. we have to allow
Document, applying the TreeScope mixin, to trace wrappers.

[Michael Lippautz, 2016-12-22 09:06:48]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-12-22 09:06:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, hlopko@chromium.org
Link to the patchset: https://codereview.chromium.org/2595053002/#ps40001
(title: "IsGarbageCollectedMixin -> NeedsAdjustAndMark")

[commit-bot: I haz the power, 2016-12-22 09:07:05]

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

[sof, 2016-12-22 09:14:22]

On 2016/12/22 09:06:44, Michael Lippautz wrote:
> On 2016/12/21 18:57:28, sof wrote:
> >
>
https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/heap/TraceTraits.h (right):
> > 
> >
>
https://codereview.chromium.org/2595053002/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/heap/TraceTraits.h:206:
> > static_assert(!NeedsAdjustAndMark<T>::value,
> > On 2016/12/21 17:22:14, Michael Lippautz wrote:
> > > This static_assert makes sure we don't trace T for T being a mixin.
> > 
> > IsGarbageCollectedMixin<T>::value is the more readable alternative.
> 
> Actually, IsGarbageCollectedMixin seems to be true also for the mixin
> application, which is not the intended assertion behavior. E.g. we have to
allow
> Document, applying the TreeScope mixin, to trace wrappers.

Hmm, ok - I thought !NeedsAdjustAndMark<T>::value ==
IsGarbageCollectedMixin<T>::value.

[commit-bot: I haz the power, 2016-12-22 09:36:20]

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

[commit-bot: I haz the power, 2016-12-22 09:36:21]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Michael Lippautz, 2016-12-22 09:40:53]

The CQ bit was checked by mlippautz@chromium.org

[Michael Lippautz, 2016-12-22 09:40:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, hlopko@chromium.org
Link to the patchset: https://codereview.chromium.org/2595053002/#ps60001
(title: "Guard DCHECK with DCHECK_IS_ON() as the method is only conditionally
compiled")

[commit-bot: I haz the power, 2016-12-22 09:41:11]

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

[Michael Lippautz, 2016-12-22 11:13:14]

Description was changed from

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

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

to

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

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

[Michael Lippautz, 2016-12-22 11:13:38]

Description was changed from

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

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

to

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

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

[Hannes Payer (out of office), 2016-12-22 11:36:45]

hpayer@chromium.org changed reviewers:
+ hpayer@chromium.org

[Hannes Payer (out of office), 2016-12-22 11:36:46]

Awesome catch Michi, LGTM

[commit-bot: I haz the power, 2016-12-22 11:52:33]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482399653843580,
"parent_rev": "2c2b8bbeb2c04cb5e30e8c1b1581ecf1ca02b362", "commit_rev":
"fe6b9b58ec9390050092264385ab338d34ab03ad"}

[commit-bot: I haz the power, 2016-12-22 11:53:01]

Description was changed from

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

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

to

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

BUG=chromium:468240

Review-Url: https://codereview.chromium.org/2595053002
==========

[commit-bot: I haz the power, 2016-12-22 11:53:02]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-22 11:56:03]

Description was changed from

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

BUG=chromium:468240

Review-Url: https://codereview.chromium.org/2595053002
==========

to

==========
[wrapper-tracing] Simplify tracing by forbidding tracing from within mixins

We don't have a single use case where we would require wrapper tracing from
within mixins. Remove the infra that would be needed, including the catch-all
handler.

Note that we can now statically check not tracing from within a mixin using a
static_assert.

BUG=chromium:468240

Committed: https://crrev.com/a22545438325b77d8926b8e6adefd0dfe34ee1a0
Cr-Commit-Position: refs/heads/master@{#440389}
==========

[commit-bot: I haz the power, 2016-12-22 11:56:04]

Patchset 4 (id:??) landed as
https://crrev.com/a22545438325b77d8926b8e6adefd0dfe34ee1a0
Cr-Commit-Position: refs/heads/master@{#440389}