Skip to content

GitLab

T
tangled
Commit 35bab68a authored by Daniel Murphy's avatar Daniel Murphy Committed by Commit Bot
Browse files
Options

Revert "Reland "Composite ScrollTimeline scrollSource when appropriate."" 

This reverts commit 48f503a8.

Reason for revert: Failing the Linux Trusty Leak bot

Original change's description:
> Reland "Composite ScrollTimeline scrollSource when appropriate."
> 
> This is a reland of ddd9f0ee
> 
> Original change's description:
> > Composite ScrollTimeline scrollSource when appropriate.
> >
> > This CL makes being the scrollSource of an active (i.e. the scrollSource is
> > scrollable) and attached (i.e. it is being used by at least one WorkletAnimation
> > as a timeline) ScrollTimeline a compositing reason. It does not change the
> > requirement that the scrollSource must be composited for the WorkletAnimation to
> > be sent to the compositor, nor does it change the case where a scrollSource stops
> > being composited whilst a WorkletAnimation is running. Those will be addressed in
> > follow-up CLs.
> >
> > Bug: 776533
> > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> > Change-Id: Icae43e1c9cfe2720cc739401c4766b592571f231
> > Reviewed-on: https://chromium-review.googlesource.com/1036934
> > Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
> > Reviewed-by: Majid Valipour <majidvp@chromium.org>
> > Reviewed-by: Philip Rogers <pdr@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#556541}
> 
> TBR=pdr@chromium.org
> 
> Bug: 776533
> Change-Id: I36cde2bb53e4c24fc57caef1e33f8dcac159ec77
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Reviewed-on: https://chromium-review.googlesource.com/1050547
> Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
> Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
> Reviewed-by: Majid Valipour <majidvp@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557347}

TBR=pdr@chromium.org,majidvp@chromium.org,smcgruer@chromium.org

Change-Id: Ia89e6d42ef89fceebb1630dcdcbe3b1af6c71964
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 776533, 841844
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/1054116Reviewed-by: default avatarDaniel Murphy <dmurph@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557583}
parent 0ae5186d
Hide whitespace changes
Inline Side-by-side
Showing with 5 additions and 178 deletions
third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-scroll-timeline-dispose.html deleted 100644 → 0
View file @ 0ae5186d
<!DOCTYPE html>
<title>Regression test to ensure detaching a ScrollTimeline shouldn't crash</title>
<script src="../../../../resources/testharness.js"></script>
<script src="../../../../resources/testharnessreport.js"></script>
<script src="resources/animation-worklet-tests.js"></script>
<style>
#scroller {
overflow: auto;
height: 100px;
width: 100px;
}
#contents {
height: 1000px;
width: 100%;
}
</style>
<div id="box"></div>
<div id="scroller">
<div id="contents"></div>
</div>
<script>
// This is a regression test for the crash seen when http://crrev.com/ddd9f0ee
// landed. The root of the crash was assuming a WeakMember from a static object
// would still be alive during a USING_PRE_FINALIZER method, which is not true.
async_test(t => {
const box = document.getElementById('box');
const effect = new KeyframeEffect(box, null);
let scroller = document.getElementById('scroller');
let timeline = new ScrollTimeline({
scrollSource: scroller,
timeRange: 1000,
orientation: 'block'
});
// Creating the animation will cause the scroller to be registered as being
// used in an attached ScrollTimeline.
let animation = new WorkletAnimation('test_animator', effect, timeline, {});
// Now free up everything at once.
scroller.remove();
animation.cancel();
scroller = undefined;
timeline = undefined;
animation = undefined;
requestAnimationFrame(t.step_func_done(() => {
// Force GC - this shouldn't crash.
gc();
}));
}, 'Disposing of an attached ScrollTimeline should not crash');
</script>
third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-scroll-timeline.html
View file @ 35bab68a
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
overflow: auto; overflow: auto;
height: 100px; height: 100px;
width: 100px; width: 100px;
will-change: transform; /* force compositing */
} }
#contents { #contents {
......
third_party/blink/renderer/core/BUILD.gn
View file @ 35bab68a
...@@ -1633,7 +1633,6 @@ jumbo_source_set("unit_tests") { ...@@ -1633,7 +1633,6 @@ jumbo_source_set("unit_tests") {
"animation/keyframe_effect_model_test.cc", "animation/keyframe_effect_model_test.cc",
"animation/keyframe_effect_test.cc", "animation/keyframe_effect_test.cc",
"animation/property_handle_test.cc", "animation/property_handle_test.cc",
"animation/scroll_timeline_test.cc",
"animation/timing_calculations_test.cc", "animation/timing_calculations_test.cc",
"animation/timing_input_test.cc", "animation/timing_input_test.cc",
"clipboard/data_object_test.cc", "clipboard/data_object_test.cc",
......
third_party/blink/renderer/core/animation/scroll_timeline.cc
View file @ 35bab68a
...@@ -6,18 +6,11 @@ ...@@ -6,18 +6,11 @@
#include "third_party/blink/renderer/core/dom/exception_code.h" #include "third_party/blink/renderer/core/dom/exception_code.h"
#include "third_party/blink/renderer/core/layout/layout_box.h" #include "third_party/blink/renderer/core/layout/layout_box.h"
#include "third_party/blink/renderer/core/layout/layout_view.h"
#include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h" #include "third_party/blink/renderer/core/paint/paint_layer_scrollable_area.h"
namespace blink { namespace blink {
namespace { namespace {
using ActiveScrollTimelineSet = PersistentHeapHashCountedSet<WeakMember<Node>>;
ActiveScrollTimelineSet& GetActiveScrollTimelineSet() {
DEFINE_STATIC_LOCAL(ActiveScrollTimelineSet, set, ());
return set;
}
bool StringToScrollDirection(String scroll_direction, bool StringToScrollDirection(String scroll_direction,
ScrollTimeline::ScrollDirection& result) { ScrollTimeline::ScrollDirection& result) {
// TODO(smcgruer): Support 'auto' value. // TODO(smcgruer): Support 'auto' value.
...@@ -53,17 +46,17 @@ ScrollTimeline* ScrollTimeline::Create(Document& document, ...@@ -53,17 +46,17 @@ ScrollTimeline* ScrollTimeline::Create(Document& document,
return nullptr; return nullptr;
} }
return new ScrollTimeline(scroll_source, orientation, return new ScrollTimeline(document, scroll_source, orientation,
options.timeRange().GetAsDouble()); options.timeRange().GetAsDouble());
} }
ScrollTimeline::ScrollTimeline(Element* scroll_source, ScrollTimeline::ScrollTimeline(const Document& document,
Element* scroll_source,
ScrollDirection orientation, ScrollDirection orientation,
double time_range) double time_range)
: scroll_source_(scroll_source), : scroll_source_(scroll_source),
orientation_(orientation), orientation_(orientation),
time_range_(time_range) { time_range_(time_range) {
DCHECK(scroll_source_);
} }
double ScrollTimeline::currentTime(bool& is_null) { double ScrollTimeline::currentTime(bool& is_null) {
...@@ -147,31 +140,9 @@ void ScrollTimeline::timeRange(DoubleOrScrollTimelineAutoKeyword& result) { ...@@ -147,31 +140,9 @@ void ScrollTimeline::timeRange(DoubleOrScrollTimelineAutoKeyword& result) {
result.SetDouble(time_range_); result.SetDouble(time_range_);
} }
void ScrollTimeline::AttachAnimation() {
GetActiveScrollTimelineSet().insert(scroll_source_);
scroll_source_->GetDocument()
.GetLayoutView()
->Compositor()
->SetNeedsCompositingUpdate(kCompositingUpdateRebuildTree);
}
void ScrollTimeline::DetachAnimation() {
GetActiveScrollTimelineSet().erase(scroll_source_);
scroll_source_->GetDocument()
.GetLayoutView()
->Compositor()
->SetNeedsCompositingUpdate(kCompositingUpdateRebuildTree);
}
void ScrollTimeline::Trace(blink::Visitor* visitor) { void ScrollTimeline::Trace(blink::Visitor* visitor) {
visitor->Trace(scroll_source_); visitor->Trace(scroll_source_);
AnimationTimeline::Trace(visitor); AnimationTimeline::Trace(visitor);
} }
bool ScrollTimeline::HasActiveScrollTimeline(Node* node) {
ActiveScrollTimelineSet& set = GetActiveScrollTimelineSet();
auto it = set.find(node);
return it != set.end() && it->value > 0;
}
} // namespace blink } // namespace blink
third_party/blink/renderer/core/animation/scroll_timeline.h
View file @ 35bab68a
...@@ -46,22 +46,10 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline { ...@@ -46,22 +46,10 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline {
ScrollDirection GetOrientation() const { return orientation_; } ScrollDirection GetOrientation() const { return orientation_; }
// Must be called when this ScrollTimeline is attached/unattached from an
// animation.
void AttachAnimation();
void DetachAnimation();
void Trace(blink::Visitor*) override; void Trace(blink::Visitor*) override;
// For the AnimationWorklet origin trial, we need to automatically composite
// elements that are targets of ScrollTimelines (http://crbug.com/776533). We
// expose a static lookup method to enable this.
//
// TODO(crbug.com/839341): Remove once WorkletAnimations can run on main.
static bool HasActiveScrollTimeline(Node* node);
private: private:
ScrollTimeline(Element*, ScrollDirection, double); ScrollTimeline(const Document&, Element*, ScrollDirection, double);
Member<Element> scroll_source_; Member<Element> scroll_source_;
ScrollDirection orientation_; ScrollDirection orientation_;
......
third_party/blink/renderer/core/animation/scroll_timeline_test.cc deleted 100644 → 0
View file @ 0ae5186d
// Copyright 2018 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 "third_party/blink/renderer/core/animation/scroll_timeline.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/renderer/bindings/core/v8/exception_state.h"
#include "third_party/blink/renderer/core/paint/paint_layer.h"
#include "third_party/blink/renderer/core/testing/core_unit_test_helper.h"
#include "third_party/blink/renderer/core/testing/dummy_page_holder.h"
namespace blink {
using ScrollTimelineTest = RenderingTest;
TEST_F(ScrollTimelineTest,
AttachingAndDetachingAnimationCausesCompositingUpdate) {
EnableCompositing();
SetBodyInnerHTML(R"HTML(
<style>#scroller { overflow: scroll; width: 100px; height: 100px; }</style>
<div id='scroller'></div>
)HTML");
LayoutBoxModelObject* scroller =
ToLayoutBoxModelObject(GetLayoutObjectByElementId("scroller"));
ASSERT_TRUE(scroller);
// Invariant: the scroller is not composited by default.
EXPECT_EQ(DocumentLifecycle::kPaintClean,
GetDocument().Lifecycle().GetState());
EXPECT_EQ(kNotComposited, scroller->Layer()->GetCompositingState());
// Create the ScrollTimeline. This shouldn't cause the scrollSource to need
// compositing, as it isn't attached to any animation yet.
ScrollTimelineOptions options;
DoubleOrScrollTimelineAutoKeyword time_range =
DoubleOrScrollTimelineAutoKeyword::FromDouble(100);
options.setTimeRange(time_range);
options.setScrollSource(GetElementById("scroller"));
ScrollTimeline* scroll_timeline =
ScrollTimeline::Create(GetDocument(), options, ASSERT_NO_EXCEPTION);
EXPECT_EQ(DocumentLifecycle::kPaintClean,
GetDocument().Lifecycle().GetState());
EXPECT_EQ(kNotComposited, scroller->Layer()->GetCompositingState());
// Now attach an animation. This should require a compositing update.
scroll_timeline->AttachAnimation();
UpdateAllLifecyclePhases();
EXPECT_NE(scroller->Layer()->GetCompositingState(), kNotComposited);
// Now detach an animation. This should again require a compositing update.
scroll_timeline->DetachAnimation();
UpdateAllLifecyclePhases();
EXPECT_EQ(scroller->Layer()->GetCompositingState(), kNotComposited);
}
} // namespace blink
third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.cc
View file @ 35bab68a
...@@ -4,7 +4,6 @@ ...@@ -4,7 +4,6 @@
#include "third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.h" #include "third_party/blink/renderer/core/paint/compositing/compositing_reason_finder.h"
#include "third_party/blink/renderer/core/animation/scroll_timeline.h"
#include "third_party/blink/renderer/core/css_property_names.h" #include "third_party/blink/renderer/core/css_property_names.h"
#include "third_party/blink/renderer/core/dom/document.h" #include "third_party/blink/renderer/core/dom/document.h"
#include "third_party/blink/renderer/core/frame/local_frame_view.h" #include "third_party/blink/renderer/core/frame/local_frame_view.h"
...@@ -178,14 +177,6 @@ CompositingReasons CompositingReasonFinder::NonStyleDeterminedDirectReasons( ...@@ -178,14 +177,6 @@ CompositingReasons CompositingReasonFinder::NonStyleDeterminedDirectReasons(
if (RequiresCompositingForScrollDependentPosition(layer, ignore_lcd_text)) if (RequiresCompositingForScrollDependentPosition(layer, ignore_lcd_text))
direct_reasons |= CompositingReason::kScrollDependentPosition; direct_reasons |= CompositingReason::kScrollDependentPosition;
// TODO(crbug.com/839341): Remove once we support main-thread AnimationWorklet
// and don't need to promote the scroll-source.
if (layer->GetScrollableArea() && layer->GetLayoutObject().GetNode() &&
ScrollTimeline::HasActiveScrollTimeline(
layer->GetLayoutObject().GetNode())) {
direct_reasons |= CompositingReason::kScrollTimelineTarget;
}
direct_reasons |= layout_object.AdditionalCompositingReasons(); direct_reasons |= layout_object.AdditionalCompositingReasons();
DCHECK( DCHECK(
......
third_party/blink/renderer/modules/animationworklet/worklet_animation.cc
View file @ 35bab68a
...@@ -231,9 +231,6 @@ WorkletAnimation::WorkletAnimation( ...@@ -231,9 +231,6 @@ WorkletAnimation::WorkletAnimation(
AnimationEffect* target_effect = effects_.at(0); AnimationEffect* target_effect = effects_.at(0);
target_effect->Attach(this); target_effect->Attach(this);
if (timeline_.IsScrollTimeline())
timeline.GetAsScrollTimeline()->AttachAnimation();
} }
String WorkletAnimation::playState() { String WorkletAnimation::playState() {
...@@ -434,8 +431,6 @@ KeyframeEffect* WorkletAnimation::GetEffect() const { ...@@ -434,8 +431,6 @@ KeyframeEffect* WorkletAnimation::GetEffect() const {
void WorkletAnimation::Dispose() { void WorkletAnimation::Dispose() {
DCHECK(IsMainThread()); DCHECK(IsMainThread());
if (timeline_.IsScrollTimeline())
timeline_.GetAsScrollTimeline()->DetachAnimation();
DestroyCompositorAnimation(); DestroyCompositorAnimation();
} }
......
third_party/blink/renderer/platform/graphics/compositing_reasons.h
View file @ 35bab68a
...@@ -35,7 +35,6 @@ using CompositingReasons = uint64_t; ...@@ -35,7 +35,6 @@ using CompositingReasons = uint64_t;
V(WillChangeCompositingHint) \ V(WillChangeCompositingHint) \
V(BackdropFilter) \ V(BackdropFilter) \
V(RootScroller) \ V(RootScroller) \
V(ScrollTimelineTarget) \
\ \
/* Overlap reasons that require knowing what's behind you in paint-order \ /* Overlap reasons that require knowing what's behind you in paint-order \
before knowing the answer. */ \ before knowing the answer. */ \
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment