Commit 8a8e7309 authored by Stephen McGruer's avatar Stephen McGruer Committed by Commit Bot

Ensure FragmentData::UniqueId is only called when an object will have an id

FragmentData::UniqueId is not intended to be relied upon as an
indicator. Instead, any call to the method should only happen if you
know there should be a UniqueId, and failures should be handled by
figuring out when a UniqueId should have been created (by paint
properties or otherwise). This CL adds a DCHECK to enforce this, and
fixes up two locations that were trying to rely on
FragmentData::UniqueId to check state.

Bug: None
Change-Id: I168ad2e91a3418e98d351df8fa3ad879ab2e3427
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730859Reviewed-by: default avatarPhilip Rogers <pdr@chromium.org>
Reviewed-by: default avatarRobert Flack <flackr@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#684361}
parent d10e5d85
...@@ -197,13 +197,6 @@ CompositorAnimations::CheckCanStartEffectOnCompositor( ...@@ -197,13 +197,6 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
LayoutObject* layout_object = target_element.GetLayoutObject(); LayoutObject* layout_object = target_element.GetLayoutObject();
if (paint_artifact_compositor) { if (paint_artifact_compositor) {
// If we are going to check that we can animate these below, we need
// to have the UniqueID to compute the target ID. Let's check it
// once in common in advance.
if (!layout_object || !layout_object->UniqueId()) {
reasons |= kTargetHasInvalidCompositingState;
}
// Elements with subtrees containing will-change: contents are not // Elements with subtrees containing will-change: contents are not
// composited for animations as if the contents change the tiles // composited for animations as if the contents change the tiles
// would need to be rerastered anyways. // would need to be rerastered anyways.
...@@ -308,8 +301,14 @@ CompositorAnimations::CheckCanStartEffectOnCompositor( ...@@ -308,8 +301,14 @@ CompositorAnimations::CheckCanStartEffectOnCompositor(
} }
if (paint_artifact_compositor) { if (paint_artifact_compositor) {
if (!layout_object || !layout_object->UniqueId()) // If we don't have paint properties, we won't have a UniqueId to use
// for checking here.
if (!target_element.GetLayoutObject() ||
!target_element.GetLayoutObject()
->FirstFragment()
.PaintProperties()) {
continue; continue;
}
CompositorElementId target_element_id = CompositorElementId target_element_id =
CompositorElementIdFromUniqueObjectId( CompositorElementIdFromUniqueObjectId(
......
...@@ -57,8 +57,10 @@ std::unique_ptr<CompositorScrollTimeline> ToCompositorScrollTimeline( ...@@ -57,8 +57,10 @@ std::unique_ptr<CompositorScrollTimeline> ToCompositorScrollTimeline(
base::Optional<CompositorElementId> GetCompositorScrollElementId( base::Optional<CompositorElementId> GetCompositorScrollElementId(
const Node* node) { const Node* node) {
if (!node || !node->GetLayoutObject() || !node->GetLayoutObject()->UniqueId()) if (!node || !node->GetLayoutObject() ||
!node->GetLayoutObject()->FirstFragment().PaintProperties()) {
return base::nullopt; return base::nullopt;
}
return CompositorElementIdFromUniqueObjectId( return CompositorElementIdFromUniqueObjectId(
node->GetLayoutObject()->UniqueId(), node->GetLayoutObject()->UniqueId(),
CompositorElementIdNamespace::kScroll); CompositorElementIdNamespace::kScroll);
......
...@@ -213,7 +213,6 @@ TEST_F(ScrollTimelineUtilTest, GetCompositorScrollElementIdNoUniqueId) { ...@@ -213,7 +213,6 @@ TEST_F(ScrollTimelineUtilTest, GetCompositorScrollElementIdNoUniqueId) {
SetBodyInnerHTML("<div id='test'></div>"); SetBodyInnerHTML("<div id='test'></div>");
Element* test = GetElementById("test"); Element* test = GetElementById("test");
ASSERT_TRUE(test->GetLayoutObject()); ASSERT_TRUE(test->GetLayoutObject());
ASSERT_FALSE(test->GetLayoutObject()->UniqueId());
EXPECT_EQ(GetCompositorScrollElementId(test), base::nullopt); EXPECT_EQ(GetCompositorScrollElementId(test), base::nullopt);
} }
......
...@@ -48,7 +48,8 @@ class CORE_EXPORT FragmentData { ...@@ -48,7 +48,8 @@ class CORE_EXPORT FragmentData {
// An id for this object that is unique for the lifetime of the WebView. // An id for this object that is unique for the lifetime of the WebView.
UniqueObjectId UniqueId() const { UniqueObjectId UniqueId() const {
return rare_data_ ? rare_data_->unique_id : 0; DCHECK(rare_data_);
return rare_data_->unique_id;
} }
// The PaintLayer associated with this LayoutBoxModelObject. This can be null // The PaintLayer associated with this LayoutBoxModelObject. This can be null
......
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