diff options
Diffstat (limited to 'chromium/ui/compositor')
25 files changed, 751 insertions, 85 deletions
diff --git a/chromium/ui/compositor/BUILD.gn b/chromium/ui/compositor/BUILD.gn index a9fa6400328..19ba19069b3 100644 --- a/chromium/ui/compositor/BUILD.gn +++ b/chromium/ui/compositor/BUILD.gn @@ -17,6 +17,8 @@ jumbo_component("compositor") { "animation_metrics_recorder.cc", "animation_metrics_recorder.h", "animation_metrics_reporter.h", + "animation_throughput_reporter.cc", + "animation_throughput_reporter.h", "callback_layer_animation_observer.cc", "callback_layer_animation_observer.h", "canvas_painter.cc", @@ -220,6 +222,7 @@ jumbo_static_library("test_support") { test("compositor_unittests") { sources = [ + "animation_throughput_reporter_unittest.cc", "callback_layer_animation_observer_unittest.cc", "canvas_painter_unittest.cc", "compositor_lock_unittest.cc", diff --git a/chromium/ui/compositor/animation_metrics_reporter.h b/chromium/ui/compositor/animation_metrics_reporter.h index d06fe232e70..1b12fb55acc 100644 --- a/chromium/ui/compositor/animation_metrics_reporter.h +++ b/chromium/ui/compositor/animation_metrics_reporter.h @@ -5,6 +5,7 @@ #ifndef UI_COMPOSITOR_ANIMATION_METRICS_REPORTER_H_ #define UI_COMPOSITOR_ANIMATION_METRICS_REPORTER_H_ +#include "base/metrics/histogram_macros.h" #include "ui/compositor/compositor_export.h" namespace ui { @@ -22,6 +23,25 @@ class COMPOSITOR_EXPORT AnimationMetricsReporter { virtual void Report(int value) = 0; }; +// A subclass of AnimationMetricsReporter that writes into a percentage +// histogram when Report() is called. +template <const char* histogram_name> +class COMPOSITOR_EXPORT HistogramPercentageMetricsReporter + : public AnimationMetricsReporter { + public: + HistogramPercentageMetricsReporter() = default; + HistogramPercentageMetricsReporter( + const HistogramPercentageMetricsReporter&) = delete; + HistogramPercentageMetricsReporter& operator=( + const HistogramPercentageMetricsReporter&) = delete; + ~HistogramPercentageMetricsReporter() override = default; + + // AnimationMetricsReporter: + void Report(int value) override { + UMA_HISTOGRAM_PERCENTAGE(histogram_name, value); + } +}; + } // namespace ui #endif // UI_COMPOSITOR_ANIMATION_METRICS_REPORTER_H_ diff --git a/chromium/ui/compositor/animation_throughput_reporter.cc b/chromium/ui/compositor/animation_throughput_reporter.cc new file mode 100644 index 00000000000..bcb963bb367 --- /dev/null +++ b/chromium/ui/compositor/animation_throughput_reporter.cc @@ -0,0 +1,151 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "ui/compositor/animation_throughput_reporter.h" + +#include <memory> +#include <utility> + +#include "base/bind.h" +#include "base/check.h" +#include "base/memory/scoped_refptr.h" +#include "base/memory/weak_ptr.h" +#include "base/optional.h" +#include "cc/animation/animation.h" +#include "ui/compositor/callback_layer_animation_observer.h" +#include "ui/compositor/compositor.h" +#include "ui/compositor/layer.h" +#include "ui/compositor/layer_animation_delegate.h" +#include "ui/compositor/layer_animator.h" +#include "ui/compositor/throughput_tracker.h" + +namespace ui { + +class AnimationThroughputReporter::AnimationTracker + : public CallbackLayerAnimationObserver { + public: + AnimationTracker(LayerAnimator* animator, ReportCallback report_callback) + : CallbackLayerAnimationObserver( + base::BindRepeating(&AnimationTracker::OnAnimationEnded, + base::Unretained(this))), + animator_(animator), + report_callback_(std::move(report_callback)) { + DCHECK(report_callback_); + } + AnimationTracker(const AnimationTracker& other) = delete; + AnimationTracker& operator=(const AnimationTracker& other) = delete; + ~AnimationTracker() override = default; + + // Whether there are attached animation sequences to track. + bool IsTrackingAnimation() const { return !attached_sequences().empty(); } + + void set_should_delete(bool should_delete) { should_delete_ = should_delete; } + + private: + // CallbackLayerAnimationObserver: + void OnAnimatorAttachedToTimeline() override { MaybeStartTracking(); } + void OnAnimatorDetachedFromTimeline() override { + // Gives up tracking when detached from the timeline. + should_start_tracking_ = false; + if (throughput_tracker_) + throughput_tracker_.reset(); + + // OnAnimationEnded would not happen after detached from the timeline. + // So do the clean up here. + if (should_delete_) + delete this; + } + void OnLayerAnimationStarted(LayerAnimationSequence* sequence) override { + CallbackLayerAnimationObserver::OnLayerAnimationStarted(sequence); + + should_start_tracking_ = true; + MaybeStartTracking(); + + // Make sure SetActive() is called so that OnAnimationEnded callback will be + // invoked when all attached layer animation sequences finish. + if (!active()) + SetActive(); + } + + void MaybeStartTracking() { + // No tracking if no layer animation sequence is started. + if (!should_start_tracking_) + return; + + // No tracking if |animator_| is not attached to a timeline. Layer animation + // sequence would not tick without a timeline. + if (!AnimationThroughputReporter::IsAnimatorAttachedToTimeline( + animator_.get())) { + return; + } + + ui::Compositor* compositor = + AnimationThroughputReporter::GetCompositor(animator_.get()); + throughput_tracker_ = compositor->RequestNewThroughputTracker(); + throughput_tracker_->Start(report_callback_); + } + + // Invoked when all animation sequences finish. + bool OnAnimationEnded(const CallbackLayerAnimationObserver& self) { + // |tracking_started_| could reset when detached from animation timeline. + // E.g. underlying Layer is moved from one Compositor to another. No report + // for such case. + if (throughput_tracker_) { + if (self.aborted_count()) + throughput_tracker_->Cancel(); + else + throughput_tracker_->Stop(); + } + + should_start_tracking_ = false; + return should_delete_; + } + + // Whether this class should delete itself on animation ended. + bool should_delete_ = false; + + scoped_refptr<LayerAnimator> animator_; + + base::Optional<ThroughputTracker> throughput_tracker_; + + // Whether |throughput_tracker_| should be started. + bool should_start_tracking_ = false; + + AnimationThroughputReporter::ReportCallback report_callback_; +}; + +AnimationThroughputReporter::AnimationThroughputReporter( + LayerAnimator* animator, + ReportCallback report_callback) + : animator_(animator), + animation_tracker_( + std::make_unique<AnimationTracker>(animator_, + std::move(report_callback))) { + animator_->AddObserver(animation_tracker_.get()); +} + +AnimationThroughputReporter::~AnimationThroughputReporter() { + // Directly remove |animation_tracker_| from |LayerAnimator::observers_| + // rather than calling LayerAnimator::RemoveObserver(), to avoid removing it + // from the scheduled animation sequences. + animator_->observers_.RemoveObserver(animation_tracker_.get()); + + // |animation_tracker_| deletes itself when its tracked animations finish. + if (animation_tracker_->IsTrackingAnimation()) + animation_tracker_.release()->set_should_delete(true); +} + +// static +Compositor* AnimationThroughputReporter::GetCompositor( + LayerAnimator* animator) { + return animator->delegate()->GetLayer()->GetCompositor(); +} + +// static +bool AnimationThroughputReporter::IsAnimatorAttachedToTimeline( + LayerAnimator* animator) { + return animator->animation_->animation_timeline(); +} + +} // namespace ui diff --git a/chromium/ui/compositor/animation_throughput_reporter.h b/chromium/ui/compositor/animation_throughput_reporter.h new file mode 100644 index 00000000000..7fb9b665728 --- /dev/null +++ b/chromium/ui/compositor/animation_throughput_reporter.h @@ -0,0 +1,66 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef UI_COMPOSITOR_ANIMATION_THROUGHPUT_REPORTER_H_ +#define UI_COMPOSITOR_ANIMATION_THROUGHPUT_REPORTER_H_ + +#include <memory> + +#include "base/callback_forward.h" +#include "cc/metrics/frame_sequence_metrics.h" +#include "ui/compositor/compositor_export.h" + +namespace ui { + +class Compositor; +class LayerAnimator; + +// Reports cc::FrameSequenceMetrics::ThroughputData of layer animations. +// +// Throughput is defined as the ratio between number frames presented (actual +// screen updates) for the animations and the number frames expected. +// DroppedFrames/SkippedFrames is the other part of the story. It is a ratio +// between dropped/skipped frames over the expected frames. +// +// See also docs/speed/graphics_metrics_definitions.md. +// +// cc::FrameSequenceMetrics::ThroughputData contains the numbers of produced +// frames and expected frames and could be used to calculate the two metrics. +// +// All layer animation sequences created after its construction are consider +// as part of the animation being tracked. Graphics throughput tracking is +// stopped when all relevant layer animation sequences finish. The report +// callback is invoked on the next frame presentation if there is enough data +// and none of the layer animation sequences is aborted. +class COMPOSITOR_EXPORT AnimationThroughputReporter { + public: + using ReportCallback = + base::RepeatingCallback<void(cc::FrameSequenceMetrics::ThroughputData)>; + AnimationThroughputReporter(LayerAnimator* animator, + ReportCallback report_callback); + AnimationThroughputReporter(const AnimationThroughputReporter&) = delete; + AnimationThroughputReporter& operator=(const AnimationThroughputReporter&) = + delete; + ~AnimationThroughputReporter(); + + private: + // Tracks when layer animation sequences are scheduled and finished. + class AnimationTracker; + + // Returns the relevant compositor of |animator|. Note it could return + // nullptr if |animator| has not attached to an animation timeline. + // Listed here to access LayerAnimator's protected delegate(). + static Compositor* GetCompositor(LayerAnimator* animator); + + // Whether |animator_| is attached to a timeline. + // List here to access LayerAnimation's private |anmation_| member. + static bool IsAnimatorAttachedToTimeline(LayerAnimator* animator); + + LayerAnimator* const animator_; + std::unique_ptr<AnimationTracker> animation_tracker_; +}; + +} // namespace ui + +#endif // UI_COMPOSITOR_ANIMATION_THROUGHPUT_REPORTER_H_ diff --git a/chromium/ui/compositor/animation_throughput_reporter_unittest.cc b/chromium/ui/compositor/animation_throughput_reporter_unittest.cc new file mode 100644 index 00000000000..c6a14ba2f45 --- /dev/null +++ b/chromium/ui/compositor/animation_throughput_reporter_unittest.cc @@ -0,0 +1,271 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "ui/compositor/animation_throughput_reporter.h" + +#include <memory> + +#include "base/run_loop.h" +#include "base/test/bind_test_util.h" +#include "base/test/task_environment.h" +#include "base/threading/thread_task_runner_handle.h" +#include "base/time/time.h" +#include "base/timer/timer.h" +#include "cc/metrics/frame_sequence_metrics.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "ui/compositor/compositor.h" +#include "ui/compositor/layer.h" +#include "ui/compositor/layer_animation_sequence.h" +#include "ui/compositor/layer_animator.h" +#include "ui/compositor/scoped_layer_animation_settings.h" +#include "ui/compositor/test/test_compositor_host.h" +#include "ui/compositor/test/test_context_factories.h" +#include "ui/gfx/geometry/rect.h" + +namespace ui { + +class AnimationThroughputReporterTest : public testing::Test { + public: + AnimationThroughputReporterTest() + : task_environment_(base::test::TaskEnvironment::MainThreadType::UI) {} + AnimationThroughputReporterTest(const AnimationThroughputReporterTest&) = + delete; + AnimationThroughputReporterTest& operator=( + const AnimationThroughputReporterTest&) = delete; + ~AnimationThroughputReporterTest() override = default; + + // testing::Test: + void SetUp() override { + context_factories_ = std::make_unique<TestContextFactories>(false); + + const gfx::Rect bounds(100, 100); + host_.reset(TestCompositorHost::Create( + bounds, context_factories_->GetContextFactory())); + host_->Show(); + + compositor()->SetRootLayer(&root_); + + frame_generation_timer_.Start( + FROM_HERE, base::TimeDelta::FromMilliseconds(50), this, + &AnimationThroughputReporterTest::GenerateOneFrame); + } + + void TearDown() override { + frame_generation_timer_.Stop(); + host_.reset(); + context_factories_.reset(); + } + + void GenerateOneFrame() { compositor()->ScheduleFullRedraw(); } + + Compositor* compositor() { return host_->GetCompositor(); } + Layer* root_layer() { return &root_; } + + private: + base::test::TaskEnvironment task_environment_; + + std::unique_ptr<TestContextFactories> context_factories_; + std::unique_ptr<TestCompositorHost> host_; + Layer root_; + + // A timer to generate continuous compositor frames to trigger throughput + // data being transferred back. + base::RepeatingTimer frame_generation_timer_; +}; + +// Tests animation throughput collection with implicit animation scenario. +TEST_F(AnimationThroughputReporterTest, ImplicitAnimation) { + Layer layer; + layer.SetOpacity(0.5f); + root_layer()->Add(&layer); + + base::RunLoop run_loop; + { + LayerAnimator* animator = layer.GetAnimator(); + AnimationThroughputReporter reporter( + animator, base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData) { + run_loop.Quit(); + })); + + ScopedLayerAnimationSettings settings(animator); + settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(50)); + layer.SetOpacity(1.0f); + } + run_loop.Run(); +} + +// Tests animation throughput collection with implicit animation setup before +// Layer is attached to a compositor. +TEST_F(AnimationThroughputReporterTest, ImplicitAnimationLateAttach) { + Layer layer; + layer.SetOpacity(0.5f); + + base::RunLoop run_loop; + { + LayerAnimator* animator = layer.GetAnimator(); + AnimationThroughputReporter reporter( + animator, base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData) { + run_loop.Quit(); + })); + + ScopedLayerAnimationSettings settings(animator); + settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(50)); + layer.SetOpacity(1.0f); + } + + // Attach to root after animation setup. + root_layer()->Add(&layer); + + run_loop.Run(); +} + +// Tests animation throughput collection with explicitly created animation +// sequence scenario. +TEST_F(AnimationThroughputReporterTest, ExplicitAnimation) { + Layer layer; + layer.SetOpacity(0.5f); + root_layer()->Add(&layer); + + base::RunLoop run_loop; + { + LayerAnimator* animator = layer.GetAnimator(); + AnimationThroughputReporter reporter( + animator, base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData) { + run_loop.Quit(); + })); + + animator->ScheduleAnimation( + new LayerAnimationSequence(LayerAnimationElement::CreateOpacityElement( + 1.0f, base::TimeDelta::FromMilliseconds(50)))); + } + run_loop.Run(); +} + +// Tests animation throughput collection for a persisted animator of a Layer. +TEST_F(AnimationThroughputReporterTest, PersistedAnimation) { + auto layer = std::make_unique<Layer>(); + layer->SetOpacity(0.5f); + root_layer()->Add(layer.get()); + + // Set a persisted animator to |layer|. + LayerAnimator* animator = + new LayerAnimator(base::TimeDelta::FromMilliseconds(50)); + layer->SetAnimator(animator); + + std::unique_ptr<base::RunLoop> run_loop = std::make_unique<base::RunLoop>(); + // |reporter| keeps reporting as long as it is alive. + AnimationThroughputReporter reporter( + animator, + base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData) { run_loop->Quit(); })); + + // Report data for animation of opacity goes to 1. + layer->SetOpacity(1.0f); + run_loop->Run(); + + // Report data for animation of opacity goes to 0.5. + run_loop = std::make_unique<base::RunLoop>(); + layer->SetOpacity(0.5f); + run_loop->Run(); +} + +// Tests animation throughput not reported when animation is aborted. +TEST_F(AnimationThroughputReporterTest, AbortedAnimation) { + auto layer = std::make_unique<Layer>(); + layer->SetOpacity(0.5f); + root_layer()->Add(layer.get()); + + { + LayerAnimator* animator = layer->GetAnimator(); + AnimationThroughputReporter reporter( + animator, base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData) { + ADD_FAILURE() << "No report for aborted animations."; + })); + + ScopedLayerAnimationSettings settings(animator); + settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(50)); + layer->SetOpacity(1.0f); + } + + // Delete |layer| to abort on-going animations. + layer.reset(); + + // Wait a bit to ensure that report does not happen. + base::RunLoop run_loop; + base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( + FROM_HERE, run_loop.QuitClosure(), + base::TimeDelta::FromMilliseconds(100)); + run_loop.Run(); +} + +// Tests animation throughput not reported when detached from timeline. +TEST_F(AnimationThroughputReporterTest, NoReportOnDetach) { + auto layer = std::make_unique<Layer>(); + layer->SetOpacity(0.5f); + root_layer()->Add(layer.get()); + + { + LayerAnimator* animator = layer->GetAnimator(); + AnimationThroughputReporter reporter( + animator, base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData) { + ADD_FAILURE() << "No report for aborted animations."; + })); + + ScopedLayerAnimationSettings settings(animator); + settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(50)); + layer->SetOpacity(1.0f); + } + + // Detach from the root and attach to a root. + root_layer()->Remove(layer.get()); + root_layer()->Add(layer.get()); + + // Wait a bit to ensure that report does not happen. + base::RunLoop run_loop; + base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( + FROM_HERE, run_loop.QuitClosure(), + base::TimeDelta::FromMilliseconds(100)); + run_loop.Run(); +} + +// Tests animation throughput not reported and no leak when animation is stopped +// without being attached to a root. +TEST_F(AnimationThroughputReporterTest, EndDetachedNoReportNoLeak) { + auto layer = std::make_unique<Layer>(); + layer->SetOpacity(0.5f); + + LayerAnimator* animator = layer->GetAnimator(); + + // Schedule an animation without being attached to a root. + { + AnimationThroughputReporter reporter( + animator, base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData) { + ADD_FAILURE() << "No report for aborted animations."; + })); + + ScopedLayerAnimationSettings settings(animator); + settings.SetTransitionDuration(base::TimeDelta::FromMilliseconds(50)); + layer->SetOpacity(1.0f); + } + + // End the animation without being attached to a root. + animator->StopAnimating(); + + // Wait a bit to ensure that report does not happen. + base::RunLoop run_loop; + base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( + FROM_HERE, run_loop.QuitClosure(), + base::TimeDelta::FromMilliseconds(100)); + run_loop.Run(); + + // AnimationTracker in |reporter| should not leak in asan. +} + +} // namespace ui diff --git a/chromium/ui/compositor/callback_layer_animation_observer.cc b/chromium/ui/compositor/callback_layer_animation_observer.cc index 8082678d7b1..3c41fe6fb35 100644 --- a/chromium/ui/compositor/callback_layer_animation_observer.cc +++ b/chromium/ui/compositor/callback_layer_animation_observer.cc @@ -5,6 +5,7 @@ #include "ui/compositor/callback_layer_animation_observer.h" #include "base/bind.h" +#include "base/logging.h" #include "ui/compositor/layer_animation_sequence.h" namespace ui { diff --git a/chromium/ui/compositor/compositor.cc b/chromium/ui/compositor/compositor.cc index 178fc2628e3..65d2f3ec773 100644 --- a/chromium/ui/compositor/compositor.cc +++ b/chromium/ui/compositor/compositor.cc @@ -450,16 +450,6 @@ void Compositor::SetDisplayColorSpaces( if (display_color_spaces_ == display_color_spaces) return; display_color_spaces_ = display_color_spaces; - // TODO(crbug.com/1012846): Remove this flag and provision when HDR is fully - // supported on ChromeOS. -#if defined(OS_CHROMEOS) - if (display_color_spaces_.SupportsHDR() && - !base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kEnableUseHDRTransferFunction)) { - display_color_spaces_ = - gfx::DisplayColorSpaces(gfx::ColorSpace::CreateSRGB()); - } -#endif host_->SetRasterColorSpace(display_color_spaces_.GetRasterColorSpace()); // Always force the ui::Compositor to re-draw all layers, because damage @@ -615,7 +605,8 @@ void Compositor::IssueExternalBeginFrame( } ThroughputTracker Compositor::RequestNewThroughputTracker() { - return ThroughputTracker(next_throughput_tracker_id_++, this); + return ThroughputTracker(next_throughput_tracker_id_++, + weak_ptr_factory_.GetWeakPtr()); } void Compositor::DidUpdateLayers() { diff --git a/chromium/ui/compositor/compositor.h b/chromium/ui/compositor/compositor.h index 48cd5ff63d7..438dd212d88 100644 --- a/chromium/ui/compositor/compositor.h +++ b/chromium/ui/compositor/compositor.h @@ -346,6 +346,13 @@ class COMPOSITOR_EXPORT Compositor : public cc::LayerTreeHostClient, override; void NotifyThroughputTrackerResults( cc::CustomTrackerResults results) override; + void SubmitThroughputData(ukm::SourceId source_id, + int aggregated_percent, + int impl_percent, + base::Optional<int> main_percent) override {} + void DidObserveFirstScrollDelay( + base::TimeDelta first_scroll_delay, + base::TimeTicks first_scroll_timestamp) override {} // cc::LayerTreeHostSingleThreadClient implementation. void DidSubmitCompositorFrame() override; @@ -483,6 +490,7 @@ class COMPOSITOR_EXPORT Compositor : public cc::LayerTreeHostClient, ThroughputTrackerMap throughput_tracker_map_; base::WeakPtrFactory<Compositor> context_creation_weak_ptr_factory_{this}; + base::WeakPtrFactory<Compositor> weak_ptr_factory_{this}; DISALLOW_COPY_AND_ASSIGN(Compositor); }; diff --git a/chromium/ui/compositor/compositor_unittest.cc b/chromium/ui/compositor/compositor_unittest.cc index e92a5eb1362..8515cdc5ab1 100644 --- a/chromium/ui/compositor/compositor_unittest.cc +++ b/chromium/ui/compositor/compositor_unittest.cc @@ -270,6 +270,19 @@ TEST_F(CompositorTestWithMessageLoop, ThroughputTracker) { run_loop.Run(); } +TEST_F(CompositorTestWithMessageLoop, ThroughputTrackerOutliveCompositor) { + auto tracker = compositor()->RequestNewThroughputTracker(); + tracker.Start(base::BindLambdaForTesting( + [&](cc::FrameSequenceMetrics::ThroughputData throughput) { + ADD_FAILURE() << "No report should happen"; + })); + + DestroyCompositor(); + + // No crash, no use-after-free and no report. + tracker.Stop(); +} + #if defined(OS_WIN) // TODO(crbug.com/608436): Flaky on windows trybots #define MAYBE_CreateAndReleaseOutputSurface \ diff --git a/chromium/ui/compositor/layer.cc b/chromium/ui/compositor/layer.cc index ec9f20471ae..27efca014fd 100644 --- a/chromium/ui/compositor/layer.cc +++ b/chromium/ui/compositor/layer.cc @@ -308,7 +308,9 @@ void Layer::SetShowReflectedLayerSubtree(Layer* subtree_reflected_layer) { scoped_refptr<cc::MirrorLayer> new_layer = cc::MirrorLayer::Create(subtree_reflected_layer->cc_layer_); - SwitchToLayer(new_layer); + if (!SwitchToLayer(new_layer)) + return; + mirror_layer_ = std::move(new_layer); subtree_reflected_layer_ = subtree_reflected_layer; @@ -375,12 +377,20 @@ void Layer::Add(Layer* child) { } void Layer::Remove(Layer* child) { + base::WeakPtr<Layer> weak_this = weak_ptr_factory_.GetWeakPtr(); + base::WeakPtr<Layer> weak_child = child->weak_ptr_factory_.GetWeakPtr(); + // Current bounds are used to calculate offsets when layers are reparented. // Stop (and complete) an ongoing animation to update the bounds immediately. LayerAnimator* child_animator = child->animator_.get(); if (child_animator) child_animator->StopAnimatingProperty(ui::LayerAnimationElement::BOUNDS); + // Do not proceed if |this| or |child| is released by an animation observer + // of |child|'s bounds animation. + if (!weak_this || !weak_child) + return; + Compositor* compositor = GetCompositor(); if (compositor) child->ResetCompositorForAnimatorsInTree(compositor); @@ -725,11 +735,19 @@ void Layer::SetName(const std::string& name) { cc_layer_->SetDebugName(name); } -void Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) { +bool Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) { // Finish animations being handled by cc_layer_. if (animator_) { + base::WeakPtr<Layer> weak_this = weak_ptr_factory_.GetWeakPtr(); + animator_->StopAnimatingProperty(LayerAnimationElement::TRANSFORM); + if (!weak_this) + return false; + animator_->StopAnimatingProperty(LayerAnimationElement::OPACITY); + if (!weak_this) + return false; + animator_->SwitchToLayer(new_layer); } @@ -781,12 +799,16 @@ void Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) { SetLayerFilters(); SetLayerBackgroundFilters(); + return true; } -void Layer::SwitchCCLayerForTest() { +bool Layer::SwitchCCLayerForTest() { scoped_refptr<cc::PictureLayer> new_layer = cc::PictureLayer::Create(this); - SwitchToLayer(new_layer); + if (!SwitchToLayer(new_layer)) + return false; + content_layer_ = std::move(new_layer); + return true; } // Note: The code that sets this flag would be responsible to unset it on that @@ -889,7 +911,9 @@ void Layer::SetTransferableResource( scoped_refptr<cc::TextureLayer> new_layer = cc::TextureLayer::CreateForMailbox(this); new_layer->SetFlipped(true); - SwitchToLayer(new_layer); + if (!SwitchToLayer(new_layer)) + return; + texture_layer_ = new_layer; // Reset the frame_size_in_dip_ so that SetTextureSize() will not early out, // the frame_size_in_dip_ was for a previous (different) |texture_layer_|. @@ -972,7 +996,9 @@ void Layer::SetShowReflectedSurface(const viz::SurfaceId& surface_id, if (!surface_layer_) { scoped_refptr<cc::SurfaceLayer> new_layer = cc::SurfaceLayer::Create(); - SwitchToLayer(new_layer); + if (!SwitchToLayer(new_layer)) + return; + surface_layer_ = new_layer; } @@ -1007,7 +1033,9 @@ void Layer::SetShowSolidColorContent() { return; scoped_refptr<cc::SolidColorLayer> new_layer = cc::SolidColorLayer::Create(); - SwitchToLayer(new_layer); + if (!SwitchToLayer(new_layer)) + return; + solid_color_layer_ = new_layer; transfer_resource_ = viz::TransferableResource(); @@ -1385,10 +1413,12 @@ void Layer::SetBoundsFromAnimation(const gfx::Rect& bounds, reflecting_layer->MatchLayerSize(this); } -void Layer::SetTransformFromAnimation(const gfx::Transform& transform, +void Layer::SetTransformFromAnimation(const gfx::Transform& new_transform, PropertyChangeReason reason) { - const gfx::Transform old_transform = this->transform(); - cc_layer_->SetTransform(transform); + const gfx::Transform old_transform = transform(); + if (old_transform == new_transform) + return; + cc_layer_->SetTransform(new_transform); // Skip recomputing position if the subpixel offset does not need updating // which is the case if an explicit offset is set. @@ -1618,7 +1648,9 @@ void Layer::CreateSurfaceLayerIfNecessary() { return; scoped_refptr<cc::SurfaceLayer> new_layer = cc::SurfaceLayer::Create(); new_layer->SetSurfaceHitTestable(true); - SwitchToLayer(new_layer); + if (!SwitchToLayer(new_layer)) + return; + surface_layer_ = new_layer; } diff --git a/chromium/ui/compositor/layer.h b/chromium/ui/compositor/layer.h index 505c81e688c..8434ac53c33 100644 --- a/chromium/ui/compositor/layer.h +++ b/chromium/ui/compositor/layer.h @@ -458,7 +458,7 @@ class COMPOSITOR_EXPORT Layer : public LayerAnimationDelegate, float device_scale_factor() const { return device_scale_factor_; } // Triggers a call to SwitchToLayer. - void SwitchCCLayerForTest(); + bool SwitchCCLayerForTest(); const cc::Region& damaged_region_for_testing() const { return damaged_region_; @@ -527,7 +527,7 @@ class COMPOSITOR_EXPORT Layer : public LayerAnimationDelegate, // Implementation of LayerAnimatorDelegate void SetBoundsFromAnimation(const gfx::Rect& bounds, PropertyChangeReason reason) override; - void SetTransformFromAnimation(const gfx::Transform& transform, + void SetTransformFromAnimation(const gfx::Transform& new_transform, PropertyChangeReason reason) override; void SetOpacityFromAnimation(float opacity, PropertyChangeReason reason) override; @@ -575,8 +575,11 @@ class COMPOSITOR_EXPORT Layer : public LayerAnimationDelegate, // Set all filters which got applied to the layer background. void SetLayerBackgroundFilters(); - // Cleanup |cc_layer_| and replaces it with |new_layer|. - void SwitchToLayer(scoped_refptr<cc::Layer> new_layer); + // Cleanup |cc_layer_| and replaces it with |new_layer|. When stopping + // animations handled by old cc layer before the switch, |this| could be + // released by an animation observer. Returns false when it happens and + // callers should take cautions as well. Otherwise returns true. + bool SwitchToLayer(scoped_refptr<cc::Layer> new_layer) WARN_UNUSED_RESULT; void SetCompositorForAnimatorsInTree(Compositor* compositor); void ResetCompositorForAnimatorsInTree(Compositor* compositor); diff --git a/chromium/ui/compositor/layer_animation_observer.cc b/chromium/ui/compositor/layer_animation_observer.cc index 2c77493b76d..043176d6ccd 100644 --- a/chromium/ui/compositor/layer_animation_observer.cc +++ b/chromium/ui/compositor/layer_animation_observer.cc @@ -59,10 +59,7 @@ void LayerAnimationObserver::DetachedFromSequence( // ImplicitAnimationObserver ImplicitAnimationObserver::ImplicitAnimationObserver() - : active_(false), - destroyed_(NULL), - first_sequence_scheduled_(false) { -} + : active_(false), destroyed_(nullptr), first_sequence_scheduled_(false) {} ImplicitAnimationObserver::~ImplicitAnimationObserver() { if (destroyed_) @@ -97,7 +94,7 @@ void ImplicitAnimationObserver::OnLayerAnimationEnded( sequence->RemoveObserver(this); if (destroyed) return; - destroyed_ = NULL; + destroyed_ = nullptr; DCHECK(attached_sequences().find(sequence) == attached_sequences().end()); CheckCompleted(); } @@ -110,7 +107,7 @@ void ImplicitAnimationObserver::OnLayerAnimationAborted( sequence->RemoveObserver(this); if (destroyed) return; - destroyed_ = NULL; + destroyed_ = nullptr; DCHECK(attached_sequences().find(sequence) == attached_sequences().end()); CheckCompleted(); } diff --git a/chromium/ui/compositor/layer_animation_observer.h b/chromium/ui/compositor/layer_animation_observer.h index b83acde04fb..eb460da8533 100644 --- a/chromium/ui/compositor/layer_animation_observer.h +++ b/chromium/ui/compositor/layer_animation_observer.h @@ -73,6 +73,12 @@ class COMPOSITOR_EXPORT LayerAnimationObserver { // Called when |this| is removed to |sequence|'s observer list. virtual void OnDetachedFromSequence(LayerAnimationSequence* sequence); + // Called when the relevant animator attaches to an animation timeline. + virtual void OnAnimatorAttachedToTimeline() {} + + // Called when the relevant animator detaches from an animation timeline. + virtual void OnAnimatorDetachedFromTimeline() {} + // Detaches this observer from all sequences it is currently observing. void StopObserving(); diff --git a/chromium/ui/compositor/layer_animation_sequence.cc b/chromium/ui/compositor/layer_animation_sequence.cc index 06995075fcf..e25e1c34541 100644 --- a/chromium/ui/compositor/layer_animation_sequence.cc +++ b/chromium/ui/compositor/layer_animation_sequence.cc @@ -249,11 +249,17 @@ void LayerAnimationSequence::OnAnimatorAttached( LayerAnimationDelegate* delegate) { for (auto& element : elements_) element->OnAnimatorAttached(delegate); + + for (LayerAnimationObserver& observer : observers_) + observer.OnAnimatorAttachedToTimeline(); } void LayerAnimationSequence::OnAnimatorDetached() { for (auto& element : elements_) element->OnAnimatorDetached(); + + for (LayerAnimationObserver& observer : observers_) + observer.OnAnimatorDetachedFromTimeline(); } void LayerAnimationSequence::SetAnimationMetricsReporter( diff --git a/chromium/ui/compositor/layer_animator.cc b/chromium/ui/compositor/layer_animator.cc index e3e564a1037..a9db6922b89 100644 --- a/chromium/ui/compositor/layer_animator.cc +++ b/chromium/ui/compositor/layer_animator.cc @@ -31,10 +31,9 @@ ((running_anim.is_sequence_alive()) \ ? function(running_anim.sequence()) \ : false) -#define SAFE_INVOKE_PTR(function, running_anim) \ - ((running_anim.is_sequence_alive()) \ - ? function(running_anim.sequence()) \ - : NULL) +#define SAFE_INVOKE_PTR(function, running_anim) \ + ((running_anim.is_sequence_alive()) ? function(running_anim.sequence()) \ + : nullptr) namespace ui { @@ -47,7 +46,7 @@ const int kLayerAnimatorDefaultTransitionDurationMs = 120; // LayerAnimator public -------------------------------------------------------- LayerAnimator::LayerAnimator(base::TimeDelta transition_duration) - : delegate_(NULL), + : delegate_(nullptr), preemption_strategy_(IMMEDIATELY_SET_NEW_TARGET), is_transition_duration_locked_(false), transition_duration_(transition_duration), @@ -66,7 +65,7 @@ LayerAnimator::~LayerAnimator() { running_animations_[i].sequence()->OnAnimatorDestroyed(); } ClearAnimationsInternal(); - delegate_ = NULL; + delegate_ = nullptr; DCHECK(!animation_->animation_timeline()); } @@ -660,7 +659,7 @@ LayerAnimator::RunningAnimation* LayerAnimator::GetRunningAnimation( if ((*iter).sequence()->properties() & property) return &(*iter); } - return NULL; + return nullptr; } void LayerAnimator::AddToQueueIfNotPresent(LayerAnimationSequence* animation) { @@ -735,7 +734,7 @@ void LayerAnimator::ImmediatelySetNewTarget(LayerAnimationSequence* sequence) { return; LayerAnimationSequence* removed = RemoveAnimation(sequence); - DCHECK(removed == NULL || removed == sequence); + DCHECK(removed == nullptr || removed == sequence); if (!weak_sequence_ptr.get()) return; @@ -950,7 +949,7 @@ void LayerAnimator::PurgeDeletedAnimations() { } LayerAnimatorCollection* LayerAnimator::GetLayerAnimatorCollection() { - return delegate_ ? delegate_->GetLayerAnimatorCollection() : NULL; + return delegate_ ? delegate_->GetLayerAnimatorCollection() : nullptr; } void LayerAnimator::NotifyAnimationStarted(base::TimeTicks monotonic_time, diff --git a/chromium/ui/compositor/layer_animator.h b/chromium/ui/compositor/layer_animator.h index c9a64b24712..fe4cb76be33 100644 --- a/chromium/ui/compositor/layer_animator.h +++ b/chromium/ui/compositor/layer_animator.h @@ -258,6 +258,7 @@ class COMPOSITOR_EXPORT LayerAnimator : public base::RefCounted<LayerAnimator>, friend class base::RefCounted<LayerAnimator>; friend class ScopedLayerAnimationSettings; friend class LayerAnimatorTestController; + friend class AnimationThroughputReporter; FRIEND_TEST_ALL_PREFIXES(LayerAnimatorTest, AnimatorStartedCorrectly); FRIEND_TEST_ALL_PREFIXES(LayerAnimatorTest, AnimatorRemovedFromCollectionWhenLayerIsDestroyed); diff --git a/chromium/ui/compositor/layer_animator_unittest.cc b/chromium/ui/compositor/layer_animator_unittest.cc index c02519f0d86..bbb9f4c5c37 100644 --- a/chromium/ui/compositor/layer_animator_unittest.cc +++ b/chromium/ui/compositor/layer_animator_unittest.cc @@ -231,8 +231,9 @@ class LayerAnimatorDestructionObserver { class TestLayerAnimator : public LayerAnimator { public: - TestLayerAnimator() : LayerAnimator(base::TimeDelta::FromSeconds(0)), - destruction_observer_(NULL) {} + TestLayerAnimator() + : LayerAnimator(base::TimeDelta::FromSeconds(0)), + destruction_observer_(nullptr) {} void SetDestructionObserver( LayerAnimatorDestructionObserver* observer) { diff --git a/chromium/ui/compositor/layer_owner.cc b/chromium/ui/compositor/layer_owner.cc index c8adc411bcb..3eaf415111b 100644 --- a/chromium/ui/compositor/layer_owner.cc +++ b/chromium/ui/compositor/layer_owner.cc @@ -33,7 +33,7 @@ void LayerOwner::SetLayer(std::unique_ptr<Layer> layer) { std::unique_ptr<Layer> LayerOwner::AcquireLayer() { if (layer_owner_) - layer_owner_->owner_ = NULL; + layer_owner_->owner_ = nullptr; return std::move(layer_owner_); } @@ -53,7 +53,7 @@ std::unique_ptr<Layer> LayerOwner::RecreateLayer() { return old_layer; LayerDelegate* old_delegate = old_layer->delegate(); - old_layer->set_delegate(NULL); + old_layer->set_delegate(nullptr); SetLayer(old_layer->Clone()); @@ -88,7 +88,7 @@ std::unique_ptr<Layer> LayerOwner::RecreateLayer() { } void LayerOwner::DestroyLayer() { - layer_ = NULL; + layer_ = nullptr; layer_owner_.reset(); } diff --git a/chromium/ui/compositor/layer_owner.h b/chromium/ui/compositor/layer_owner.h index 7d4bc47b536..c541ca919c8 100644 --- a/chromium/ui/compositor/layer_owner.h +++ b/chromium/ui/compositor/layer_owner.h @@ -50,7 +50,7 @@ class COMPOSITOR_EXPORT LayerOwner { // |layer|. void Reset(std::unique_ptr<Layer> layer); - // Asks the owner to recreate the layer, returning the old Layer. NULL is + // Asks the owner to recreate the layer, returning the old Layer. nullptr is // returned if there is no existing layer, or recreate is not supported. // // This does not recurse. Existing children of the layer are moved to the new @@ -69,7 +69,7 @@ class COMPOSITOR_EXPORT LayerOwner { private: // The LayerOwner owns its layer unless ownership is relinquished via a call // to AcquireLayer(). After that moment |layer_| will still be valid but - // |layer_owner_| will be NULL. The reason for releasing ownership is that + // |layer_owner_| will be nullptr. The reason for releasing ownership is that // the client may wish to animate the layer beyond the lifetime of the owner, // e.g. fading it out when it is destroyed. std::unique_ptr<Layer> layer_owner_; diff --git a/chromium/ui/compositor/layer_tree_owner.h b/chromium/ui/compositor/layer_tree_owner.h index 4412c2225a0..ed0e53b14af 100644 --- a/chromium/ui/compositor/layer_tree_owner.h +++ b/chromium/ui/compositor/layer_tree_owner.h @@ -23,7 +23,7 @@ class COMPOSITOR_EXPORT LayerTreeOwner { Layer* release() WARN_UNUSED_RESULT { Layer* root = root_; - root_ = NULL; + root_ = nullptr; return root; } diff --git a/chromium/ui/compositor/layer_unittest.cc b/chromium/ui/compositor/layer_unittest.cc index d97c7e846af..77945544e9f 100644 --- a/chromium/ui/compositor/layer_unittest.cc +++ b/chromium/ui/compositor/layer_unittest.cc @@ -790,7 +790,7 @@ TEST_F(LayerWithDelegateTest, Cloning) { layer->SetFillsBoundsOpaquely(false); // Color and opaqueness targets should be preserved during cloning, even after // switching away from solid color content. - layer->SwitchCCLayerForTest(); + ASSERT_TRUE(layer->SwitchCCLayerForTest()); clone = layer->Clone(); @@ -1971,7 +1971,7 @@ namespace { class SchedulePaintLayerDelegate : public LayerDelegate { public: - SchedulePaintLayerDelegate() : paint_count_(0), layer_(NULL) {} + SchedulePaintLayerDelegate() : paint_count_(0), layer_(nullptr) {} ~SchedulePaintLayerDelegate() override {} @@ -2427,7 +2427,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerAnimations) { l1->SetOpacity(0.5f); // Change l1's cc::Layer. - l1->SwitchCCLayerForTest(); + ASSERT_TRUE(l1->SwitchCCLayerForTest()); // Ensure that the opacity animation completed. EXPECT_FLOAT_EQ(l1->opacity(), 0.5f); @@ -2449,7 +2449,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerSolidColorNotAnimating) { EXPECT_EQ(transparent, root->GetTargetColor()); // Changing the underlying layer should not affect targets. - root->SwitchCCLayerForTest(); + ASSERT_TRUE(root->SwitchCCLayerForTest()); EXPECT_FALSE(root->fills_bounds_opaquely()); EXPECT_FALSE( @@ -2487,7 +2487,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerSolidColorWhileAnimating) { EXPECT_EQ(transparent, root->GetTargetColor()); // Changing the underlying layer should not affect targets. - root->SwitchCCLayerForTest(); + ASSERT_TRUE(root->SwitchCCLayerForTest()); EXPECT_TRUE(root->fills_bounds_opaquely()); EXPECT_TRUE( @@ -2515,7 +2515,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerCacheRenderSurface) { l1->AddCacheRenderSurfaceRequest(); // Change l1's cc::Layer. - l1->SwitchCCLayerForTest(); + ASSERT_TRUE(l1->SwitchCCLayerForTest()); // Ensure that the cache_render_surface flag is maintained. EXPECT_TRUE(l1->cc_layer_for_testing()->cache_render_surface()); @@ -2532,7 +2532,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerTrilinearFiltering) { l1->AddTrilinearFilteringRequest(); // Change l1's cc::Layer. - l1->SwitchCCLayerForTest(); + ASSERT_TRUE(l1->SwitchCCLayerForTest()); // Ensure that the trilinear_filtering flag is maintained. EXPECT_TRUE(l1->cc_layer_for_testing()->trilinear_filtering()); @@ -2550,12 +2550,38 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerMasksToBounds) { EXPECT_TRUE(l1->cc_layer_for_testing()->masks_to_bounds()); // Change l1's cc::Layer. - l1->SwitchCCLayerForTest(); + ASSERT_TRUE(l1->SwitchCCLayerForTest()); // Ensure that the trilinear_filtering flag is maintained. EXPECT_TRUE(l1->cc_layer_for_testing()->masks_to_bounds()); } +// Tests that no crash happens when switching cc layer with an animation +// observer that deletes the layer itself. +TEST_P(LayerWithRealCompositorTest, SwitchCCLayerDeleteLayer) { + std::unique_ptr<Layer> root(CreateLayer(LAYER_TEXTURED)); + std::unique_ptr<Layer> l1(CreateLayer(LAYER_TEXTURED)); + GetCompositor()->SetRootLayer(root.get()); + root->Add(l1.get()); + + TestCallbackAnimationObserver animation_observer; + animation_observer.SetCallback( + base::BindLambdaForTesting([&]() { l1.reset(); })); + + auto long_duration_animation = + std::make_unique<ui::ScopedAnimationDurationScaleMode>( + ui::ScopedAnimationDurationScaleMode::SLOW_DURATION); + { + ui::ScopedLayerAnimationSettings animation(l1->GetAnimator()); + animation.AddObserver(&animation_observer); + animation.SetTransitionDuration(base::TimeDelta::FromMilliseconds(1000)); + l1->SetOpacity(0.f); + } + + // Fails but no crash. + EXPECT_FALSE(l1->SwitchCCLayerForTest()); +} + // Triggerring a OnDeviceScaleFactorChanged while a layer is undergoing // transform animation, may cause a crash. This is because an animation observer // may mutate the tree, e.g. deleting a layer, changing ancestor z-order etc, @@ -2604,9 +2630,6 @@ TEST_P(LayerWithRealCompositorTest, TreeMutationDuringScaleFactorChange) { root->Add(layer_to_delete.get()); layer_to_delete->Add(child.get()); - long_duration_animation = - std::make_unique<ui::ScopedAnimationDurationScaleMode>( - ui::ScopedAnimationDurationScaleMode::SLOW_DURATION); { ui::ScopedLayerAnimationSettings animation(layer_to_delete->GetAnimator()); animation.AddObserver(&animation_observer); @@ -2629,9 +2652,6 @@ TEST_P(LayerWithRealCompositorTest, TreeMutationDuringScaleFactorChange) { layer_to_delete->Add(child.get()); layer_to_delete->Add(child2.get()); - long_duration_animation = - std::make_unique<ui::ScopedAnimationDurationScaleMode>( - ui::ScopedAnimationDurationScaleMode::SLOW_DURATION); { ui::ScopedLayerAnimationSettings animation(child->GetAnimator()); animation.AddObserver(&animation_observer); @@ -2666,6 +2686,60 @@ TEST_P(LayerWithRealCompositorTest, TreeMutationDuringScaleFactorChange) { root->OnDeviceScaleFactorChanged(1.5f); } +// Tests that no crash when parent/child layer is released by an animation +// observer of the child layer bounds animation. +TEST_P(LayerWithRealCompositorTest, ParentOrChildGoneDuringRemove) { + std::unique_ptr<Layer> root = CreateLayer(LAYER_SOLID_COLOR); + GetCompositor()->SetRootLayer(root.get()); + root->SetBounds(gfx::Rect(0, 0, 100, 100)); + + // Actions to taken on animation ends. + enum class Action { + kReleaseParent, + kReleaseChild, + }; + + for (auto action : {Action::kReleaseParent, Action::kReleaseChild}) { + SCOPED_TRACE(::testing::Message() << "action=" << static_cast<int>(action)); + + std::unique_ptr<Layer> parent = CreateLayer(LAYER_SOLID_COLOR); + parent->SetBounds(gfx::Rect(0, 0, 100, 100)); + + std::unique_ptr<Layer> child = CreateLayer(LAYER_SOLID_COLOR); + child->SetBounds(gfx::Rect(0, 0, 100, 100)); + parent->Add(child.get()); + + root->Add(parent.get()); + + // An animation observer that takes action on animation ends. + TestCallbackAnimationObserver animation_observer; + animation_observer.SetCallback(base::BindLambdaForTesting([&]() { + switch (action) { + case Action::kReleaseParent: + parent.reset(); + break; + case Action::kReleaseChild: + child.reset(); + break; + } + })); + + // Schedule a bounds animation on |child|. + auto long_duration_animation = + std::make_unique<ui::ScopedAnimationDurationScaleMode>( + ui::ScopedAnimationDurationScaleMode::SLOW_DURATION); + { + ui::ScopedLayerAnimationSettings animation(child->GetAnimator()); + animation.AddObserver(&animation_observer); + animation.SetTransitionDuration(base::TimeDelta::FromMilliseconds(1000)); + child->SetBounds(gfx::Rect(10, 20, 100, 100)); + } + + // No crash should happen when removing |child| with a bounds animation. + parent->Remove(child.get()); + } +} + // Tests that the animators in the layer tree is added to the // animator-collection when the root-layer is set to the compositor. TEST_F(LayerWithDelegateTest, RootLayerAnimatorsInCompositor) { @@ -2910,7 +2984,7 @@ TEST(LayerDelegateTest, OnLayerTransformed) { layer->SetTransform(target_transform1); } gfx::Transform target_transform2; - target_transform2.Skew(10.0f, 5.0f); + target_transform2.Skew(15.0f, 5.0f); EXPECT_CALL(delegate, OnLayerTransformed(target_transform1, PropertyChangeReason::NOT_FROM_ANIMATION)) @@ -2923,6 +2997,18 @@ TEST(LayerDelegateTest, OnLayerTransformed) { layer->SetTransform(target_transform2); } +// Verify that LayerDelegate::OnLayerTransformed() is not called when the +// transform isn't actually changed. +TEST(LayerDelegateTest, OnLayerTransformedNotCalledWhenUnchanged) { + auto layer = std::make_unique<Layer>(LAYER_TEXTURED); + testing::StrictMock<TestLayerDelegate> delegate; + layer->set_delegate(&delegate); + + gfx::Transform target_transform; + EXPECT_CALL(delegate, OnLayerTransformed).Times(0); + layer->SetTransform(target_transform); +} + // Verify that LayerDelegate::OnLayerTransformed() is called at every step of a // non-threaded transform transition. TEST(LayerDelegateTest, OnLayerTransformedNonThreadedAnimation) { @@ -2996,8 +3082,8 @@ TEST(LayerDelegateTest, OnLayerTransformedNonThreadedAnimation) { testing::Mock::VerifyAndClear(&delegate); } -// Verify that LayerDelegate::OnLayerTransformed() is called at the beginning -// and at the end of a threaded transform transition. +// Verify that LayerDelegate::OnLayerTransformed() is called at the end of a +// threaded transform transition. TEST(LayerDelegateTest, OnLayerTransformedThreadedAnimation) { ScopedAnimationDurationScaleMode scoped_animation_duration_scale_mode( ScopedAnimationDurationScaleMode::NORMAL_DURATION); @@ -3019,17 +3105,12 @@ TEST(LayerDelegateTest, OnLayerTransformedThreadedAnimation) { target_transform, base::TimeDelta::FromSeconds(1)); ASSERT_TRUE(element->IsThreaded(layer.get())); LayerAnimationElement* element_raw = element.get(); + // At the beginning, setting the transform actually does not change, so + // OnLayerTransformed isn't called. EXPECT_CALL(delegate, OnLayerTransformed(gfx::Transform(), PropertyChangeReason::FROM_ANIMATION)) - .WillOnce(testing::Invoke([&](const gfx::Transform& old_transform, - PropertyChangeReason) { - // Verify that |layer->transform()| returns the correct value when the - // delegate is notified. - EXPECT_EQ(layer->transform(), initial_transform); - EXPECT_TRUE( - animator->IsAnimatingProperty(LayerAnimationElement::TRANSFORM)); - })); + .Times(0); animator->StartAnimation(new LayerAnimationSequence(std::move(element))); testing::Mock::VerifyAndClear(&delegate); test_controller.StartThreadedAnimationsIfNeeded(); diff --git a/chromium/ui/compositor/overscroll/scroll_input_handler.cc b/chromium/ui/compositor/overscroll/scroll_input_handler.cc index 6b2a80c5cfe..15f1a6a0250 100644 --- a/chromium/ui/compositor/overscroll/scroll_input_handler.cc +++ b/chromium/ui/compositor/overscroll/scroll_input_handler.cc @@ -65,8 +65,12 @@ bool ScrollInputHandler::OnScrollEvent(const ScrollEvent& event, // Note: the WHEEL type covers both actual wheels as well as trackpad // scrolling. - input_handler_weak_ptr_->ScrollBegin(&scroll_state_begin, - ui::ScrollInputType::kWheel); + cc::InputHandler::ScrollStatus result = input_handler_weak_ptr_->ScrollBegin( + &scroll_state_begin, ui::ScrollInputType::kWheel); + + // Falling back to the main thread should never be required when an explicit + // ElementId is provided. + DCHECK(!result.needs_main_thread_hit_test); cc::ScrollState scroll_state = CreateScrollState(event, false); input_handler_weak_ptr_->ScrollUpdate(&scroll_state, base::TimeDelta()); diff --git a/chromium/ui/compositor/paint_context.h b/chromium/ui/compositor/paint_context.h index 63e5b75b7d0..c2d3dcaa2af 100644 --- a/chromium/ui/compositor/paint_context.h +++ b/chromium/ui/compositor/paint_context.h @@ -7,7 +7,7 @@ #include <memory> -#include "base/logging.h" +#include "base/check.h" #include "base/macros.h" #include "cc/paint/paint_recorder.h" #include "ui/compositor/compositor_export.h" diff --git a/chromium/ui/compositor/throughput_tracker.cc b/chromium/ui/compositor/throughput_tracker.cc index 04e9b22c3e8..50e379fdc3f 100644 --- a/chromium/ui/compositor/throughput_tracker.cc +++ b/chromium/ui/compositor/throughput_tracker.cc @@ -11,8 +11,9 @@ namespace ui { -ThroughputTracker::ThroughputTracker(TrackerId id, ThroughputTrackerHost* host) - : id_(id), host_(host) { +ThroughputTracker::ThroughputTracker(TrackerId id, + base::WeakPtr<ThroughputTrackerHost> host) + : id_(id), host_(std::move(host)) { DCHECK(host_); } @@ -22,11 +23,11 @@ ThroughputTracker::ThroughputTracker(ThroughputTracker&& other) { ThroughputTracker& ThroughputTracker::operator=(ThroughputTracker&& other) { id_ = other.id_; - host_ = other.host_; + host_ = std::move(other.host_); started_ = other.started_; other.id_ = kInvalidId; - other.host_ = nullptr; + other.host_.reset(); other.started_ = false; return *this; } @@ -37,18 +38,28 @@ ThroughputTracker::~ThroughputTracker() { } void ThroughputTracker::Start(ThroughputTrackerHost::ReportCallback callback) { + // Start after |host_| destruction is likely an error. + DCHECK(host_); + DCHECK(!started_); + started_ = true; host_->StartThroughputTracker(id_, std::move(callback)); } void ThroughputTracker::Stop() { + DCHECK(started_); + started_ = false; - host_->StopThroughtputTracker(id_); + if (host_) + host_->StopThroughtputTracker(id_); } void ThroughputTracker::Cancel() { + DCHECK(started_); + started_ = false; - host_->CancelThroughtputTracker(id_); + if (host_) + host_->CancelThroughtputTracker(id_); } } // namespace ui diff --git a/chromium/ui/compositor/throughput_tracker.h b/chromium/ui/compositor/throughput_tracker.h index def502b4229..ae18af42b66 100644 --- a/chromium/ui/compositor/throughput_tracker.h +++ b/chromium/ui/compositor/throughput_tracker.h @@ -6,6 +6,7 @@ #define UI_COMPOSITOR_THROUGHPUT_TRACKER_H_ #include "base/callback_forward.h" +#include "base/memory/weak_ptr.h" #include "ui/compositor/compositor_export.h" #include "ui/compositor/throughput_tracker_host.h" @@ -47,11 +48,11 @@ class COMPOSITOR_EXPORT ThroughputTracker { // Private since it should only be created via Compositor's // RequestNewThroughputTracker call. - ThroughputTracker(TrackerId id, ThroughputTrackerHost* host); + ThroughputTracker(TrackerId id, base::WeakPtr<ThroughputTrackerHost> host); static const TrackerId kInvalidId = 0u; TrackerId id_ = kInvalidId; - ThroughputTrackerHost* host_ = nullptr; + base::WeakPtr<ThroughputTrackerHost> host_; bool started_ = false; }; |