Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps

cc/trees/layer_tree_impl.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/trees/layer_tree_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 144 matching lines @@
     DCHECK(OuterViewportContainerLayer()->viewport_layer_type() ==
            OUTER_VIEWPORT_CONTAINER);
 #endif
   if (auto* layer = LayerById(id))
     return layer->viewport_layer_type() != NOT_VIEWPORT_LAYER;
   return false;
 }
 
 void LayerTreeImpl::DidUpdateScrollOffset(int layer_id) {
   DidUpdateScrollState(layer_id);
+
+  DCHECK(PropertyTreeSyncComplete());
   TransformTree& transform_tree = property_trees()->transform_tree;
   ScrollTree& scroll_tree = property_trees()->scroll_tree;
   int transform_id = TransformTree::kInvalidNodeId;
 
   // If pending tree topology changed and we still want to notify the pending
   // tree about scroll offset in the active tree, we may not find the
   // corresponding pending layer.
   if (LayerById(layer_id)) {
     // TODO(sunxd): when we have a layer_id to property_tree index map in
     // property trees, use the transform_id parameter instead of looking for
@@ skipping 17 matching lines @@
   }
 
   if (IsActiveTree() && layer_tree_host_impl_->pending_tree())
     layer_tree_host_impl_->pending_tree()->DidUpdateScrollOffset(layer_id);
 }
 
 void LayerTreeImpl::DidUpdateScrollState(int layer_id) {
   if (!IsActiveTree())
     return;
 
+  // If we are syncing, the scroll_clip_layer properties should be up-to-date.
+  // TODO(pdr): This DCHECK fails on existing tests but should be enabled.
+  // DCHECK(layer_tree_host_impl_->SyncStateComplete(SYNCED_LAYER_PROPERTIES));

[jaydasika, 2017/05/12 18:19:16]

Something related I just noticed : LayerImpl::SetBounds calls this function and
bounds are set on a layer before the scroll_clip_layer_id is set in
Layer/LayerImpl::PushPropertiesTo =>
not just the scroll_clip_layer properties, even the scroll_clip_layer_id is not
up to date, so we might be fetching the wrong layer here.

[pdr., 2017/05/15 18:30:24]

+1, this bug is all over :(

+
   if (layer_id == Layer::INVALID_ID)
     return;
 
   int scroll_layer_id, clip_layer_id;
   if (IsViewportLayerId(layer_id)) {
     if (!InnerViewportContainerLayer())
       return;
 
     // For scrollbar purposes, a change to any of the four viewport layers
     // should affect the scrollbars tied to the outermost layers, which express
@@ skipping 163 matching lines @@
       property_trees_.effect_tree.CreateOrReuseRenderSurfaces(
           &old_render_surfaces, this);
   if (render_surfaces_changed)
     set_needs_update_draw_properties();
   property_trees->effect_tree.PushCopyRequestsTo(&property_trees_.effect_tree);
   property_trees_.is_main_thread = false;
   property_trees_.is_active = IsActiveTree();
   property_trees_.transform_tree.set_source_to_parent_updates_allowed(false);
 }
 
-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);
-
+void LayerTreeImpl::PushPropertyTreesTo(LayerTreeImpl* target_tree) {
   // To maintain the current scrolling node we need to use element ids which
   // are stable across the property tree update in SetPropertyTrees.
   ElementId scrolling_element_id;
   if (ScrollNode* scrolling_node = target_tree->CurrentlyScrollingNode())
     scrolling_element_id = scrolling_node->element_id;
 
   target_tree->SetPropertyTrees(&property_trees_);
 
   ScrollNode* scrolling_node = nullptr;
   if (scrolling_element_id) {
     auto& scroll_tree = target_tree->property_trees()->scroll_tree;
     scrolling_node = scroll_tree.FindNodeFromElementId(scrolling_element_id);
   }
   target_tree->SetCurrentlyScrollingNode(scrolling_node);

[chrishtr, 2017/05/12 23:13:29]

"just noting" --wkorman

It seems perhaps this field should go on the LayerTreeImpl, so that it
doesn't get clobbered during the copy constructor.

+}
+
+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);
 
   target_tree->property_trees()->scroll_tree.PushScrollUpdatesFromPendingTree(
       &property_trees_, target_tree);
 
   if (next_activation_forces_redraw_) {
     target_tree->ForceRedrawNextActivation();
     next_activation_forces_redraw_ = false;
   }
 
   target_tree->PassSwapPromises(std::move(swap_promise_list_));
@@ skipping 348 matching lines @@
     ++element_id_to_transform;
   }
 
   LayerTreeHostCommon::CallFunctionForEveryLayer(this, [](LayerImpl* layer) {
     layer->UpdatePropertyTreeForScrollingAndAnimationIfNeeded();
   });
 }
 
 void LayerTreeImpl::SetPageScaleOnActiveTree(float active_page_scale) {
   DCHECK(IsActiveTree());
+  DCHECK(PropertyTreeSyncComplete());
   if (page_scale_factor()->SetCurrent(
           ClampPageScaleFactorToLimits(active_page_scale))) {
     DidUpdatePageScale();
     if (PageScaleLayer()) {
       draw_property_utils::UpdatePageScaleFactor(
           property_trees(), PageScaleLayer(), current_page_scale_factor(),
           device_scale_factor(), layer_tree_host_impl_->DrawTransform());
     } else {
       DCHECK(layer_list_.empty() || active_page_scale == 1);
     }
@@ skipping 30 matching lines @@
     // TODO(enne): Pushing from pending to active should never require
     // DidUpdatePageScale.  The values should already be set by the fully
     // computed property trees being synced from one tree to another.  Remove
     // this once CDP goes away.
     changed_page_scale |= page_scale_factor_->PushPendingToActive();
   }
 
   if (changed_page_scale)
     DidUpdatePageScale();
 
+  DCHECK(PropertyTreeSyncComplete());
   if (page_scale_factor) {
     if (PageScaleLayer()) {
       draw_property_utils::UpdatePageScaleFactor(
           property_trees(), PageScaleLayer(), current_page_scale_factor(),
           device_scale_factor(), layer_tree_host_impl_->DrawTransform());
     } else {
       DCHECK(layer_list_.empty() || *page_scale_factor == 1);
     }
   }
 }
@@ skipping 156 matching lines @@
 
   UpdateViewportLayerTypes();
 }
 
 void LayerTreeImpl::ClearViewportLayers() {
   SetViewportLayersFromIds(Layer::INVALID_ID, Layer::INVALID_ID,
                            Layer::INVALID_ID, Layer::INVALID_ID);
 }
 
 void LayerTreeImpl::UpdateViewportLayerTypes() {
+  // If we are syncing, the scroll_clip_layer properties should be up-to-date.
+  // TODO(pdr): This DCHECK fails on existing tests but should be enabled.

[enne (OOO), 2017/05/12 17:25:06]

This is a pre-existing data inconsistency, I guess?

[pdr., 2017/05/15 18:30:24]

This is due to how https://codereview.chromium.org/2867793002 updates the
viewport layer types more than needed (during layer prop sync and host prop
sync) because of the ordering bug that's fixed in this patch. I've gone ahead
and enabled this DCHECK by removing the call to UpdateViewportLayerTypes during
Layer tree property syncing.

[pdr., 2017/05/15 18:56:42]

I've reverted this change and will do it in a followup. Patchset 5 shows the
design and how we can enable this DCHECK.

+  // DCHECK(layer_tree_host_impl_->SyncStateComplete(SYNCED_LAYER_PROPERTIES));
+
   if (auto* inner_scroll = LayerById(inner_viewport_scroll_layer_id_)) {
     inner_scroll->SetViewportLayerType(INNER_VIEWPORT_SCROLL);
     if (auto* inner_container = inner_scroll->scroll_clip_layer())
       inner_container->SetViewportLayerType(INNER_VIEWPORT_CONTAINER);
   }
   if (auto* outer_scroll = LayerById(outer_viewport_scroll_layer_id_)) {
     outer_scroll->SetViewportLayerType(OUTER_VIEWPORT_SCROLL);
     if (auto* outer_container = outer_scroll->scroll_clip_layer())
       outer_container->SetViewportLayerType(OUTER_VIEWPORT_CONTAINER);
   }
@@ skipping 1102 matching lines @@
 
 void LayerTreeImpl::ResetAllChangeTracking() {
   layers_that_should_push_properties_.clear();
   // Iterate over all layers, including masks.
   for (auto& layer : *layers_)
     layer->ResetChangeTracking();
   property_trees_.ResetAllChangeTracking();
 }
 
 }  // namespace cc