[tracing] Support ConvertableToTraceFormat argument type.

[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277
Committed: https://crrev.com/dcac49af485fe5d4c0027f153901435dbb29c232
Committed: https://crrev.com/4810f41a521227ccab4f231aa4a439d790428953
Cr-Original-Commit-Position: refs/heads/master@{#39742}
Cr-Commit-Position: refs/heads/master@{#39789}

[alph, 2016-09-22 23:31:12]

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

[commit-bot: I haz the power, 2016-09-22 23:31:13]

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

[alph, 2016-09-22 23:40:11]

alph@chromium.org changed reviewers:
+ caseq@chromium.org, fmeawad@chromium.org, jochen@chromium.org

[alph, 2016-09-22 23:40:12]

Folks, please take a look. This one does not introduce TracedValue.

[commit-bot: I haz the power, 2016-09-23 00:10:42]

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

[commit-bot: I haz the power, 2016-09-23 00:10:43]

Dry run: This issue passed the CQ dry run.

[caseq, 2016-09-23 01:00:50]

https://codereview.chromium.org/2367603002/diff/1/src/tracing/trace-event.h
File src/tracing/trace-event.h (right):

https://codereview.chromium.org/2367603002/diff/1/src/tracing/trace-event.h#n...
src/tracing/trace-event.h:505: static V8_INLINE void
SetTraceValue(std::unique_ptr<T>&& ptr,
This does not look well -- we thus accept unique_ptrs to anything, but will only
properly manage ownership for those that are parametrized as
ConvertableToTraceFormat. Should this one just be non-template?

Also, style guide forbids rvalue refs usage outside of moveable constructors and
perfect forwarding.

[alph, 2016-09-23 06:33:24]

PTAL

https://codereview.chromium.org/2367603002/diff/1/src/tracing/trace-event.h
File src/tracing/trace-event.h (right):

https://codereview.chromium.org/2367603002/diff/1/src/tracing/trace-event.h#n...
src/tracing/trace-event.h:505: static V8_INLINE void
SetTraceValue(std::unique_ptr<T>&& ptr,
On 2016/09/23 01:00:50, caseq wrote:
> This does not look well -- we thus accept unique_ptrs to anything, but will
only
> properly manage ownership for those that are parametrized as
> ConvertableToTraceFormat. Should this one just be non-template?
> 
> Also, style guide forbids rvalue refs usage outside of moveable constructors
and
> perfect forwarding.

Done.

[caseq, 2016-09-23 17:12:50]

This mostly look good, but I'm a bit concerned with the way we manage ownership
of the ConvertableToTraceFormat -- this is not how we used to do it in other
places (e.g. chrome's base or blink) and this is more dangerous (to give an
example, an older implementation of platform's AddTraceEvent() would I assume
quietly leak memory. I reckon the trade off is some simplicity of the interface,
but I'm not sure it's justified. Fadi, wdyt?

[alph, 2016-09-23 22:06:39]

On 2016/09/23 17:12:50, caseq wrote:
> This mostly look good, but I'm a bit concerned with the way we manage
ownership
> of the ConvertableToTraceFormat -- this is not how we used to do it in other
> places (e.g. chrome's base or blink) and this is more dangerous (to give an
> example, an older implementation of platform's AddTraceEvent() would I assume
> quietly leak memory. I reckon the trade off is some simplicity of the
interface,
> but I'm not sure it's justified. Fadi, wdyt?

Great idea! Done.

[caseq, 2016-09-23 23:48:34]

lgtm

https://codereview.chromium.org/2367603002/diff/60001/include/v8-platform.h
File include/v8-platform.h (right):

https://codereview.chromium.org/2367603002/diff/60001/include/v8-platform.h#n...
include/v8-platform.h:192: return 0;
call another one here instead, so that when the embedder implements this one,
another one is not called.

https://codereview.chromium.org/2367603002/diff/60001/src/tracing/trace-event.h
File src/tracing/trace-event.h (right):

https://codereview.chromium.org/2367603002/diff/60001/src/tracing/trace-event...
src/tracing/trace-event.h:479: return platform->AddTraceEvent(phase,
category_group_enabled, name, scope, id,
just drop this one and let the default implementation of the former to do the
job.

https://codereview.chromium.org/2367603002/diff/60001/src/tracing/trace-event...
src/tracing/trace-event.h:531: static V8_INLINE void
SetTraceValue(std::unique_ptr<T> ptr, unsigned char* type,
nit: consider std::enable_if<std::is_convertible<T, ConvertibleToTraceFormat>>

[alph, 2016-09-24 00:24:07]

All done. Jochen could you please do the owner review. Thanks!

https://codereview.chromium.org/2367603002/diff/60001/include/v8-platform.h
File include/v8-platform.h (right):

https://codereview.chromium.org/2367603002/diff/60001/include/v8-platform.h#n...
include/v8-platform.h:192: return 0;
On 2016/09/23 23:48:34, caseq wrote:
> call another one here instead, so that when the embedder implements this one,
> another one is not called.

Done.

https://codereview.chromium.org/2367603002/diff/60001/src/tracing/trace-event.h
File src/tracing/trace-event.h (right):

https://codereview.chromium.org/2367603002/diff/60001/src/tracing/trace-event...
src/tracing/trace-event.h:479: return platform->AddTraceEvent(phase,
category_group_enabled, name, scope, id,
On 2016/09/23 23:48:34, caseq wrote:
> just drop this one and let the default implementation of the former to do the
> job.

Done.

https://codereview.chromium.org/2367603002/diff/60001/src/tracing/trace-event...
src/tracing/trace-event.h:531: static V8_INLINE void
SetTraceValue(std::unique_ptr<T> ptr, unsigned char* type,
On 2016/09/23 23:48:34, caseq wrote:
> nit: consider std::enable_if<std::is_convertible<T, ConvertibleToTraceFormat>>

Done.

[alph, 2016-09-24 00:24:26]

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

[commit-bot: I haz the power, 2016-09-24 00:24:33]

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-09-24 00:55:25]

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

[commit-bot: I haz the power, 2016-09-24 00:55:25]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-09-26 15:05:44]

lgtm

[fmeawad, 2016-09-26 15:11:11]

lgtm

[lpy, 2016-09-26 20:28:06]

Description was changed from

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277
==========

to

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277
==========

[alph, 2016-09-26 21:22:22]

The CQ bit was checked by alph@chromium.org

[alph, 2016-09-26 21:22:23]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2367603002/#ps80001
(title: "fixing nits")

[commit-bot: I haz the power, 2016-09-26 21:22:38]

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-09-26 21:54:39]

Description was changed from

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277
==========

to

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277
==========

[commit-bot: I haz the power, 2016-09-26 21:54:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-26 21:55:04]

Description was changed from

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277
==========

to

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277

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

[commit-bot: I haz the power, 2016-09-26 21:55:05]

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

[Michael Achenbach, 2016-09-27 07:01:25]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2371103002/ by machenbach@chromium.org.

The reason for reverting is: Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

See also:
https://github.com/v8/v8/wiki/Blink%20layout%20tests.

[alph, 2016-09-27 17:36:24]

Description was changed from

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277

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

to

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277

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

[alph, 2016-09-27 17:39:55]

The CQ bit was checked by alph@chromium.org

[alph, 2016-09-27 17:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org,
jochen@chromium.org, fmeawad@chromium.org
Link to the patchset: https://codereview.chromium.org/2367603002/#ps100001
(title: "Fix layout tests failures & reland.")

[commit-bot: I haz the power, 2016-09-27 17:40:03]

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-09-27 18:08:40]

Description was changed from

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277

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

to

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277

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

[commit-bot: I haz the power, 2016-09-27 18:08:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-27 18:09:02]

Description was changed from

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277

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

to

==========
[tracing] Support ConvertableToTraceFormat argument type.

Drive-by: Use perfect forwarding for AddTraceEvent arguments.

BUG=406277

Committed: https://crrev.com/dcac49af485fe5d4c0027f153901435dbb29c232
Committed: https://crrev.com/4810f41a521227ccab4f231aa4a439d790428953
Cr-Original-Commit-Position: refs/heads/master@{#39742}
Cr-Commit-Position: refs/heads/master@{#39789}
==========

[commit-bot: I haz the power, 2016-09-27 18:09:04]

Patchset 6 (id:??) landed as
https://crrev.com/4810f41a521227ccab4f231aa4a439d790428953
Cr-Commit-Position: refs/heads/master@{#39789}

[Toon Verwaest, 2016-09-28 18:13:53]

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

[Toon Verwaest, 2016-09-28 18:13:54]

This seems to break node integration:
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr...