cc: Fix scrollbar animation tests

cc: Fix scrollbar animation tests.

The ScrollbarFadePinchZoomScrollbars test wasn't really testing any
pinch zoom functionality so it has been removed. There were no tests
for thinning scrollbar animations so those have been added.

Committed: https://crrev.com/102dbf69bebd519dedb439a22083f7c722383984
Cr-Commit-Position: refs/heads/master@{#319546}

[sunnyps, 2015-02-26 00:34:10]

sunnyps@chromium.org changed reviewers:
+ ajuma@chromium.org, bokan@chromium.org

[sunnyps, 2015-02-26 00:34:11]

PTAL

[ajuma, 2015-02-26 15:30:45]

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1735:
host_impl_->SetPageScaleOnActiveTree(1.1f);
Does this line have no effect on behavior? (As I understand it, the
justification for removing this test is that the use_pinch_zoom_scrollbars
setting isn't read anywhere, so this is really testing the same thing as
ScrollbarLinearFadeScheduling, but that's only true if changing page scale
doesn't change expected scrollbar animation behavior.)

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1686: // Unnecessarily Fade animation
of solid color scrollbar is not triggered.
While you're moving this: s/Unnecessarily/Unnecessary

[sunnyps, 2015-03-02 19:19:07]

Sorry for the late reply.

@bokan, Can you look at my comment about the page scale triggering the scrollbar
animation? Please let me know if you think that's something we should test.
Thanks!

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1735:
host_impl_->SetPageScaleOnActiveTree(1.1f);
On 2015/02/26 15:30:45, ajuma wrote:
> Does this line have no effect on behavior? (As I understand it, the
> justification for removing this test is that the use_pinch_zoom_scrollbars
> setting isn't read anywhere, so this is really testing the same thing as
> ScrollbarLinearFadeScheduling, but that's only true if changing page scale
> doesn't change expected scrollbar animation behavior.)

It seems the scrollbar fade animation is triggered by the
SetPageScaleOnActiveTree call rather than being triggered by the
ScrollBegin/ScrollBy/ScrollEnd calls below as the test would have you believe.

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1686: // Unnecessarily Fade animation
of solid color scrollbar is not triggered.
On 2015/02/26 15:30:45, ajuma wrote:
> While you're moving this: s/Unnecessarily/Unnecessary

Acknowledged.

[bokan, 2015-03-02 22:49:06]

LGTM with nit.

Please add a description to the CL before committing it.

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1735:
host_impl_->SetPageScaleOnActiveTree(1.1f);
On 2015/03/02 19:19:07, sunnyps wrote:
> On 2015/02/26 15:30:45, ajuma wrote:
> > Does this line have no effect on behavior? (As I understand it, the
> > justification for removing this test is that the use_pinch_zoom_scrollbars
> > setting isn't read anywhere, so this is really testing the same thing as
> > ScrollbarLinearFadeScheduling, but that's only true if changing page scale
> > doesn't change expected scrollbar animation behavior.)
> 
> It seems the scrollbar fade animation is triggered by the
> SetPageScaleOnActiveTree call rather than being triggered by the
> ScrollBegin/ScrollBy/ScrollEnd calls below as the test would have you believe.

That's not true, ScrollBy will activate the fade animation, but this used to be
gated based on the page scale. If page scale was at the minimum then we wouldn't
trigger the animation. This was to prevent overlay scrollbars from showing up
over top of page scrollbars when they're unneeded. Now we run the animation
regardless of page scale but hide the subtree when page scale is near the
minimum. See https://codereview.chromium.org/885063002/ for more details.

That said, we use the same scrollbar code on Android and desktop now so I think
this is indeed testing the same thing as ScrollbarLinearFadeScheduling

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1640: // After a scroll, a fade
animation should be scheduled about 20ms from now.
Nit: this is no longer specifically for fade, please update the comments.

[bokan, 2015-03-02 22:49:53]

LGTM with nit.

Please add a description to the CL before committing it.

[sunnyps, 2015-03-03 01:53:32]

On 2015/03/02 22:49:06, bokan wrote:
> LGTM with nit.
> 
> Please add a description to the CL before committing it.
> 
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> File cc/trees/layer_tree_host_impl_unittest.cc (left):
> 
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> cc/trees/layer_tree_host_impl_unittest.cc:1735:
> host_impl_->SetPageScaleOnActiveTree(1.1f);
> On 2015/03/02 19:19:07, sunnyps wrote:
> > On 2015/02/26 15:30:45, ajuma wrote:
> > > Does this line have no effect on behavior? (As I understand it, the
> > > justification for removing this test is that the use_pinch_zoom_scrollbars
> > > setting isn't read anywhere, so this is really testing the same thing as
> > > ScrollbarLinearFadeScheduling, but that's only true if changing page scale
> > > doesn't change expected scrollbar animation behavior.)
> > 
> > It seems the scrollbar fade animation is triggered by the
> > SetPageScaleOnActiveTree call rather than being triggered by the
> > ScrollBegin/ScrollBy/ScrollEnd calls below as the test would have you
believe.
> 
> That's not true, ScrollBy will activate the fade animation, but this used to
be
> gated based on the page scale. If page scale was at the minimum then we
wouldn't
> trigger the animation. This was to prevent overlay scrollbars from showing up
> over top of page scrollbars when they're unneeded. Now we run the animation
> regardless of page scale but hide the subtree when page scale is near the
> minimum. See https://codereview.chromium.org/885063002/ for more details.

If you add these lines

  host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 4.f);
  host_impl_->SetPageScaleOnActiveTree(1.1f);

to the beginning of the test (before the first set of EXPECTs) the scrollbar
animation is triggered immediately without a scroll being performed.

So it's not the ScrollBy that's activating the fade. The old test seems to
suggest that ScrollBy should cause the fade.

> 
> That said, we use the same scrollbar code on Android and desktop now so I
think
> this is indeed testing the same thing as ScrollbarLinearFadeScheduling
> 
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> File cc/trees/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> cc/trees/layer_tree_host_impl_unittest.cc:1640: // After a scroll, a fade
> animation should be scheduled about 20ms from now.
> Nit: this is no longer specifically for fade, please update the comments.

[bokan, 2015-03-03 12:05:32]

On 2015/03/03 01:53:32, sunnyps wrote:
> On 2015/03/02 22:49:06, bokan wrote:
> > LGTM with nit.
> > 
> > Please add a description to the CL before committing it.
> > 
> >
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> > File cc/trees/layer_tree_host_impl_unittest.cc (left):
> > 
> >
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> > cc/trees/layer_tree_host_impl_unittest.cc:1735:
> > host_impl_->SetPageScaleOnActiveTree(1.1f);
> > On 2015/03/02 19:19:07, sunnyps wrote:
> > > On 2015/02/26 15:30:45, ajuma wrote:
> > > > Does this line have no effect on behavior? (As I understand it, the
> > > > justification for removing this test is that the
use_pinch_zoom_scrollbars
> > > > setting isn't read anywhere, so this is really testing the same thing as
> > > > ScrollbarLinearFadeScheduling, but that's only true if changing page
scale
> > > > doesn't change expected scrollbar animation behavior.)
> > > 
> > > It seems the scrollbar fade animation is triggered by the
> > > SetPageScaleOnActiveTree call rather than being triggered by the
> > > ScrollBegin/ScrollBy/ScrollEnd calls below as the test would have you
> believe.
> > 
> > That's not true, ScrollBy will activate the fade animation, but this used to
> be
> > gated based on the page scale. If page scale was at the minimum then we
> wouldn't
> > trigger the animation. This was to prevent overlay scrollbars from showing
up
> > over top of page scrollbars when they're unneeded. Now we run the animation
> > regardless of page scale but hide the subtree when page scale is near the
> > minimum. See https://codereview.chromium.org/885063002/ for more details.
> 
> If you add these lines
> 
>   host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 4.f);
>   host_impl_->SetPageScaleOnActiveTree(1.1f);
> 
> to the beginning of the test (before the first set of EXPECTs) the scrollbar
> animation is triggered immediately without a scroll being performed.
> 
> So it's not the ScrollBy that's activating the fade. The old test seems to
> suggest that ScrollBy should cause the fade.
> 

Right, I should have mentioned, changing page scale *also* activates the
animation but so does scrolling.

[sunnyps, 2015-03-03 19:34:34]

On 2015/03/03 12:05:32, bokan wrote:
> On 2015/03/03 01:53:32, sunnyps wrote:
> > On 2015/03/02 22:49:06, bokan wrote:
> > > LGTM with nit.
> > > 
> > > Please add a description to the CL before committing it.
> > > 
> > >
> >
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> > > File cc/trees/layer_tree_host_impl_unittest.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> > > cc/trees/layer_tree_host_impl_unittest.cc:1735:
> > > host_impl_->SetPageScaleOnActiveTree(1.1f);
> > > On 2015/03/02 19:19:07, sunnyps wrote:
> > > > On 2015/02/26 15:30:45, ajuma wrote:
> > > > > Does this line have no effect on behavior? (As I understand it, the
> > > > > justification for removing this test is that the
> use_pinch_zoom_scrollbars
> > > > > setting isn't read anywhere, so this is really testing the same thing
as
> > > > > ScrollbarLinearFadeScheduling, but that's only true if changing page
> scale
> > > > > doesn't change expected scrollbar animation behavior.)
> > > > 
> > > > It seems the scrollbar fade animation is triggered by the
> > > > SetPageScaleOnActiveTree call rather than being triggered by the
> > > > ScrollBegin/ScrollBy/ScrollEnd calls below as the test would have you
> > believe.
> > > 
> > > That's not true, ScrollBy will activate the fade animation, but this used
to
> > be
> > > gated based on the page scale. If page scale was at the minimum then we
> > wouldn't
> > > trigger the animation. This was to prevent overlay scrollbars from showing
> up
> > > over top of page scrollbars when they're unneeded. Now we run the
animation
> > > regardless of page scale but hide the subtree when page scale is near the
> > > minimum. See https://codereview.chromium.org/885063002/ for more details.
> > 
> > If you add these lines
> > 
> >   host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 4.f);
> >   host_impl_->SetPageScaleOnActiveTree(1.1f);
> > 
> > to the beginning of the test (before the first set of EXPECTs) the scrollbar
> > animation is triggered immediately without a scroll being performed.
> > 
> > So it's not the ScrollBy that's activating the fade. The old test seems to
> > suggest that ScrollBy should cause the fade.
> > 
> 
> Right, I should have mentioned, changing page scale *also* activates the
> animation but so does scrolling.

In the test that's being removed (ScrollbarFadePinchZoomScrollbars) the
scrolling did not cause the scrollbar animation. I believe this is because the
scroll was along the x-axis (5, 0) which doesn't result in a successful scroll
based on how the layer tree is setup. Instead the animation was caused by
changing the page scale.

What I really meant to ask was: should we add a test for changing page scale
causes a scrollbar animation?

[bokan, 2015-03-03 19:50:31]

On 2015/03/03 19:34:34, sunnyps wrote:
> On 2015/03/03 12:05:32, bokan wrote:
> > On 2015/03/03 01:53:32, sunnyps wrote:
> > > On 2015/03/02 22:49:06, bokan wrote:
> > > > LGTM with nit.
> > > > 
> > > > Please add a description to the CL before committing it.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> > > > File cc/trees/layer_tree_host_impl_unittest.cc (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
> > > > cc/trees/layer_tree_host_impl_unittest.cc:1735:
> > > > host_impl_->SetPageScaleOnActiveTree(1.1f);
> > > > On 2015/03/02 19:19:07, sunnyps wrote:
> > > > > On 2015/02/26 15:30:45, ajuma wrote:
> > > > > > Does this line have no effect on behavior? (As I understand it, the
> > > > > > justification for removing this test is that the
> > use_pinch_zoom_scrollbars
> > > > > > setting isn't read anywhere, so this is really testing the same
thing
> as
> > > > > > ScrollbarLinearFadeScheduling, but that's only true if changing page
> > scale
> > > > > > doesn't change expected scrollbar animation behavior.)
> > > > > 
> > > > > It seems the scrollbar fade animation is triggered by the
> > > > > SetPageScaleOnActiveTree call rather than being triggered by the
> > > > > ScrollBegin/ScrollBy/ScrollEnd calls below as the test would have you
> > > believe.
> > > > 
> > > > That's not true, ScrollBy will activate the fade animation, but this
used
> to
> > > be
> > > > gated based on the page scale. If page scale was at the minimum then we
> > > wouldn't
> > > > trigger the animation. This was to prevent overlay scrollbars from
showing
> > up
> > > > over top of page scrollbars when they're unneeded. Now we run the
> animation
> > > > regardless of page scale but hide the subtree when page scale is near
the
> > > > minimum. See https://codereview.chromium.org/885063002/ for more
details.
> > > 
> > > If you add these lines
> > > 
> > >   host_impl_->active_tree()->PushPageScaleFromMainThread(1.f, 1.f, 4.f);
> > >   host_impl_->SetPageScaleOnActiveTree(1.1f);
> > > 
> > > to the beginning of the test (before the first set of EXPECTs) the
scrollbar
> > > animation is triggered immediately without a scroll being performed.
> > > 
> > > So it's not the ScrollBy that's activating the fade. The old test seems to
> > > suggest that ScrollBy should cause the fade.
> > > 
> > 
> > Right, I should have mentioned, changing page scale *also* activates the
> > animation but so does scrolling.
> 
> In the test that's being removed (ScrollbarFadePinchZoomScrollbars) the
> scrolling did not cause the scrollbar animation. I believe this is because the
> scroll was along the x-axis (5, 0) which doesn't result in a successful scroll
> based on how the layer tree is setup. Instead the animation was caused by
> changing the page scale.
> 
> What I really meant to ask was: should we add a test for changing page scale
> causes a scrollbar animation?

I think it'd be sufficient to just add that to the existing test.

[sunnyps, 2015-03-03 20:47:09]

Addressed comments. PTAL.

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1640: // After a scroll, a fade
animation should be scheduled about 20ms from now.
On 2015/03/02 22:49:06, bokan wrote:
> Nit: this is no longer specifically for fade, please update the comments.

Done.

https://codereview.chromium.org/955233002/diff/1/cc/trees/layer_tree_host_imp...
cc/trees/layer_tree_host_impl_unittest.cc:1686: // Unnecessarily Fade animation
of solid color scrollbar is not triggered.
On 2015/03/02 19:19:07, sunnyps wrote:
> On 2015/02/26 15:30:45, ajuma wrote:
> > While you're moving this: s/Unnecessarily/Unnecessary
> 
> Acknowledged.

Done.

[bokan, 2015-03-04 16:41:01]

lgtm, thanks!

[ajuma, 2015-03-04 16:42:54]

lgtm too

[sunnyps, 2015-03-04 19:20:23]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-03-04 19:20:35]

The CQ bit was unchecked by sunnyps@chromium.org

[sunnyps, 2015-03-04 19:22:06]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-03-04 19:24:15]

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

[commit-bot: I haz the power, 2015-03-04 20:21:17]

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

[commit-bot: I haz the power, 2015-03-04 20:21:18]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunnyps, 2015-03-07 00:04:34]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-03-07 00:05:22]

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

[commit-bot: I haz the power, 2015-03-07 02:06:21]

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

[commit-bot: I haz the power, 2015-03-07 02:06:23]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
Timed out jobs are not retried to avoid causing additional load on the builders
with large pending queues.

[sunnyps, 2015-03-07 02:11:35]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-03-07 02:12:38]

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

[commit-bot: I haz the power, 2015-03-07 02:17:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-07 02:18:45]

Patchset 3 (id:??) landed as
https://crrev.com/102dbf69bebd519dedb439a22083f7c722383984
Cr-Commit-Position: refs/heads/master@{#319546}