Harmonize LayerTreeHost/LayerTreeHostImpl synchronization steps

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 7290a5d122b036a0134f2af27ba0e156759eacca..23fe894d2d109e11e314aa1b69ab26cd3e430230 100644
--- a/cc/trees/layer_tree_impl.cc
+++ b/cc/trees/layer_tree_impl.cc
@@ -162,6 +162,8 @@ bool LayerTreeImpl::IsViewportLayerId(int id) const {
 
 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;
@@ -199,6 +201,10 @@ 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;
 
@@ -382,10 +388,7 @@ void LayerTreeImpl::SetPropertyTrees(PropertyTrees* property_trees) {
   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;
@@ -400,6 +403,11 @@ void LayerTreeImpl::PushPropertiesTo(LayerTreeImpl* target_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);
@@ -768,6 +776,7 @@ void LayerTreeImpl::UpdatePropertyTreeScrollingAndAnimationFromMainThread(
 
 void LayerTreeImpl::SetPageScaleOnActiveTree(float active_page_scale) {
   DCHECK(IsActiveTree());
+  DCHECK(PropertyTreeSyncComplete());
   if (page_scale_factor()->SetCurrent(
           ClampPageScaleFactorToLimits(active_page_scale))) {
     DidUpdatePageScale();
@@ -818,6 +827,7 @@ void LayerTreeImpl::PushPageScaleFactorAndLimits(const float* page_scale_factor,
   if (changed_page_scale)
     DidUpdatePageScale();
 
+  DCHECK(PropertyTreeSyncComplete());
   if (page_scale_factor) {
     if (PageScaleLayer()) {
       draw_property_utils::UpdatePageScaleFactor(
@@ -994,6 +1004,10 @@ void LayerTreeImpl::ClearViewportLayers() {
 }
 
 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())