Use LatencyInfoSwapPromise to track LatencyInfo through compositor

Use LatencyInfoSwapPromise to track LatencyInfo through compositor

Originally we cache LatencyInfo directly in compositor. Now that
with the support of SwapPromise, we can use LatencyInfoSwapPromise
to track the LatencyInfo.

BUG=246034
TEST=Input LatencyInfo are still correctly passed to output surface
     through LatencyInfoSwapPromise.

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

[Yufeng Shen (Slow to review), 2013-11-21 20:05:44]

This is based on the SwapPromise CL

https://codereview.chromium.org/60513007/

[Yufeng Shen (Slow to review), 2013-11-26 00:05:25]

Hey Dana,

Now that the the SwapPromise CL https://codereview.chromium.org/60513007/
is close to be merged, can you also give a review for
this one which is the actual client that uses SwapPromise
to carry LatencyInfo ?

[danakj, 2013-11-26 19:17:53]

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.cc
File cc/base/swap_promise.cc (right):

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.cc#...
cc/base/swap_promise.cc:54:
latency_.AddLatencyNumber(DidNotSwapReasonToLatencyComponentType(reason),
these methods can be called on either thread. you need to use a base::Lock
around any access to latency_.

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:59: class LatencyInfoSwapPromise : public SwapPromise {
one class per file please

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:474: scoped_ptr<cc::SwapPromise> swap_promise(
don't need cc:: inside cc::

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:701: void
LayerTreeImpl::TakeLatencyInfo(ui::LatencyInfo* latency_info) {
Why isn't this going through the DidSwap() method? We shouldn't be using the
CompositorFrame for plumbing this info at all anymore should we?

[Yufeng Shen (Slow to review), 2013-11-27 20:05:32]

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.cc
File cc/base/swap_promise.cc (right):

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.cc#...
cc/base/swap_promise.cc:54:
latency_.AddLatencyNumber(DidNotSwapReasonToLatencyComponentType(reason),
On 2013/11/26 19:17:53, danakj wrote:
> these methods can be called on either thread. you need to use a base::Lock
> around any access to latency_.

But one SwapPromise can only live either on impl or main thread, so
even DidNotSwap can happen concurrently on impl/main threads, it won't operate
on the same latency_ (it's a local copy of the LatencyInfoSwapPromise) right ?

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/81533002/diff/150001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:59: class LatencyInfoSwapPromise : public SwapPromise {
On 2013/11/26 19:17:53, danakj wrote:
> one class per file please

Done.

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:474: scoped_ptr<cc::SwapPromise> swap_promise(
On 2013/11/26 19:17:53, danakj wrote:
> don't need cc:: inside cc::

Done.

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/81533002/diff/150001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:701: void
LayerTreeImpl::TakeLatencyInfo(ui::LatencyInfo* latency_info) {
On 2013/11/26 19:17:53, danakj wrote:
> Why isn't this going through the DidSwap() method? We shouldn't be using the
> CompositorFrame for plumbing this info at all anymore should we?

Removed.

Changed the DidSwap() signature to be
virtual void DidSwap(CompositorFrameMetadata* metadata)

so that LatencyInfoSwapPromise::DidSwap(CompositorFrameMetadata* metadata)
will merge its latencyinfo into metadata.

[danakj, 2013-11-27 20:11:41]

This is looking good, now you can remove some more things too:

https://codereview.chromium.org/81533002/diff/190001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise.h (right):

https://codereview.chromium.org/81533002/diff/190001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.h:22: const ui::LatencyInfo& GetLatencyInfo();
This is no longer used, remove it.

https://codereview.chromium.org/81533002/diff/190001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/81533002/diff/190001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:34: enum SwapPromiseType {
This is no longer needed

https://codereview.chromium.org/81533002/diff/190001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:57: SwapPromiseType type_;
The type should be able to go away now.

[Yufeng Shen (Slow to review), 2013-11-27 20:18:52]

https://codereview.chromium.org/81533002/diff/190001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise.h (right):

https://codereview.chromium.org/81533002/diff/190001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.h:22: const ui::LatencyInfo& GetLatencyInfo();
On 2013/11/27 20:11:42, danakj wrote:
> This is no longer used, remove it.

Done.

https://codereview.chromium.org/81533002/diff/190001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/81533002/diff/190001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:34: enum SwapPromiseType {
On 2013/11/27 20:11:42, danakj wrote:
> This is no longer needed

Done.

https://codereview.chromium.org/81533002/diff/190001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:57: SwapPromiseType type_;
On 2013/11/27 20:11:42, danakj wrote:
> The type should be able to go away now.

Done.

[danakj, 2013-11-27 20:26:10]

Cool!

Is there any cc_unittest to verify that LatencyInfo gets added to the Metadata
correctly? If not, can we add one to verify this code path in the future?

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise.cc (right):

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:16: break;
nit: break is kinda pointless after a return statement

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:24: DLOG(WARNING) << "Unhandled
DidNotSwapReason.\n";
Should this be a NOTREACHED()?

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:24: DLOG(WARNING) << "Unhandled
DidNotSwapReason.\n";
Generally you don't need \n at the end of LOG lines, the \n is added for free
already.

[Yufeng Shen (Slow to review), 2013-11-27 21:19:33]

tests added.

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise.cc (right):

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:16: break;
On 2013/11/27 20:26:10, danakj wrote:
> nit: break is kinda pointless after a return statement

Done.

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:24: DLOG(WARNING) << "Unhandled
DidNotSwapReason.\n";
On 2013/11/27 20:26:10, danakj wrote:
> Should this be a NOTREACHED()?

Done.

https://codereview.chromium.org/81533002/diff/210001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:24: DLOG(WARNING) << "Unhandled
DidNotSwapReason.\n";
On 2013/11/27 20:26:10, danakj wrote:
> Generally you don't need \n at the end of LOG lines, the \n is added for free
> already.

Done.

[danakj, 2013-11-27 21:21:40]

Thanks for the test! LGTM with one more quibble.

https://codereview.chromium.org/81533002/diff/230001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise.cc (right):

https://codereview.chromium.org/81533002/diff/230001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:21: NOTREACHED() << "Unhandled
DidNotSwapReason.";
Cool! OK, if this should be a notreached, can you move it below the switch and
delete the default: case, so we can get compiler errors here if a new
SwapPromise::DidNotSwapReason is added?

[Yufeng Shen (Slow to review), 2013-11-27 21:28:16]

+ jamesr@ for OWNER of content/renderer/gpu/render_widget_compositor.cc

+ jbauman@ for OWNER of
ui/events/latency_info.*

https://codereview.chromium.org/81533002/diff/230001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise.cc (right):

https://codereview.chromium.org/81533002/diff/230001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise.cc:21: NOTREACHED() << "Unhandled
DidNotSwapReason.";
On 2013/11/27 21:21:41, danakj wrote:
> Cool! OK, if this should be a notreached, can you move it below the switch and
> delete the default: case, so we can get compiler errors here if a new
> SwapPromise::DidNotSwapReason is added?

Done.

[jamesr, 2013-11-27 21:29:08]

content/renderer/ lgtm

[sadrul, 2013-11-27 22:48:12]

ui/events/ lgtm

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h...
ui/events/latency_info.h:58: // This component indicats that the input causes a
commit to be scheduled
*indicates

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h...
ui/events/latency_info.h:61: // This component indicats that the input causes a
swap to be scheduled
ditto

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h...
ui/events/latency_info.h:64: // This component indicats that the cached
LatencyInfo number exceeds the
ditto

[Yufeng Shen (Slow to review), 2013-11-27 22:55:05]

Thanks Sadrul.

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h
File ui/events/latency_info.h (right):

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h...
ui/events/latency_info.h:58: // This component indicats that the input causes a
commit to be scheduled
On 2013/11/27 22:48:13, sadrul wrote:
> *indicates

Done.

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h...
ui/events/latency_info.h:61: // This component indicats that the input causes a
swap to be scheduled
On 2013/11/27 22:48:13, sadrul wrote:
> ditto

Done.

https://codereview.chromium.org/81533002/diff/250001/ui/events/latency_info.h...
ui/events/latency_info.h:64: // This component indicats that the cached
LatencyInfo number exceeds the
On 2013/11/27 22:48:13, sadrul wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-11-27 23:38:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/270001

[commit-bot: I haz the power, 2013-11-28 01:22:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/270001

[commit-bot: I haz the power, 2013-11-28 02:02:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/270001

[commit-bot: I haz the power, 2013-11-28 02:23:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/270001

[commit-bot: I haz the power, 2013-11-28 02:31:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/270001

[commit-bot: I haz the power, 2013-11-28 03:02:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/290001

[commit-bot: I haz the power, 2013-11-28 03:30:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/290001

[commit-bot: I haz the power, 2013-11-28 04:13:14]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-11-28 06:21:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/290001

[commit-bot: I haz the power, 2013-11-28 07:02:45]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-11-28 17:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/310001

[commit-bot: I haz the power, 2013-11-28 18:01:07]

Failed to trigger a try job on win_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-11-28 18:02:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/330001

[commit-bot: I haz the power, 2013-11-28 19:50:20]

Retried try job too often on linux_chromeos for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-11-28 19:53:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/330001

[commit-bot: I haz the power, 2013-11-28 20:06:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/81533002/370001

[commit-bot: I haz the power, 2013-11-29 00:01:24]

Change committed as 237848