Trace main thread commit in flow events.

Trace main thread commit in flow events.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[yuhaoz, 2015-06-15 22:17:55]

yuhaoz@google.com changed reviewers:
+ aelias@chromium.org, brianderson@chromium.org, miletus@chromium.org

[yuhaoz, 2015-06-15 22:17:55]

Include main thread side commit in flow events.

[Yufeng Shen (Slow to review), 2015-06-15 23:48:25]

https://codereview.chromium.org/1178963003/diff/1/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1178963003/diff/1/cc/trees/layer_tree_host.h#...
cc/trees/layer_tree_host.h:320: TRACE_ID_DONT_MANGLE(swap_promise_->TraceId()),
Not all SwapPromise is carrying LatencyInfo with it, so its TraceId()
can'be always interpreted as the trace_id for LatencyInfo.

I think you should have a
virtual SwapPromise::OnCommit() {}

and only in LatencyInfoSwapPromise::OnCommit()
you can use the trace_id for adding the flow event.

[yuhaoz, 2015-06-16 00:36:15]

Make OnCommit virtual and only LatencyInfoSwapPromise can call it to trace flow
events.

[yuhaoz, 2015-06-16 00:37:02]

yuhaoz@google.com changed reviewers:
+ enne@chromium.org

[Yufeng Shen (Slow to review), 2015-06-16 15:11:05]

https://codereview.chromium.org/1178963003/diff/20001/cc/output/swap_promise.h
File cc/output/swap_promise.h (right):

https://codereview.chromium.org/1178963003/diff/20001/cc/output/swap_promise....
cc/output/swap_promise.h:58: 
maybe giving it an empty implementation (non-virtual) so that you don't have to
provide empty implementation to every subclass ? OnCommit() does not seem to be
an essential callback that every SwapPromise has to
react like Did(Not)Swap.

Also can you give a comment on OnCommit() either here or at the top in the
description for SwapPromise.

https://codereview.chromium.org/1178963003/diff/20001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/1178963003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:314: void trace_commit() {
Can you use Camel style ? underscore style is usually only for simple
setter/getter. And probably move the implementation to .cc file.

Also a different name more specific to SwapPromise, maybe
OnCommitForSwapPromises()

https://codereview.chromium.org/1178963003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:319: }
It can be simplified like this:

for (auto* swap_promise : swap_promise_list_)
    swap_promise->OnCommit()

[yuhaoz, 2015-06-16 17:11:54]

Make OnCommitForSwapPromises non pure so that not all the derived classes need
to implement it.

[Yufeng Shen (Slow to review), 2015-06-16 17:32:18]

https://codereview.chromium.org/1178963003/diff/40001/cc/output/swap_promise.h
File cc/output/swap_promise.h (right):

https://codereview.chromium.org/1178963003/diff/40001/cc/output/swap_promise....
cc/output/swap_promise.h:43: // For each SwapPromise, OnCommitForSwapPromises
traces its original LatencyInfo
SwapPromise is generic, so you should only mention here when is OnCommit()
getting called, but move the "trace its original LatencyInfo" into
LatencyInfoSwapPromise::OnCommit() 's implementation.

https://codereview.chromium.org/1178963003/diff/40001/cc/output/swap_promise....
cc/output/swap_promise.h:60: 
ah, I actually meant the naming to be LayerTreeHost::OnCommitForSwapPromises()
and SwapPromise::OnCommit()

[yuhaoz, 2015-06-16 17:45:38]

Naming and comments improvements.

[Yufeng Shen (Slow to review), 2015-06-16 17:54:16]

nits with some of the comments.

cc OWNERS, mind taking a look ?

https://codereview.chromium.org/1178963003/diff/60001/cc/output/swap_promise.h
File cc/output/swap_promise.h (right):

https://codereview.chromium.org/1178963003/diff/60001/cc/output/swap_promise....
cc/output/swap_promise.h:42: //
extra line

https://codereview.chromium.org/1178963003/diff/60001/cc/output/swap_promise....
cc/output/swap_promise.h:59: virtual void OnCommit() {}
Should not mention subclass's behavior, only mention under what conditionis this
callback triggered, e,g,

"This is called when main thread starts commit."

[Yufeng Shen (Slow to review), 2015-06-17 14:33:43]

lgtm

[brianderson, 2015-06-17 18:18:04]

https://codereview.chromium.org/1178963003/diff/80001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/1178963003/diff/80001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1303: void LayerTreeHost::OnCommitForSwapPromises()
{
Can you group this with the other swap promise logic near line 1266?

Also, instead of making this a public method can you make it protected and call
it from LayerTreeHost::WillCommit?

[yuhaoz, 2015-06-18 04:00:51]

Make OnCommitForSwapPromises protected and be called from WillCommit.

[brianderson, 2015-06-18 17:50:56]

lgtm

[yuhaoz, 2015-06-18 17:55:51]

The CQ bit was checked by yuhaoz@google.com

[yuhaoz, 2015-06-18 17:55:51]

The patchset sent to the CQ was uploaded after l-g-t-m from miletus@chromium.org
Link to the patchset: https://codereview.chromium.org/1178963003/#ps100001
(title: "Make OnCommitForSwapPromises protected and be called from WillCommit.")

[commit-bot: I haz the power, 2015-06-18 17:57:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1178963003/100001

[commit-bot: I haz the power, 2015-06-18 19:01:32]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-18 19:02:32]

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