Revert "Skip commits for painted scrollbar thumb movement"

This reverts commit 3088046f.

Reason for revert: Suspected as causing a ProxyCommit regression.

Original change's description:
> 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/+/2078847
> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> Commit-Queue: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#745544}

TBR=wangxianzhu@chromium.org,pdr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1048384f, 1059436
Change-Id: I7e6e36fca7afecae35d969ca655965984a90e891
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2092185Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747976}

cc/layers/painted_scrollbar_layer.cc

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

cc/layers/scrollbar_layer_unittest.cc

@@ -227,66 +227,6 @@ 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() {
@@ -1323,8 +1263,6 @@ 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());