Adding tracing to measure synthetic smooth scrolling correctness

Adding tracing to measure synthetic smooth scrolling correctness 

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

[hartmanng, 2012-11-27 23:41:06]

Please take a look.

shawnsignh, please review for cc/ side, piman please review for content/

[nduca, 2012-11-27 23:50:27]

https://codereview.chromium.org/11413199/diff/2001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11413199/diff/2001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:806: TRACE_COUNTER_ID1("gpu",
"frame_counter_translate_y", this,
maybe root_layer_y_translate?

[shawnsingh, 2012-11-27 23:59:57]

https://codereview.chromium.org/11413199/diff/2001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11413199/diff/2001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:806: TRACE_COUNTER_ID1("gpu",
"frame_counter_translate_y", this,
On 2012/11/27 23:50:27, nduca wrote:
> maybe root_layer_y_translate?

I also like nduca's proposed rename.

What assumptions are we allowed to make about the transform that the root
scrollLayer will ever have?  Are we certain it will only ever have translation? 
If this layer has more complicated transforms, are you sure this is the value
you want to trace?

assuming you know that's the value you want, it LGTM.

[jamesr, 2012-11-28 00:02:16]

Can we just trace this when we actually scroll?  Doing it on every draw is
pretty spammy

https://codereview.chromium.org/11413199/diff/2001/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11413199/diff/2001/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:807:
m_rootScrollLayerImpl->drawTransform().matrix().getDouble(
cc::LayerImpl has a scrollOffset, can't you use that instead of looking at the
draw transform?

[nduca, 2012-11-28 00:03:18]

/me wonders if the impl-side scrollbars know this sort of thing... @wjmaclean
may know how those hook up.

[piman, 2012-11-28 01:50:24]

lgtm for content/

[hartmanng, 2012-11-28 16:27:57]

On 2012/11/28 00:03:18, nduca wrote:
> /me wonders if the impl-side scrollbars know this sort of thing... @wjmaclean
> may know how those hook up.

I went with jamesr's suggestion for now, I don't really know the pros/cons of
doing this instead. I tried it on my local copy (putting a trace in
ScrollbarLayerImpl::appendQuads as @wjmaclean recommended), the trace output
looked pretty similar, except that it had a small plateau at the end (I assume
this is where we continue sending scroll events after the bottom of the page has
been reached). The LayerImpl route usually doesn't have a tail at the end.

[shawnsingh, 2012-11-30 19:28:02]

As I understand, scrollOffset is received from main thread, while scrollDelta is
any impl-side scrolling that occurs.   If this is supposed to be a TRACE for
measuring _smooth_ scrolling, should we be including scrollOffset + scrollDelta?

for example, m_currentOffset y value might be what you really want to be
logging, in the scrollbar_animation_controller

nduca, jamesr - of course please correct me if i'm wrong =)

[jamesr, 2012-11-30 19:29:16]

On 2012/11/30 19:28:02, shawnsingh wrote:
> As I understand, scrollOffset is received from main thread, while scrollDelta
is
> any impl-side scrolling that occurs.   If this is supposed to be a TRACE for
> measuring _smooth_ scrolling, should we be including scrollOffset +
scrollDelta?
> 

Correct

> for example, m_currentOffset y value might be what you really want to be
> logging, in the scrollbar_animation_controller
> 
> nduca, jamesr - of course please correct me if i'm wrong =)

[hartmanng, 2012-11-30 20:09:07]

Moving the TRACE over to ScrollbarAnimationController::updateScrollOffsetAtTime
to include the scrollDelta as discussed over IM.

PTAL shawnsingh

[shawnsingh, 2012-11-30 20:13:48]

On 2012/11/30 20:09:07, hartmanng wrote:
> Moving the TRACE over to
ScrollbarAnimationController::updateScrollOffsetAtTime
> to include the scrollDelta as discussed over IM.
> 
> PTAL shawnsingh

LGTM =)

[commit-bot: I haz the power, 2012-11-30 20:19:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11413199/15004

[commit-bot: I haz the power, 2012-11-30 21:47:10]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-30 21:56:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11413199/15004

[commit-bot: I haz the power, 2012-12-01 03:14:57]

Change committed as 170642

[vangelis, 2013-01-04 20:44:17]

On 2012/12/01 03:14:57, I haz the power (commit-bot) wrote:
> Change committed as 170642

This additional trace uses unique IDs and each one of them draws in its own
"line" in about:tracing, making the tracing output super long...

[nduca, 2013-01-04 22:42:13]

https://chromiumcodereview.appspot.com/11413199/diff/15004/cc/scrollbar_anima...
File cc/scrollbar_animation_controller.cc (right):

https://chromiumcodereview.appspot.com/11413199/diff/15004/cc/scrollbar_anima...
cc/scrollbar_animation_controller.cc:87: TRACE_COUNTER_ID1("gpu",
"scroll_offset_y", this, m_currentOffset.y());
we should be passing a layerTreeHostImpl pointer here.