Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2554913002:
Prevent overlay scrollbars expand or hover together (Closed)
DescriptionPrevent overlay scrollbars expand or hover together
Overlay scrollbars will expand and hover together because we use same state to
calculate two scrollbars' animation.
In this patch, we separate responsibilities of
ScrollbarAnimationControllerThinning. We keep fade in/out animation stills in
ScrollbarAnimationControllerThinning and move thinning animation to
SingleScrollbarAnimationControllerThinning.
BUG=669677
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2554913002
Cr-Commit-Position: refs/heads/master@{#446594}
Committed: https://chromium.googlesource.com/chromium/src/+/2c3e1700308bc1756a408cfac4b5cfa5f84da0a3
Patch Set 1 #
Total comments: 2
Patch Set 2 : add ScrollbarAnimationControllerThinningTest #
Total comments: 12
Patch Set 3 : merged sahel's commit #Patch Set 4 : fix for fade in/out together #
Total comments: 6
Patch Set 5 : separate responsibilities of SACT and SSACT #
Total comments: 13
Patch Set 6 : testcase fixed #Patch Set 7 : fix for test #
Total comments: 8
Patch Set 8 : bokan comment addressed. #
Total comments: 1
Patch Set 9 : bokan@ comment#25 addressed #Patch Set 10 : Merge remote-tracking branch 'origin/master' into fix-669677 #
Total comments: 8
Patch Set 11 : bokan@ comments addressed #Patch Set 12 : rebase update #
Total comments: 5
Patch Set 13 : bokan comment#29 addressed #
Total comments: 3
Patch Set 14 : bokan comments#31 addressed. #
Total comments: 1
Patch Set 15 : move single scrollbar anitmation controller thinning to scrollbar anitmation controller #Patch Set 16 : add scrollbar_animation_controller_client.h #
Total comments: 1
Patch Set 17 : move ScrollbarAnimationControllerClient back #
Total comments: 4
Patch Set 18 : for weiliangc's nit #Messages
Total messages: 57 (27 generated)
Description was changed from ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calc two scrollbar. In this patch, we still using ScrollbarAnimationControllerThinning as entry, but use two separated SingleScrollbarAnimationControllerThinning to save states and do the animation. BUG=669677 ========== to ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calc two scrollbar. In this patch, we still using ScrollbarAnimationControllerThinning as entry, but use two separated SingleScrollbarAnimationControllerThinning to save states and do the animation. BUG=669677 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calc two scrollbar. In this patch, we still using ScrollbarAnimationControllerThinning as entry, but use two separated SingleScrollbarAnimationControllerThinning to save states and do the animation. BUG=669677 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calc two scrollbar. In this patch, we still using ScrollbarAnimationControllerThinning as entry, but use two separated SingleScrollbarAnimationControllerThinning to save states and do the animation. BUG=669677 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
Ok, overall approach looks fine to me. For tests, I think you should move the existing *thinning_unittests to be the tests for the SingleScrollbarAnimationController and add new tests for the parent ScrollbarAnimationControllerThinning that make sure the animations kicked off are independent. https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller.h (right): https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller.h:59: virtual void DidMouseMoveNear(vector<DistanceToScrollbar>) {} Rather than introducing DistanceToScrollbar, I think using (Orientation, distance) as params here is better. If you need to just make two calls at the call site. https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... File cc/input/scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... cc/input/scrollbar_animation_controller_thinning.h:63: vector<unique_ptr<SingleScrollbarAnimationControllerThinning>> I think it would be simpler just to have two unique_ptr<SingleScrollbarAnimationControllerThinning> m_horizontal... and m_vertical...
On 2016/12/06 19:44:46, bokan wrote: > Ok, overall approach looks fine to me. > > For tests, I think you should move the existing *thinning_unittests to be the > tests for the SingleScrollbarAnimationController and add new tests for the > parent ScrollbarAnimationControllerThinning that make sure the animations kicked > off are independent. > > https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... > File cc/input/scrollbar_animation_controller.h (right): > > https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... > cc/input/scrollbar_animation_controller.h:59: virtual void > DidMouseMoveNear(vector<DistanceToScrollbar>) {} > Rather than introducing DistanceToScrollbar, I think using (Orientation, > distance) as params here is better. If you need to just make two calls at the > call site. > > https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... > File cc/input/scrollbar_animation_controller_thinning.h (right): > > https://codereview.chromium.org/2554913002/diff/1/cc/input/scrollbar_animatio... > cc/input/scrollbar_animation_controller_thinning.h:63: > vector<unique_ptr<SingleScrollbarAnimationControllerThinning>> > I think it would be simpler just to have two > unique_ptr<SingleScrollbarAnimationControllerThinning> m_horizontal... and > m_vertical... PTAL. Thank you.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.cc:51: SingleScrollbarAnimationControllerThinning* This should return a ref since we'll always have a controller. https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.cc:73: return v_scrollbar_controller->ScrollbarsHidden(); Hidden state should be part of this class since scrollbars can't be independently hidden. https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.cc:89: // This method is deleted in ScrollbarAnimationControllerThinning It'd be more appropriate to say that we delegate running the animation to the child controllers. https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.h:8: #include <algorithm> I think this is unneeded? https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.h:18: using std::unique_ptr; I would prefer just using std:: in the declarations. https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.h:22: class CC_EXPORT ScrollbarAnimationControllerThinning Please file a bug and add a TODO to clean up the inheritance hierarchy here. The reason we have an abstract ScrollbarAnimationController is because we have the thinning and fade versions of SAC. Issue 656606 (you can block the new bug on this one) tracks merging the fade version into this one. Once we do that, there's no reason for the SingleScrollbarAnimationControllerThinning to be related to ScrollbarAnimationControllerThinning so we can remove the empty overrides and NOTREACHED() checks. https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.h:62: unique_ptr<SingleScrollbarAnimationControllerThinning> h_scrollbar_controller; Call these vertical_controller_ and horizontal_controller_. Don't forget the trailing _. https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller_thinning_unittest.cc (right): https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning_unittest.cc:113: Some more cases I'd like to see: -Move mouse near both scrollbars at the same time. -Move mouse from one to the other scrollbar before animation is finished. -Move mouse from one to the other as above and then back. Basically, the tests here should make sure the coordination between the two isn't getting confused. https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning_unittest.cc:124: // Scroll content. Move the mouse near the scrollbar and confirm it becomes "mouse near the" -> "mouse near each" https://codereview.chromium.org/2554913002/diff/40001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning_unittest.cc:137: // Now move the mouse near the v scrollbar. This should start animating Nit: v -> vertical https://codereview.chromium.org/2554913002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2554913002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3276: DeviceSpaceDistanceToLayer(device_viewport_point, scrollbar) / Will this return 0 if the mouse is over the scrollbar layer?
Please take a early look at this patch and ignore the test files. https://codereview.chromium.org/2554913002/diff/40001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2554913002/diff/40001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3276: DeviceSpaceDistanceToLayer(device_viewport_point, scrollbar) / On 2016/12/08 19:34:07, bokan wrote: > Will this return 0 if the mouse is over the scrollbar layer? Yes
I didn't look at all the details but generally, I think it'll be easier if you better separate the responsibilities between the two controllers. The parent should keep all the state about opacity and visibility. It should also have all the code to run delayed tasks and animate the opacity itself. The child controllers should only have state and code necessary for thinning. https://codereview.chromium.org/2554913002/diff/80001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/80001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.cc:99: DidChangeScrollbarVisibility(vertical_controller_->ScrollbarsHidden()); The visibility should be a property of this parent controller. More generally, I think you should separate the responsibilities: The parent controller should be responsible for tracking and animating visibility/opacity. The child controllers should only worry about thinning/thickening. https://codereview.chromium.org/2554913002/diff/80001/cc/input/scrollbar_anim... File cc/input/scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/80001/cc/input/scrollbar_anim... cc/input/scrollbar_animation_controller_thinning.h:42: bool captured() const; This can be private? https://codereview.chromium.org/2554913002/diff/80001/cc/input/single_scrollb... File cc/input/single_scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/80001/cc/input/single_scrollb... cc/input/single_scrollbar_animation_controller_thinning.cc:99: ApplyOpacity(1.f - progress); I think you should move opacity to be scrolled on the parent controller and let it worry about animating opacity. https://codereview.chromium.org/2554913002/diff/80001/cc/input/single_scrollb... File cc/input/single_scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/80001/cc/input/single_scrollb... cc/input/single_scrollbar_animation_controller_thinning.h:83: ScrollbarSet Scrollbars() const; This should just return one scrollbar. https://codereview.chromium.org/2554913002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2554913002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3277: new_animation_controller->EnsureScrollbarFadeIn(); Make this private and move it into DidMouseMoveNear. Also, shouldn't this only be called if the mouse is actually over the scrollbar?
PTAL, Thank you. https://codereview.chromium.org/2554913002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2554913002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:3277: new_animation_controller->EnsureScrollbarFadeIn(); On 2016/12/16 14:57:42, bokan wrote: > Make this private and move it into DidMouseMoveNear. > Also, shouldn't this only be called if the mouse is actually over the scrollbar? No, we need to keep scrollbar fade in while the mouse is near.
https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:39: opacity_(1.0f), Why did we change starting opacity to 1? IIRC, starting at 0 was to ensure proper initialization of the hidden state. https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:91: return ScrollbarAnimationController::Animate(now); As I noted in a comment in the SingleScrollbar class, rather than returning here, let them both run simultaneously. That should simplify things. https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:94: need_animate = horizontal_controller_->Animate(now) || need_animate; need_animate |= https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:156: void ScrollbarAnimationControllerThinning::EnsureScrollbarFadeIn() { Call this FadeInIfNeeded https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:157: // Don't fade out the scrollbar when mouse is near. I don't get why we need this method, the fade in/out of scrollbars shouldn't depend on the mouse position. Why do we want to fade the scrollbars in when the scroll ends and why only when the mouse is near? Also, the comment is about fade out. https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.h:43: bool ScrollbarsHidden() const override; Move this back under DidMouseMoveNear (needless moves make tracing through git blame more tedious). https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.h:51: void DidMouseMoveNear(ScrollbarOrientation, float) override; Omitting parameter name in header file is only called for in Blink code style, in CC we should keep param names. https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.h:66: bool captured() const; Methods in Chromium should only start with lower case if they're inline, i.e. bool captured() const { return captured_; } https://codereview.chromium.org/2554913002/diff/100001/cc/input/single_scroll... File cc/input/single_scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/100001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning.cc:96: delayed_scrollbar_fade_->Cancel(); This is still splitting responsibility for fade animations between controllers. This controller shouldn't even know about the fade animation. I suspect you're trying to sequence the two animations, am I right? This will be much simpler and less error prone if you just let the animations run independently. So that we might be fading and thickening at the same time but that should be ok.
In this patch, I move fade in/out to scrollbar_animation_controller_thinning and fix all testcases. Also add new testcases: -Move mouse near each. -Move mouse near both scrollbars at the same time. -Move mouse from one to the other scrollbar before animation is finished, then away. PTAL. https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:39: opacity_(1.0f), On 2016/12/19 16:11:47, bokan wrote: > Why did we change starting opacity to 1? IIRC, starting at 0 was to ensure > proper initialization of the hidden state. Because scrollbars show when page load is done, then fade out. https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:157: // Don't fade out the scrollbar when mouse is near. On 2016/12/19 16:11:47, bokan wrote: > I don't get why we need this method, the fade in/out of scrollbars shouldn't > depend on the mouse position. Why do we want to fade the scrollbars in when the > scroll ends and why only when the mouse is near? Also, the comment is about fade > out. I changed it to `if (mouse_is_near_any_scrollbar() && !ScrollbarsHidden())`, we need this to make scrollbars fade in and stop fade out animation when we move mouse near scrollbars in middle of fade out animation.
Description was changed from ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calc two scrollbar. In this patch, we still using ScrollbarAnimationControllerThinning as entry, but use two separated SingleScrollbarAnimationControllerThinning to save states and do the animation. BUG=669677 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calculate two scrollbars' animation. In this patch, we separate responsibilities of ScrollbarAnimationControllerThinning. We keep fade in/out animation stills in ScrollbarAnimationControllerThinning and move thinning animation to SingleScrollbarAnimationControllerThinning. BUG=669677 ==========
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calculate two scrollbars' animation. In this patch, we separate responsibilities of ScrollbarAnimationControllerThinning. We keep fade in/out animation stills in ScrollbarAnimationControllerThinning and move thinning animation to SingleScrollbarAnimationControllerThinning. BUG=669677 ========== to ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calculate two scrollbars' animation. In this patch, we separate responsibilities of ScrollbarAnimationControllerThinning. We keep fade in/out animation stills in ScrollbarAnimationControllerThinning and move thinning animation to SingleScrollbarAnimationControllerThinning. BUG=669677 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:39: opacity_(1.0f), On 2016/12/20 20:05:55, chaopeng wrote: > On 2016/12/19 16:11:47, bokan wrote: > > Why did we change starting opacity to 1? IIRC, starting at 0 was to ensure > > proper initialization of the hidden state. > > Because scrollbars show when page load is done, then fade out. Right, but this worked before. Blink is responsible to causing the scrollbars to show on load. https://codereview.chromium.org/2554913002/diff/100001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:157: // Don't fade out the scrollbar when mouse is near. On 2016/12/20 20:05:54, chaopeng wrote: > On 2016/12/19 16:11:47, bokan wrote: > > I don't get why we need this method, the fade in/out of scrollbars shouldn't > > depend on the mouse position. Why do we want to fade the scrollbars in when > the > > scroll ends and why only when the mouse is near? Also, the comment is about > fade > > out. > > I changed it to `if (mouse_is_near_any_scrollbar() && !ScrollbarsHidden())`, we > need this to make scrollbars fade in and stop fade out animation when we move > mouse near scrollbars in middle of fade out animation. Ah, ok. In that case, I would just inline this insie DidMouseMove since that's the only place it's used. I'd also negate the if and wrap ApplyOpacity and StopAnimation in it. https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.h (right): https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.h:12: #include "cc/input/scrollbar.h" Not used. https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.h:72: bool animating() const { return is_animating_; } this is confusing when used in the child class. Rename it to animating_fade() https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:97: !currently_scrolling() && !mouse_is_near_any_scrollbar()) This shouldn't be necessary. Why do we need to keep trying to post the animation? Why are the places the fade animation is currently posted from not enough? https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:108: client_->SetNeedsAnimateForScrollbarAnimation(); Why do you need this? https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:148: StopAnimation(); This seems wrong. Why would we stop the animation only if the mouse is near? A scroll update should stop the animation regardless. I don't see why we're not just Posting the animation if the mouse isn't near as we did before. https://codereview.chromium.org/2554913002/diff/140001/cc/input/single_scroll... File cc/input/single_scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/140001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning.cc:102: if (!mouse_is_over_scrollbar_ || hidden_) This class shouldn't worry about hidden_ state either. I.e. if you remove it, we'll do things like animate even when they're faded out but who cares? The user can't see it.
PTAL, Thank you. https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:148: StopAnimation(); On 2016/12/21 15:56:02, bokan wrote: > This seems wrong. Why would we stop the animation only if the mouse is near? A > scroll update should stop the animation regardless. I don't see why we're not > just Posting the animation if the mouse isn't near as we did before. because we post delay task in ScrollbarAnimationController::DidScrollUpdate https://cs.chromium.org/chromium/src/cc/input/scrollbar_animation_controller....
Thanks, it's starting to shape up. I'll take a look at tests tomorrow. https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/140001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:148: StopAnimation(); On 2016/12/21 16:57:47, chaopeng wrote: > On 2016/12/21 15:56:02, bokan wrote: > > This seems wrong. Why would we stop the animation only if the mouse is near? A > > scroll update should stop the animation regardless. I don't see why we're not > > just Posting the animation if the mouse isn't near as we did before. > > because we post delay task in ScrollbarAnimationController::DidScrollUpdate > https://cs.chromium.org/chromium/src/cc/input/scrollbar_animation_controller.... Oh yes, that's obvious now that you say it. Please add a comment that we started a fade out timer but don't want to fade out if the mouse is nearby. https://codereview.chromium.org/2554913002/diff/160001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/160001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:133: } else if (!animating_fade() && !currently_scrolling()) { I don't think you need !currently_scrolling, it's used to distinguish a continuous scroll like a touch scroll from wheel like discrete scrolls. If we're touch scrolling (so that currently_scrolling == true) we can't be moving the mouse. Instead, I would check that the scrollbars aren't hidden (actually just return early if ScrollbarsHidden() above) in this branch as well since we don't want to be spamming animation tasks when the scrollbars are already hidden.
https://codereview.chromium.org/2554913002/diff/200001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/200001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:117: horizontal_controller_->DidMouseUp(); If we were captured here, you need to kick off a delayed fade out here. Right now, we won't ever fade out scrollbars until a mouse move. https://codereview.chromium.org/2554913002/diff/200001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:152: // |ScrollbarAnimationControllerThinning::DidScrollUpdate| but don't want to This should be |ScrollbarAnimationController::DidScrollUpdate|. https://codereview.chromium.org/2554913002/diff/200001/cc/input/single_scroll... File cc/input/single_scrollbar_animation_controller_thinning_unittest.cc (right): https://codereview.chromium.org/2554913002/diff/200001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning_unittest.cc:37: void PostDelayedScrollbarAnimationTask(const base::Closure& start_fade, This should never be called, you can mock this. In fact, you should be able to mock most of the methods here. https://codereview.chromium.org/2554913002/diff/200001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning_unittest.cc:228: // thickening until the new animation catches up to the current thickness. Hmm, I don't like this. Could we make it that the scrollbar starts thickening immediately? Is this the way it works today? If so we can change it in a followup. https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl.cc:3250: LayerImpl* scroll_layer_impl = FindScrollLayerForDeviceViewportPoint( While trying your patch out, I noticed scroll_layer_impl is always nullptr here (without your patch). Something's broken and it's causing the scrollbars not to fade out properly in some cases (e.g. capture, drag, move mouse "far", release...scrollbar never fades out). The scrollbars in ToT will fade out but probably by accident, could you investigate and see where this is going wrong? https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:2804: host_impl_->Animate(); // All Animate will check need fade out? I don't understand what this comment is saying.
PTAL https://codereview.chromium.org/2554913002/diff/200001/cc/input/single_scroll... File cc/input/single_scrollbar_animation_controller_thinning_unittest.cc (right): https://codereview.chromium.org/2554913002/diff/200001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning_unittest.cc:228: // thickening until the new animation catches up to the current thickness. On 2017/01/06 20:17:56, bokan wrote: > Hmm, I don't like this. Could we make it that the scrollbar starts thickening > immediately? > > Is this the way it works today? If so we can change it in a followup. Yes, today we just change the animation direction and set need animation(in startAnimation) when we receive a mouse move. https://cs.chromium.org/chromium/src/cc/input/scrollbar_animation_controller_... https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host_impl_unittest.cc:2804: host_impl_->Animate(); // All Animate will check need fade out? On 2017/01/06 20:17:57, bokan wrote: > I don't understand what this comment is saying. It just a note I forgot to remove...
On 2017/01/10 20:49:50, chaopeng wrote: > PTAL > > https://codereview.chromium.org/2554913002/diff/200001/cc/input/single_scroll... > File cc/input/single_scrollbar_animation_controller_thinning_unittest.cc > (right): > > https://codereview.chromium.org/2554913002/diff/200001/cc/input/single_scroll... > cc/input/single_scrollbar_animation_controller_thinning_unittest.cc:228: // > thickening until the new animation catches up to the current thickness. > On 2017/01/06 20:17:56, bokan wrote: > > Hmm, I don't like this. Could we make it that the scrollbar starts thickening > > immediately? > > > > Is this the way it works today? If so we can change it in a followup. > > Yes, today we just change the animation direction and set need animation(in > startAnimation) when we receive a mouse move. > https://cs.chromium.org/chromium/src/cc/input/scrollbar_animation_controller_... > > https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... > File cc/trees/layer_tree_host_impl_unittest.cc (right): > > https://codereview.chromium.org/2554913002/diff/200001/cc/trees/layer_tree_ho... > cc/trees/layer_tree_host_impl_unittest.cc:2804: host_impl_->Animate(); // All > Animate will check need fade out? > On 2017/01/06 20:17:57, bokan wrote: > > I don't understand what this comment is saying. > > It just a note I forgot to remove... Sorry I haven't gotten to this yet. I've been bogged down with 56 release blockers which are urgent. I'll find time for this tomorrow, promise :)
Mostly minor nits but I did find a bug. https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.h (right): https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.h:62: Nit: remove new line https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:140: PostDelayedAnimationTask(false); We shouldn't post a fade out task if a scrollbar is captured. Right now, if I capture a scrollbar, move the mouse far away and wait, the scrollbars will fade out. Scrolling will now not bring back that scrollbar! I would also add a check in Animate whether the scrollbar is captured and if it is, avoid the base Animate method and StopAnimation(). https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.h:18: // and reacts to mouse movements. Add to this comment an explanation of how this class works, that the fade out works on both scrollbars and is controlled in this class. The thinning animations are independent between vertical/horizontal and are managed by the SingleScrollbarControllers. https://codereview.chromium.org/2554913002/diff/240001/cc/input/single_scroll... File cc/input/single_scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/240001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning.cc:144: if (captured_) { You don't need this block, you can just wrap the thickness_change_ and StartAnimation() in !captured_. https://codereview.chromium.org/2554913002/diff/240001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning.cc:154: if (mouse_is_over_scrollbar_ != mouse_is_over_scrollbar) Likewise, you don't need these checks.
On 2017/01/20 18:57:25, bokan wrote: > Mostly minor nits but I did find a bug. > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... > File cc/input/scrollbar_animation_controller.h (right): > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... > cc/input/scrollbar_animation_controller.h:62: > Nit: remove new line > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... > File cc/input/scrollbar_animation_controller_thinning.cc (right): > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... > cc/input/scrollbar_animation_controller_thinning.cc:140: > PostDelayedAnimationTask(false); > We shouldn't post a fade out task if a scrollbar is captured. Right now, if I > capture a scrollbar, move the mouse far away and wait, the scrollbars will fade > out. Scrolling will now not bring back that scrollbar! > > I would also add a check in Animate whether the scrollbar is captured and if it > is, avoid the base Animate method and StopAnimation(). > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... > File cc/input/scrollbar_animation_controller_thinning.h (right): > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/scrollbar_ani... > cc/input/scrollbar_animation_controller_thinning.h:18: // and reacts to mouse > movements. > Add to this comment an explanation of how this class works, that the fade out > works on both scrollbars and is controlled in this class. The thinning > animations are independent between vertical/horizontal and are managed by the > SingleScrollbarControllers. > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/single_scroll... > File cc/input/single_scrollbar_animation_controller_thinning.cc (right): > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/single_scroll... > cc/input/single_scrollbar_animation_controller_thinning.cc:144: if (captured_) { > You don't need this block, you can just wrap the thickness_change_ and > StartAnimation() in !captured_. > > https://codereview.chromium.org/2554913002/diff/240001/cc/input/single_scroll... > cc/input/single_scrollbar_animation_controller_thinning.cc:154: if > (mouse_is_over_scrollbar_ != mouse_is_over_scrollbar) > Likewise, you don't need these checks. Updated, PTAL. Thank you.
Ok, few more small issues but lgtm otherwise. https://codereview.chromium.org/2554913002/diff/260001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/260001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.cc:91: bool animated = ScrollbarAnimationController::Animate(now); Please add a DCHECK(!Captured()) here. https://codereview.chromium.org/2554913002/diff/260001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/260001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.h:19: // This class It passes the mouse state to each Nit: "class It passes" -> "class passes" https://codereview.chromium.org/2554913002/diff/260001/cc/input/single_scroll... File cc/input/single_scrollbar_animation_controller_thinning.cc (right): https://codereview.chromium.org/2554913002/diff/260001/cc/input/single_scroll... cc/input/single_scrollbar_animation_controller_thinning.cc:145: mouse_is_near_scrollbar_ = mouse_is_near_scrollbar; This is set at the end, just use the argument in the thickness_change_ and remove this line
chaopeng@chromium.org changed reviewers: + weiliangc@chromium.org
weiliangc@chromium.org: Please review changes
As far as I can tell nothing uses LinearFade right now. (Only place setting linear fade has comment saying it's not used https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...) If it is indeed the case, I think we can clean that up in the same CL? https://codereview.chromium.org/2554913002/diff/280001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning.h (right): https://codereview.chromium.org/2554913002/diff/280001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning.h:24: // TODO(chaopeng) clean up the inheritance hierarchy after Is there a reason we are not doing this in this CL?
On 2017/01/24 23:19:57, weiliangc wrote: > As far as I can tell nothing uses LinearFade right now. (Only place setting > linear fade has comment saying it's not used > https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...) > > If it is indeed the case, I think we can clean that up in the same CL? LinearFade is used on Android so it needs to stay around for now. I'm guessing you missed it cause it's #ifdef ANDROID so code search doesn't see it. > > https://codereview.chromium.org/2554913002/diff/280001/cc/input/scrollbar_ani... > File cc/input/scrollbar_animation_controller_thinning.h (right): > > https://codereview.chromium.org/2554913002/diff/280001/cc/input/scrollbar_ani... > cc/input/scrollbar_animation_controller_thinning.h:24: // TODO(chaopeng) clean > up the inheritance hierarchy after > Is there a reason we are not doing this in this CL? Since we still need LinearFade. We need to merge LinearFade and Thinning first. Then we can get rid of the whole hierarchy with one concrete class. But that'll be a big change.
On 2017/01/24 at 23:22:40, bokan wrote: > On 2017/01/24 23:19:57, weiliangc wrote: > > As far as I can tell nothing uses LinearFade right now. (Only place setting > > linear fade has comment saying it's not used > > https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...) > > > > If it is indeed the case, I think we can clean that up in the same CL? > > LinearFade is used on Android so it needs to stay around for now. I'm guessing you missed it cause it's #ifdef ANDROID so code search doesn't see it. Ah I see. Sorry I missed that. I'm not very happy about where this CL ended up. A bunch of functions on SingleScrollbarAnimationControllerThinning regarding mouse position needs to pass through ScrollbarAnimationControllerThinning, while those oerride functions seems pretty much separated from what ScrollbarAnimationControllerThinning is trying to do. Since this CL is trying to separate the fading and thinning, I'd prefer this CL ends up in a cleaner state. How about: - ScrollbarAnimationController rename to ScrollbarAnimationControllerFading, remove all the mouse related function here. - SingleScrollbarAnimationControllerThinning moved to live on scrollbar layers This would also avoid awkward places in this CL where set_mouse_distance_for_test sets the same distance for both scrollbars, and animating_fade() returns is_animating_ on ScrollbarAnimationController. WDYT? > > > > > https://codereview.chromium.org/2554913002/diff/280001/cc/input/scrollbar_ani... > > File cc/input/scrollbar_animation_controller_thinning.h (right): > > > > https://codereview.chromium.org/2554913002/diff/280001/cc/input/scrollbar_ani... > > cc/input/scrollbar_animation_controller_thinning.h:24: // TODO(chaopeng) clean > > up the inheritance hierarchy after > > Is there a reason we are not doing this in this CL? > > Since we still need LinearFade. We need to merge LinearFade and Thinning first. Then we can get rid of the whole hierarchy with one concrete class. But that'll be a big change.
On 2017/01/25 00:02:11, weiliangc wrote: > On 2017/01/24 at 23:22:40, bokan wrote: > > On 2017/01/24 23:19:57, weiliangc wrote: > > > As far as I can tell nothing uses LinearFade right now. (Only place setting > > > linear fade has comment saying it's not used > > > > https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...) > > > > > > If it is indeed the case, I think we can clean that up in the same CL? > > > > LinearFade is used on Android so it needs to stay around for now. I'm guessing > you missed it cause it's #ifdef ANDROID so code search doesn't see it. > > Ah I see. Sorry I missed that. > > I'm not very happy about where this CL ended up. A bunch of functions on > SingleScrollbarAnimationControllerThinning regarding mouse position needs to > pass through ScrollbarAnimationControllerThinning, while those oerride functions > seems pretty much separated from what ScrollbarAnimationControllerThinning is > trying to do. > > Since this CL is trying to separate the fading and thinning, I'd prefer this CL > ends up in a cleaner state. > > How about: > - ScrollbarAnimationController rename to ScrollbarAnimationControllerFading, > remove all the mouse related function here. > - SingleScrollbarAnimationControllerThinning moved to live on scrollbar layers > > This would also avoid awkward places in this CL where > set_mouse_distance_for_test sets the same distance for both scrollbars, and > animating_fade() returns is_animating_ on ScrollbarAnimationController. > > WDYT? Re: naming, we'll need to consolidate and rename all the controller classes but renames make reviews messy so I asked Chao to defer on that until a followup patch. Once we merge the Linear and Thinning classes, I think we can just fold ScrollbarAnimationController into ScrollbarAnimationControllerThinning and call the result ScrollbarAnimationController. The SingleScrollbarAnimationControllerThinning can then be renamed to ScrollbarThinningAnimator or something similar. I don't think we should move SingleScrollbarAnimationControllerThinning out of the parent controller though. There's some linked behavior between the two that needs to be controlled from the parent controller (e.g. we shouldn't allow capturing when the scrollbars are hidden) and this composition keeps the complexity and implementation details encapsulated rather than spreading it out at the call sites. The fact that the animations are independent between horizontal/vertical should be an implementation detail of the scroll layer's ScrollbarAnimationController.
On 2017/01/25 at 14:09:24, bokan wrote: > On 2017/01/25 00:02:11, weiliangc wrote: > > On 2017/01/24 at 23:22:40, bokan wrote: > > > On 2017/01/24 23:19:57, weiliangc wrote: > > > > As far as I can tell nothing uses LinearFade right now. (Only place setting > > > > linear fade has comment saying it's not used > > > > > > https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...) > > > > > > > > If it is indeed the case, I think we can clean that up in the same CL? > > > > > > LinearFade is used on Android so it needs to stay around for now. I'm guessing > > you missed it cause it's #ifdef ANDROID so code search doesn't see it. > > > > Ah I see. Sorry I missed that. > > > > I'm not very happy about where this CL ended up. A bunch of functions on > > SingleScrollbarAnimationControllerThinning regarding mouse position needs to > > pass through ScrollbarAnimationControllerThinning, while those oerride functions > > seems pretty much separated from what ScrollbarAnimationControllerThinning is > > trying to do. > > > > Since this CL is trying to separate the fading and thinning, I'd prefer this CL > > ends up in a cleaner state. > > > > How about: > > - ScrollbarAnimationController rename to ScrollbarAnimationControllerFading, > > remove all the mouse related function here. > > - SingleScrollbarAnimationControllerThinning moved to live on scrollbar layers > > > > This would also avoid awkward places in this CL where > > set_mouse_distance_for_test sets the same distance for both scrollbars, and > > animating_fade() returns is_animating_ on ScrollbarAnimationController. > > > > WDYT? > > Re: naming, we'll need to consolidate and rename all the controller classes but renames make reviews messy so I asked Chao to defer on that until a followup patch. Once we merge the Linear and Thinning classes, I think we can just fold ScrollbarAnimationController into ScrollbarAnimationControllerThinning and call the result ScrollbarAnimationController. The SingleScrollbarAnimationControllerThinning can then be renamed to ScrollbarThinningAnimator or something similar. Sounds reasonable. > > I don't think we should move SingleScrollbarAnimationControllerThinning out of the parent controller though. There's some linked behavior between the two that needs to be controlled from the parent controller (e.g. we shouldn't allow capturing when the scrollbars are hidden) and this composition keeps the complexity and implementation details encapsulated rather than spreading it out at the call sites. The fact that the animations are independent between horizontal/vertical should be an implementation detail of the scroll layer's ScrollbarAnimationController. Keeping the ScollbarAnimatorController as a consistent entry point sounds great. Thanks for pointing that out. How about we move the single scrollbar controllers to live inside ScrollbarAnimationController? All the mouse positions related call can just live in ScrollbarAnimationController without being override. ScrollbarAnimationController could know whether scrollbars are hidden and aid with capturing logic. We can meet up and talk more if you want?
On 2017/01/25 17:03:27, weiliangc wrote: > On 2017/01/25 at 14:09:24, bokan wrote: > > On 2017/01/25 00:02:11, weiliangc wrote: > > > On 2017/01/24 at 23:22:40, bokan wrote: > > > > On 2017/01/24 23:19:57, weiliangc wrote: > > > > > As far as I can tell nothing uses LinearFade right now. (Only place > setting > > > > > linear fade has comment saying it's not used > > > > > > > > > https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...) > > > > > > > > > > If it is indeed the case, I think we can clean that up in the same CL? > > > > > > > > LinearFade is used on Android so it needs to stay around for now. I'm > guessing > > > you missed it cause it's #ifdef ANDROID so code search doesn't see it. > > > > > > Ah I see. Sorry I missed that. > > > > > > I'm not very happy about where this CL ended up. A bunch of functions on > > > SingleScrollbarAnimationControllerThinning regarding mouse position needs to > > > pass through ScrollbarAnimationControllerThinning, while those oerride > functions > > > seems pretty much separated from what ScrollbarAnimationControllerThinning > is > > > trying to do. > > > > > > Since this CL is trying to separate the fading and thinning, I'd prefer this > CL > > > ends up in a cleaner state. > > > > > > How about: > > > - ScrollbarAnimationController rename to ScrollbarAnimationControllerFading, > > > remove all the mouse related function here. > > > - SingleScrollbarAnimationControllerThinning moved to live on scrollbar > layers > > > > > > This would also avoid awkward places in this CL where > > > set_mouse_distance_for_test sets the same distance for both scrollbars, and > > > animating_fade() returns is_animating_ on ScrollbarAnimationController. > > > > > > WDYT? > > > > Re: naming, we'll need to consolidate and rename all the controller classes > but renames make reviews messy so I asked Chao to defer on that until a followup > patch. Once we merge the Linear and Thinning classes, I think we can just fold > ScrollbarAnimationController into ScrollbarAnimationControllerThinning and call > the result ScrollbarAnimationController. The > SingleScrollbarAnimationControllerThinning can then be renamed to > ScrollbarThinningAnimator or something similar. > > Sounds reasonable. > > > > > I don't think we should move SingleScrollbarAnimationControllerThinning out of > the parent controller though. There's some linked behavior between the two that > needs to be controlled from the parent controller (e.g. we shouldn't allow > capturing when the scrollbars are hidden) and this composition keeps the > complexity and implementation details encapsulated rather than spreading it out > at the call sites. The fact that the animations are independent between > horizontal/vertical should be an implementation detail of the scroll layer's > ScrollbarAnimationController. > > Keeping the ScollbarAnimatorController as a consistent entry point sounds great. > Thanks for pointing that out. > > How about we move the single scrollbar controllers to live inside > ScrollbarAnimationController? > All the mouse positions related call can just live in > ScrollbarAnimationController without being override. Did you mean SingleScrollbarAnimationController here? > ScrollbarAnimationController could know whether scrollbars are hidden and aid > with capturing logic. > > We can meet up and talk more if you want? Your suggestion sounds reasonable. I've got 1 hour this afternoon from 1-2 if that works for both you and Chao we could sync up quickly for ~5 min to get on the same page. Otherwise I think I'm fine with what you propose if I understand correctly.
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
moved single_scrollbar_anitmation_controller_thinning to hold in scrollbar_anitmation_controller. PTAL thank you.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2554913002/diff/320001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_client.h (right): https://codereview.chromium.org/2554913002/diff/320001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_client.h:15: class CC_EXPORT ScrollbarAnimationControllerClient { Why did this have to get split out into a separate file?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/25 22:20:54, bokan wrote: > https://codereview.chromium.org/2554913002/diff/320001/cc/input/scrollbar_ani... > File cc/input/scrollbar_animation_controller_client.h (right): > > https://codereview.chromium.org/2554913002/diff/320001/cc/input/scrollbar_ani... > cc/input/scrollbar_animation_controller_client.h:15: class CC_EXPORT > ScrollbarAnimationControllerClient { > Why did this have to get split out into a separate file? Yes, I should not move it out. Already move it back. PTAL. Thank you.
LGTM https://codereview.chromium.org/2554913002/diff/340001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller.h (right): https://codereview.chromium.org/2554913002/diff/340001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller.h:49: virtual bool NeedThinningAnimation() const; Not need to change this, just providing some alternative way to implement this: could either do a const bool that is set during constructor, or only call Create of SingleScrollbarAnimationControllerThinning in ScrollbarAnimationControllerThinning constructor and check whether the pointers are null whenever try to access. https://codereview.chromium.org/2554913002/diff/340001/cc/input/scrollbar_ani... File cc/input/scrollbar_animation_controller_thinning_unittest.cc (right): https://codereview.chromium.org/2554913002/diff/340001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning_unittest.cc:128: // Check initialization of scrollbar. Should start of invisible and thin. nit: not need to change comment. https://codereview.chromium.org/2554913002/diff/340001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning_unittest.cc:168: // Shrink along X axis, Horizontal Scrollbar should appear. nit: not need to change comment. https://codereview.chromium.org/2554913002/diff/340001/cc/input/scrollbar_ani... cc/input/scrollbar_animation_controller_thinning_unittest.cc:179: // Shrink along Y axis and expand along X, Horizontal Scrollbar nit: not need to change comment.
The CQ bit was checked by chaopeng@chromium.org
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/2554913002/#ps360001 (title: "for weiliangc's nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 360001, "attempt_start_ts": 1485484717056570,
"parent_rev": "e56024e9bcb988df909d4914aa48ea762b5cd0d3", "commit_rev":
"2c3e1700308bc1756a408cfac4b5cfa5f84da0a3"}
Message was sent while issue was closed.
Description was changed from ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calculate two scrollbars' animation. In this patch, we separate responsibilities of ScrollbarAnimationControllerThinning. We keep fade in/out animation stills in ScrollbarAnimationControllerThinning and move thinning animation to SingleScrollbarAnimationControllerThinning. BUG=669677 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Prevent overlay scrollbars expand or hover together Overlay scrollbars will expand and hover together because we use same state to calculate two scrollbars' animation. In this patch, we separate responsibilities of ScrollbarAnimationControllerThinning. We keep fade in/out animation stills in ScrollbarAnimationControllerThinning and move thinning animation to SingleScrollbarAnimationControllerThinning. BUG=669677 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2554913002 Cr-Commit-Position: refs/heads/master@{#446594} Committed: https://chromium.googlesource.com/chromium/src/+/2c3e1700308bc1756a408cfac4b5... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:360001) as https://chromium.googlesource.com/chromium/src/+/2c3e1700308bc1756a408cfac4b5... |
