Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2840433002:
Move LayerImpl's bounds_delta to property trees (Closed)
DescriptionMove LayerImpl's bounds_delta to property trees
This patch fully moves LayerImpl::bounds_delta_ to PropertyTrees which
frees storage on LayerImpl and unifies logic. PropertyTrees already
tracked these deltas in "*bounds_delta_" members, so they were stored
in two places. With the bounds deltas only on PropertyTrees,
LayerTreeImpl::UpdatePropertyTreesForBoundsDelta can be removed since
it is redundant.
As part of this refactoring, the bounds delta concept has been renamed
"ViewportBoundsDelta" to make it clear that it is only used for special
viewport layers, and to differentiate it from scroll deltas.
BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2840433002
Cr-Commit-Position: refs/heads/master@{#467111}
Committed: https://chromium.googlesource.com/chromium/src/+/23d381fd827c8880b85fe5794627e98af07454cc
Patch Set 1 #
Total comments: 11
Patch Set 2 : NOTREACHED #
Total comments: 1
Patch Set 3 : Document ViewportBoundsDelta better #
Messages
Total messages: 37 (18 generated)
The CQ bit was checked by pdr@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.
pdr@chromium.org changed reviewers: + ajuma@chromium.org, enne@chromium.org, wkorman@chromium.org
https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:545: DCHECK(false); NOTREACHED() https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:473: target_tree->UpdatePropertyTreesForBoundsDelta(); Just to make sure, the newly-pushed property trees now get their bounds delta values set whenever UpdateViewportContainerSizes is called next?
https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:534: if (bounds_delta == ViewportBoundsDelta()) This is premature optimization, but I think this check is redundant. You have to do a series of conditionals to get this value, so might as well just do a series of conditionals and try to set it and then the set function can early out if needed (which they do). https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl_unitte... File cc/layers/layer_impl_unittest.cc (right): https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl_unitte... cc/layers/layer_impl_unittest.cc:141: // Make root the inner viewport scroll layer. This ensures the later call to Should this done in FakeLayerTreeHostImpl?
https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:534: if (bounds_delta == ViewportBoundsDelta()) On 2017/04/24 at 18:05:08, enne wrote: > This is premature optimization, but I think this check is redundant. You have to do a series of conditionals to get this value, so might as well just do a series of conditionals and try to set it and then the set function can early out if needed (which they do). I wanted to preserve the behavior that DidUpdateScrollState is not called unless the value really changes. I agree that this could be optimized further though. https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl.cc#new... cc/layers/layer_impl.cc:545: DCHECK(false); On 2017/04/24 at 17:37:09, ajuma wrote: > NOTREACHED() Done https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl_unitte... File cc/layers/layer_impl_unittest.cc (right): https://codereview.chromium.org/2840433002/diff/1/cc/layers/layer_impl_unitte... cc/layers/layer_impl_unittest.cc:141: // Make root the inner viewport scroll layer. This ensures the later call to On 2017/04/24 at 18:05:08, enne wrote: > Should this done in FakeLayerTreeHostImpl? I don't think so because the layers are setup in this function. FakeLayerTreeHostImpl doesn't really know what the test wants to do with the layers. https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:473: target_tree->UpdatePropertyTreesForBoundsDelta(); On 2017/04/24 at 17:37:09, ajuma wrote: > Just to make sure, the newly-pushed property trees now get their bounds delta values set whenever UpdateViewportContainerSizes is called next? I don't think so. Instead, the newly-pushed property trees are pushed with the correct viewport bounds deltas to begin with in target_tree->SetPropertyTrees(&property_trees_); We no longer need to handle the case when the layer impl's viewport bounds delta is different from the property trees's bounds delta.
The CQ bit was checked by pdr@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...
lgtm then % ajuma.
https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:473: target_tree->UpdatePropertyTreesForBoundsDelta(); On 2017/04/24 20:06:49, pdr. wrote: > On 2017/04/24 at 17:37:09, ajuma wrote: > > Just to make sure, the newly-pushed property trees now get their bounds delta > values set whenever UpdateViewportContainerSizes is called next? > > I don't think so. Instead, the newly-pushed property trees are pushed with the > correct viewport bounds deltas to begin with in > target_tree->SetPropertyTrees(&property_trees_); We no longer need to handle the > case when the layer impl's viewport bounds delta is different from the property > trees's bounds delta. My understanding was that the pending tree never found about about bounds delta values (e.g., the only non-test caller of LayerImpl::SetBoundsDelta is UpdateViewportContainerSizes, and this only touches the active tree), so the newly pushed trees wouldn't have the right bounds delta values to start with.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pdr@chromium.org changed reviewers: + bokan@chromium.org
https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:473: target_tree->UpdatePropertyTreesForBoundsDelta(); On 2017/04/24 at 21:06:20, ajuma wrote: > On 2017/04/24 20:06:49, pdr. wrote: > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > Just to make sure, the newly-pushed property trees now get their bounds delta > > values set whenever UpdateViewportContainerSizes is called next? > > > > I don't think so. Instead, the newly-pushed property trees are pushed with the > > correct viewport bounds deltas to begin with in > > target_tree->SetPropertyTrees(&property_trees_); We no longer need to handle the > > case when the layer impl's viewport bounds delta is different from the property > > trees's bounds delta. > > My understanding was that the pending tree never found about about bounds delta values (e.g., the only non-test caller of LayerImpl::SetBoundsDelta is UpdateViewportContainerSizes, and this only touches the active tree), so the newly pushed trees wouldn't have the right bounds delta values to start with. You are right, here's my (new) understanding: The viewport bounds deltas are only ever set on the active tree. ActivateSyncTree does two things: 1) pushes pending->active which can set an incorrect (uninitialized pending) value for the bounds delta 2) calls UpdateViewportContainerSizes to calculate the correct bounds delta on the active tree It's safe to remove this call because UpdateViewportContainerSizes is called in ActivateSyncTree. +reviewer Bokan, you wrote https://codereview.chromium.org/2149033002 and may have context here. Could you please review this too?
lgtm https://codereview.chromium.org/2840433002/diff/20001/cc/layers/layer_impl.h File cc/layers/layer_impl.h (right): https://codereview.chromium.org/2840433002/diff/20001/cc/layers/layer_impl.h#... cc/layers/layer_impl.h:273: gfx::Vector2dF ViewportBoundsDelta() const; Document what this does in brief? In particular, re: the case where it could return empty value, is that expected, when would it happen lifecycle-wise.
On 2017/04/25 at 18:38:05, wkorman wrote: > lgtm > > https://codereview.chromium.org/2840433002/diff/20001/cc/layers/layer_impl.h > File cc/layers/layer_impl.h (right): > > https://codereview.chromium.org/2840433002/diff/20001/cc/layers/layer_impl.h#... > cc/layers/layer_impl.h:273: gfx::Vector2dF ViewportBoundsDelta() const; > Document what this does in brief? In particular, re: the case where it could return empty value, is that expected, when would it happen lifecycle-wise. Good idea, done. Ali and David, could you take another look as well?
The CQ bit was checked by pdr@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/2840433002/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (left): https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:473: target_tree->UpdatePropertyTreesForBoundsDelta(); On 2017/04/25 04:01:55, pdr. wrote: > On 2017/04/24 at 21:06:20, ajuma wrote: > > On 2017/04/24 20:06:49, pdr. wrote: > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > Just to make sure, the newly-pushed property trees now get their bounds > delta > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > I don't think so. Instead, the newly-pushed property trees are pushed with > the > > > correct viewport bounds deltas to begin with in > > > target_tree->SetPropertyTrees(&property_trees_); We no longer need to handle > the > > > case when the layer impl's viewport bounds delta is different from the > property > > > trees's bounds delta. > > > > My understanding was that the pending tree never found about about bounds > delta values (e.g., the only non-test caller of LayerImpl::SetBoundsDelta is > UpdateViewportContainerSizes, and this only touches the active tree), so the > newly pushed trees wouldn't have the right bounds delta values to start with. > > You are right, here's my (new) understanding: > The viewport bounds deltas are only ever set on the active tree. > ActivateSyncTree does two things: > 1) pushes pending->active which can set an incorrect (uninitialized pending) > value for the bounds delta > 2) calls UpdateViewportContainerSizes to calculate the correct bounds delta on > the active tree > It's safe to remove this call because UpdateViewportContainerSizes is called in > ActivateSyncTree. > > +reviewer Bokan, you wrote https://codereview.chromium.org/2149033002 and may > have context here. Could you please review this too? Yes, I think your understanding is correct but I'm not understanding my original fix now that I look back on it. The same reasoning should have applied back then - the SetBoundsDelta calls in UpdateViewportContainerSizes should have been updating the deltas in the new property tree. I may have gotten the analysis wrong in my commit message - perhaps it wasn't the UpdateViewportContainerSizes call after all. One suspicious thing that jumps out at me is ScrollTree::MaxScrollOffset only accounts for the delta on the inner viewport layer and not the outer. PDR, does the bug in https://crbug.com/627911 reproduce with your patch?
On 2017/04/25 at 19:01:46, bokan wrote: > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc > File cc/trees/layer_tree_impl.cc (left): > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... > cc/trees/layer_tree_impl.cc:473: target_tree->UpdatePropertyTreesForBoundsDelta(); > On 2017/04/25 04:01:55, pdr. wrote: > > On 2017/04/24 at 21:06:20, ajuma wrote: > > > On 2017/04/24 20:06:49, pdr. wrote: > > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > > Just to make sure, the newly-pushed property trees now get their bounds > > delta > > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > > > I don't think so. Instead, the newly-pushed property trees are pushed with > > the > > > > correct viewport bounds deltas to begin with in > > > > target_tree->SetPropertyTrees(&property_trees_); We no longer need to handle > > the > > > > case when the layer impl's viewport bounds delta is different from the > > property > > > > trees's bounds delta. > > > > > > My understanding was that the pending tree never found about about bounds > > delta values (e.g., the only non-test caller of LayerImpl::SetBoundsDelta is > > UpdateViewportContainerSizes, and this only touches the active tree), so the > > newly pushed trees wouldn't have the right bounds delta values to start with. > > > > You are right, here's my (new) understanding: > > The viewport bounds deltas are only ever set on the active tree. > > ActivateSyncTree does two things: > > 1) pushes pending->active which can set an incorrect (uninitialized pending) > > value for the bounds delta > > 2) calls UpdateViewportContainerSizes to calculate the correct bounds delta on > > the active tree > > It's safe to remove this call because UpdateViewportContainerSizes is called in > > ActivateSyncTree. > > > > +reviewer Bokan, you wrote https://codereview.chromium.org/2149033002 and may > > have context here. Could you please review this too? > > Yes, I think your understanding is correct but I'm not understanding my original fix now that I look back on it. The same reasoning should have applied back then - the SetBoundsDelta calls in UpdateViewportContainerSizes should have been updating the deltas in the new property tree. I may have gotten the analysis wrong in my commit message - perhaps it wasn't the UpdateViewportContainerSizes call after all. > > One suspicious thing that jumps out at me is ScrollTree::MaxScrollOffset only accounts for the delta on the inner viewport layer and not the outer. PDR, does the bug in https://crbug.com/627911 reproduce with your patch? Is it possible to test this on desktop?
On 2017/04/25 19:43:03, pdr. wrote: > On 2017/04/25 at 19:01:46, bokan wrote: > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc > > File cc/trees/layer_tree_impl.cc (left): > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... > > cc/trees/layer_tree_impl.cc:473: > target_tree->UpdatePropertyTreesForBoundsDelta(); > > On 2017/04/25 04:01:55, pdr. wrote: > > > On 2017/04/24 at 21:06:20, ajuma wrote: > > > > On 2017/04/24 20:06:49, pdr. wrote: > > > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > > > Just to make sure, the newly-pushed property trees now get their > bounds > > > delta > > > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > > > > > I don't think so. Instead, the newly-pushed property trees are pushed > with > > > the > > > > > correct viewport bounds deltas to begin with in > > > > > target_tree->SetPropertyTrees(&property_trees_); We no longer need to > handle > > > the > > > > > case when the layer impl's viewport bounds delta is different from the > > > property > > > > > trees's bounds delta. > > > > > > > > My understanding was that the pending tree never found about about bounds > > > delta values (e.g., the only non-test caller of LayerImpl::SetBoundsDelta is > > > UpdateViewportContainerSizes, and this only touches the active tree), so the > > > newly pushed trees wouldn't have the right bounds delta values to start > with. > > > > > > You are right, here's my (new) understanding: > > > The viewport bounds deltas are only ever set on the active tree. > > > ActivateSyncTree does two things: > > > 1) pushes pending->active which can set an incorrect (uninitialized pending) > > > value for the bounds delta > > > 2) calls UpdateViewportContainerSizes to calculate the correct bounds delta > on > > > the active tree > > > It's safe to remove this call because UpdateViewportContainerSizes is called > in > > > ActivateSyncTree. > > > > > > +reviewer Bokan, you wrote https://codereview.chromium.org/2149033002 and > may > > > have context here. Could you please review this too? > > > > Yes, I think your understanding is correct but I'm not understanding my > original fix now that I look back on it. The same reasoning should have applied > back then - the SetBoundsDelta calls in UpdateViewportContainerSizes should have > been updating the deltas in the new property tree. I may have gotten the > analysis wrong in my commit message - perhaps it wasn't the > UpdateViewportContainerSizes call after all. > > > > One suspicious thing that jumps out at me is ScrollTree::MaxScrollOffset only > accounts for the delta on the inner viewport layer and not the outer. PDR, does > the bug in https://crbug.com/627911 reproduce with your patch? > > Is it possible to test this on desktop? Nope, it's an Android only bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/25 19:47:35, bokan wrote: > On 2017/04/25 19:43:03, pdr. wrote: > > On 2017/04/25 at 19:01:46, bokan wrote: > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc > > > File cc/trees/layer_tree_impl.cc (left): > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... > > > cc/trees/layer_tree_impl.cc:473: > > target_tree->UpdatePropertyTreesForBoundsDelta(); > > > On 2017/04/25 04:01:55, pdr. wrote: > > > > On 2017/04/24 at 21:06:20, ajuma wrote: > > > > > On 2017/04/24 20:06:49, pdr. wrote: > > > > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > > > > Just to make sure, the newly-pushed property trees now get their > > bounds > > > > delta > > > > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > > > > > > > I don't think so. Instead, the newly-pushed property trees are pushed > > with > > > > the > > > > > > correct viewport bounds deltas to begin with in > > > > > > target_tree->SetPropertyTrees(&property_trees_); We no longer need to > > handle > > > > the > > > > > > case when the layer impl's viewport bounds delta is different from the > > > > property > > > > > > trees's bounds delta. > > > > > > > > > > My understanding was that the pending tree never found about about > bounds > > > > delta values (e.g., the only non-test caller of LayerImpl::SetBoundsDelta > is > > > > UpdateViewportContainerSizes, and this only touches the active tree), so > the > > > > newly pushed trees wouldn't have the right bounds delta values to start > > with. > > > > > > > > You are right, here's my (new) understanding: > > > > The viewport bounds deltas are only ever set on the active tree. > > > > ActivateSyncTree does two things: > > > > 1) pushes pending->active which can set an incorrect (uninitialized > pending) > > > > value for the bounds delta > > > > 2) calls UpdateViewportContainerSizes to calculate the correct bounds > delta > > on > > > > the active tree > > > > It's safe to remove this call because UpdateViewportContainerSizes is > called > > in > > > > ActivateSyncTree. > > > > > > > > +reviewer Bokan, you wrote https://codereview.chromium.org/2149033002 and > > may > > > > have context here. Could you please review this too? > > > > > > Yes, I think your understanding is correct but I'm not understanding my > > original fix now that I look back on it. The same reasoning should have > applied > > back then - the SetBoundsDelta calls in UpdateViewportContainerSizes should > have > > been updating the deltas in the new property tree. I may have gotten the > > analysis wrong in my commit message - perhaps it wasn't the > > UpdateViewportContainerSizes call after all. > > > > > > One suspicious thing that jumps out at me is ScrollTree::MaxScrollOffset > only > > accounts for the delta on the inner viewport layer and not the outer. PDR, > does > > the bug in https://crbug.com/627911 reproduce with your patch? > > > > Is it possible to test this on desktop? > > Nope, it's an Android only bug. I can give it a try later tonight if you don't have an Android build setup. (but you should have an Android build :)
On 2017/04/25 at 20:22:15, bokan wrote: > On 2017/04/25 19:47:35, bokan wrote: > > On 2017/04/25 19:43:03, pdr. wrote: > > > On 2017/04/25 at 19:01:46, bokan wrote: > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc > > > > File cc/trees/layer_tree_impl.cc (left): > > > > > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... > > > > cc/trees/layer_tree_impl.cc:473: > > > target_tree->UpdatePropertyTreesForBoundsDelta(); > > > > On 2017/04/25 04:01:55, pdr. wrote: > > > > > On 2017/04/24 at 21:06:20, ajuma wrote: > > > > > > On 2017/04/24 20:06:49, pdr. wrote: > > > > > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > > > > > Just to make sure, the newly-pushed property trees now get their > > > bounds > > > > > delta > > > > > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > > > > > > > > > I don't think so. Instead, the newly-pushed property trees are pushed > > > with > > > > > the > > > > > > > correct viewport bounds deltas to begin with in > > > > > > > target_tree->SetPropertyTrees(&property_trees_); We no longer need to > > > handle > > > > > the > > > > > > > case when the layer impl's viewport bounds delta is different from the > > > > > property > > > > > > > trees's bounds delta. > > > > > > > > > > > > My understanding was that the pending tree never found about about > > bounds > > > > > delta values (e.g., the only non-test caller of LayerImpl::SetBoundsDelta > > is > > > > > UpdateViewportContainerSizes, and this only touches the active tree), so > > the > > > > > newly pushed trees wouldn't have the right bounds delta values to start > > > with. > > > > > > > > > > You are right, here's my (new) understanding: > > > > > The viewport bounds deltas are only ever set on the active tree. > > > > > ActivateSyncTree does two things: > > > > > 1) pushes pending->active which can set an incorrect (uninitialized > > pending) > > > > > value for the bounds delta > > > > > 2) calls UpdateViewportContainerSizes to calculate the correct bounds > > delta > > > on > > > > > the active tree > > > > > It's safe to remove this call because UpdateViewportContainerSizes is > > called > > > in > > > > > ActivateSyncTree. > > > > > > > > > > +reviewer Bokan, you wrote https://codereview.chromium.org/2149033002 and > > > may > > > > > have context here. Could you please review this too? > > > > > > > > Yes, I think your understanding is correct but I'm not understanding my > > > original fix now that I look back on it. The same reasoning should have > > applied > > > back then - the SetBoundsDelta calls in UpdateViewportContainerSizes should > > have > > > been updating the deltas in the new property tree. I may have gotten the > > > analysis wrong in my commit message - perhaps it wasn't the > > > UpdateViewportContainerSizes call after all. > > > > > > > > One suspicious thing that jumps out at me is ScrollTree::MaxScrollOffset > > only > > > accounts for the delta on the inner viewport layer and not the outer. PDR, > > does > > > the bug in https://crbug.com/627911 reproduce with your patch? > > > > > > Is it possible to test this on desktop? > > > > Nope, it's an Android only bug. > > I can give it a try later tonight if you don't have an Android build setup. (but you should have an Android build :) I just gave it a try. I don't see any scrolling occurring when following the steps on that bug.
On 2017/04/25 20:50:50, pdr. wrote: > On 2017/04/25 at 20:22:15, bokan wrote: > > On 2017/04/25 19:47:35, bokan wrote: > > > On 2017/04/25 19:43:03, pdr. wrote: > > > > On 2017/04/25 at 19:01:46, bokan wrote: > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc > > > > > File cc/trees/layer_tree_impl.cc (left): > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... > > > > > cc/trees/layer_tree_impl.cc:473: > > > > target_tree->UpdatePropertyTreesForBoundsDelta(); > > > > > On 2017/04/25 04:01:55, pdr. wrote: > > > > > > On 2017/04/24 at 21:06:20, ajuma wrote: > > > > > > > On 2017/04/24 20:06:49, pdr. wrote: > > > > > > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > > > > > > Just to make sure, the newly-pushed property trees now get their > > > > bounds > > > > > > delta > > > > > > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > > > > > > > > > > > I don't think so. Instead, the newly-pushed property trees are > pushed > > > > with > > > > > > the > > > > > > > > correct viewport bounds deltas to begin with in > > > > > > > > target_tree->SetPropertyTrees(&property_trees_); We no longer need > to > > > > handle > > > > > > the > > > > > > > > case when the layer impl's viewport bounds delta is different from > the > > > > > > property > > > > > > > > trees's bounds delta. > > > > > > > > > > > > > > My understanding was that the pending tree never found about about > > > bounds > > > > > > delta values (e.g., the only non-test caller of > LayerImpl::SetBoundsDelta > > > is > > > > > > UpdateViewportContainerSizes, and this only touches the active tree), > so > > > the > > > > > > newly pushed trees wouldn't have the right bounds delta values to > start > > > > with. > > > > > > > > > > > > You are right, here's my (new) understanding: > > > > > > The viewport bounds deltas are only ever set on the active tree. > > > > > > ActivateSyncTree does two things: > > > > > > 1) pushes pending->active which can set an incorrect (uninitialized > > > pending) > > > > > > value for the bounds delta > > > > > > 2) calls UpdateViewportContainerSizes to calculate the correct bounds > > > delta > > > > on > > > > > > the active tree > > > > > > It's safe to remove this call because UpdateViewportContainerSizes is > > > called > > > > in > > > > > > ActivateSyncTree. > > > > > > > > > > > > +reviewer Bokan, you wrote https://codereview.chromium.org/2149033002 > and > > > > may > > > > > > have context here. Could you please review this too? > > > > > > > > > > Yes, I think your understanding is correct but I'm not understanding my > > > > original fix now that I look back on it. The same reasoning should have > > > applied > > > > back then - the SetBoundsDelta calls in UpdateViewportContainerSizes > should > > > have > > > > been updating the deltas in the new property tree. I may have gotten the > > > > analysis wrong in my commit message - perhaps it wasn't the > > > > UpdateViewportContainerSizes call after all. > > > > > > > > > > One suspicious thing that jumps out at me is ScrollTree::MaxScrollOffset > > > only > > > > accounts for the delta on the inner viewport layer and not the outer. PDR, > > > does > > > > the bug in https://crbug.com/627911 reproduce with your patch? > > > > > > > > Is it possible to test this on desktop? > > > > > > Nope, it's an Android only bug. > > > > I can give it a try later tonight if you don't have an Android build setup. > (but you should have an Android build :) > > I just gave it a try. I don't see any scrolling occurring when following the > steps on that bug. OK, that is, this change lgtm then. I'll file a bug to follow up for MaxScrollOffset
On 2017/04/25 20:54:08, bokan wrote: > On 2017/04/25 20:50:50, pdr. wrote: > > On 2017/04/25 at 20:22:15, bokan wrote: > > > On 2017/04/25 19:47:35, bokan wrote: > > > > On 2017/04/25 19:43:03, pdr. wrote: > > > > > On 2017/04/25 at 19:01:46, bokan wrote: > > > > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc > > > > > > File cc/trees/layer_tree_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... > > > > > > cc/trees/layer_tree_impl.cc:473: > > > > > target_tree->UpdatePropertyTreesForBoundsDelta(); > > > > > > On 2017/04/25 04:01:55, pdr. wrote: > > > > > > > On 2017/04/24 at 21:06:20, ajuma wrote: > > > > > > > > On 2017/04/24 20:06:49, pdr. wrote: > > > > > > > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > > > > > > > Just to make sure, the newly-pushed property trees now get > their > > > > > bounds > > > > > > > delta > > > > > > > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > > > > > > > > > > > > > I don't think so. Instead, the newly-pushed property trees are > > pushed > > > > > with > > > > > > > the > > > > > > > > > correct viewport bounds deltas to begin with in > > > > > > > > > target_tree->SetPropertyTrees(&property_trees_); We no longer > need > > to > > > > > handle > > > > > > > the > > > > > > > > > case when the layer impl's viewport bounds delta is different > from > > the > > > > > > > property > > > > > > > > > trees's bounds delta. > > > > > > > > > > > > > > > > My understanding was that the pending tree never found about about > > > > bounds > > > > > > > delta values (e.g., the only non-test caller of > > LayerImpl::SetBoundsDelta > > > > is > > > > > > > UpdateViewportContainerSizes, and this only touches the active > tree), > > so > > > > the > > > > > > > newly pushed trees wouldn't have the right bounds delta values to > > start > > > > > with. > > > > > > > > > > > > > > You are right, here's my (new) understanding: > > > > > > > The viewport bounds deltas are only ever set on the active tree. > > > > > > > ActivateSyncTree does two things: > > > > > > > 1) pushes pending->active which can set an incorrect (uninitialized > > > > pending) > > > > > > > value for the bounds delta > > > > > > > 2) calls UpdateViewportContainerSizes to calculate the correct > bounds > > > > delta > > > > > on > > > > > > > the active tree > > > > > > > It's safe to remove this call because UpdateViewportContainerSizes > is > > > > called > > > > > in > > > > > > > ActivateSyncTree. > > > > > > > > > > > > > > +reviewer Bokan, you wrote > https://codereview.chromium.org/2149033002 > > and > > > > > may > > > > > > > have context here. Could you please review this too? > > > > > > > > > > > > Yes, I think your understanding is correct but I'm not understanding > my > > > > > original fix now that I look back on it. The same reasoning should have > > > > applied > > > > > back then - the SetBoundsDelta calls in UpdateViewportContainerSizes > > should > > > > have > > > > > been updating the deltas in the new property tree. I may have gotten the > > > > > analysis wrong in my commit message - perhaps it wasn't the > > > > > UpdateViewportContainerSizes call after all. > > > > > > > > > > > > One suspicious thing that jumps out at me is > ScrollTree::MaxScrollOffset > > > > only > > > > > accounts for the delta on the inner viewport layer and not the outer. > PDR, > > > > does > > > > > the bug in https://crbug.com/627911 reproduce with your patch? > > > > > > > > > > Is it possible to test this on desktop? > > > > > > > > Nope, it's an Android only bug. > > > > > > I can give it a try later tonight if you don't have an Android build setup. > > (but you should have an Android build :) > > > > I just gave it a try. I don't see any scrolling occurring when following the > > steps on that bug. > > OK, that is, this change lgtm then. I'll file a bug to follow up for > MaxScrollOffset lgtm too
On 2017/04/25 at 20:54:53, ajuma wrote: > On 2017/04/25 20:54:08, bokan wrote: > > On 2017/04/25 20:50:50, pdr. wrote: > > > On 2017/04/25 at 20:22:15, bokan wrote: > > > > On 2017/04/25 19:47:35, bokan wrote: > > > > > On 2017/04/25 19:43:03, pdr. wrote: > > > > > > On 2017/04/25 at 19:01:46, bokan wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc > > > > > > > File cc/trees/layer_tree_impl.cc (left): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2840433002/diff/1/cc/trees/layer_tree_impl.cc... > > > > > > > cc/trees/layer_tree_impl.cc:473: > > > > > > target_tree->UpdatePropertyTreesForBoundsDelta(); > > > > > > > On 2017/04/25 04:01:55, pdr. wrote: > > > > > > > > On 2017/04/24 at 21:06:20, ajuma wrote: > > > > > > > > > On 2017/04/24 20:06:49, pdr. wrote: > > > > > > > > > > On 2017/04/24 at 17:37:09, ajuma wrote: > > > > > > > > > > > Just to make sure, the newly-pushed property trees now get > > their > > > > > > bounds > > > > > > > > delta > > > > > > > > > > values set whenever UpdateViewportContainerSizes is called next? > > > > > > > > > > > > > > > > > > > > I don't think so. Instead, the newly-pushed property trees are > > > pushed > > > > > > with > > > > > > > > the > > > > > > > > > > correct viewport bounds deltas to begin with in > > > > > > > > > > target_tree->SetPropertyTrees(&property_trees_); We no longer > > need > > > to > > > > > > handle > > > > > > > > the > > > > > > > > > > case when the layer impl's viewport bounds delta is different > > from > > > the > > > > > > > > property > > > > > > > > > > trees's bounds delta. > > > > > > > > > > > > > > > > > > My understanding was that the pending tree never found about about > > > > > bounds > > > > > > > > delta values (e.g., the only non-test caller of > > > LayerImpl::SetBoundsDelta > > > > > is > > > > > > > > UpdateViewportContainerSizes, and this only touches the active > > tree), > > > so > > > > > the > > > > > > > > newly pushed trees wouldn't have the right bounds delta values to > > > start > > > > > > with. > > > > > > > > > > > > > > > > You are right, here's my (new) understanding: > > > > > > > > The viewport bounds deltas are only ever set on the active tree. > > > > > > > > ActivateSyncTree does two things: > > > > > > > > 1) pushes pending->active which can set an incorrect (uninitialized > > > > > pending) > > > > > > > > value for the bounds delta > > > > > > > > 2) calls UpdateViewportContainerSizes to calculate the correct > > bounds > > > > > delta > > > > > > on > > > > > > > > the active tree > > > > > > > > It's safe to remove this call because UpdateViewportContainerSizes > > is > > > > > called > > > > > > in > > > > > > > > ActivateSyncTree. > > > > > > > > > > > > > > > > +reviewer Bokan, you wrote > > https://codereview.chromium.org/2149033002 > > > and > > > > > > may > > > > > > > > have context here. Could you please review this too? > > > > > > > > > > > > > > Yes, I think your understanding is correct but I'm not understanding > > my > > > > > > original fix now that I look back on it. The same reasoning should have > > > > > applied > > > > > > back then - the SetBoundsDelta calls in UpdateViewportContainerSizes > > > should > > > > > have > > > > > > been updating the deltas in the new property tree. I may have gotten the > > > > > > analysis wrong in my commit message - perhaps it wasn't the > > > > > > UpdateViewportContainerSizes call after all. > > > > > > > > > > > > > > One suspicious thing that jumps out at me is > > ScrollTree::MaxScrollOffset > > > > > only > > > > > > accounts for the delta on the inner viewport layer and not the outer. > > PDR, > > > > > does > > > > > > the bug in https://crbug.com/627911 reproduce with your patch? > > > > > > > > > > > > Is it possible to test this on desktop? > > > > > > > > > > Nope, it's an Android only bug. > > > > > > > > I can give it a try later tonight if you don't have an Android build setup. > > > (but you should have an Android build :) > > > > > > I just gave it a try. I don't see any scrolling occurring when following the > > > steps on that bug. > > > > OK, that is, this change lgtm then. I'll file a bug to follow up for > > MaxScrollOffset > > lgtm too Thanks everyone! This was a hairy change and I learned a lot thanks to your reviews. Please do be on the lookout for issues caused by this.
The CQ bit was checked by pdr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2840433002/#ps40001 (title: "Document ViewportBoundsDelta better")
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": 40001, "attempt_start_ts": 1493153779352450,
"parent_rev": "be89a27c0b24eec0e9ef42c5131061d28cb61c2f", "commit_rev":
"23d381fd827c8880b85fe5794627e98af07454cc"}
Message was sent while issue was closed.
Description was changed from ========== Move LayerImpl's bounds_delta to property trees This patch fully moves LayerImpl::bounds_delta_ to PropertyTrees which frees storage on LayerImpl and unifies logic. PropertyTrees already tracked these deltas in "*bounds_delta_" members, so they were stored in two places. With the bounds deltas only on PropertyTrees, LayerTreeImpl::UpdatePropertyTreesForBoundsDelta can be removed since it is redundant. As part of this refactoring, the bounds delta concept has been renamed "ViewportBoundsDelta" to make it clear that it is only used for special viewport layers, and to differentiate it from scroll deltas. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move LayerImpl's bounds_delta to property trees This patch fully moves LayerImpl::bounds_delta_ to PropertyTrees which frees storage on LayerImpl and unifies logic. PropertyTrees already tracked these deltas in "*bounds_delta_" members, so they were stored in two places. With the bounds deltas only on PropertyTrees, LayerTreeImpl::UpdatePropertyTreesForBoundsDelta can be removed since it is redundant. As part of this refactoring, the bounds delta concept has been renamed "ViewportBoundsDelta" to make it clear that it is only used for special viewport layers, and to differentiate it from scroll deltas. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2840433002 Cr-Commit-Position: refs/heads/master@{#467111} Committed: https://chromium.googlesource.com/chromium/src/+/23d381fd827c8880b85fe5794627... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/23d381fd827c8880b85fe5794627... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
