Skip to content

Commit

Permalink
Reland "Correct and efficient handling of SolidColorScrollbarLayer co…
Browse files Browse the repository at this point in the history
…lor change""

This reverts commit 716a958.

Uninitialized cc::FakeScrollbar::solid_color_ is fixed.

Original change's description:
> [Gardener] Revert "Correct and efficient handling of SolidColorScrollbarLayer color change"
>
> This reverts commit 16692da.
>
> Reason for revert: Change is causing the following tests to fail:
> - ScrollbarDisplayItemTest.HorizontalSolidColorScrollbar
> - ScrollbarDisplayItemTest.VerticalSolidColorScrollbar
> Example build:
> https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20ChromiumOS%20MSan%20Tests/39242/overview
>
> Original change's description:
> > Correct and efficient handling of SolidColorScrollbarLayer color change
> >
> > - Now a SolidColorScrollbarLayer can change color without recreating
> >   the scrollbar, which reduces cost.
> > - When color changes, ensure SolidColorScrollbarLayer::SetColor() is
> >   called.
> >
> > Also remove LayerTreeSettings::solid_color_scrollbar_color. It was for
> > the default color of the composited solid color scrollbars, and was
> > duplicate with the default color for non-composited solid color
> > scrollbars. Now let blink::ScrollbarThemeOverlayMobile manage the
> > default color for both overlay and non-overlay scrollbars.
> >
> > Also move the WebView scrollbar hiding logic into
> > SolidColorScrollbarLayer::SetColor(), which make it easy to apply the
> > logic to the root scrollbars only.
> >
> > Bug: 1490799
> > Change-Id: Ifd2118268b8bc6cb6b789045d800582a695c6616
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4968601
> > Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
> > Reviewed-by: Vladimir Levin <vmpstr@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1216214}
>
> Bug: 1490799
> Change-Id: I53b16557775df3826153be848981e5f8336cba14
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4985662
> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Cr-Commit-Position: refs/heads/main@{#1216397}

Bug: 1490799
Change-Id: Ifd383a064d4a00fc15a4603f952040dbad17bd9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4985464
Auto-Submit: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Di Zhang <dizhangg@chromium.org>
Owners-Override: Di Zhang <dizhangg@google.com>
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1216469}
  • Loading branch information
wangxianzhu authored and Chromium LUCI CQ committed Oct 28, 2023
1 parent 9afb4bd commit 328fa55
Show file tree
Hide file tree
Showing 26 changed files with 200 additions and 137 deletions.
4 changes: 3 additions & 1 deletion cc/input/scrollbar.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,15 @@ class Scrollbar : public base::RefCounted<Scrollbar> {
virtual ScrollbarOrientation Orientation() const = 0;
virtual bool IsLeftSideVerticalScrollbar() const = 0;
virtual bool IsSolidColor() const = 0;
// The color for a solid color scrollbar. This is meaningful only
// when IsSolidColor() is true.
virtual SkColor4f GetSolidColor() const = 0;
virtual bool IsOverlay() const = 0;
virtual bool IsFluentOverlayScrollbarMinimalMode() const = 0;
virtual bool HasThumb() const = 0;
virtual bool SupportsDragSnapBack() const = 0;
virtual bool JumpOnTrackClick() const = 0;
virtual bool IsOpaque() const = 0;
virtual absl::optional<SkColor4f> ThumbColor() const = 0;

// The following rects are all relative to the scrollbar's origin.
// The location of ThumbRect reflects scroll offset, but cc will ignore it
Expand Down
6 changes: 3 additions & 3 deletions cc/input/scrollbar_animation_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,10 @@ class ScrollbarAnimationControllerOverlayTest
scroll_layer_ = AddLayer<LayerImpl>();
h_scrollbar_layer_ = AddLayer<SolidColorScrollbarLayerImpl>(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);
v_scrollbar_layer_ = AddLayer<SolidColorScrollbarLayerImpl>(
ScrollbarOrientation::kVertical, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);
SetElementIdsForTesting();

clip_layer_ = root_layer();
Expand Down Expand Up @@ -1503,7 +1503,7 @@ class ScrollbarAnimationControllerAndroidTest
scroll_layer_ = AddLayer<LayerImpl>();
scrollbar_layer_ = AddLayer<SolidColorScrollbarLayerImpl>(
orientation(), kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);
SetElementIdsForTesting();

scroll_layer_->SetBounds(gfx::Size(200, 200));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ class SingleScrollbarAnimationControllerThinningTest

scrollbar_layer_ = AddLayer<SolidColorScrollbarLayerImpl>(
ScrollbarOrientation::kVertical, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);

scrollbar_layer_->SetBounds(gfx::Size(kThumbThickness, kTrackLength));
scrollbar_layer_->SetScrollElementId(scroll_layer->element_id());
Expand Down
37 changes: 17 additions & 20 deletions cc/layers/scrollbar_layer_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ TEST_F(ScrollbarLayerTest, ScrollElementIdPushedAcrossCommit) {
painted_overlay_scrollbar_layer->SetScrollElementId(layer_a->element_id());
scoped_refptr<SolidColorScrollbarLayer> solid_color_scrollbar_layer =
SolidColorScrollbarLayer::Create(ScrollbarOrientation::kVertical, 1, 1,
false, absl::nullopt);
false);
solid_color_scrollbar_layer->SetScrollElementId(layer_a->element_id());

layer_tree_host_->SetRootLayer(layer_tree_root);
Expand Down Expand Up @@ -621,8 +621,7 @@ TEST_F(ScrollbarLayerTest, SolidColorDrawQuads) {
scoped_refptr<Layer> child = Layer::Create();
scoped_refptr<ScrollbarLayerBase> scrollbar_layer =
SolidColorScrollbarLayer::Create(ScrollbarOrientation::kHorizontal,
kThumbThickness, kTrackStart, false,
absl::nullopt);
kThumbThickness, kTrackStart, false);
root->AddChild(child);
root->AddChild(scrollbar_layer);
layer_tree_host_->SetRootLayer(root);
Expand Down Expand Up @@ -691,9 +690,9 @@ TEST_F(ScrollbarLayerTest, LayerDrivenSolidColorDrawQuads) {
scoped_refptr<Layer> child1 = Layer::Create();
const bool kIsLeftSideVerticalScrollbar = false;
scoped_refptr<SolidColorScrollbarLayer> child2 =
SolidColorScrollbarLayer::Create(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
SolidColorScrollbarLayer::Create(ScrollbarOrientation::kHorizontal,
kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar);
child2->SetScrollElementId(scroll_layer->element_id());
scroll_layer->AddChild(child1);
scroll_layer->InsertChild(child2, 1);
Expand Down Expand Up @@ -748,7 +747,7 @@ TEST_F(ScrollbarLayerTest, ScrollbarLayerOpacity) {
const bool kIsLeftSideVerticalScrollbar = false;
scrollbar_layer = SolidColorScrollbarLayer::Create(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);
scrollbar_layer->SetScrollElementId(scroll_layer->element_id());
scrollbar_layer->SetElementId(ElementId(300));
scroll_layer->AddChild(child1);
Expand Down Expand Up @@ -823,9 +822,9 @@ TEST_F(AuraScrollbarLayerTest, ScrollbarLayerPushProperties) {
scoped_refptr<Layer> child1 = Layer::Create();
const bool kIsLeftSideVerticalScrollbar = false;
scoped_refptr<SolidColorScrollbarLayer> scrollbar_layer =
SolidColorScrollbarLayer::Create(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
SolidColorScrollbarLayer::Create(ScrollbarOrientation::kHorizontal,
kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar);
scrollbar_layer->SetScrollElementId(scroll_layer->element_id());
scroll_layer->AddChild(child1);
scroll_layer->InsertChild(scrollbar_layer, 1);
Expand Down Expand Up @@ -870,7 +869,7 @@ TEST_F(ScrollbarLayerTest, SubPixelCanScrollOrientation) {
SolidColorScrollbarLayerImpl* scrollbar_layer =
impl.AddLayer<SolidColorScrollbarLayerImpl>(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);

scrollbar_layer->SetScrollElementId(scroll_layer->element_id());
scroll_layer->SetBounds(gfx::Size(980, 980));
Expand Down Expand Up @@ -910,7 +909,7 @@ TEST_F(ScrollbarLayerTest, LayerChangesAffectingScrollbarGeometries) {
SolidColorScrollbarLayerImpl* scrollbar_layer =
impl.AddLayer<SolidColorScrollbarLayerImpl>(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);
scrollbar_layer->SetScrollElementId(scroll_layer->element_id());
EXPECT_TRUE(impl.host_impl()->active_tree()->ScrollbarGeometriesNeedUpdate());
impl.host_impl()->active_tree()->UpdateScrollbarGeometries();
Expand Down Expand Up @@ -1012,9 +1011,9 @@ TEST_F(AuraScrollbarLayerTest, ScrollbarLayerCreateAfterSetScrollable) {
host_impl->ActivateSyncTree();

scoped_refptr<SolidColorScrollbarLayer> scrollbar_layer =
SolidColorScrollbarLayer::Create(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
SolidColorScrollbarLayer::Create(ScrollbarOrientation::kHorizontal,
kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar);
scrollbar_layer->SetScrollElementId(scroll_layer->element_id());
scroll_layer->InsertChild(scrollbar_layer, 1);

Expand Down Expand Up @@ -1044,12 +1043,10 @@ class ScrollbarLayerSolidColorThumbTest : public testing::Test {

horizontal_scrollbar_layer_ = SolidColorScrollbarLayerImpl::Create(
host_impl_->active_tree(), 1, ScrollbarOrientation::kHorizontal,
kThumbThickness, kTrackStart, kIsLeftSideVerticalScrollbar,
absl::nullopt);
kThumbThickness, kTrackStart, kIsLeftSideVerticalScrollbar);
vertical_scrollbar_layer_ = SolidColorScrollbarLayerImpl::Create(
host_impl_->active_tree(), 2, ScrollbarOrientation::kVertical,
kThumbThickness, kTrackStart, kIsLeftSideVerticalScrollbar,
absl::nullopt);
kThumbThickness, kTrackStart, kIsLeftSideVerticalScrollbar);
}

protected:
Expand Down Expand Up @@ -1141,7 +1138,7 @@ class ScrollbarLayerTestResourceCreationAndRelease : public ScrollbarLayerTest {
const bool kIsLeftSideVerticalScrollbar = false;
scrollbar_layer = SolidColorScrollbarLayer::Create(
ScrollbarOrientation::kHorizontal, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar, absl::nullopt);
kIsLeftSideVerticalScrollbar);
} else {
auto scrollbar = base::MakeRefCounted<FakeScrollbar>();
scrollbar->set_has_thumb(true);
Expand Down
64 changes: 48 additions & 16 deletions cc/layers/solid_color_scrollbar_layer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,16 @@

#include "cc/layers/layer_impl.h"
#include "cc/layers/solid_color_scrollbar_layer_impl.h"
#include "cc/trees/layer_tree_host.h"
#include "cc/trees/layer_tree_settings.h"

namespace cc {

std::unique_ptr<LayerImpl> SolidColorScrollbarLayer::CreateLayerImpl(
LayerTreeImpl* tree_impl) const {
return SolidColorScrollbarLayerImpl::Create(
tree_impl, id(), orientation(), thumb_thickness_, track_start_,
is_left_side_vertical_scrollbar(), thumb_color_);
is_left_side_vertical_scrollbar());
}

scoped_refptr<SolidColorScrollbarLayer> SolidColorScrollbarLayer::CreateOrReuse(
Expand All @@ -29,45 +31,44 @@ scoped_refptr<SolidColorScrollbarLayer> SolidColorScrollbarLayer::CreateOrReuse(
is_horizontal ? thumb_rect.height() : thumb_rect.width();
gfx::Rect track_rect = scrollbar->TrackRect();
int track_start = is_horizontal ? track_rect.x() : track_rect.y();
absl::optional<SkColor4f> thumb_color = scrollbar->ThumbColor();

scoped_refptr<SolidColorScrollbarLayer> result;
if (existing_layer &&
// We don't support change of these fields in a layer.
existing_layer->thumb_thickness() == thumb_thickness &&
existing_layer->track_start() == track_start &&
existing_layer->thumb_color() == thumb_color) {
existing_layer->track_start() == track_start) {
// These fields have been checked in ScrollbarLayerBase::CreateOrReuse().
DCHECK_EQ(scrollbar->Orientation(), existing_layer->orientation());
DCHECK_EQ(scrollbar->IsLeftSideVerticalScrollbar(),
existing_layer->is_left_side_vertical_scrollbar());
return existing_layer;
result = existing_layer;
} else {
result = Create(scrollbar->Orientation(), thumb_thickness, track_start,
scrollbar->IsLeftSideVerticalScrollbar());
}

return Create(scrollbar->Orientation(), thumb_thickness, track_start,
scrollbar->IsLeftSideVerticalScrollbar(), thumb_color);
result->SetColor(scrollbar->GetSolidColor());
return result;
}

scoped_refptr<SolidColorScrollbarLayer> SolidColorScrollbarLayer::Create(
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color) {
return base::WrapRefCounted(new SolidColorScrollbarLayer(
orientation, thumb_thickness, track_start,
is_left_side_vertical_scrollbar, thumb_color));
bool is_left_side_vertical_scrollbar) {
return base::WrapRefCounted(
new SolidColorScrollbarLayer(orientation, thumb_thickness, track_start,
is_left_side_vertical_scrollbar));
}

SolidColorScrollbarLayer::SolidColorScrollbarLayer(
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color)
bool is_left_side_vertical_scrollbar)
: ScrollbarLayerBase(orientation, is_left_side_vertical_scrollbar),
thumb_thickness_(thumb_thickness),
track_start_(track_start),
thumb_color_(thumb_color) {
color_(SkColors::kTransparent) {
Layer::SetOpacity(0.f);
}

Expand All @@ -90,4 +91,35 @@ SolidColorScrollbarLayer::GetScrollbarLayerType() const {
return kSolidColor;
}

void SolidColorScrollbarLayer::PushPropertiesTo(
LayerImpl* layer,
const CommitState& commit_state,
const ThreadUnsafeCommitState& unsafe_state) {
ScrollbarLayerBase::PushPropertiesTo(layer, commit_state, unsafe_state);
static_cast<SolidColorScrollbarLayerImpl*>(layer)->set_color(color());
}

void SolidColorScrollbarLayer::SetLayerTreeHost(LayerTreeHost* host) {
if (host != layer_tree_host()) {
ScrollbarLayerBase::SetLayerTreeHost(host);
SetColor(color());
}
}

void SolidColorScrollbarLayer::SetColor(SkColor4f color) {
if (layer_tree_host() &&
layer_tree_host()->GetSettings().using_synchronous_renderer_compositor) {
// Root frame in Android WebView uses system scrollbars, so make ours
// invisible. TODO(crbug.com/1327047): We should apply this to the root
// scrollbars only, or consider other choices listed in the bug.
color = SkColors::kTransparent;
}

if (color != color_.Read(*this)) {
color_.Write(*this) = color;
ScrollbarLayerBase::SetNeedsDisplayRect(gfx::Rect(bounds()));
SetNeedsCommit();
}
}

} // namespace cc
16 changes: 10 additions & 6 deletions cc/layers/solid_color_scrollbar_layer.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ class CC_EXPORT SolidColorScrollbarLayer : public ScrollbarLayerBase {
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color);
bool is_left_side_vertical_scrollbar);

SolidColorScrollbarLayer(const SolidColorScrollbarLayer&) = delete;
SolidColorScrollbarLayer& operator=(const SolidColorScrollbarLayer&) = delete;
Expand All @@ -38,24 +37,29 @@ class CC_EXPORT SolidColorScrollbarLayer : public ScrollbarLayerBase {
bool OpacityCanAnimateOnImplThread() const override;
void SetOpacity(float opacity) override;
void SetNeedsDisplayRect(const gfx::Rect& rect) override;
void SetLayerTreeHost(LayerTreeHost* host) override;
void PushPropertiesTo(LayerImpl* layer,
const CommitState& commit_state,
const ThreadUnsafeCommitState& unsafe_state) override;

int thumb_thickness() const { return thumb_thickness_; }
int track_start() const { return track_start_; }
absl::optional<SkColor4f> thumb_color() const { return thumb_color_; }

void SetColor(SkColor4f color);
SkColor4f color() const { return color_.Read(*this); }

ScrollbarLayerType GetScrollbarLayerType() const override;

private:
SolidColorScrollbarLayer(ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color);
bool is_left_side_vertical_scrollbar);
~SolidColorScrollbarLayer() override;

int thumb_thickness_;
int track_start_;
absl::optional<SkColor4f> thumb_color_;
ProtectedSequenceReadable<SkColor4f> color_;
};

} // namespace cc
Expand Down
21 changes: 8 additions & 13 deletions cc/layers/solid_color_scrollbar_layer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
#include <memory>

#include "base/memory/ptr_util.h"
#include "cc/trees/layer_tree_impl.h"
#include "cc/trees/layer_tree_settings.h"
#include "cc/trees/occlusion.h"
#include "components/viz/common/quads/solid_color_draw_quad.h"

Expand All @@ -21,11 +19,10 @@ SolidColorScrollbarLayerImpl::Create(LayerTreeImpl* tree_impl,
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color) {
bool is_left_side_vertical_scrollbar) {
return base::WrapUnique(new SolidColorScrollbarLayerImpl(
tree_impl, id, orientation, thumb_thickness, track_start,
is_left_side_vertical_scrollbar, thumb_color));
is_left_side_vertical_scrollbar));
}

SolidColorScrollbarLayerImpl::~SolidColorScrollbarLayerImpl() = default;
Expand All @@ -34,7 +31,7 @@ std::unique_ptr<LayerImpl> SolidColorScrollbarLayerImpl::CreateLayerImpl(
LayerTreeImpl* tree_impl) const {
return SolidColorScrollbarLayerImpl::Create(
tree_impl, id(), orientation(), thumb_thickness_, track_start_,
is_left_side_vertical_scrollbar(), thumb_color_);
is_left_side_vertical_scrollbar());
}

SolidColorScrollbarLayerImpl::SolidColorScrollbarLayerImpl(
Expand All @@ -43,21 +40,19 @@ SolidColorScrollbarLayerImpl::SolidColorScrollbarLayerImpl(
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color)
bool is_left_side_vertical_scrollbar)
: ScrollbarLayerImplBase(tree_impl,
id,
orientation,
is_left_side_vertical_scrollbar,
/*is_overlay*/ true),
thumb_thickness_(thumb_thickness),
track_start_(track_start),
thumb_color_(thumb_color),
default_color_(tree_impl->settings().solid_color_scrollbar_color) {}
track_start_(track_start) {}

void SolidColorScrollbarLayerImpl::PushPropertiesTo(LayerImpl* layer) {
ScrollbarLayerImplBase::PushPropertiesTo(layer);
CHECK(!layer->HitTestable());
static_cast<SolidColorScrollbarLayerImpl*>(layer)->set_color(color_);
}

int SolidColorScrollbarLayerImpl::ThumbThickness() const {
Expand Down Expand Up @@ -111,8 +106,8 @@ void SolidColorScrollbarLayerImpl::AppendQuads(
return;

auto* quad = render_pass->CreateAndAppendDrawQuad<viz::SolidColorDrawQuad>();
quad->SetNew(shared_quad_state, thumb_quad_rect, visible_quad_rect,
thumb_color_.value_or(default_color_), false);
quad->SetNew(shared_quad_state, thumb_quad_rect, visible_quad_rect, color_,
false);
}

const char* SolidColorScrollbarLayerImpl::LayerTypeAsString() const {
Expand Down

0 comments on commit 328fa55

Please sign in to comment.