Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
571873003:
[Android]Optimization of scrollbar animation. (Closed)
Description[Android]Optimization of scrollbar animation.
Scrollbar animation is required if scrollbar parameters are changed.
If any of the scrollbar parameters are changed that should trigger the
scrollbar animation. If any of the scrollbar parameter is not changed,
that doesn't require scrollbar animation. Current Patch ensures that
scrollbar is animated only when any of the scrollbar parameter is
changed.
BUG=414238
Committed: https://crrev.com/68d7d9b4b8f1cb8f2924c9809f72e0aada530946
Cr-Commit-Position: refs/heads/master@{#295978}
Patch Set 1 #
Total comments: 16
Patch Set 2 : addresed review comments, using SetScrollbarNeedsAnimation #Patch Set 3 : addressed review comments, using ResetScrollbarNeedsAnimation #Patch Set 4 : Crashing Text case fixed #Patch Set 5 : Adding test case for above patch #
Total comments: 8
Patch Set 6 : Addressed review comments #
Total comments: 2
Patch Set 7 : #
Total comments: 1
Messages
Total messages: 41 (13 generated)
sataya.m@samsung.com changed reviewers: + danakj@chromium.org
PTAL. Thanks, MuVen.
weiliangc@chromium.org changed reviewers: + weiliangc@chromium.org
Do you have tests for this? Have you run cc_unittest with your change? https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1279: float current_pos = (scrollbar_layer->orientation() == HORIZONTAL) nit: Prefer one if statement than three ternary operator that checks the same condition. https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1289: if (!scrollbar_layer->ScrollbarNeedsUpdate( As per comment in scrollbar_layer_impl_base file, no need to write a new function. Setters functions already check whether values have changed. https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1298: layer_tree_impl()->set_needs_update_draw_properties(); The four Setter function seems to call this line when value changes, maybe we don't need this line any more? https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1303: if (scrollbar_animation_controller_) { I am not too familiar with pinch-zoom scrollbar behaviors. cc bokan@ who works on pinch-zoom scrollbars. https://codereview.chromium.org/571873003/diff/1/cc/layers/scrollbar_layer_im... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/571873003/diff/1/cc/layers/scrollbar_layer_im... cc/layers/scrollbar_layer_impl_base.h:61: bool ScrollbarNeedsUpdate(float vertical_adjust, The four setter function already checks whether the value has changed. I don't think you need a new function for this.
bokan@chromium.org changed reviewers: + bokan@chromium.org
https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1278: float vertical_adjust = scrollbar_clip_layer->bounds_delta().y(); I think this should only affect the vertical scrollbar (we vertically adjust the bounds delta when top controls shrink the viewport). https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1279: float current_pos = (scrollbar_layer->orientation() == HORIZONTAL) On 2014/09/15 17:14:37, weiliangc wrote: > nit: Prefer one if statement than three ternary operator that checks the same > condition. Agreed. https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1285: float visible_ratio = (scrollbar_layer->orientation() == HORIZONTAL) Do we need this? I think if the maximum changes the visible ratio must change as well, no? https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1298: layer_tree_impl()->set_needs_update_draw_properties(); On 2014/09/15 17:14:37, weiliangc wrote: > The four Setter function seems to call this line when value changes, maybe we > don't need this line any more? I think his idea is that we need to skip the DidScrollUpdate call below so that we don't schedule an animation on the scrollbar. That said, I'd prefer you remove the method and just do the check inline. ScrollbarNeedsUpdate is vague and the helper method isn't saving us any duplication or readibility.
I have ran the unittest for the same , LayerTreeHostImplTest.ScrollbarLinearFadeScheduling test has failed. Working on this. https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1279: float current_pos = (scrollbar_layer->orientation() == HORIZONTAL) On 2014/09/15 17:14:37, weiliangc wrote: > nit: Prefer one if statement than three ternary operator that checks the same > condition. Done. https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1285: float visible_ratio = (scrollbar_layer->orientation() == HORIZONTAL) if maximum changes, visible ration also changes as well. On 2014/09/15 17:29:55, bokan wrote: > Do we need this? I think if the maximum changes the visible ratio must change as > well, no? https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1289: if (!scrollbar_layer->ScrollbarNeedsUpdate( Yes i agree weiliangc, Setter functions are already checking whether values have changed. On 2014/09/15 17:14:37, weiliangc wrote: > As per comment in scrollbar_layer_impl_base file, no need to write a new > function. Setters functions already check whether values have changed. https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1298: layer_tree_impl()->set_needs_update_draw_properties(); On 2014/09/15 17:14:37, weiliangc wrote: > The four Setter function seems to call this line when value changes, maybe we > don't need this line any more? Done. https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1298: layer_tree_impl()->set_needs_update_draw_properties(); Yes bokan, Main idea is to skip DidScrollUpdate call below so that we dont schedule an animation on the scrollbar. To do inline check i think we need to have a bool variable set when CurrentPos or Maximum or visible ratio has new value.For Which Before calling setters of CurrentPos or Maximum or visible ratio have to reset the bool variable to false and check it inline. On 2014/09/15 17:29:55, bokan wrote: > On 2014/09/15 17:14:37, weiliangc wrote: > > The four Setter function seems to call this line when value changes, maybe we > > don't need this line any more? > > I think his idea is that we need to skip the DidScrollUpdate call below so that > we don't schedule an animation on the scrollbar. That said, I'd prefer you > remove the method and just do the check inline. ScrollbarNeedsUpdate is vague > and the helper method isn't saving us any duplication or readibility.
https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1298: layer_tree_impl()->set_needs_update_draw_properties(); or may be can i use LayerPropertyChanged() ??
https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/1/cc/layers/layer_impl.cc#newc... cc/layers/layer_impl.cc:1298: layer_tree_impl()->set_needs_update_draw_properties(); On 2014/09/15 17:57:07, muven wrote: > or may be can i use LayerPropertyChanged() ?? That's not the same thing. When something changes that will affect transforms/opacity/positions/sizes/clips/etc... set_needs_update_draw_properties() needs to happen. If helper methods do then, then that's fine.
Hi, PTAL. There is an improvement of 1.75% of power consumption with this patch as it prevents un-necessary scrollbar animations. I have added small test case for the above patch, and fixed the unittest which was crashing. Thanks,
https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1362: host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0, 5)); Just want to make sure, why would this fix the test crash? What is the reason for crash in the first place?
https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl_unittest.cc:1362: host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0, 5)); Crash in the sense Expect_LT has failed.Sorry or my confusing english. In this test Vertical Scrollbar is created and there is scrollby by offset in horizontal direction, due to which there is no change for vertical scrollbar layer properties(because of my patch), due to which animation is not triggered and because of which EXPECT_LT has failed after scrollend. On 2014/09/16 16:02:11, weiliangc wrote: > Just want to make sure, why would this fix the test crash? What is the reason > for crash in the first place?
On 2014/09/16 16:10:14, muven wrote: > https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... > File cc/trees/layer_tree_host_impl_unittest.cc (right): > > https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... > cc/trees/layer_tree_host_impl_unittest.cc:1362: > host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0, 5)); > Crash in the sense Expect_LT has failed.Sorry or my confusing english. In this > test Vertical Scrollbar is created and there is scrollby by offset in horizontal > direction, due to which there is no change for vertical scrollbar layer > properties(because of my patch), due to which animation is not triggered and > because of which EXPECT_LT has failed after scrollend. > On 2014/09/16 16:02:11, weiliangc wrote: > > Just want to make sure, why would this fix the test crash? What is the reason > > for crash in the first place? Awesome, thanks for taking time to do this. lgtm.
The CQ bit was checked by sataya.m@samsung.com
On 2014/09/16 16:58:26, weiliangc wrote: > On 2014/09/16 16:10:14, muven wrote: > > > https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... > > File cc/trees/layer_tree_host_impl_unittest.cc (right): > > > > > https://codereview.chromium.org/571873003/diff/80001/cc/trees/layer_tree_host... > > cc/trees/layer_tree_host_impl_unittest.cc:1362: > > host_impl_->ScrollBy(gfx::Point(), gfx::Vector2dF(0, 5)); > > Crash in the sense Expect_LT has failed.Sorry or my confusing english. In this > > test Vertical Scrollbar is created and there is scrollby by offset in > horizontal > > direction, due to which there is no change for vertical scrollbar layer > > properties(because of my patch), due to which animation is not triggered and > > because of which EXPECT_LT has failed after scrollend. > > On 2014/09/16 16:02:11, weiliangc wrote: > > > Just want to make sure, why would this fix the test crash? What is the > reason > > > for crash in the first place? > > Awesome, thanks for taking time to do this. lgtm. thanks :)
The CQ bit was unchecked by sataya.m@samsung.com
The CQ bit was checked by sataya.m@samsung.com
may be need owners LGTM for this. @dana, PTAL. Thanks,
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/571873003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM https://codereview.chromium.org/571873003/diff/80001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/80001/cc/layers/layer_impl.cc#... cc/layers/layer_impl.cc:1296: if (scrollbar_animation_controller_) { move it up here: if (controller && needs animation) https://codereview.chromium.org/571873003/diff/80001/cc/layers/layer_impl.cc#... cc/layers/layer_impl.cc:1302: scrollbar_layer->scrollbar_needs_animation() && remove this here, it's not part of |is_animatable_scrollbar|
The CQ bit was checked by danakj@chromium.org
The CQ bit was unchecked by danakj@chromium.org
Can you add a bit more to your patch description to say what this CL is doing, not just what is expected from the code?
Also the BUG linked in your description doesn't seem to really be about this change, is it? Can you file a bug more about this and assign to yourself?
aelias@chromium.org changed reviewers: + aelias@chromium.org
Sorry, I just noticed this patch and I have one more change I'd like before this can land. https://codereview.chromium.org/571873003/diff/80001/cc/layers/scrollbar_laye... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/571873003/diff/80001/cc/layers/scrollbar_laye... cc/layers/scrollbar_layer_impl_base.h:105: bool scrollbar_needs_animation_; I don't think it makes any sense to add a new boolean in this class because the state is entirely transient and can be local to LayerImpl::SetScrollbarPosition instead. Please remove all the changes to this class and instead determine the animatable boolean by comparing old values with new values within LayerImpl::SetScrollbarPosition.
https://codereview.chromium.org/571873003/diff/80001/cc/layers/scrollbar_laye... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/571873003/diff/80001/cc/layers/scrollbar_laye... cc/layers/scrollbar_layer_impl_base.h:105: bool scrollbar_needs_animation_; On 2014/09/19 23:20:17, aelias wrote: > I don't think it makes any sense to add a new boolean in this class because the > state is entirely transient and can be local to LayerImpl::SetScrollbarPosition > instead. Please remove all the changes to this class and instead determine the > animatable boolean by comparing old values with new values within > LayerImpl::SetScrollbarPosition. That's a good idea. Wei previously pointed out these Set methods already do an == check, so we could make use of that by making them each return bool if they changed something. Then LayerImpl can decide to animate if any of the functions returns true.
Patchset #6 (id:100001) has been deleted
PTAL. https://codereview.chromium.org/571873003/diff/80001/cc/layers/scrollbar_laye... File cc/layers/scrollbar_layer_impl_base.h (right): https://codereview.chromium.org/571873003/diff/80001/cc/layers/scrollbar_laye... cc/layers/scrollbar_layer_impl_base.h:105: bool scrollbar_needs_animation_; On 2014/09/19 23:20:17, aelias wrote: > I don't think it makes any sense to add a new boolean in this class because the > state is entirely transient and can be local to LayerImpl::SetScrollbarPosition > instead. Please remove all the changes to this class and instead determine the > animatable boolean by comparing old values with new values within > LayerImpl::SetScrollbarPosition. Done. https://codereview.chromium.org/571873003/diff/80001/cc/layers/scrollbar_laye... cc/layers/scrollbar_layer_impl_base.h:105: bool scrollbar_needs_animation_; Yes this is good idea.This eliminates the needs for new bool and reseting the bool. Thanks for the suggestions.
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/571873003/diff/140001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/140001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1283: scrollbar_needs_animation = This will reset to false if any later line is false. Use |= in all these lines.
PTAL. https://codereview.chromium.org/571873003/diff/140001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/140001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1283: scrollbar_needs_animation = On 2014/09/22 06:53:01, aelias wrote: > This will reset to false if any later line is false. Use |= in all these lines. True, MissRead the logic :( Done.
LGTM https://codereview.chromium.org/571873003/diff/160001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/571873003/diff/160001/cc/layers/layer_impl.cc... cc/layers/layer_impl.cc:1299: layer_tree_impl()->set_needs_update_draw_properties(); Maybe for a follow up, but what happens if you also make this conditional on things changing? This has the potential to save even more power?
The CQ bit was checked by sataya.m@samsung.com
On 2014/09/22 14:41:25, danakj wrote: > LGTM > > https://codereview.chromium.org/571873003/diff/160001/cc/layers/layer_impl.cc > File cc/layers/layer_impl.cc (right): > > https://codereview.chromium.org/571873003/diff/160001/cc/layers/layer_impl.cc... > cc/layers/layer_impl.cc:1299: > layer_tree_impl()->set_needs_update_draw_properties(); > Maybe for a follow up, but what happens if you also make this conditional on > things changing? This has the potential to save even more power? Sure, Will check.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/571873003/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as 889fb31311b701795993cb4739ce92b7c69c3c4d
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/68d7d9b4b8f1cb8f2924c9809f72e0aada530946 Cr-Commit-Position: refs/heads/master@{#295978} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
