Move LayerImpl's bounds_delta to property trees

cc/trees/layer_tree_impl.cc

Index: cc/trees/layer_tree_impl.cc
diff --git a/cc/trees/layer_tree_impl.cc b/cc/trees/layer_tree_impl.cc
index 2f8b95e0d9385ca8447427a28d507b4d5576681d..de155989338c82b1294da3e412a8ca02dd716819 100644
--- a/cc/trees/layer_tree_impl.cc
+++ b/cc/trees/layer_tree_impl.cc
@@ -262,7 +262,7 @@ void LayerTreeImpl::UpdateScrollbars(int scroll_layer_id, int clip_layer_id) {
           scrollbar->SetScrollLayerLength(scroll_size.height());
     }
     scrollbar_needs_animation |=
-        scrollbar->SetVerticalAdjust(clip_layer->bounds_delta().y());
+        scrollbar->SetVerticalAdjust(clip_layer->ViewportBoundsDelta().y());
   }
 
   scrollbar_needs_animation |=
@@ -386,21 +386,6 @@ std::unique_ptr<OwnedLayerImplList> LayerTreeImpl::DetachLayers() {
   return ret;
 }
 
-static void UpdateClipTreeForBoundsDeltaOnLayer(LayerImpl* layer,
-                                                ClipTree* clip_tree) {
-  if (layer && layer->masks_to_bounds()) {
-    ClipNode* clip_node = clip_tree->Node(layer->clip_tree_index());
-    if (clip_node) {
-      DCHECK_EQ(layer->id(), clip_node->owning_layer_id);
-      gfx::SizeF bounds = gfx::SizeF(layer->bounds());
-      if (clip_node->clip.size() != bounds) {
-        clip_node->clip.set_size(bounds);
-        clip_tree->set_needs_update(true);
-      }
-    }
-  }
-}
-
 void LayerTreeImpl::SetPropertyTrees(PropertyTrees* property_trees) {
   std::vector<std::unique_ptr<RenderSurfaceImpl>> old_render_surfaces;
   property_trees_.effect_tree.TakeRenderSurfaces(&old_render_surfaces);
@@ -421,30 +406,6 @@ void LayerTreeImpl::SetPropertyTrees(PropertyTrees* property_trees) {
     property_trees_.effect_tree.set_needs_update(true);
 }
 
-void LayerTreeImpl::UpdatePropertyTreesForBoundsDelta() {
-  DCHECK(IsActiveTree());
-  LayerImpl* inner_container = InnerViewportContainerLayer();
-  LayerImpl* outer_container = OuterViewportContainerLayer();
-  LayerImpl* inner_scroll = InnerViewportScrollLayer();
-
-  UpdateClipTreeForBoundsDeltaOnLayer(inner_container,
-                                      &property_trees_.clip_tree);
-  UpdateClipTreeForBoundsDeltaOnLayer(InnerViewportScrollLayer(),
-                                      &property_trees_.clip_tree);
-  UpdateClipTreeForBoundsDeltaOnLayer(outer_container,
-                                      &property_trees_.clip_tree);
-
-  if (inner_container)
-    property_trees_.SetInnerViewportContainerBoundsDelta(
-        inner_container->bounds_delta());
-  if (outer_container)
-    property_trees_.SetOuterViewportContainerBoundsDelta(
-        outer_container->bounds_delta());
-  if (inner_scroll)
-    property_trees_.SetInnerViewportScrollBoundsDelta(
-        inner_scroll->bounds_delta());
-}
-
 void LayerTreeImpl::PushPropertiesTo(LayerTreeImpl* target_tree) {
   // The request queue should have been processed and does not require a push.
   DCHECK_EQ(ui_resource_request_queue_.size(), 0u);
@@ -467,11 +428,6 @@ void LayerTreeImpl::PushPropertiesTo(LayerTreeImpl* target_tree) {
   target_tree->property_trees()->scroll_tree.PushScrollUpdatesFromPendingTree(
       &property_trees_, target_tree);
 
-  // This needs to be called early so that we don't clamp with incorrect max
-  // offsets when UpdateViewportContainerSizes is called from e.g.
-  // PushBrowserControls
-  target_tree->UpdatePropertyTreesForBoundsDelta();

[ajuma, 2017/04/24 17:37:09]

Just to make sure, the newly-pushed property trees now get their bounds delta
values set whenever UpdateViewportContainerSizes is called next?

[pdr., 2017/04/24 20:06:49]

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.

[ajuma, 2017/04/24 21:06:20]

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.

[pdr., 2017/04/25 04:01:55]

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?

[bokan, 2017/04/25 19:01:46]

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?

-
   if (next_activation_forces_redraw_) {
     target_tree->ForceRedrawNextActivation();
     next_activation_forces_redraw_ = false;