Merge Compositor's ScrollbarAnimationControllers into single class

Merge Compositor's ScrollbarAnimationControllers into single class

This patch is for merge ScrollbarAnimationControllerLinearFade and
ScrollbarAnimationControllerThinning into ScrollbarAnimationController.

BUG=656606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2692243005
Cr-Commit-Position: refs/heads/master@{#451397}
Committed: https://chromium.googlesource.com/chromium/src/+/59e6fc09543e4cb2aa20e1869130a51bff279bae

[chaopeng, 2017-02-14 17:13:49]

Description was changed from

==========
Merge Compositor's ScrollbarAnimationControllers into single class

This patch is for merge ScrollbarAnimationControllerLinearFade and
ScrollbarAnimationControllerThinning into ScrollbarAnimationController.

BUG=656606
==========

to

==========
Merge Compositor's ScrollbarAnimationControllers into single class

This patch is for merge ScrollbarAnimationControllerLinearFade and
ScrollbarAnimationControllerThinning into ScrollbarAnimationController.

BUG=656606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[chaopeng, 2017-02-14 17:15:23]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 17:15:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chaopeng, 2017-02-14 18:29:43]

chaopeng@chromium.org changed reviewers:
+ weiliangc@chromium.org

[chaopeng, 2017-02-14 18:29:43]

PTAL. Thank you.

[commit-bot: I haz the power, 2017-02-14 18:42:29]

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

[commit-bot: I haz the power, 2017-02-14 18:42:30]

Dry run: This issue passed the CQ dry run.

[weiliangc, 2017-02-15 19:41:15]

Probably bikeshedding, but do you think would it be better to change the names
of the two type of animation controllers? Thinning v.s. LinearFade seem to imply
that the first one does not fade. Do you think naming it Overlay v.s. Android
and explain in comment what the difference is a good idea?

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:277: void
ScrollbarAnimationController::set_mouse_move_distance_for_test(
Since set_moust_move_distance_for_test is available on
SingleScrollbarAnimationControllerThinning probably don't need this class here?

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
File cc/input/scrollbar_animation_controller.h (right):

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.h:49: // ScrollbarAnimationController
for Desktop Overlay Scrollbar.
Could you elaborate more about how this is different than Android? Mentioning
this will have the thinning animation and Android won't.

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.h:128: bool need_thinning_animation_;
Make this const to show it is not changed outside constructor.

[chaopeng, 2017-02-15 19:59:50]

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:277: void
ScrollbarAnimationController::set_mouse_move_distance_for_test(
On 2017/02/15 19:41:15, weiliangc wrote:
> Since set_moust_move_distance_for_test is available on
> SingleScrollbarAnimationControllerThinning probably don't need this class
here?

This is the entry for layer_tree_host_impl_unittest.cc access.

https://codereview.chromium.org/2692243005/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2692243005/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1379: case LayerTreeSettings::LINEAR_FADE: {
I agree change name to ScrollbarAnimationControllerOverlay vs
ScrollbarAnimationControllerAndroid would be better. But should we also change
the LayerTreeSettings?

[weiliangc, 2017-02-15 20:06:08]

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2692243005/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:277: void
ScrollbarAnimationController::set_mouse_move_distance_for_test(
On 2017/02/15 at 19:59:50, chaopeng wrote:
> On 2017/02/15 19:41:15, weiliangc wrote:
> > Since set_moust_move_distance_for_test is available on
> > SingleScrollbarAnimationControllerThinning probably don't need this class
here?
> 
> This is the entry for layer_tree_host_impl_unittest.cc access.

I see three use cases there, none of which seems like the call is really
necessary? They are all followed by actually setting a mouse position.

https://codereview.chromium.org/2692243005/diff/1/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/2692243005/diff/1/cc/trees/layer_tree_impl.cc...
cc/trees/layer_tree_impl.cc:1379: case LayerTreeSettings::LINEAR_FADE: {
On 2017/02/15 at 19:59:50, chaopeng wrote:
> I agree change name to ScrollbarAnimationControllerOverlay vs
ScrollbarAnimationControllerAndroid would be better. But should we also change
the LayerTreeSettings?

Yeah sounds good. Make sure to comment there too.

[chaopeng, 2017-02-16 21:07:10]

Updated, PTAL.

[chaopeng, 2017-02-16 21:15:07]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 21:16:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 21:39:56]

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

[commit-bot: I haz the power, 2017-02-16 21:39:57]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-02-16 21:46:20]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 21:47:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 22:11:18]

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

[commit-bot: I haz the power, 2017-02-16 22:11:19]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-02-17 00:02:08]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-17 00:04:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weiliangc, 2017-02-17 00:29:55]

LGTM w/ nit.

https://codereview.chromium.org/2692243005/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/2692243005/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:3216: EXPECT_TRUE(
Is this test case not needed now?

[chaopeng, 2017-02-17 00:36:07]

https://codereview.chromium.org/2692243005/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (left):

https://codereview.chromium.org/2692243005/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:3216: EXPECT_TRUE(
On 2017/02/17 00:29:54, weiliangc wrote:
> Is this test case not needed now?

Yes, This testcase is for test set_mouse_move_distance_for_test.

[chaopeng, 2017-02-17 00:39:45]

chaopeng@chromium.org changed reviewers:
+ nick@chromium.org

[chaopeng, 2017-02-17 00:39:45]

nick@chromium.org: Please review changes in content. Thank you.

[commit-bot: I haz the power, 2017-02-17 03:06:48]

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

[commit-bot: I haz the power, 2017-02-17 03:06:49]

Dry run: This issue passed the CQ dry run.

[ncarter (slow), 2017-02-17 20:47:17]

https://codereview.chromium.org/2692243005/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2692243005/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:441:
settings.scrollbar_animator = cc::LayerTreeSettings::ANDROID_OVERLAY;
As a result of the renaming, this looks weirder than before, because we're using
an ANDROID_ constant inside a !defined(OS_ANDROID) #ifdef context.

I don't particularly mind either style of name, but as someone who isn't
familiar with this code, I found it much easier to understand the original
THINNING/LINEAR_FADE names.

lgtm because I don't want to bikeshed, but maybe you could add comments to the
enum declarations like: // Android-style linear fade animation ?

[chaopeng, 2017-02-17 21:32:53]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-02-17 21:33:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chaopeng, 2017-02-17 21:35:58]

https://codereview.chromium.org/2692243005/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2692243005/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:441:
settings.scrollbar_animator = cc::LayerTreeSettings::ANDROID_OVERLAY;
On 2017/02/17 20:47:17, ncarter wrote:
> As a result of the renaming, this looks weirder than before, because we're
using
> an ANDROID_ constant inside a !defined(OS_ANDROID) #ifdef context.
> 
> I don't particularly mind either style of name, but as someone who isn't
> familiar with this code, I found it much easier to understand the original
> THINNING/LINEAR_FADE names.
> 
> lgtm because I don't want to bikeshed, but maybe you could add comments to the
> enum declarations like: // Android-style linear fade animation ?

We have a plan to fix Mac scrollbar and this TODO soon.

[commit-bot: I haz the power, 2017-02-17 23:36:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487367173602650,
"parent_rev": "76f81913e233d3c4e9911ba822133c0600f550bd", "commit_rev":
"59e6fc09543e4cb2aa20e1869130a51bff279bae"}

[commit-bot: I haz the power, 2017-02-17 23:37:38]

Description was changed from

==========
Merge Compositor's ScrollbarAnimationControllers into single class

This patch is for merge ScrollbarAnimationControllerLinearFade and
ScrollbarAnimationControllerThinning into ScrollbarAnimationController.

BUG=656606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Merge Compositor's ScrollbarAnimationControllers into single class

This patch is for merge ScrollbarAnimationControllerLinearFade and
ScrollbarAnimationControllerThinning into ScrollbarAnimationController.

BUG=656606
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2692243005
Cr-Commit-Position: refs/heads/master@{#451397}
Committed:
https://chromium.googlesource.com/chromium/src/+/59e6fc09543e4cb2aa20e1869130...
==========

[commit-bot: I haz the power, 2017-02-17 23:37:39]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/59e6fc09543e4cb2aa20e1869130...

[bokan, 2017-02-21 15:49:56]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2017-02-21 15:49:56]

https://codereview.chromium.org/2692243005/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2692243005/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:441:
settings.scrollbar_animator = cc::LayerTreeSettings::ANDROID_OVERLAY;
On 2017/02/17 21:35:58, chaopeng wrote:
> On 2017/02/17 20:47:17, ncarter wrote:
> > As a result of the renaming, this looks weirder than before, because we're
> using
> > an ANDROID_ constant inside a !defined(OS_ANDROID) #ifdef context.
> > 
> > I don't particularly mind either style of name, but as someone who isn't
> > familiar with this code, I found it much easier to understand the original
> > THINNING/LINEAR_FADE names.
> > 
> > lgtm because I don't want to bikeshed, but maybe you could add comments to
the
> > enum declarations like: // Android-style linear fade animation ?
> 
> We have a plan to fix Mac scrollbar and this TODO soon. 

This is already for non-Mac. I think we could just remove this section without
breaking anything. Could you put up a patch?