[Reland] Skip commits for threaded scrolls.

Threaded scrolls unnecessarily cause commits on every change. This
patch implements an optimization from pre-BlinkGenPropertyTrees in
layer list mode to avoid these commits.

When an impl-side scroll occurs, cc can immediately apply the scroll
before notifying blink so that the blink update only causes a commit if
needed (e.g., interest rect changed). This has been implemented in
LayerTreeHost::UpdateScrollOffsetFromImpl. A second change was needed in
PropertyTreeManager::DirectlyUpdateScrollOffsetTransform to avoid
commits if cc already matches blink.

This is a reland of https://crrev.com/745110.

Bug: 1048384f
Change-Id: Id50a0d0a944f7d1f763906304bbcfccdf81cb301
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2297709
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Auto-Submit: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790598}

cc/trees/layer_tree_host.cc

@@ -885,23 +885,11 @@ void LayerTreeHost::ApplyViewportChanges(const ScrollAndScaleSet& info) {
   }
   is_pinch_gesture_active_from_impl_ = info.is_pinch_gesture_active;
 
-  // Preemptively apply the scroll offset and scale delta here before sending
-  // it to the client.  If the client comes back and sets it to the same
-  // value, then the layer can early out without needing a full commit.
   if (auto* inner_scroll = property_trees()->scroll_tree.Node(
           viewport_property_ids_.inner_scroll)) {
-    if (IsUsingLayerLists()) {
-      auto& scroll_tree = property_trees()->scroll_tree;
-      scroll_tree.NotifyDidScroll(
-          inner_scroll->element_id,
-          scroll_tree.current_scroll_offset(inner_scroll->element_id) +
-              inner_viewport_scroll_delta,
-          info.inner_viewport_scroll.snap_target_element_ids);
-    } else if (auto* inner_scroll_layer =
-                   LayerByElementId(inner_scroll->element_id)) {
-      inner_scroll_layer->SetScrollOffsetFromImplSide(
-          inner_scroll_layer->scroll_offset() + inner_viewport_scroll_delta);
-    }
+    UpdateScrollOffsetFromImpl(
+        inner_scroll->element_id, inner_viewport_scroll_delta,
+        info.inner_viewport_scroll.snap_target_element_ids);
   }
 
   ApplyPageScaleDeltaFromImplSide(info.page_scale_delta);
@@ -935,6 +923,53 @@ void LayerTreeHost::SendOverscrollAndScrollEndEventsFromImplSide(
     client_->SendScrollEndEventFromImplSide(info.scroll_latched_element_id);
 }
 
+void LayerTreeHost::UpdateScrollOffsetFromImpl(
+    const ElementId& id,
+    const gfx::ScrollOffset& delta,
+    const base::Optional<TargetSnapAreaElementIds>& snap_target_ids) {
+  if (IsUsingLayerLists()) {
+    auto& scroll_tree = property_trees()->scroll_tree;
+    auto new_offset = scroll_tree.current_scroll_offset(id) + delta;
+    TRACE_EVENT_INSTANT2("cc", "NotifyDidScroll", TRACE_EVENT_SCOPE_THREAD,
+                         "cur_y", scroll_tree.current_scroll_offset(id).y(),
+                         "delta", delta.y());
+    if (auto* scroll_node = scroll_tree.FindNodeFromElementId(id)) {
+      // This update closely follows
+      // blink::PropertyTreeManager::DirectlyUpdateScrollOffsetTransform.
+
+      // Update the offset in the scroll tree.
+      if (scroll_tree.SetScrollOffset(id, new_offset)) {
+        // Scroll offset animations are clobbered via |Layer::PushPropertiesTo|.
+        // TODO(pdr): This follows the pattern of |SetScrollOffsetFromImplSide|
+        // and PropertyTreeManager::DirectlyUpdateScrollOffsetTransform but do
+        // we need to clobber scroll animations as a result of impl scrolls?
+        if (auto* layer = LayerByElementId(id))
+          layer->SetNeedsPushProperties();
+      }
+
+      // Update the offset in the transform node.
+      DCHECK(scroll_node->transform_id != TransformTree::kInvalidNodeId);
+      TransformTree& transform_tree = property_trees()->transform_tree;
+      auto* transform_node = transform_tree.Node(scroll_node->transform_id);
+      if (transform_node->scroll_offset != new_offset) {
+        transform_node->scroll_offset = new_offset;
+        transform_node->needs_local_transform_update = true;
+        transform_node->transform_changed = true;
+        transform_tree.set_needs_update(true);
+      }
+
+      // The transform tree has been modified which requires a call to
+      // |LayerTreeHost::UpdateLayers| to update the property trees.
+      SetNeedsUpdateLayers();
+    }
+
+    scroll_tree.NotifyDidScroll(id, new_offset, snap_target_ids);
+  } else if (Layer* layer = LayerByElementId(id)) {
+    layer->SetScrollOffsetFromImplSide(layer->scroll_offset() + delta);
+    SetNeedsUpdateLayers();
+  }
+}
+
 void LayerTreeHost::ApplyScrollAndScale(ScrollAndScaleSet* info) {
   DCHECK(info);
   TRACE_EVENT0("cc", "LayerTreeHost::ApplyScrollAndScale");
@@ -958,21 +993,8 @@ void LayerTreeHost::ApplyScrollAndScale(ScrollAndScaleSet* info) {
   if (root_layer_) {
     auto& scroll_tree = property_trees()->scroll_tree;
     for (auto& scroll : info->scrolls) {
-      if (IsUsingLayerLists()) {
-        TRACE_EVENT_INSTANT2(
-            "cc", "NotifyDidScroll", TRACE_EVENT_SCOPE_THREAD, "cur_y",
-            scroll_tree.current_scroll_offset(scroll.element_id).y(), "delta",
-            scroll.scroll_delta.y());
-        scroll_tree.NotifyDidScroll(
-            scroll.element_id,
-            scroll_tree.current_scroll_offset(scroll.element_id) +
-                scroll.scroll_delta,
-            scroll.snap_target_element_ids);
-      } else if (Layer* layer = LayerByElementId(scroll.element_id)) {
-        layer->SetScrollOffsetFromImplSide(layer->scroll_offset() +
-                                           scroll.scroll_delta);
-        SetNeedsUpdateLayers();
-      }
+      UpdateScrollOffsetFromImpl(scroll.element_id, scroll.scroll_delta,
+                                 scroll.snap_target_element_ids);
     }
     for (auto& scrollbar : info->scrollbars) {
       scroll_tree.NotifyDidChangeScrollbarsHidden(scrollbar.element_id,

cc/trees/layer_tree_host.h

@@ -775,6 +775,13 @@ class CC_EXPORT LayerTreeHost : public MutatorHostClient {
 
   void UpdateDeferMainFrameUpdateInternal();
 
+  // Preemptively applies the scroll offset and delta before sending it to the
+  // client. This lets the client skip a commit if the value does not change.
+  void UpdateScrollOffsetFromImpl(
+      const ElementId&,
+      const gfx::ScrollOffset& delta,
+      const base::Optional<TargetSnapAreaElementIds>&);
+
   const CompositorMode compositor_mode_;
 
   std::unique_ptr<UIResourceManager> ui_resource_manager_;

cc/trees/layer_tree_host_unittest.cc

@@ -3339,9 +3339,11 @@ class ViewportDeltasAppliedDuringPinch : public LayerTreeHostTest,
         layer_tree_host()->InnerViewportScrollLayerForTesting();
     EXPECT_EQ(scroll_layer->element_id(), last_scrolled_element_id_);
     EXPECT_EQ(gfx::ScrollOffset(50, 50), last_scrolled_offset_);
-    // The scroll offset in scroll tree needs update from blink which doesn't
-    // exist in this test.
-    EXPECT_EQ(gfx::ScrollOffset(), CurrentScrollOffset(scroll_layer));
+    // The scroll offset in the scroll tree is typically updated from blink
+    // which doesn't exist in this test. Because we preemptively apply the
+    // scroll offset in LayerTreeHost::UpdateScrollOffsetFromImpl, the current
+    // scroll offset will still be updated.
+    EXPECT_EQ(gfx::ScrollOffset(50, 50), CurrentScrollOffset(scroll_layer));
     EndTest();
   }
 

cc/trees/property_tree.h

@@ -453,6 +453,7 @@ class CC_EXPORT ScrollTree final : public PropertyTree<ScrollNode> {
 
   void SetBaseScrollOffset(ElementId id,
                            const gfx::ScrollOffset& scroll_offset);
+  // Returns true if the scroll offset is changed.
   bool SetScrollOffset(ElementId id, const gfx::ScrollOffset& scroll_offset);
   void SetScrollOffsetClobberActiveValue(ElementId id) {
     GetOrCreateSyncedScrollOffset(id)->set_clobber_active_value();

cc/trees/single_thread_proxy.cc

@@ -53,6 +53,7 @@ SingleThreadProxy::SingleThreadProxy(LayerTreeHost* layer_tree_host,
       defer_main_frame_update_(false),
       defer_commits_(false),
       animate_requested_(false),
+      update_layers_requested_(false),
       commit_requested_(false),
       inside_synchronous_composite_(false),
       needs_impl_frame_(false),
@@ -171,7 +172,12 @@ void SingleThreadProxy::SetNeedsAnimate() {
 void SingleThreadProxy::SetNeedsUpdateLayers() {
   TRACE_EVENT0("cc", "SingleThreadProxy::SetNeedsUpdateLayers");
   DCHECK(task_runner_provider_->IsMainThread());
-  SetNeedsCommit();
+  if (!RequestedAnimatePending()) {
+    DebugScopedSetImplThread impl(task_runner_provider_);
+    if (scheduler_on_impl_thread_)
+      scheduler_on_impl_thread_->SetNeedsBeginMainFrame();
+  }
+  update_layers_requested_ = true;
 }
 
 void SingleThreadProxy::DoCommit(const viz::BeginFrameArgs& commit_args) {
@@ -256,7 +262,8 @@ void SingleThreadProxy::SetNextCommitWaitsForActivation() {
 }
 
 bool SingleThreadProxy::RequestedAnimatePending() {
-  return animate_requested_ || commit_requested_ || needs_impl_frame_;
+  return animate_requested_ || update_layers_requested_ || commit_requested_ ||
+         needs_impl_frame_;
 }
 
 void SingleThreadProxy::SetDeferMainFrameUpdate(bool defer_main_frame_update) {
@@ -785,6 +792,7 @@ void SingleThreadProxy::BeginMainFrame(
   commit_requested_ = false;
   needs_impl_frame_ = false;
   animate_requested_ = false;
+  update_layers_requested_ = false;
 
   if (defer_main_frame_update_) {
     TRACE_EVENT_INSTANT0("cc", "EarlyOut_DeferBeginMainFrame",
@@ -853,6 +861,7 @@ void SingleThreadProxy::DoBeginMainFrame(
 
 void SingleThreadProxy::DoPainting() {
   layer_tree_host_->UpdateLayers();
+  update_layers_requested_ = false;
 
   // TODO(enne): SingleThreadProxy does not support cancelling commits yet,
   // search for CommitEarlyOutReason::FINISHED_NO_UPDATES inside

cc/trees/single_thread_proxy.h

@@ -200,6 +200,7 @@ class CC_EXPORT SingleThreadProxy : public Proxy,
   bool defer_main_frame_update_;
   bool defer_commits_;
   bool animate_requested_;
+  bool update_layers_requested_;
   bool commit_requested_;
   bool inside_synchronous_composite_;
   bool needs_impl_frame_;

third_party/blink/renderer/core/paint/compositing/compositing_test.cc

@@ -1533,4 +1533,48 @@ TEST_P(CompositingTest, EffectNodesShouldHaveStableIds) {
   }
 }
 
+TEST_P(CompositingSimTest, ImplSideScrollSkipsCommit) {
+  // TODO(crbug.com/1046544): This test fails with CompositeAfterPaint because
+  // PaintArtifactCompositor::Update is run for scroll offset changes. When we
+  // have an early-out to avoid SetNeedsCommit for non-changing interest-rects,
+  // this test will pass.
+  if (RuntimeEnabledFeatures::CompositeAfterPaintEnabled())
+    return;
+
+  InitializeWithHTML(R"HTML(
+    <div id='scroller' style='will-change: transform; overflow: scroll;
+        width: 100px; height: 100px'>
+      <div style='height: 1000px'></div>
+    </div>
+  )HTML");
+  Compositor().BeginFrame();
+
+  auto* scroller = GetDocument().getElementById("scroller");
+  auto* scrollable_area = scroller->GetLayoutBox()->GetScrollableArea();
+  auto element_id = scrollable_area->GetScrollElementId();
+
+  EXPECT_FALSE(Compositor().layer_tree_host()->CommitRequested());
+
+  // Simulate the scroll update with scroll delta from impl-side.
+  cc::ScrollAndScaleSet scroll_and_scale;
+  scroll_and_scale.scrolls.emplace_back(cc::ScrollAndScaleSet::ScrollUpdateInfo(
+      element_id, gfx::ScrollOffset(0, 10), base::nullopt));
+  Compositor().layer_tree_host()->ApplyScrollAndScale(&scroll_and_scale);
+  EXPECT_EQ(FloatPoint(0, 10), scrollable_area->ScrollPosition());
+  EXPECT_EQ(gfx::ScrollOffset(0, 10),
+            GetPropertyTrees()->scroll_tree.current_scroll_offset(element_id));
+
+  // Update just the blink lifecycle because a full frame would clear the bit
+  // for whether a commit was requested.
+  UpdateAllLifecyclePhases();
+
+  // A main frame is needed to call UpdateLayers which updates property trees,
+  // re-calculating cached to/from-screen transforms.
+  EXPECT_TRUE(
+      Compositor().layer_tree_host()->RequestedMainFramePendingForTesting());
+
+  // A full commit is not needed.
+  EXPECT_FALSE(Compositor().layer_tree_host()->CommitRequested());
+}
+
 }  // namespace blink

third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.cc

@@ -148,12 +148,18 @@ bool PropertyTreeManager::DirectlyUpdateScrollOffsetTransform(
 
   DCHECK(!cc_transform->is_currently_animating);
 
-  UpdateCcTransformLocalMatrix(*cc_transform, transform);
+  auto translation = transform.Translation2D();
+  auto scroll_offset =
+      gfx::ScrollOffset(-translation.Width(), -translation.Height());
+
   DirectlySetScrollOffset(host, scroll_node->GetCompositorElementId(),
-                          cc_transform->scroll_offset);
-  cc_transform->transform_changed = true;
-  property_trees->transform_tree.set_needs_update(true);
-  host.SetNeedsCommit();
+                          scroll_offset);
+  if (cc_transform->scroll_offset != scroll_offset) {
+    UpdateCcTransformLocalMatrix(*cc_transform, transform);
+    cc_transform->transform_changed = true;
+    property_trees->transform_tree.set_needs_update(true);
+    host.SetNeedsCommit();
+  }
   return true;
 }
 
@@ -203,13 +209,13 @@ bool PropertyTreeManager::DirectlyUpdatePageScaleTransform(
   return true;
 }
 
-// static
 void PropertyTreeManager::DirectlySetScrollOffset(
     cc::LayerTreeHost& host,
     CompositorElementId element_id,
     const gfx::ScrollOffset& scroll_offset) {
   auto* property_trees = host.property_trees();
   if (property_trees->scroll_tree.SetScrollOffset(element_id, scroll_offset)) {
+    // Scroll offset animations are clobbered via |Layer::PushPropertiesTo|.
     if (auto* layer = host.LayerByElementId(element_id))
       layer->SetNeedsPushProperties();
     host.SetNeedsCommit();

third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.h

@@ -131,6 +131,9 @@ class PropertyTreeManager {
   static bool DirectlyUpdateCompositedOpacityValue(
       cc::LayerTreeHost&,
       const EffectPaintPropertyNode&);
+  // Returns true if the compositor scroll offsets were updated, even if the
+  // values did not change. This function updates both the cc scroll tree scroll
+  // offset and the cc transform node's scroll offset.
   static bool DirectlyUpdateScrollOffsetTransform(
       cc::LayerTreeHost&,
       const TransformPaintPropertyNode&);
@@ -140,6 +143,8 @@ class PropertyTreeManager {
       cc::LayerTreeHost&,
       const TransformPaintPropertyNode&);
 
+  // This function only updates the cc scroll tree scroll offset and does not
+  // update the cc transform node's scroll offset.
   static void DirectlySetScrollOffset(cc::LayerTreeHost&,
                                       CompositorElementId,
                                       const gfx::ScrollOffset&);