Skip commits for painted scrollbar thumb movement

Painted scrollbars are used for overlay scrollbars on MacOS. Thumb
movement would cause a commit on every scroll change because the update
rect (set via Layer::SetNeedsDisplay) would change. This patch ignores
the update rect in PaintedScrollbarLayer::Update and uses part
invalidation for causing commits. With this change, scroll offset
changes do not cause commits.

Bug: 1048384f
Change-Id: I79bac7fbd9756cb71af7c3b83be3df824dd7a5cf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078847Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745544}

cc/layers/painted_scrollbar_layer.cc

@@ -96,7 +96,7 @@ gfx::Size PaintedScrollbarLayer::LayerSizeToContentSize(
   return content_size;
 }
 
-void PaintedScrollbarLayer::UpdateThumbAndTrackGeometry() {
+bool PaintedScrollbarLayer::UpdateThumbAndTrackGeometry() {
   // These properties should never change.
   DCHECK_EQ(supports_drag_snap_back_, scrollbar_->SupportsDragSnapBack());
   DCHECK_EQ(is_left_side_vertical_scrollbar(),
@@ -104,20 +104,23 @@ void PaintedScrollbarLayer::UpdateThumbAndTrackGeometry() {
   DCHECK_EQ(is_overlay_, scrollbar_->IsOverlay());
   DCHECK_EQ(orientation(), scrollbar_->Orientation());
 
-  UpdateProperty(scrollbar_->TrackRect(), &track_rect_);
-  UpdateProperty(scrollbar_->BackButtonRect(), &back_button_rect_);
-  UpdateProperty(scrollbar_->ForwardButtonRect(), &forward_button_rect_);
-  UpdateProperty(scrollbar_->HasThumb(), &has_thumb_);
+  bool updated = false;
+  updated |= UpdateProperty(scrollbar_->TrackRect(), &track_rect_);
+  updated |= UpdateProperty(scrollbar_->BackButtonRect(), &back_button_rect_);
+  updated |=
+      UpdateProperty(scrollbar_->ForwardButtonRect(), &forward_button_rect_);
+  updated |= UpdateProperty(scrollbar_->HasThumb(), &has_thumb_);
   if (has_thumb_) {
     // Ignore ThumbRect's location because the PaintedScrollbarLayerImpl will
     // compute it from scroll offset.
-    UpdateProperty(scrollbar_->ThumbRect().size(), &thumb_size_);
+    updated |= UpdateProperty(scrollbar_->ThumbRect().size(), &thumb_size_);
   } else {
-    UpdateProperty(gfx::Size(), &thumb_size_);
+    updated |= UpdateProperty(gfx::Size(), &thumb_size_);
   }
+  return updated;
 }
 
-void PaintedScrollbarLayer::UpdateInternalContentScale() {
+bool PaintedScrollbarLayer::UpdateInternalContentScale() {
   gfx::Transform transform;
   transform = draw_property_utils::ScreenSpaceTransform(
       this, layer_tree_host()->property_trees()->transform_tree);
@@ -126,31 +129,24 @@ void PaintedScrollbarLayer::UpdateInternalContentScale() {
       transform, layer_tree_host()->device_scale_factor());
   float scale = std::max(transform_scales.x(), transform_scales.y());
 
-  bool changed = false;
-  changed |= UpdateProperty(scale, &internal_contents_scale_);
-  changed |=
+  bool updated = false;
+  updated |= UpdateProperty(scale, &internal_contents_scale_);
+  updated |=
       UpdateProperty(gfx::ScaleToCeiledSize(bounds(), internal_contents_scale_),
                      &internal_content_bounds_);
-  if (changed) {
-    // If the content scale or bounds change, repaint.
-    SetNeedsDisplay();
-  }
+  return updated;
 }
 
 bool PaintedScrollbarLayer::Update() {
-  {
-    auto ignore_set_needs_commit = IgnoreSetNeedsCommit();
-    ScrollbarLayerBase::Update();
-    UpdateInternalContentScale();
-  }
+  bool updated = false;
 
-  UpdateThumbAndTrackGeometry();
+  updated |= ScrollbarLayerBase::Update();
+  updated |= UpdateInternalContentScale();
+  updated |= UpdateThumbAndTrackGeometry();
 
   gfx::Size size = bounds();
   gfx::Size scaled_size = internal_content_bounds_;
 
-  bool updated = false;
-
   if (scaled_size.IsEmpty()) {
     if (track_resource_) {
       track_resource_ = nullptr;
@@ -167,14 +163,13 @@ bool PaintedScrollbarLayer::Update() {
     updated = true;
   }
 
-  if (update_rect().IsEmpty() && track_resource_)
-    return updated;
-
   if (!track_resource_ ||
       scrollbar_->NeedsRepaintPart(TRACK_BUTTONS_TICKMARKS)) {
     track_resource_ = ScopedUIResource::Create(
         layer_tree_host()->GetUIResourceManager(),
         RasterizeScrollbarPart(size, scaled_size, TRACK_BUTTONS_TICKMARKS));
+    SetNeedsPushProperties();
+    updated = true;
   }
 
   gfx::Size scaled_thumb_size = LayerSizeToContentSize(thumb_size_);
@@ -184,13 +179,12 @@ bool PaintedScrollbarLayer::Update() {
       thumb_resource_ = ScopedUIResource::Create(
           layer_tree_host()->GetUIResourceManager(),
           RasterizeScrollbarPart(thumb_size_, scaled_thumb_size, THUMB));
+      SetNeedsPushProperties();
+      updated = true;
     }
-    thumb_opacity_ = scrollbar_->ThumbOpacity();
+    updated |= UpdateProperty(scrollbar_->ThumbOpacity(), &thumb_opacity_);
   }
 
-  // UI resources changed so push properties is needed.
-  SetNeedsPushProperties();
-  updated = true;
   return updated;
 }
 

cc/layers/painted_scrollbar_layer.h

@@ -45,8 +45,8 @@ class CC_EXPORT PaintedScrollbarLayer : public ScrollbarLayerBase {
   UIResourceId thumb_resource_id() {
     return thumb_resource_.get() ? thumb_resource_->id() : 0;
   }
-  void UpdateInternalContentScale();
-  void UpdateThumbAndTrackGeometry();
+  bool UpdateInternalContentScale();
+  bool UpdateThumbAndTrackGeometry();
 
  private:
   gfx::Size LayerSizeToContentSize(const gfx::Size& layer_size) const;

cc/layers/scrollbar_layer_unittest.cc

@@ -227,6 +227,66 @@ TEST_F(ScrollbarLayerTest, RepaintOverlayWhenResourceDisposed) {
   }
 }
 
+TEST_F(ScrollbarLayerTest, SetNeedsDisplayDoesNotRequireUpdate) {
+  scoped_refptr<Layer> layer_tree_root = Layer::Create();
+  scoped_refptr<Layer> content_layer = Layer::Create();
+  scoped_refptr<FakePaintedScrollbarLayer> scrollbar_layer =
+      FakePaintedScrollbarLayer::Create(true, true,
+                                        layer_tree_root->element_id());
+
+  // Setup.
+  {
+    layer_tree_root->AddChild(content_layer);
+    layer_tree_root->AddChild(scrollbar_layer);
+    layer_tree_host_->SetRootLayer(layer_tree_root);
+    scrollbar_layer->SetIsDrawable(true);
+    scrollbar_layer->SetBounds(gfx::Size(100, 100));
+    layer_tree_root->SetBounds(gfx::Size(100, 200));
+    content_layer->SetBounds(gfx::Size(100, 200));
+  }
+
+  layer_tree_host_->UpdateLayers();
+
+  // Simulate commit to compositor thread.
+  scrollbar_layer->PushPropertiesTo(
+      scrollbar_layer->CreateLayerImpl(layer_tree_host_->active_tree()).get());
+  scrollbar_layer->fake_scrollbar()->set_needs_repaint_thumb(false);
+  scrollbar_layer->fake_scrollbar()->set_needs_repaint_track(false);
+
+  EXPECT_FALSE(scrollbar_layer->Update());
+
+  // Opacity changes should cause an update.
+  {
+    scrollbar_layer->fake_scrollbar()->set_thumb_opacity(0.3f);
+    EXPECT_TRUE(scrollbar_layer->Update());
+  }
+
+  // Needing a thumb repaint should cause an update.
+  {
+    scrollbar_layer->fake_scrollbar()->set_needs_repaint_thumb(true);
+    EXPECT_TRUE(scrollbar_layer->Update());
+    scrollbar_layer->fake_scrollbar()->set_needs_repaint_thumb(false);
+    EXPECT_FALSE(scrollbar_layer->Update());
+  }
+
+  // Needing a track repaint should cause an update.
+  {
+    scrollbar_layer->fake_scrollbar()->set_needs_repaint_track(true);
+    EXPECT_TRUE(scrollbar_layer->Update());
+    scrollbar_layer->fake_scrollbar()->set_needs_repaint_track(false);
+    EXPECT_FALSE(scrollbar_layer->Update());
+  }
+
+  // A scroll will cause |SetNeedsDisplay| to be called, but the scrollbar parts
+  // are used for invalidation, rather than the scrollbar layer itself. This
+  // should not cause an update. This is important for performance as an update
+  // will cause a commit on every scroll offset change.
+  {
+    scrollbar_layer->SetNeedsDisplay();
+    EXPECT_FALSE(scrollbar_layer->Update());
+  }
+}
+
 class FakeNinePatchScrollbar : public FakeScrollbar {
  public:
   FakeNinePatchScrollbar() {
@@ -1263,6 +1323,8 @@ TEST_F(ScrollbarLayerTestResourceCreationAndRelease, TestResourceUpdate) {
   // Simulate commit to compositor thread.
   scrollbar_layer->PushPropertiesTo(
       scrollbar_layer->CreateLayerImpl(layer_tree_host_->active_tree()).get());
+  scrollbar_layer->fake_scrollbar()->set_needs_repaint_thumb(false);
+  scrollbar_layer->fake_scrollbar()->set_needs_repaint_track(false);
 
   EXPECT_FALSE(scrollbar_layer->Update());
   EXPECT_NE(0, scrollbar_layer->track_resource_id());