cc: add DevTools instrumentation for impl-side frames

cc: add DevTools instrumentation for impl-side frames

This adds devtools_instrumentation::didBeginFrame() invoked from
Scheduler::BeginImplFrame() and
devtools_instrumentation::didCommitMainThreadFrame invoked from
LayerTreeHost::FinishCommitOnImplThread(). We use the latter to
associate impl-side frames with main thread ones on the DevTools
side.
Related blink-side patch: https://codereview.chromium.org/59793003/

BUG=305863
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238150

[nduca, 2013-11-06 19:05:12]

if we put the source_frame_number on layer_tree_impl, could we get away with
just using frame_id?

https://codereview.chromium.org/60353002/diff/190001/cc/debug/devtools_instru...
File cc/debug/devtools_instrumentation.h (right):

https://codereview.chromium.org/60353002/diff/190001/cc/debug/devtools_instru...
cc/debug/devtools_instrumentation.h:90: inline void
didCommitMainThreadFrame(uint64 tree_id, int frame_id) {
remind me what the tree id does?

https://codereview.chromium.org/60353002/diff/190001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/60353002/diff/190001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:141: int tree_id_;
can we call this layer_tree_host_id throughout? We actually have LayerTrees
which are different than the hosts...

[caseq, 2013-11-07 06:36:26]

> if we put the source_frame_number on layer_tree_impl, 
> could we get away with just using frame_id?

We would still need tree^W LTH ids for cases where we don't have (main thread)
frame ids, e.g. in Scheduler::BeginImplFrame. So while it would be possible to
get rid of it in DidCommitMainThreadFrame, I'd rather keep it for consistency
with other events.

https://codereview.chromium.org/60353002/diff/190001/cc/debug/devtools_instru...
File cc/debug/devtools_instrumentation.h (right):

https://codereview.chromium.org/60353002/diff/190001/cc/debug/devtools_instru...
cc/debug/devtools_instrumentation.h:90: inline void
didCommitMainThreadFrame(uint64 tree_id, int frame_id) {
On 2013/11/06 19:05:12, nduca wrote:
> remind me what the tree id does?

It's actually LTH id -- we use it on the Timeline's side to tell apart LTHs of
different views within the same process, so that we only process events from the
inspected page.

https://codereview.chromium.org/60353002/diff/190001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/60353002/diff/190001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:141: int tree_id_;
On 2013/11/06 19:05:12, nduca wrote:
> can we call this layer_tree_host_id throughout? We actually have LayerTrees
> which are different than the hosts...

Sure, done (everywhere except devtools_instrumentation, where we were already
using it, as it should match the parameter name on the blink side).

[dshwang, 2013-11-07 10:28:51]

https://codereview.chromium.org/60353002/diff/150020/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60353002/diff/150020/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:45: static base::StaticAtomicSequenceNumber
s_layer_tree_host_sequence_number;
oh, great! thx.

[caseq, 2013-11-11 16:17:28]

Nat, ping :)

[nduca, 2013-11-12 05:51:17]

lgtm with some minor nits

https://codereview.chromium.org/60353002/diff/150020/cc/debug/devtools_instru...
File cc/debug/devtools_instrumentation.h (right):

https://codereview.chromium.org/60353002/diff/150020/cc/debug/devtools_instru...
cc/debug/devtools_instrumentation.h:90: inline void
didCommitMainThreadFrame(uint64 tree_id, int frame_id) {
oh i get it now, i gave you bad advice...

you want to call these layer_tree_host_id ... tree ids are actually a group of
layers, they are actually different from the hosts.

https://codereview.chromium.org/60353002/diff/150020/cc/debug/devtools_instru...
cc/debug/devtools_instrumentation.h:102: internal::kLayerTreeId, tree_id);
would it be better to include the frame id on this? i assume you're going to use
wall time to sort out the mapping but i think we know it so we could just
include it every time?

[caseq, 2013-11-12 13:04:33]

On 2013/11/12 05:51:17, nduca wrote:
> lgtm with some minor nits
> 
>
https://codereview.chromium.org/60353002/diff/150020/cc/debug/devtools_instru...
> File cc/debug/devtools_instrumentation.h (right):
> 
>
https://codereview.chromium.org/60353002/diff/150020/cc/debug/devtools_instru...
> cc/debug/devtools_instrumentation.h:90: inline void
> didCommitMainThreadFrame(uint64 tree_id, int frame_id) {
> oh i get it now, i gave you bad advice...

Not really :)

> you want to call these layer_tree_host_id ... tree ids are actually a group of
> layers, they are actually different from the hosts.

That was what you advised and that's what I've done everywhere except the
instrumentation itself, as we already have
treeId used on the blink side. I now renamed everything except for actual event
argument name to lessen confusion.

>
https://codereview.chromium.org/60353002/diff/150020/cc/debug/devtools_instru...
> cc/debug/devtools_instrumentation.h:102: internal::kLayerTreeId, tree_id);
> would it be better to include the frame id on this? i assume you're going to
use
> wall time to sort out the mapping but i think we know it so we could just
> include it every time?

As per our chat, I'm switching to emitting/processing tree activation events,
and in case of impl-side painting the
frame number is taken from the tree being activated -- this is expected to work
correctly for both impl- and main-
thread painting. This also required pushing layer_tree_host_id to
LayerTreeHostImpl.

[alph, 2013-11-15 23:46:15]

https://codereview.chromium.org/60353002/diff/420001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60353002/diff/420001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:120: id_(s_layer_tree_host_sequence_number.GetNext()
+ 1),
nit: why not keep inline GetNextTreeId hiding +1 logic there.

[commit-bot: I haz the power, 2013-12-02 16:48:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caseq@chromium.org/60353002/480001

[commit-bot: I haz the power, 2013-12-02 19:23:04]

Change committed as 238150