Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740493003:
Call setNeedsRedraw when scrollbar opacity changes. (Closed)
DescriptionCall setNeedsRedraw when scrollbar opacity changes.
This is needed to ensure that the change to scrollbar opacity is actually
shown if there's no other side effects.
BUG=698275
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2740493003
Cr-Commit-Position: refs/heads/master@{#456782}
Committed: https://chromium.googlesource.com/chromium/src/+/7858bc70062f09e1cd60e9a99dd8e5129d2c0ff2
Patch Set 1 #
Total comments: 4
Patch Set 2 : Call SetNeedsRedraw when changing opacity #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Fix test that was causing redraw during initialization #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Call setNeedsRedraw when scrollbar visibility changes. This is needed to ensure that the change to scrollbar opacity is actually shown. BUG=698275 ========== to ========== Call setNeedsRedraw when scrollbar visibility changes. This is needed to ensure that the change to scrollbar opacity is actually shown. BUG=698275 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
bokan@chromium.org changed reviewers: + weiliangc@chromium.org
bokan@chromium.org changed reviewers: + weiliangc@chromium.org
ptal
ptal
Still thinking it through, sharing some initial thoughts. https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:2298: client_->SetNeedsCommitOnImplThread(); For setNeedsRedraw to work correctly we need to make sure damage is set. Maybe it's possible to utilize how opacity animation works on effect tree? Also I think maybe requesting a commit here is overkill?
https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:2298: client_->SetNeedsCommitOnImplThread(); On 2017/03/07 22:53:34, weiliangc wrote: > For setNeedsRedraw to work correctly we need to make sure damage is set. > Maybe it's possible to utilize how opacity animation works on effect tree? I'm not sure what you mean about utilising animations on the opacity tree, could you elaborate? Is it possible the damage is already being set, since just adding this makes the scrollbar show up now? Or are we defaulting to damaging everything by not setting a region? > Also I think maybe requesting a commit here is overkill? The commit is needed because we propagate the visibility back to Blink since all event handling (i.e. clicking/dragging the scrollbars) is handled on the main thread and so it needs to know when the scrollbars fade in/out on the compositor.
Sorry for the delay. https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:2298: client_->SetNeedsCommitOnImplThread(); On 2017/03/07 at 23:19:49, bokan wrote: > On 2017/03/07 22:53:34, weiliangc wrote: > > For setNeedsRedraw to work correctly we need to make sure damage is set. > > Maybe it's possible to utilize how opacity animation works on effect tree? > > I'm not sure what you mean about utilising animations on the opacity tree, could you elaborate? > I was wondering why when the opacity is changed and not the visibility, we would be able to continue to animate. Seems like we make call to https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=3650 in other animation cases. This is called when animation is run but not on Show(), and is probably needed whenever opacity of scrollbar changes. My suggestion would be try to reduce the ScrollbarAnimationControllerClient interface by calling directly of SetNeedsCommit when visibility change, and always call SetNeedsRedraw on when opacity changes. > Is it possible the damage is already being set, since just adding this makes the scrollbar show up now? Or are we defaulting to damaging everything by not setting a region? > The damage for this is probably set when opacity on property tree is updated. > > Also I think maybe requesting a commit here is overkill? > > The commit is needed because we propagate the visibility back to Blink since all event handling (i.e. clicking/dragging the scrollbars) is handled on the main thread and so it needs to know when the scrollbars fade in/out on the compositor. Thanks for the explanation. Could you add this as comment?
https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2740493003/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:2298: client_->SetNeedsCommitOnImplThread(); On 2017/03/13 20:11:09, weiliangc wrote: > On 2017/03/07 at 23:19:49, bokan wrote: > > On 2017/03/07 22:53:34, weiliangc wrote: > > > For setNeedsRedraw to work correctly we need to make sure damage is set. > > > Maybe it's possible to utilize how opacity animation works on effect tree? > > > > I'm not sure what you mean about utilising animations on the opacity tree, > could you elaborate? > > > > I was wondering why when the opacity is changed and not the visibility, we would > be able to continue to animate. > > Seems like we make call to > https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=3650 in > other animation cases. This is called when animation is run but not on Show(), > and is probably needed whenever opacity of scrollbar changes. > > My suggestion would be try to reduce the ScrollbarAnimationControllerClient > interface by calling directly of SetNeedsCommit when visibility change, and > always call SetNeedsRedraw on when opacity changes. I think code changed so your link is pointing to a different line than you intended. Did you mean that we should call LayerTreeHostImpl::SetNeedsRedrawForScrollbarAnimation()? I've moved that call from RunAnimationFrame into ApplyOpacityToScrollbars so that any change in opacity causes a redraw. I don't think we can reduce the interface of ScrollbarAnimationControllerClient since we can't directly call SetNeedsCommit (we don't have a direct pointer to LTHI) but DidChangeScrollbarVisibility now only has the SetNeedsCommit call > > > Is it possible the damage is already being set, since just adding this makes > the scrollbar show up now? Or are we defaulting to damaging everything by not > setting a region? > > > > The damage for this is probably set when opacity on property tree is updated. Got it, thanks. > > > > Also I think maybe requesting a commit here is overkill? > > > > The commit is needed because we propagate the visibility back to Blink since > all event handling (i.e. clicking/dragging the scrollbars) is handled on the > main thread and so it needs to know when the scrollbars fade in/out on the > compositor. > > Thanks for the explanation. Could you add this as comment? Done.
Description was changed from ========== Call setNeedsRedraw when scrollbar visibility changes. This is needed to ensure that the change to scrollbar opacity is actually shown. BUG=698275 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Call setNeedsRedraw when scrollbar opacity changes. This is needed to ensure that the change to scrollbar opacity is actually shown if there's no other side effects. BUG=698275 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Thanks! LGTM
The CQ bit was checked by bokan@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from weiliangc@chromium.org Link to the patchset: https://codereview.chromium.org/2740493003/#ps80001 (title: "Fix test that was causing redraw during initialization")
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": 80001, "attempt_start_ts": 1489514826123790,
"parent_rev": "258e2a65da4e87393f83ba9755c4be38b983222c", "commit_rev":
"7858bc70062f09e1cd60e9a99dd8e5129d2c0ff2"}
Message was sent while issue was closed.
Description was changed from ========== Call setNeedsRedraw when scrollbar opacity changes. This is needed to ensure that the change to scrollbar opacity is actually shown if there's no other side effects. BUG=698275 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Call setNeedsRedraw when scrollbar opacity changes. This is needed to ensure that the change to scrollbar opacity is actually shown if there's no other side effects. BUG=698275 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2740493003 Cr-Commit-Position: refs/heads/master@{#456782} Committed: https://chromium.googlesource.com/chromium/src/+/7858bc70062f09e1cd60e9a99dd8... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7858bc70062f09e1cd60e9a99dd8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
