Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14017008:
Ensure that pinch-zoom scrollbar animations trigger a draw (Closed)
|
Created:
7 years, 8 months ago by ajuma Modified:
7 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org, Ian Vollick Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnsure that pinch-zoom scrollbar animations trigger a draw
Pinch-zoom scrollbar animations are impl-only animations that
are added directly to an impl-side animation controller rather
than pushed during a commit. If no draw is triggered when they
are created, they can be left waiting indefinitely to get
started. This CL adds calls to SetNeedsRedraw when these
animations are created; once these animations are started,
LayerTreeHostImpl::AnimateLayers already takes care of
triggering additional draws.
BUG=165842
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195068
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 10
Patch Set 3 : Prevent unnecessary commits #
Total comments: 7
Patch Set 4 : Fix nits #
Messages
Total messages: 20 (0 generated)
Can we has a test?
On 2013/04/17 18:46:57, danakj wrote: > Can we has a test? Done.
LGTM with some comments for the test (thanks for it!) https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:784: layer_tree_host()->SetRootLayer(root_layer_); Can you set up the layer tree in SetupTree() instead of BeginTest()? Then you'll get a viewport of the right size and stuff. Just make sure you call LayerTreeHostAnimationTest::SetupTree(); after you set the root layer so it can set up the rest. The page scale stuff can stay here. https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:785: layer_tree_host()->SetPageScaleFactorAndLimits(0.5, 0.5, 0.9); These are floats. 0.5f etc https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:791: if (host_impl->active_tree()->page_scale_factor() == 1.0) 1.f https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:794: if (started_times_ == 0) { seems like a good spot for a switch?
l gtm, with one minor nit. https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:785: layer_tree_host()->SetPageScaleFactorAndLimits(0.5, 0.5, 0.9); Minor nit: we generally would only have pinch zoom scrollbars if page scale > 1. Typical values might be SetPageScaleFactorAndLimits(1.xx, 1, 4); // Probably '2' is less preferable to 1.xx in terms of generality.
https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:785: layer_tree_host()->SetPageScaleFactorAndLimits(0.5, 0.5, 0.9); On 2013/04/17 20:56:54, wjmaclean wrote: > Minor nit: we generally would only have pinch zoom scrollbars if page scale > 1. > Typical values might be > > SetPageScaleFactorAndLimits(1.xx, 1, 4); // Probably '2' is less preferable to > 1.xx in terms of generality. Oh, is this test actually triggering the scrollbars then, or is it testing something else? Can you verify that it fails without your change?
On 2013/04/17 21:01:03, danakj wrote: > https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... > File cc/trees/layer_tree_host_unittest_animation.cc (right): > > https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... > cc/trees/layer_tree_host_unittest_animation.cc:785: > layer_tree_host()->SetPageScaleFactorAndLimits(0.5, 0.5, 0.9); > On 2013/04/17 20:56:54, wjmaclean wrote: > > Minor nit: we generally would only have pinch zoom scrollbars if page scale > > 1. > > Typical values might be > > > > SetPageScaleFactorAndLimits(1.xx, 1, 4); // Probably '2' is less preferable to > > 1.xx in terms of generality. > > Oh, is this test actually triggering the scrollbars then, or is it testing > something else? Can you verify that it fails without your change? Based on my reading of the test, it is setting up an environment that contains active pinch-zoom scrollbars ("settings->use_pinch_zoom_scrollbars = true;" plus "layer_tree_host()->SetPageScaleFactorAndLimits(0.5, 0.5, 0.9);"), and then triggering the events that should initiate fade-in/fade-out animations ("host_impl->active_tree()->DidBeginScroll();" and "host_impl->active_tree()->DidEndScroll();"), and then verifying that these animations begin as expected by tracking calls to AnimateLayers().
On Wed, Apr 17, 2013 at 6:44 PM, <wjmaclean@chromium.org> wrote: > Based on my reading of the test, it is setting up an environment that > contains > active pinch-zoom scrollbars ("settings->use_pinch_zoom_**scrollbars = > true;" plus > "layer_tree_host()->**SetPageScaleFactorAndLimits(0.**5, 0.5, 0.9);"), > and then > triggering the events that should initiate fade-in/fade-out animations > ("host_impl->active_tree()->**DidBeginScroll();" and > "host_impl->active_tree()->**DidEndScroll();"), and then verifying that > these > animations begin as expected by tracking calls to AnimateLayers(). > Ah, my understanding of your first comment was that a scrollbar wouldn't be created for scales < 1, but in fact they will right now (tho > 1 would be better as you said).
On 2013/04/17 21:01:03, danakj wrote: > https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... > File cc/trees/layer_tree_host_unittest_animation.cc (right): > > https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... > cc/trees/layer_tree_host_unittest_animation.cc:785: > layer_tree_host()->SetPageScaleFactorAndLimits(0.5, 0.5, 0.9); > On 2013/04/17 20:56:54, wjmaclean wrote: > > Minor nit: we generally would only have pinch zoom scrollbars if page scale > > 1. > > Typical values might be > > > > SetPageScaleFactorAndLimits(1.xx, 1, 4); // Probably '2' is less preferable to > > 1.xx in terms of generality. > > Oh, is this test actually triggering the scrollbars then, or is it testing > something else? Can you verify that it fails without your change? Hmmm. Testing without the patch, the test is passing because we're always getting 2 or 3 draws after the commit (even without the animation), rather than the 1 draw that the test implicitly assumes we'll get, and this is enough to start the animations. One alternative approach would be to post a delayed task to add the animation sufficiently in the future (say 100 ms) to ensure that all the draws triggered by the commit are done. Any ideas for a better way to wait till LayerTreeHostImpl is done with draws?
On Wed, Apr 17, 2013 at 7:05 PM, <ajuma@chromium.org> wrote: > On 2013/04/17 21:01:03, danakj wrote: > > https://codereview.chromium.**org/14017008/diff/5002/cc/** > trees/layer_tree_host_**unittest_animation.cc<https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_unittest_animation.cc> > >> File cc/trees/layer_tree_host_**unittest_animation.cc (right): >> > > > https://codereview.chromium.**org/14017008/diff/5002/cc/** > trees/layer_tree_host_**unittest_animation.cc#**newcode785<https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_unittest_animation.cc#newcode785> > >> cc/trees/layer_tree_host_**unittest_animation.cc:785: >> layer_tree_host()->**SetPageScaleFactorAndLimits(0.**5, 0.5, 0.9); >> On 2013/04/17 20:56:54, wjmaclean wrote: >> > Minor nit: we generally would only have pinch zoom scrollbars if page >> scale >> > > 1. >> > Typical values might be >> > >> > SetPageScaleFactorAndLimits(1.**xx, 1, 4); // Probably '2' is less >> preferable >> > to > >> > 1.xx in terms of generality. >> > > Oh, is this test actually triggering the scrollbars then, or is it testing >> something else? Can you verify that it fails without your change? >> > > Hmmm. Testing without the patch, the test is passing because we're always > getting 2 or 3 draws after the commit (even without the animation), rather > than > the 1 draw that the test implicitly assumes we'll get, and this is enough > to > start the animations. One alternative approach would be to post a delayed > task > to add the animation sufficiently in the future (say 100 ms) to ensure > that all > the draws triggered by the commit are done. Any ideas for a better way to > wait > till LayerTreeHostImpl is done with draws? We should only draw if something changes the tree/invalidates/requests redraw, can you determine what is causing the phantom draws?
On 2013/04/17 23:08:50, danakj wrote: > On Wed, Apr 17, 2013 at 7:05 PM, <mailto:ajuma@chromium.org> wrote: > > > On 2013/04/17 21:01:03, danakj wrote: > > > > https://codereview.chromium.**org/14017008/diff/5002/cc/** > > > trees/layer_tree_host_**unittest_animation.cc<https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_unittest_animation.cc> > > > >> File cc/trees/layer_tree_host_**unittest_animation.cc (right): > >> > > > > > > https://codereview.chromium.**org/14017008/diff/5002/cc/** > > > trees/layer_tree_host_**unittest_animation.cc#**newcode785<https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_unittest_animation.cc#newcode785> > > > >> cc/trees/layer_tree_host_**unittest_animation.cc:785: > >> layer_tree_host()->**SetPageScaleFactorAndLimits(0.**5, 0.5, 0.9); > >> On 2013/04/17 20:56:54, wjmaclean wrote: > >> > Minor nit: we generally would only have pinch zoom scrollbars if page > >> scale > >> > > > > 1. > >> > Typical values might be > >> > > >> > SetPageScaleFactorAndLimits(1.**xx, 1, 4); // Probably '2' is less > >> preferable > >> > > to > > > >> > 1.xx in terms of generality. > >> > > > > Oh, is this test actually triggering the scrollbars then, or is it testing > >> something else? Can you verify that it fails without your change? > >> > > > > Hmmm. Testing without the patch, the test is passing because we're always > > getting 2 or 3 draws after the commit (even without the animation), rather > > than > > the 1 draw that the test implicitly assumes we'll get, and this is enough > > to > > start the animations. One alternative approach would be to post a delayed > > task > > to add the animation sufficiently in the future (say 100 ms) to ensure > > that all > > the draws triggered by the commit are done. Any ideas for a better way to > > wait > > till LayerTreeHostImpl is done with draws? > > > We should only draw if something changes the tree/invalidates/requests > redraw, can you determine what is causing the phantom draws? Turns out we're getting 3 commits: 1) The one explicitly triggered by the test. 2) During the following ThreadProxy::BeginFrame, LayerTreeHost::CreateAndAddPinchZoomScrollbars triggers a commit. 3) During the following ThreadProxy::BeginFrame, ScrollbarLayer::Update triggers a commit.
On Wed, Apr 17, 2013 at 8:16 PM, <ajuma@chromium.org> wrote: > Turns out we're getting 3 commits: > aha... > 1) The one explicitly triggered by the test. > 2) During the following ThreadProxy::BeginFrame, > LayerTreeHost::**CreateAndAddPinchZoomScrollbar**s triggers a commit. 3) During the following ThreadProxy::BeginFrame, ScrollbarLayer::Update > triggers > a commit. > These both sounds like bugs similar to what tiled layer used to do. (see https://chromiumcodereview.appspot.com/11644036)
PTAL. The commit triggered by LayerTreeHost::CreateAndAddPinchZoomScrollbars is actually needed, since this creates new layers and ::Update won't get called on these layers till the following commit. However, the commit triggered by ScrollbarLayer::Update is unnecessary, since it handles the invalidation immediately. I've fixed this using the same approach that was used for TiledLayer, and added a test. And I've updated the original test to wait for two commits before triggering scrollbar animations. With the fix to ScrollbarLayer::Update, the test now does indeed fail without the changes to LayerTreeImpl, and (of course) passes with the changes. https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:784: layer_tree_host()->SetRootLayer(root_layer_); On 2013/04/17 20:54:09, danakj wrote: > Can you set up the layer tree in SetupTree() instead of BeginTest()? Then you'll > get a viewport of the right size and stuff. Just make sure you call > LayerTreeHostAnimationTest::SetupTree(); after you set the root layer so it can > set up the rest. > > The page scale stuff can stay here. Done. https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:785: layer_tree_host()->SetPageScaleFactorAndLimits(0.5, 0.5, 0.9); On 2013/04/17 20:54:09, danakj wrote: > These are floats. 0.5f etc Done. https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:791: if (host_impl->active_tree()->page_scale_factor() == 1.0) On 2013/04/17 20:54:09, danakj wrote: > 1.f Done. https://codereview.chromium.org/14017008/diff/5002/cc/trees/layer_tree_host_u... cc/trees/layer_tree_host_unittest_animation.cc:794: if (started_times_ == 0) { On 2013/04/17 20:54:09, danakj wrote: > seems like a good spot for a switch? Done.
LGTM with a few nits for the tests. https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:310: EndTest(); Can you not tie this to the number of draws? You might get 2 draws for the first commit. Just stick it in the DidCommit() method like the test above. Looks like you won't need num_draws_ at all then. https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:329: scoped_refptr<ContentLayer> root_layer_; can you use FakeContentLayer instead? it does stuff like set up an AnchorPoint you're expecting but not setting here. https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest_animation.cc:825: scoped_refptr<ContentLayer> root_layer_; Can you use FakeContentLayer here as well? Then you can skip the SetIsDrawable() call also.
(Thanks for fixing up the scrollbar extra commit)
https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:310: EndTest(); On 2013/04/18 18:53:32, danakj wrote: > Can you not tie this to the number of draws? You might get 2 draws for the first > commit. Just stick it in the DidCommit() method like the test above. > > Looks like you won't need num_draws_ at all then. I tried this approach, but this wasn't enough to get a test that failed (for the multi-thread case) without the fix to ScrollbarLayer, since the test would end before the third commit arrived (but if we wait long enough, as the approach of counting draws does, the third commit does arrive and cause the test to fail, as we want).
https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:310: EndTest(); On 2013/04/18 18:53:32, danakj wrote: > Can you not tie this to the number of draws? You might get 2 draws for the first > commit. Just stick it in the DidCommit() method like the test above. > > Looks like you won't need num_draws_ at all then. As discussed offline, changed this to use source_frame_number(), and made the same change to the test above. https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest.cc:329: scoped_refptr<ContentLayer> root_layer_; On 2013/04/18 18:53:32, danakj wrote: > can you use FakeContentLayer instead? it does stuff like set up an AnchorPoint > you're expecting but not setting here. Done. https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/14017008/diff/20001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_unittest_animation.cc:825: scoped_refptr<ContentLayer> root_layer_; On 2013/04/18 18:53:32, danakj wrote: > Can you use FakeContentLayer here as well? Then you can skip the SetIsDrawable() > call also. Done.
Thanks :) lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/14017008/27001
Message was sent while issue was closed.
Change committed as 195068 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
