tracing: Make ConvertableToTraceFormat move-only scoped_ptr

tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState.
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer.

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=thakis,jochen,tbarzic,mnaganov,skyostil

Committed: https://crrev.com/cb1afb351f2232e22b4be9bbbe7c0449decee471
Cr-Commit-Position: refs/heads/master@{#378263}

[Primiano Tucci (use gerrit), 2016-02-22 21:34:04]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent move-only.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.
 - Introduce a CTTFRefCounted to deal with the very singular cases
   of tracing clients wanting the CTTF to be refcounted (really just
   cc::Layer)

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 - Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator

BUG=559117
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent move-only.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.
 - Introduce a CTTFRefCounted to deal with the very singular cases
   of tracing clients wanting the CTTF to be refcounted (really just
   cc::Layer)

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 - Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Primiano Tucci (use gerrit), 2016-02-22 21:50:40]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent move-only.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.
 - Introduce a CTTFRefCounted to deal with the very singular cases
   of tracing clients wanting the CTTF to be refcounted (really just
   cc::Layer)

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 - Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent move-only.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.
 - Introduce a CTTFRefCounted to deal with the very singular cases
   of tracing clients wanting the CTTF to be refcounted (really just
   cc::Layer)

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 - Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 - cc::Layer DebugInfo

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Primiano Tucci (use gerrit), 2016-02-22 22:39:30]

primiano@chromium.org changed reviewers:
+ oysteine@chromium.org

[Primiano Tucci (use gerrit), 2016-02-22 22:39:31]

FYI. I think we should have first the discussion on
https://groups.google.com/a/chromium.org/forum/#!topic/tracing/JBk2-0P5Lmk
before getting to the actual review.
If you have any general thought feel free to comment here.

[Primiano Tucci (use gerrit), 2016-02-25 20:39:02]

Patchset #1 (id:1) has been deleted

[Primiano Tucci (use gerrit), 2016-02-25 20:39:19]

Patchset #1 (id:20001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-25 20:43:31]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent move-only.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.
 - Introduce a CTTFRefCounted to deal with the very singular cases
   of tracing clients wanting the CTTF to be refcounted (really just
   cc::Layer)

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 - Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 - cc::Layer DebugInfo

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent move-only.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.
 - Introduce a CTTFRefCounted to deal with the very singular cases
   of tracing clients wanting the CTTF to be refcounted (really just
   cc::Layer)

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
     moved scoped_ptr (as opposite as copied), effectively moving the info
     to the pending layer only. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Primiano Tucci (use gerrit), 2016-02-25 20:44:09]

Patchset #1 (id:40001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-25 20:45:32]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent move-only.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.
 - Introduce a CTTFRefCounted to deal with the very singular cases
   of tracing clients wanting the CTTF to be refcounted (really just
   cc::Layer)

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
     moved scoped_ptr (as opposite as copied), effectively moving the info
     to the pending layer only. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
     moved scoped_ptr (as opposite as copied), effectively moving the info
     to the pending layer only. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Primiano Tucci (use gerrit), 2016-02-25 20:52:44]

Patchset #1 (id:60001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-25 21:17:21]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
     moved scoped_ptr (as opposite as copied), effectively moving the info
     to the pending layer only. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), effectively moving the info
   to the pending layer only. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Primiano Tucci (use gerrit), 2016-02-25 21:17:22]

primiano@chromium.org changed reviewers:
+ danakj@chromium.org, ssid@chromium.org

[Primiano Tucci (use gerrit), 2016-02-25 21:18:27]

+danakj for cc/layers/* (as per offline thread)
+ssid for a second eye on base/trace_event/*memory*
+oysteine for everything else, good luck :P

[danakj, 2016-02-25 23:10:07]

cc LGTM

https://codereview.chromium.org/1717283003/diff/80001/cc/debug/benchmark_inst...
File cc/debug/benchmark_instrumentation.cc (right):

https://codereview.chromium.org/1717283003/diff/80001/cc/debug/benchmark_inst...
cc/debug/benchmark_instrumentation.cc:23: auto record_data = make_scoped_ptr(new
base::trace_event::TracedValue());
current guidance is "don't spell smart pointers as auto".

we haz plans to improve that but for now spell it out please. you could write
scoped_ptr<T> value(new T); to keep it more terse while writing the type.

https://codereview.chromium.org/1717283003/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1717283003/diff/80001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:612: layer->SetDebugInfo(std::move(debug_info_));
For future archeologists. I don't see us tracing this info on the pending tree
ever currently, and if we only trace it for layers that have something novel
from the active tree that is probably okay too.

If it ends up being some serious problem, we could have the pending tree trace
its active twin's debug_info_ also.

[Primiano Tucci (use gerrit), 2016-02-26 01:45:46]

Patchset #2 (id:100001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-26 02:07:13]

Patchset #3 (id:140001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-26 07:01:03]

primiano@chromium.org changed reviewers:
+ petrcermak@chromium.org
- ssid@chromium.org

[Primiano Tucci (use gerrit), 2016-02-26 07:01:05]

+petrcermak for base/trace_event/*memory*

https://codereview.chromium.org/1717283003/diff/80001/cc/debug/benchmark_inst...
File cc/debug/benchmark_instrumentation.cc (right):

https://codereview.chromium.org/1717283003/diff/80001/cc/debug/benchmark_inst...
cc/debug/benchmark_instrumentation.cc:23: auto record_data = make_scoped_ptr(new
base::trace_event::TracedValue());
On 2016/02/25 23:10:07, danakj wrote:
> current guidance is "don't spell smart pointers as auto".
> 
> we haz plans to improve that but for now spell it out please. you could write
> scoped_ptr<T> value(new T); to keep it more terse while writing the type.

Ok changed all the places in the CL where I did auto ... = make_scoped to be
more verbose.

FWIW btw, despite this CL the rest of the codebase seems to have plenty of auto
.* = make_scoped (I might have contributed quite to that a bit in the past :-/)

[petrcermak, 2016-02-26 10:21:03]

LGTM for base/trace_event/*memory* and base/trace_event/*heap_profiler* with a
few comments.

Thanks,
Petr

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
File base/trace_event/memory_allocator_dump_unittest.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_allocator_dump_unittest.cc:11: #include
"base/trace_event/heap_profiler_stack_frame_deduplicator.h"
I don't think you need these imports here

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
File base/trace_event/memory_dump_session_state.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_dump_session_state.cc:15:
scoped_ptr<StackFrameDeduplicator> stack_frame_deduplicator) {
Perhaps DCHECK(stack_frame_deduplicator_ == nullptr)?

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_dump_session_state.cc:21: type_name_deduplicator_ =
std::move(type_name_deduplicator);
ditto

[oystein (OOO til 10th of July), 2016-02-26 19:46:55]

Enthusiastic lgtm :D

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:558:
scoped_ptr<ConvertableToTraceFormat> event_value(traced_value);
This looks a bit funky. Why do we still keep the raw pointer around here?

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
base/trace_event/trace_event.h:846: template <class ARG1_CONVERTABLE_TYPE, class
ARG2_TYPE>
nit: No technical difference, but 'typename' for ARG2_TYPE feels cleaner to me
(since these typically won't be classes).

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
File base/trace_event/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
base/trace_event/trace_event_argument_unittest.cc:133: auto dict_value =
make_scoped_ptr(new TracedValue);
nit: inconsistent usage of 'new TracedValue' vs 'new TracedValue()' here.

[Primiano Tucci (use gerrit), 2016-02-26 19:57:02]

Patchset #6 (id:220001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-26 21:26:59]

Patchset #6 (id:240001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-26 22:52:50]

danak@ sorry for the double review. I realized only later that moving the
DebugInfo to the active layer makes some tracing layout tests fail. So sounds
like we cannot have pure move semantics there
I introduced a refptr wrapper in cc::debug, can I ask you to take another look
to changes in cc/ ?

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
File base/trace_event/memory_allocator_dump_unittest.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_allocator_dump_unittest.cc:11: #include
"base/trace_event/heap_profiler_stack_frame_deduplicator.h"
On 2016/02/26 10:21:03, petrcermak wrote:
> I don't think you need these imports here

Done.

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_dump_manager.cc:558:
scoped_ptr<ConvertableToTraceFormat> event_value(traced_value);
On 2016/02/26 19:46:55, Oystein wrote:
> This looks a bit funky. Why do we still keep the raw pointer around here?

Ah it was a funky way to get around the fact that TRACE_EVENT_API wants a
_ptr<CTTF>* but here we have and need a _ptr<TracedValue>.
But now with std::move semantics is easier to convert. Done

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
File base/trace_event/memory_dump_session_state.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_dump_session_state.cc:15:
scoped_ptr<StackFrameDeduplicator> stack_frame_deduplicator) {
On 2016/02/26 10:21:03, petrcermak wrote:
> Perhaps DCHECK(stack_frame_deduplicator_ == nullptr)?

Done.

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/memor...
base/trace_event/memory_dump_session_state.cc:21: type_name_deduplicator_ =
std::move(type_name_deduplicator);
On 2016/02/26 10:21:03, petrcermak wrote:
> ditto

Done.

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
base/trace_event/trace_event.h:846: template <class ARG1_CONVERTABLE_TYPE, class
ARG2_TYPE>
On 2016/02/26 19:46:55, Oystein wrote:
> nit: No technical difference, but 'typename' for ARG2_TYPE feels cleaner to me
> (since these typically won't be classes).

Yeah I know I was very tempted, the thing is that we use class everywhere here
and I kept consistent.
Let's postpone it to another cleanup-only CL? (would be nice if we find a better
way to deal with this crazyness and all these macros/templates)

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
File base/trace_event/trace_event_argument_unittest.cc (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
base/trace_event/trace_event_argument_unittest.cc:133: auto dict_value =
make_scoped_ptr(new TracedValue);
On 2016/02/26 19:46:55, Oystein wrote:
> nit: inconsistent usage of 'new TracedValue' vs 'new TracedValue()' here.

Fixed (even if I think is a broader problem, should do  a massive pass in
another CL to fix this)

[Primiano Tucci (use gerrit), 2016-02-26 22:53:21]

Forgot the link to the failure:
https://storage.googleapis.com/chromium-layout-test-archives/win_chromium_rel...

[danakj, 2016-02-26 22:59:20]

On 2016/02/26 22:52:50, Primiano (throttled til Mar 4) wrote:
> danak@ sorry for the double review. I realized only later that moving the
> DebugInfo to the active layer makes some tracing layout tests fail. So sounds
> like we cannot have pure move semantics there
> I introduced a refptr wrapper in cc::debug, can I ask you to take another look
> to changes in cc/ ?

You could probably move ownership to the active layer, but keep a raw pointer on
the pending layer. The pending layer would always be destroyed before the active
layer. That would avoid refcounting. If not.. then just use move when you don't
need to hold a ref somewhere.

[danakj, 2016-02-26 22:59:38]

https://codereview.chromium.org/1717283003/diff/260001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1717283003/diff/260001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:207: debug_info_ = debug_info;
you should move() here, don't need to hold a reference in this local var.

[oystein (OOO til 10th of July), 2016-02-26 23:14:36]

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1717283003/diff/200001/base/trace_event/trace...
base/trace_event/trace_event.h:846: template <class ARG1_CONVERTABLE_TYPE, class
ARG2_TYPE>
On 2016/02/26 22:52:50, Primiano (throttled til Mar 4) wrote:
> On 2016/02/26 19:46:55, Oystein wrote:
> > nit: No technical difference, but 'typename' for ARG2_TYPE feels cleaner to
me
> > (since these typically won't be classes).
> 
> Yeah I know I was very tempted, the thing is that we use class everywhere here
> and I kept consistent.
> Let's postpone it to another cleanup-only CL? (would be nice if we find a
better
> way to deal with this crazyness and all these macros/templates)

sgtm!

[Primiano Tucci (use gerrit), 2016-02-26 23:39:29]

Patchset #7 (id:280001) has been deleted

[Primiano Tucci (use gerrit), 2016-02-26 23:41:17]

On 2016/02/26 22:59:20, danakj wrote:
> You could probably move ownership to the active layer, but keep a raw pointer
on
> the pending layer. The pending layer would always be destroyed before the
active
> layer. That would avoid refcounting. If not.. then just use move when you
don't
> need to hold a ref somewhere.

No idea about what I am doing but seems to work, also tracing with all default
categories + non-default cc categories didn't crash, which is a good sign I
suppose.
Mind taking another look?

[Primiano Tucci (use gerrit), 2016-02-26 23:43:02]

P.S. I am leaving to catch a flight to the bay. If that LG and bots are green
would you plz tick commit.
If there are non-major changes required, I'll be happy to do as a follow-up
tonight or whenever, just would be great to not having to rebase this nightmare
CL on a daily basis :)
Thanks

[danakj, 2016-02-26 23:46:24]

https://codereview.chromium.org/1717283003/diff/300001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1717283003/diff/300001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:614: layer->debug_info_ = debug_info_;
So not quite right. You want to call SetDebugInfo here, pass ownership to the
active tree. Else the active layer has a dangling pointer when the pending tree
goes away.

https://codereview.chromium.org/1717283003/diff/300001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:615: layer->SetNeedsPushProperties();
And I don't think you need anything like this.

[Primiano Tucci (use gerrit), 2016-02-27 03:12:04]

I missed my flight (stupid traffic jams) but on the good side this time I might
have gotten it right, perhaps :)

https://codereview.chromium.org/1717283003/diff/300001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1717283003/diff/300001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:614: layer->debug_info_ = debug_info_;
On 2016/02/26 23:46:24, danakj wrote:
> So not quite right. You want to call SetDebugInfo here, pass ownership to the
> active tree. Else the active layer has a dangling pointer when the pending
tree
> goes away.

Done.

https://codereview.chromium.org/1717283003/diff/300001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:615: layer->SetNeedsPushProperties();
On 2016/02/26 23:46:24, danakj wrote:
> And I don't think you need anything like this.

Done.

[Primiano Tucci (use gerrit), 2016-02-27 03:21:19]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), effectively moving the info
   to the pending layer only. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-02-29 18:19:18]

LGTM

https://codereview.chromium.org/1717283003/diff/340001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1717283003/diff/340001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:614: if (owned_debug_info_)
For my own understand, was it failing without this if? I guess that'd mean
sometimes the layer is pushing without being pushed to from the main thread.

[Primiano Tucci (use gerrit), 2016-02-29 18:32:38]

https://codereview.chromium.org/1717283003/diff/340001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1717283003/diff/340001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:614: if (owned_debug_info_)
On 2016/02/29 18:19:18, danakj wrote:
> For my own understand, was it failing without this if? 
Yes precisely. It was not crashing but failing the layouttest.

> I guess that'd mean
> sometimes the layer is pushing without being pushed to from the main thread.
No ide. Could it happen that:
- layer A owns the debug info.
- layer A pushes to B (now B becomes owner, A just keeps a ptr)
- Later B pushes to C (now C is owner, A and B just raw ptr)
- Later A pushes to C.
At this point, without this if, it would invalidate C's debug info.
 ?

[Primiano Tucci (use gerrit), 2016-02-29 18:40:31]

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

[commit-bot: I haz the power, 2016-02-29 18:40:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717283003/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717283003/340001

[Primiano Tucci (use gerrit), 2016-02-29 18:53:11]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=thakis,jochen,tbarzic,mnaganov,skyostil
==========

[Primiano Tucci (use gerrit), 2016-02-29 18:54:35]

primiano@chromium.org changed reviewers:
+ jochen@chromium.org, mnaganov@chromium.org, skyostil@google.com,
tbarzic@chromium.org

[Primiano Tucci (use gerrit), 2016-02-29 18:54:36]

TBR other folks due to mechanical nature of the remaining changes (it's a
s/scoped_refpr/scoped_ptr/ elsewhere).

TBR routing map:
jochen:
  components/exo/
  content/browser/devtools/devtools_frame_trace_recorder.cc
  content/browser/renderer_host/render_widget_host_impl.h
  content/common/gpu/gpu_command_buffer_stub.cc
  content/renderer/devtools/v8_sampling_profiler.cc
  third_party/WebKit/Source/platform
mnaganov:
  android_webview/
skyostil:
  components/scheduler/
  rest of cc/
tbarzic:
  chrome/browser/chromeos/extensions/wallpaper_api.h
  chrome/browser/chromeos/extensions/wallpaper_private_api.h
 
extensions/browser/api/printer_provider_internal/printer_provider_internal_api.h
thakis:
  base/test/
  chrome/browser/chromeos/extensions/
  third_party/WebKit/Source/platform/
  ui/events/

[danakj, 2016-02-29 19:19:07]

https://codereview.chromium.org/1717283003/diff/340001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1717283003/diff/340001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:614: if (owned_debug_info_)
On 2016/02/29 18:32:38, Primiano (throttled til Mar 4) wrote:
> On 2016/02/29 18:19:18, danakj wrote:
> > For my own understand, was it failing without this if? 
> Yes precisely. It was not crashing but failing the layouttest.
> 
> > I guess that'd mean
> > sometimes the layer is pushing without being pushed to from the main thread.
> No ide. Could it happen that:
> - layer A owns the debug info.
> - layer A pushes to B (now B becomes owner, A just keeps a ptr)
> - Later B pushes to C (now C is owner, A and B just raw ptr)
> - Later A pushes to C.
> At this point, without this if, it would invalidate C's debug info.
>  ?

There's only A & B. But I guess it means that we SetNeedsPushProps on the
pending tree sometimes other than a main thread push. I wasn't sure, it's good
to know that happens.

[commit-bot: I haz the power, 2016-02-29 20:09:45]

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

[commit-bot: I haz the power, 2016-02-29 20:09:47]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-02-29 20:37:06]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-02-29 20:37:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org, oysteine@chromium.org
Link to the patchset: https://codereview.chromium.org/1717283003/#ps340001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-29 20:37:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1717283003/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1717283003/340001

[commit-bot: I haz the power, 2016-02-29 20:46:16]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=thakis,jochen,tbarzic,mnaganov,skyostil
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=thakis,jochen,tbarzic,mnaganov,skyostil
==========

[commit-bot: I haz the power, 2016-02-29 20:46:18]

Committed patchset #9 (id:340001)

[commit-bot: I haz the power, 2016-02-29 20:47:41]

Description was changed from

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState. 
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer. 

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=thakis,jochen,tbarzic,mnaganov,skyostil
==========

to

==========
tracing: Make ConvertableToTraceFormat move-only

Summary. This CL:
 - Makes TraceEvent ownership a move-only scoped_ptr.
 - Makes ConvertableToTraceFormat (CTTF) itself move-only scoped_ptr.
 - Updates all the codebase that uses CTTF in TRACE_EVENT macros
   to use move-only semantics.

Background:
Historically ConvertableToTraceFormat (CTTF) was RefCounted. The main
reason seems to be supporting monitoring mode (now deprecated) where
tracing needed to copy TraceEvents without flushing the TraceLog.
Not what monitoring mode is gone, there is no reason why TraceEvent(s)
should not be move-only.
Unfortunately CTTF being RefCounted exposed that implementation detail
to its public interface. Fortunately, most of the codebase doesn't care
about the fact that CTTF is RefCounted.
The only exceptions are:
 1. Memory-infra heap profiler {StackFrame,TypeInfo}Deduplicator
 2. cc::Layer DebugInfo

1) Is addressed creating a proxy class which delegates the CTTF methods
   to the duplicators inside MDSessionState. Essentially it makes
   the CTTF metadata events shared co-owners of the MDSessionState.
2) After an offline chat with danakj@, it seems OK to make DebugInfo(s)
   moved scoped_ptr (as opposite as copied), moving the ownership to the
   active layer and keeping a raw ptr into the pending layer.

BUG=559117
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=thakis,jochen,tbarzic,mnaganov,skyostil

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

[commit-bot: I haz the power, 2016-02-29 20:47:43]

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