Skip to content

Commit

Permalink
[Gardener] Revert "Correct and efficient handling of SolidColorScroll…
Browse files Browse the repository at this point in the history
…barLayer 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}
  • Loading branch information
dizhang168 authored and Chromium LUCI CQ committed Oct 27, 2023
1 parent f8479c5 commit 716a958
Show file tree
Hide file tree
Showing 26 changed files with 137 additions and 200 deletions.
4 changes: 1 addition & 3 deletions cc/input/scrollbar.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,13 @@ 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);
kIsLeftSideVerticalScrollbar, absl::nullopt);
v_scrollbar_layer_ = AddLayer<SolidColorScrollbarLayerImpl>(
ScrollbarOrientation::kVertical, kThumbThickness, kTrackStart,
kIsLeftSideVerticalScrollbar);
kIsLeftSideVerticalScrollbar, absl::nullopt);
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);
kIsLeftSideVerticalScrollbar, absl::nullopt);
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);
kIsLeftSideVerticalScrollbar, absl::nullopt);

scrollbar_layer_->SetBounds(gfx::Size(kThumbThickness, kTrackLength));
scrollbar_layer_->SetScrollElementId(scroll_layer->element_id());
Expand Down
37 changes: 20 additions & 17 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);
false, absl::nullopt);
solid_color_scrollbar_layer->SetScrollElementId(layer_a->element_id());

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

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

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

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

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

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

#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());
is_left_side_vertical_scrollbar(), thumb_color_);
}

scoped_refptr<SolidColorScrollbarLayer> SolidColorScrollbarLayer::CreateOrReuse(
Expand All @@ -31,44 +29,45 @@ 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->track_start() == track_start &&
existing_layer->thumb_color() == thumb_color) {
// 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());
result = existing_layer;
} else {
result = Create(scrollbar->Orientation(), thumb_thickness, track_start,
scrollbar->IsLeftSideVerticalScrollbar());
return existing_layer;
}
result->SetColor(scrollbar->GetSolidColor());
return result;

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

scoped_refptr<SolidColorScrollbarLayer> SolidColorScrollbarLayer::Create(
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar) {
return base::WrapRefCounted(
new SolidColorScrollbarLayer(orientation, thumb_thickness, track_start,
is_left_side_vertical_scrollbar));
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));
}

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

Expand All @@ -91,35 +90,4 @@ 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: 6 additions & 10 deletions cc/layers/solid_color_scrollbar_layer.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ class CC_EXPORT SolidColorScrollbarLayer : public ScrollbarLayerBase {
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar);
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color);

SolidColorScrollbarLayer(const SolidColorScrollbarLayer&) = delete;
SolidColorScrollbarLayer& operator=(const SolidColorScrollbarLayer&) = delete;
Expand All @@ -37,29 +38,24 @@ 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_; }

void SetColor(SkColor4f color);
SkColor4f color() const { return color_.Read(*this); }
absl::optional<SkColor4f> thumb_color() const { return thumb_color_; }

ScrollbarLayerType GetScrollbarLayerType() const override;

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

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

} // namespace cc
Expand Down
21 changes: 13 additions & 8 deletions cc/layers/solid_color_scrollbar_layer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
#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 @@ -19,10 +21,11 @@ SolidColorScrollbarLayerImpl::Create(LayerTreeImpl* tree_impl,
ScrollbarOrientation orientation,
int thumb_thickness,
int track_start,
bool is_left_side_vertical_scrollbar) {
bool is_left_side_vertical_scrollbar,
absl::optional<SkColor4f> thumb_color) {
return base::WrapUnique(new SolidColorScrollbarLayerImpl(
tree_impl, id, orientation, thumb_thickness, track_start,
is_left_side_vertical_scrollbar));
is_left_side_vertical_scrollbar, thumb_color));
}

SolidColorScrollbarLayerImpl::~SolidColorScrollbarLayerImpl() = default;
Expand All @@ -31,7 +34,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());
is_left_side_vertical_scrollbar(), thumb_color_);
}

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

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 @@ -106,8 +111,8 @@ void SolidColorScrollbarLayerImpl::AppendQuads(
return;

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

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

0 comments on commit 716a958

Please sign in to comment.