Revert of Generalize scroll parent work in CalculateDrawProperties (patchset...

Revert of Generalize scroll parent work in CalculateDrawProperties (patchset #6 id:160001 of https://codereview.chromium.org/548963002/)

Reason for revert:
This patch seems to be exposing some bugs. They bugs appear not to be directly related to this patch, but I'm going to revert for now until I can investigate.

Original issue's description:
> Generalize scroll parent work in CalculateDrawProperties
>
> Previously, CDP would fail if the scroll child wasn't the immediate
> descendant of the lowest common ancestor of the scroll parent and child.
>
> With this patch I've fixed up two things so that CDP can handle these
> cases.
>
> 1. Dumber sorting.
>   I've made sorting simpler by removing assumptions about the tree
>   topology. Instead, I assigned "sort weights" to the layers in
>   PreCalculateMetaInformation. The approach is pretty naive. As I
>   walk the tree I assign a monotonically increasing weight. If I ever
>   hit a scroll child before its parent, I jump up to the lowest common
>   ancestor and make sure that the child of the LCA is assigned the
>   smallest weight in the tree so far (the negative of the next
>   monotonically increasing weight). This ensures that the scroll parent
>   will be visited first. Here's a picture of a layer tree and the
>   weights that would be assigned.
>
>   lca(0)
>   + filler(1)
>   | + scroll child(2)
>   + more-filler(-3)
>     + scroll parent(4)
>
>   It would be a huge bummer if we had to sort all the time, though, so
>   I've taken care to flag the LCA if I ever have to assign a negative
>   weight so that we only sort in that case.
>
>   Here's my argument for the correctness of the algorithm. Clearly,
>   we force correct ordering of a particular scroll child/parent pair,
>   but the worry is that by setting a negative weight, we'll force an
>   illegal ordering (i.e., we'll "undo" an ordering we've forced
>   earlier in the algorithm). Here's a diagram.
>
>   lca
>   + subtree with scroll parent #1
>   + subtree with scroll child #1 and scroll_parent #2
>   + subtree with scroll child #2.
>
>   Now, say we're processing scroll child #2. We want to be sure that
>   we don't go reordering subtrees and mess up the relative ordering of
>   scroll child #1 and scroll parent #1's subtrees.
>
>   But we know that this is impossible. Since we've already visited the
>   subtree with scroll child #1, scroll child #2 will necessarily get a
>   higher sort weight, so we'll never need to force any reordering in
>   this case; we won't assign any negative weights here.
>
>   Say the ordering were changed such that when processing scroll child
>   #2, we haven't seen scroll child #1's subtree. We're also fine in
>   this case because we will do any ordering fixups required when we do
>   eventually visit scroll child #1.
>
>   The only case where we get into trouble is when we have a cycle. For
>   example,
>
>   lca
>    + scroll parent 1
>    | + scroll child 2
>    + scroll parent 2
>      + scroll child 1
>
>   No ordering could satisfy a situation with a cycle, but this is
>   an impossible situation as CSS stacking defines a well defined, non
>   circular order.
>
> 2. Proper clip transformations.
>   We sometimes need to put the scroll parent's clip into the space of
>   the scroll child's immediate parent. This needs to go through the
>   LCA. Previously, we'd assumed that the LCA was going to be the scroll
>   child's immediate parent, but we no longer make that assumption.
>
> BUG=403866
>
> Committed: https://crrev.com/7faedca87705b714845842e595bca262c4090fa4
> Cr-Commit-Position: refs/heads/master@{#294521}

TBR=danakj@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=403866

Review URL: https://codereview.chromium.org/563313002

Cr-Commit-Position: refs/heads/master@{#294611}

cc/layers/draw_properties.h

@@ -29,6 +29,8 @@ struct CC_EXPORT DrawProperties {
         num_unclipped_descendants(0),
         layer_or_descendant_has_copy_request(false),
         layer_or_descendant_has_input_handler(false),
+        has_child_with_a_scroll_parent(false),
+        sorted_for_recursion(false),
         index_of_first_descendants_addition(0),
         num_descendants_added(0),
         index_of_first_render_surface_layer_list_addition(0),
@@ -37,12 +39,7 @@ struct CC_EXPORT DrawProperties {
         ideal_contents_scale(0.f),
         maximum_animation_contents_scale(0.f),
         page_scale_factor(0.f),
-        device_scale_factor(0.f),
-        children_need_sorting(false),
-        sequence_number(0),
-        sort_weight(0),
-        sort_weight_sequence_number(0),
-        depth(0) {}
+        device_scale_factor(0.f) {}
 
   // Transforms objects from content space to target surface space, where
   // this layer would be drawn.
@@ -109,6 +106,16 @@ struct CC_EXPORT DrawProperties {
   // If true, the layer or one of its descendants has a wheel or touch handler.
   bool layer_or_descendant_has_input_handler;
 
+  // This is true if the layer has any direct child that has a scroll parent.
+  // This layer will not be the scroll parent in this case. This information
+  // lets us avoid work in CalculateDrawPropertiesInternal -- if none of our
+  // children have scroll parents, we will not need to recur out of order.
+  bool has_child_with_a_scroll_parent;
+
+  // This is true if the order (wrt to its siblings in the tree) in which the
+  // layer will be visited while computing draw properties has been determined.
+  bool sorted_for_recursion;
+
   // If this layer is visited out of order, its contribution to the descendant
   // and render surface layer lists will be put aside in a temporary list.
   // These values will allow for an efficient reordering of these additions.
@@ -140,30 +147,6 @@ struct CC_EXPORT DrawProperties {
 
   // The device scale factor that is applied to the layer.
   float device_scale_factor;
-
-  // This is used to detect cases when a layer needs to sort its children to
-  // ensure that scroll parents are visited before scroll children.
-  bool children_need_sorting;
-
-  // This sequence number is used to determine if we've visited this layer in
-  // PreCalculateMetaInformation.
-  int sequence_number;
-
-  // Determines the order the layer should be processed in
-  // CalculateDrawProperties wrt to its siblings. If you have lower
-  // |sort_weight| you get visited first.
-  int sort_weight;
-
-  // This sequence number is used to determine if a sort weight has already been
-  // assigned for a layer. We may assign a sort weight before we visit a layer,
-  // and it's important that we don't clobber it when we do eventually visit the
-  // layer.
-  int sort_weight_sequence_number;
-
-  // This is the topological depth in the tree. (E.g., root has depth zero).
-  // This is used to make it faster to find the lowest common ancestor of two
-  // layers.
-  int depth;
 };
 
 }  // namespace cc

cc/trees/layer_tree_host.cc

@@ -123,8 +123,7 @@ LayerTreeHost::LayerTreeHost(LayerTreeHostClient* client,
       total_frames_used_for_lcd_text_metrics_(0),
       id_(s_layer_tree_host_sequence_number.GetNext() + 1),
       next_commit_forces_redraw_(false),
-      shared_bitmap_manager_(manager),
-      render_surface_layer_list_id_(0) {
+      shared_bitmap_manager_(manager) {
   if (settings_.accelerated_animation_enabled)
     animation_registrar_ = AnimationRegistrar::Create();
   rendering_stats_instrumentation_->set_record_rendering_stats(
@@ -828,7 +827,10 @@ bool LayerTreeHost::UpdateLayers(Layer* root_layer,
 
     TRACE_EVENT0("cc", "LayerTreeHost::UpdateLayers::CalcDrawProps");
     bool can_render_to_separate_surface = true;
-    ++render_surface_layer_list_id_;
+    // TODO(vmpstr): Passing 0 as the current render surface layer list id means
+    // that we won't be able to detect if a layer is part of |update_list|.
+    // Change this if this information is required.
+    int render_surface_layer_list_id = 0;
     LayerTreeHostCommon::CalcDrawPropsMainInputs inputs(
         root_layer,
         device_viewport_size(),
@@ -841,7 +843,7 @@ bool LayerTreeHost::UpdateLayers(Layer* root_layer,
         can_render_to_separate_surface,
         settings_.layer_transforms_should_scale_layer_contents,
         &update_list,
-        render_surface_layer_list_id_);
+        render_surface_layer_list_id);
     LayerTreeHostCommon::CalculateDrawProperties(&inputs);
 
     if (total_frames_used_for_lcd_text_metrics_ <=

cc/trees/layer_tree_host.h

@@ -456,10 +456,6 @@ class CC_EXPORT LayerTreeHost {
   ScopedPtrVector<SwapPromise> swap_promise_list_;
   std::set<SwapPromiseMonitor*> swap_promise_monitor_;
 
-  // We associate work done in CalculateDrawProperties with a sequence number
-  // to avoid having to do extra walks to clear values.
-  int render_surface_layer_list_id_;
-
   DISALLOW_COPY_AND_ASSIGN(LayerTreeHost);
 };
 

cc/trees/layer_tree_host_common.h

@@ -223,7 +223,7 @@ LayerTreeHostCommon::CalcDrawPropsInputsForTesting<LayerType,
           true,
           false,
           render_surface_layer_list,
-          1) {
+          0) {
   DCHECK(root_layer);
   DCHECK(render_surface_layer_list);
 }
@@ -247,7 +247,7 @@ LayerTreeHostCommon::CalcDrawPropsInputsForTesting<LayerType,
           true,
           false,
           render_surface_layer_list,
-          1) {
+          0) {
   DCHECK(root_layer);
   DCHECK(render_surface_layer_list);
 }