Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand when mouse is near the whole scrollbar today.
In this patch, we change it to expand only when mouse is near thumb.

We pass mouse position to SingleScrollbarAnimationControllerThinning
instead of calculating the distance from mouse position to scrollbar in
LayerTreeHostImpl.

In SingleScrollbarAnimationControllerThinning::DidMouseMove we
calculate:

1. distance from mouse to scrollbar: determind should scrollbar keep
appear, fade out or hover fade in.
2. distance from mouse to scrollbar thumb: determind should thick/thin
scrollbar

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

Review-Url: https://codereview.chromium.org/2841943002
Cr-Commit-Position: refs/heads/master@{#468428}
Committed: https://chromium.googlesource.com/chromium/src/+/982642b47574426b124f75c2e0b74f83c8705161

[chaopeng, 2017-04-25 16:15:03]

Description was changed from

==========
Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand when mouse is near the whole scrollbar today.
In this patch, we change it to expand only when mouse is near thumb.

We pass mouse position to ScrollbarAnimationController instead of calculating
the distance from mouse position to scrollbar in LayerTreeHostImpl.

In ScrollbarAnimationController::DidMouseMoveNear we calculate:

1. distance from mouse to scrollbar: determind should scrollbar keep appear,
   fade out or hover fade in.
2. distance from mouse to scrollbar thumb: pass to
   SingleScrollbarAnimationController and determind should thick/thin scrollbar


BUG=713218
==========

to

==========
Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand when mouse is near the whole scrollbar today.
In this patch, we change it to expand only when mouse is near thumb.

We pass mouse position to ScrollbarAnimationController instead of calculating
the distance from mouse position to scrollbar in LayerTreeHostImpl.

In ScrollbarAnimationController::DidMouseMoveNear we calculate:

1. distance from mouse to scrollbar: determind should scrollbar keep appear,
   fade out or hover fade in.
2. distance from mouse to scrollbar thumb: pass to
   SingleScrollbarAnimationController and determind should thick/thin scrollbar


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

[chaopeng, 2017-04-25 16:16:14]

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

[chaopeng, 2017-04-25 16:16:14]

Please take a early look at this patch. I will fix and add test later. Thank
you.

[bokan, 2017-04-25 19:36:21]

Looks fine to me, a few suggestions inline...

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

https://codereview.chromium.org/2841943002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:14: float DistanceToScrollbar(const
gfx::PointF& device_viewport_point,
This should be called "DistanceToScrollbarTrack"

https://codereview.chromium.org/2841943002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:332: void
ScrollbarAnimationController::DidMouseMoveNear(
This no longer cares about distance - lets just call it DidMoveMouse

https://codereview.chromium.org/2841943002/diff/1/cc/input/scrollbar_animatio...
cc/input/scrollbar_animation_controller.cc:379: if
(vertical_controller_->mouse_is_over_scrollbar_thumb() ||
I think the show should still happen for "over the whole track". But I think
this method's accumulated some obsolete code. The scrollbars should show when
the mouse is near, not over, which is covered below so I think this is redundant
and can be removed, right?

https://codereview.chromium.org/2841943002/diff/1/cc/input/single_scrollbar_a...
File cc/input/single_scrollbar_animation_controller_thinning.cc (right):

https://codereview.chromium.org/2841943002/diff/1/cc/input/single_scrollbar_a...
cc/input/single_scrollbar_animation_controller_thinning.cc:133: void
SingleScrollbarAnimationControllerThinning::DidMouseMoveNear(
This can also just be DidMoveMouse

https://codereview.chromium.org/2841943002/diff/1/cc/input/single_scrollbar_a...
cc/input/single_scrollbar_animation_controller_thinning.cc:134: float
distance_to_scrollbar,
Why does the thinning controller need to know the distance to the track now? All
the thickness related animations should be based on distance to thumb, I think
this is unused.

https://codereview.chromium.org/2841943002/diff/1/cc/layers/scrollbar_layer_i...
File cc/layers/scrollbar_layer_impl_base.cc (right):

https://codereview.chromium.org/2841943002/diff/1/cc/layers/scrollbar_layer_i...
cc/layers/scrollbar_layer_impl_base.cc:218: float thumb_thickness_adjustment =
This method should just call the new one above and apply the
thumb_thickness_adjustment to the rect returned from there.

[chaopeng, 2017-04-27 03:13:01]

Last comments addressed and added a test
`ScrollbarAnimationControllerAuraOverlayTest.MoveNearTrackThenNearThumb`. PTAL.
Thank you.

[bokan, 2017-04-28 14:15:17]

https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:46: const ScrollbarOrientation
orientations[] = {HORIZONTAL, VERTICAL};
This is only used for one loop. Move this into the function where it's used.

https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.h (right):

https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.h:162: scrollbar_controllers_[2];
Please keep these as vertical_controller_ and horizontal_controller_. IMO,
having `for` loops to do something for both controllers ends up being less
readable and, in most cases, doesn't save us from any code copying. For cases
where you want to do something based on ScrollbarOrientation, you can keep the
helper return the correct controller.

https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.h:164: bool mouse_is_near_scrollbar_[2];
Lets move this into the thinning controller, pass the actual point in
DidMoveMouse, and calculate "is near/over track/thumb" inside the individual
controller. Then this class can just ask the Thinning controllers for the
current mouse<->scrollbar state.

The "Thinning" controller should really be an "IndividualScrollbar" controller,
but we can rename later. The point being that ThinningController manages state
for each individual scrollbar. That way the code becomes more natural as you
don't have to duplicate the Vertical/Horizontal split in this class more than
once.

[chaopeng, 2017-04-28 16:18:13]

On 2017/04/28 14:15:17, bokan (OOO until May 9) wrote:
>
https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
> File cc/input/scrollbar_animation_controller.cc (right):
> 
>
https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
> cc/input/scrollbar_animation_controller.cc:46: const ScrollbarOrientation
> orientations[] = {HORIZONTAL, VERTICAL};
> This is only used for one loop. Move this into the function where it's used.
> 
>
https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
> File cc/input/scrollbar_animation_controller.h (right):
> 
>
https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
> cc/input/scrollbar_animation_controller.h:162: scrollbar_controllers_[2];
> Please keep these as vertical_controller_ and horizontal_controller_. IMO,
> having `for` loops to do something for both controllers ends up being less
> readable and, in most cases, doesn't save us from any code copying. For cases
> where you want to do something based on ScrollbarOrientation, you can keep the
> helper return the correct controller.
> 
>
https://codereview.chromium.org/2841943002/diff/40001/cc/input/scrollbar_anim...
> cc/input/scrollbar_animation_controller.h:164: bool
mouse_is_near_scrollbar_[2];
> Lets move this into the thinning controller, pass the actual point in
> DidMoveMouse, and calculate "is near/over track/thumb" inside the individual
> controller. Then this class can just ask the Thinning controllers for the
> current mouse<->scrollbar state.
> 
> The "Thinning" controller should really be an "IndividualScrollbar"
controller,
> but we can rename later. The point being that ThinningController manages state
> for each individual scrollbar. That way the code becomes more natural as you
> don't have to duplicate the Vertical/Horizontal split in this class more than
> once.

Updated. PTAL. Thank you.

[bokan, 2017-04-28 18:08:11]

Thanks, looks good! I have a few style comments but once those are addressed,
lgtm!

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.cc (right):

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.cc:292: const ScrollbarOrientation
orientations[] = {HORIZONTAL, VERTICAL};
No need for loop, just: 

vertical_controller_->DidMouseMove(device_viewport_point)
horizontal_controller_->DidMouesMove(device_viewport_point)

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller.h (right):

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller.h:91: bool
MouseIsOverScrollbarThumb(ScrollbarOrientation orientation) const;
Add a comment that these methods are public for testing

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
File cc/input/scrollbar_animation_controller_unittest.cc (right):

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:133: gfx::PointF
NearVerticalScrollbarBegin(float offset_x, float offset_y) {
Please add a short description comment above these methods describing what
they're used for and what they return.

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:291: // and confirm it keeps
thick. Move the mouse near the scrollbar thumb and
I think you meant "confirm it stays thin" on this line, right?

https://codereview.chromium.org/2841943002/diff/80001/cc/input/scrollbar_anim...
cc/input/scrollbar_animation_controller_unittest.cc:308: // the currently queued
fading animation and start animating thickness.
We don't start animating thickness until you move near the thumb so move that
part of the comment below. You want to confirm it stays thin though (which you
do but add that to the comment).

https://codereview.chromium.org/2841943002/diff/80001/cc/input/single_scrollb...
File cc/input/single_scrollbar_animation_controller_thinning.cc (right):

https://codereview.chromium.org/2841943002/diff/80001/cc/input/single_scrollb...
cc/input/single_scrollbar_animation_controller_thinning.cc:22: if (!scrollbar)
Make the scrollbar param a ref and avoid calling the function at the callsite if
there's no scrollbar for the orientation.

https://codereview.chromium.org/2841943002/diff/80001/cc/input/single_scrollb...
cc/input/single_scrollbar_animation_controller_thinning.cc:85: if
(!scrollbar->is_overlay_scrollbar())
This can be a DCHECK right? We should never have a thinning controller if the
scrollbar isn't overlay.

[chaopeng, 2017-04-28 18:54:56]

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

[chaopeng, 2017-04-28 18:54:56]

weiliangc@chromium.org: Please review changes.

[weiliangc, 2017-05-01 19:48:02]

LGTM

https://codereview.chromium.org/2841943002/diff/100001/cc/input/single_scroll...
File cc/input/single_scrollbar_animation_controller_thinning.cc (right):

https://codereview.chromium.org/2841943002/diff/100001/cc/input/single_scroll...
cc/input/single_scrollbar_animation_controller_thinning.cc:20: float
DistanceToScrollbarTrack(const gfx::PointF& device_viewport_point,
nit: move common code to another function and share.

[chaopeng, 2017-05-01 20:18:04]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2017-05-01 20:18:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/2841943002/#ps120001
(title: "weiliangc comment addressed")

[commit-bot: I haz the power, 2017-05-01 20:18:45]

CQ is trying da patch.

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

[chaopeng, 2017-05-01 20:19:27]

The CQ bit was unchecked by chaopeng@chromium.org

[chaopeng, 2017-05-01 20:23:00]

Description was changed from

==========
Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand when mouse is near the whole scrollbar today.
In this patch, we change it to expand only when mouse is near thumb.

We pass mouse position to ScrollbarAnimationController instead of calculating
the distance from mouse position to scrollbar in LayerTreeHostImpl.

In ScrollbarAnimationController::DidMouseMoveNear we calculate:

1. distance from mouse to scrollbar: determind should scrollbar keep appear,
   fade out or hover fade in.
2. distance from mouse to scrollbar thumb: pass to
   SingleScrollbarAnimationController and determind should thick/thin scrollbar


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

to

==========
Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand when mouse is near the whole scrollbar today.
In this patch, we change it to expand only when mouse is near thumb.

We pass mouse position to SingleScrollbarAnimationControllerThinning
instead of calculating the distance from mouse position to scrollbar in
LayerTreeHostImpl.

In SingleScrollbarAnimationControllerThinning::DidMouseMove we
calculate:

1. distance from mouse to scrollbar: determind should scrollbar keep
appear, fade out or hover fade in.
2. distance from mouse to scrollbar thumb: determind should thick/thin
scrollbar

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

[chaopeng, 2017-05-01 20:27:03]

The CQ bit was checked by chaopeng@chromium.org

[commit-bot: I haz the power, 2017-05-01 20:28:02]

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-05-01 21:45:35]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493670423969280,
"parent_rev": "6fb00869e878cde5d1eb55fae0397c3bfea4c9c8", "commit_rev":
"982642b47574426b124f75c2e0b74f83c8705161"}

[commit-bot: I haz the power, 2017-05-01 21:45:47]

Description was changed from

==========
Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand when mouse is near the whole scrollbar today.
In this patch, we change it to expand only when mouse is near thumb.

We pass mouse position to SingleScrollbarAnimationControllerThinning
instead of calculating the distance from mouse position to scrollbar in
LayerTreeHostImpl.

In SingleScrollbarAnimationControllerThinning::DidMouseMove we
calculate:

1. distance from mouse to scrollbar: determind should scrollbar keep
appear, fade out or hover fade in.
2. distance from mouse to scrollbar thumb: determind should thick/thin
scrollbar

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

to

==========
Overlay scrollbars expand only when mouse is near thumb

Overlay scrollbars expand when mouse is near the whole scrollbar today.
In this patch, we change it to expand only when mouse is near thumb.

We pass mouse position to SingleScrollbarAnimationControllerThinning
instead of calculating the distance from mouse position to scrollbar in
LayerTreeHostImpl.

In SingleScrollbarAnimationControllerThinning::DidMouseMove we
calculate:

1. distance from mouse to scrollbar: determind should scrollbar keep
appear, fade out or hover fade in.
2. distance from mouse to scrollbar thumb: determind should thick/thin
scrollbar

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

Review-Url: https://codereview.chromium.org/2841943002
Cr-Commit-Position: refs/heads/master@{#468428}
Committed:
https://chromium.googlesource.com/chromium/src/+/982642b47574426b124f75c2e0b7...
==========

[commit-bot: I haz the power, 2017-05-01 21:45:48]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/982642b47574426b124f75c2e0b7...