Use base computed style optimization with DCHECKs enabled.

We used to skip the whole base computed style optimization and just
compare the new style with the old stored base, but used the newly
computed style always when DCHECK was enabled.

This CL always computes the computed style without the base, but then,
after verifying it matches the base, use the base computed style.

Bug: 669790
Change-Id: Ifc82de4644fae104d69d8abf91261ba1cfe52f73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2074399Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745043}

third_party/blink/renderer/core/animation/element_animations.cc

@@ -53,29 +53,6 @@ void UpdateAnimationFlagsForEffect(const KeyframeEffect& effect,
     style.SetHasCurrentBackdropFilterAnimation(true);
 }
 
-#if DCHECK_IS_ON()
-// Under certain conditions ComputedStyle::operator==() may return false for
-// differences that are permitted during an animation.
-bool ShouldCheckComputedStyles(const ComputedStyle& base_computed_style,
-                               const ComputedStyle& computed_style) {
-  // The FontFaceCache version number may be increased without forcing a style
-  // recalc (see crbug.com/471079).
-  if (!base_computed_style.GetFont().IsFallbackValid())
-    return false;
-  // Images use instance equality rather than value equality (see
-  // crbug.com/781461).
-  for (CSSPropertyID id :
-       {CSSPropertyID::kBackgroundImage, CSSPropertyID::kWebkitMaskImage}) {
-    if (!CSSPropertyEquality::PropertiesEqual(
-            PropertyHandle(CSSProperty::Get(id)), base_computed_style,
-            computed_style)) {
-      return false;
-    }
-  }
-  return true;
-}
-#endif  // DCHECK_IS_ON()
-
 }  // namespace
 
 ElementAnimations::ElementAnimations() : animation_style_change_(false) {}
@@ -137,13 +114,8 @@ void ElementAnimations::Trace(Visitor* visitor) {
 }
 
 const ComputedStyle* ElementAnimations::BaseComputedStyle() const {
-// When DCHECK is on we lie and claim to never have a base computed style
-// stored. This allows us to check that an invariant holds; see the comments in
-// |UpdateBaseComputedStyle|.
-#if !DCHECK_IS_ON()
   if (IsAnimationStyleChange())
     return base_computed_style_.get();
-#endif
   return nullptr;
 }
 
@@ -154,18 +126,6 @@ void ElementAnimations::UpdateBaseComputedStyle(
     base_computed_style_ = nullptr;
     return;
   }
-#if DCHECK_IS_ON()
-  // The invariant in the base computed style optimization is that as long as
-  // |IsAnimationStyleChange| is true, the computed style that would be
-  // generated by the style resolver is equivalent to the one we hold
-  // internally. To ensure this we disable the optimization when DCHECKs are
-  // enabled, but keep the internal base computed style and make sure the
-  // equivalency holds here.
-  if (base_computed_style_ && computed_style &&
-      ShouldCheckComputedStyles(*base_computed_style_, *computed_style)) {
-    DCHECK(*base_computed_style_ == *computed_style);
-  }
-#endif
   base_computed_style_ = ComputedStyle::Clone(*computed_style);
 }
 

third_party/blink/renderer/core/css/resolver/style_resolver.cc

@@ -124,6 +124,48 @@ bool HasAnimationsOrTransitions(const StyleResolverState& state) {
           state.GetAnimatingElement()->HasAnimations());
 }
 
+bool ShouldComputeBaseComputedStyle(const ComputedStyle* base_computed_style) {
+#if DCHECK_IS_ON()
+  // The invariant in the base computed style optimization is that as long as
+  // |IsAnimationStyleChange| is true, the computed style that would be
+  // generated by the style resolver is equivalent to the one we hold
+  // internally. To ensure this, we always compute a new style here disregarding
+  // the fact that we have a base computed style when DCHECKs are enabled, and
+  // call ValidateBaseComputedStyle() to check that the optimization was sound.
+  return true;
+#endif  // !DCHECK_IS_ON()
+  return !base_computed_style;
+}
+
+// Compare the base computed style with the one we compute to validate that the
+// optimization is sound.
+bool ValidateBaseComputedStyle(const ComputedStyle* base_computed_style,
+                               const ComputedStyle& computed_style) {
+#if DCHECK_IS_ON()
+  if (!base_computed_style)
+    return true;
+  // Under certain conditions ComputedStyle::operator==() may return false for
+  // differences that are permitted during an animation.
+  // The FontFaceCache version number may be increased without forcing a style
+  // recalc (see crbug.com/471079).
+  if (!base_computed_style->GetFont().IsFallbackValid())
+    return true;
+  // Images use instance equality rather than value equality (see
+  // crbug.com/781461).
+  for (CSSPropertyID id :
+       {CSSPropertyID::kBackgroundImage, CSSPropertyID::kWebkitMaskImage}) {
+    if (!CSSPropertyEquality::PropertiesEqual(
+            PropertyHandle(CSSProperty::Get(id)), *base_computed_style,
+            computed_style)) {
+      return true;
+    }
+  }
+  return *base_computed_style == computed_style;
+#else
+  return true;
+#endif  // DCHECK_IS_ON()
+}
+
 }  // namespace
 
 static CSSPropertyValueSet* LeftToRightDeclaration() {
@@ -794,72 +836,67 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement(
   return state.TakeStyle();
 }
 
-void StyleResolver::ApplyBaseComputedStyle(
-    Element* element,
-    StyleResolverState& state,
-    RuleMatchingBehavior matching_behavior,
-    bool can_cache_animation_base_computed_style) {
-  const ComputedStyle* animation_base_computed_style =
-      can_cache_animation_base_computed_style
-          ? CachedAnimationBaseComputedStyle(state)
-          : nullptr;
+void StyleResolver::InitStyleAndApplyInheritance(Element& element,
+                                                 StyleResolverState& state) {
+  if (state.ParentStyle()) {
+    scoped_refptr<ComputedStyle> style = ComputedStyle::Create();
+    style->InheritFrom(*state.ParentStyle(),
+                       IsAtShadowBoundary(&element)
+                           ? ComputedStyle::kAtShadowBoundary
+                           : ComputedStyle::kNotAtShadowBoundary);
+    state.SetStyle(std::move(style));
 
-  if (animation_base_computed_style) {
-    state.SetStyle(ComputedStyle::Clone(*animation_base_computed_style));
-    if (!state.ParentStyle()) {
-      state.SetParentStyle(InitialStyleForElement(GetDocument()));
-      state.SetLayoutParentStyle(state.ParentStyle());
+    // contenteditable attribute (implemented by -webkit-user-modify) should
+    // be propagated from shadow host to distributed node.
+    if (state.DistributedToV0InsertionPoint() || element.AssignedSlot()) {
+      Element* parent = element.parentElement();
+      DCHECK(parent);
+      if (const ComputedStyle* shadow_host_style = parent->GetComputedStyle())
+        state.Style()->SetUserModify(shadow_host_style->UserModify());
     }
   } else {
-    if (state.ParentStyle()) {
-      scoped_refptr<ComputedStyle> style = ComputedStyle::Create();
-      style->InheritFrom(*state.ParentStyle(),
-                         IsAtShadowBoundary(element)
-                             ? ComputedStyle::kAtShadowBoundary
-                             : ComputedStyle::kNotAtShadowBoundary);
-      state.SetStyle(std::move(style));
-    } else {
-      state.SetStyle(InitialStyleForElement(GetDocument()));
-      state.SetParentStyle(ComputedStyle::Clone(*state.Style()));
-      state.SetLayoutParentStyle(state.ParentStyle());
-      if (element != GetDocument().documentElement()) {
-        // Strictly, we should only allow the root element to inherit from
-        // initial styles, but we allow getComputedStyle() for connected
-        // elements outside the flat tree rooted at an unassigned shadow host
-        // child, or Shadow DOM V0 insertion points.
-        DCHECK(element->IsV0InsertionPoint() ||
-               (IsShadowHost(element->parentNode()) &&
-                !LayoutTreeBuilderTraversal::ParentElement(*element)));
-        state.Style()->SetIsEnsuredOutsideFlatTree();
-      }
-    }
-  }
-
-  // contenteditable attribute (implemented by -webkit-user-modify) should
-  // be propagated from shadow host to distributed node.
-  if (state.DistributedToV0InsertionPoint() || element->AssignedSlot()) {
-    if (Element* parent = element->parentElement()) {
-      if (const ComputedStyle* style_of_shadow_host =
-              parent->GetComputedStyle()) {
-        state.Style()->SetUserModify(style_of_shadow_host->UserModify());
-      }
+    state.SetStyle(InitialStyleForElement(GetDocument()));
+    state.SetParentStyle(ComputedStyle::Clone(*state.Style()));
+    state.SetLayoutParentStyle(state.ParentStyle());
+    if (element != GetDocument().documentElement()) {
+      // Strictly, we should only allow the root element to inherit from
+      // initial styles, but we allow getComputedStyle() for connected
+      // elements outside the flat tree rooted at an unassigned shadow host
+      // child, or Shadow DOM V0 insertion points.
+      DCHECK(element.IsV0InsertionPoint() ||
+             (IsShadowHost(element.parentNode()) &&
+              !LayoutTreeBuilderTraversal::ParentElement(element)));
+      state.Style()->SetIsEnsuredOutsideFlatTree();
     }
   }
 
-  if (element->IsLink()) {
+  if (element.IsLink()) {
     state.Style()->SetIsLink();
     EInsideLink link_state = state.ElementLinkState();
     if (link_state != EInsideLink::kNotInsideLink) {
       bool force_visited = false;
-      probe::ForcePseudoState(element, CSSSelector::kPseudoVisited,
+      probe::ForcePseudoState(&element, CSSSelector::kPseudoVisited,
                               &force_visited);
       if (force_visited)
         link_state = EInsideLink::kInsideVisitedLink;
     }
     state.Style()->SetInsideLink(link_state);
   }
+}
+
+void StyleResolver::ApplyBaseComputedStyle(
+    Element* element,
+    StyleResolverState& state,
+    RuleMatchingBehavior matching_behavior,
+    bool can_cache_animation_base_computed_style) {
+  const ComputedStyle* animation_base_computed_style =
+      can_cache_animation_base_computed_style
+          ? CachedAnimationBaseComputedStyle(state)
+          : nullptr;
+
+  if (ShouldComputeBaseComputedStyle(animation_base_computed_style)) {
+    InitStyleAndApplyInheritance(*element, state);
 
-  if (!animation_base_computed_style) {
     GetDocument().GetStyleEngine().EnsureUAStyleForElement(*element);
 
     ElementRuleCollector collector(state.ElementContext(), selector_filter_,
@@ -910,9 +947,20 @@ void StyleResolver::ApplyBaseComputedStyle(
 
     StyleAdjuster::AdjustComputedStyle(state, element);
 
-    if (can_cache_animation_base_computed_style)
-      UpdateAnimationBaseComputedStyle(state);
-  } else {
+    if (can_cache_animation_base_computed_style) {
+      DCHECK(ValidateBaseComputedStyle(animation_base_computed_style,
+                                       *state.Style()));
+      if (!animation_base_computed_style)
+        UpdateAnimationBaseComputedStyle(state);
+    }
+  }
+
+  if (animation_base_computed_style) {
+    state.SetStyle(ComputedStyle::Clone(*animation_base_computed_style));
+    if (!state.ParentStyle()) {
+      state.SetParentStyle(InitialStyleForElement(GetDocument()));
+      state.SetLayoutParentStyle(state.ParentStyle());
+    }
     INCREMENT_STYLE_STATS_COUNTER(GetDocument().GetStyleEngine(),
                                   base_styles_used, 1);
   }
@@ -965,27 +1013,24 @@ bool StyleResolver::PseudoStyleForElementInternal(
   const ComputedStyle* animation_base_computed_style =
       CachedAnimationBaseComputedStyle(state);
 
-  if (animation_base_computed_style) {
-    state.SetStyle(ComputedStyle::Clone(*animation_base_computed_style));
-  } else if (pseudo_style_request.AllowsInheritance(state.ParentStyle())) {
-    scoped_refptr<ComputedStyle> style = ComputedStyle::Create();
-    style->InheritFrom(*state.ParentStyle());
-    state.SetStyle(std::move(style));
-  } else {
-    // ::backdrop inherits from initial styles. All other pseudo elements
-    // inherit from their originating element (::before/::after), or originating
-    // element descendants (::first-line/::first-letter).
-    DCHECK(pseudo_style_request.pseudo_id == kPseudoIdBackdrop);
-    state.SetStyle(InitialStyleForElement(GetDocument()));
-    state.SetParentStyle(ComputedStyle::Clone(*state.Style()));
-  }
-
-  state.Style()->SetStyleType(pseudo_style_request.pseudo_id);
-
   // Since we don't use pseudo-elements in any of our quirk/print
   // user agent rules, don't waste time walking those rules.
 
-  if (!animation_base_computed_style) {
+  if (ShouldComputeBaseComputedStyle(animation_base_computed_style)) {
+    if (pseudo_style_request.AllowsInheritance(state.ParentStyle())) {
+      scoped_refptr<ComputedStyle> style = ComputedStyle::Create();
+      style->InheritFrom(*state.ParentStyle());
+      state.SetStyle(std::move(style));
+    } else {
+      // ::backdrop inherits from initial styles. All other pseudo elements
+      // inherit from their originating element (::before/::after), or
+      // originating element descendants (::first-line/::first-letter).
+      DCHECK(pseudo_style_request.pseudo_id == kPseudoIdBackdrop);
+      state.SetStyle(InitialStyleForElement(GetDocument()));
+      state.SetParentStyle(ComputedStyle::Clone(*state.Style()));
+    }
+    state.Style()->SetStyleType(pseudo_style_request.pseudo_id);
+
     // Check UA, user and author rules.
     ElementRuleCollector collector(state.ElementContext(), selector_filter_,
                                    state.Style(), state.Style()->InsideLink());
@@ -1034,7 +1079,16 @@ bool StyleResolver::PseudoStyleForElementInternal(
     // in the StyleAdjuster::AdjustComputedStyle code.
     StyleAdjuster::AdjustComputedStyle(state, nullptr);
 
-    UpdateAnimationBaseComputedStyle(state);
+    DCHECK(ValidateBaseComputedStyle(animation_base_computed_style,
+                                     *state.Style()));
+
+    if (!animation_base_computed_style)
+      UpdateAnimationBaseComputedStyle(state);
+  }
+
+  if (animation_base_computed_style) {
+    state.SetStyle(ComputedStyle::Clone(*animation_base_computed_style));
+    state.Style()->SetStyleType(pseudo_style_request.pseudo_id);
   }
 
   // FIXME: The CSSWG wants to specify that the effects of animations are

third_party/blink/renderer/core/css/resolver/style_resolver.h

@@ -139,6 +139,8 @@ class CORE_EXPORT StyleResolver final : public GarbageCollected<StyleResolver> {
   void Trace(Visitor*);
 
  private:
+  void InitStyleAndApplyInheritance(Element& element,
+                                    StyleResolverState& state);
   void ApplyBaseComputedStyle(Element* element,
                               StyleResolverState& state,
                               RuleMatchingBehavior matching_behavior,

third_party/blink/renderer/core/css/resolver/style_resolver_test.cc

@@ -52,12 +52,8 @@ TEST_F(StyleResolverTest, AnimationBaseComputedStyle) {
 
   ASSERT_TRUE(resolver->StyleForElement(div));
   EXPECT_EQ(20, resolver->StyleForElement(div)->FontSize());
-#if DCHECK_IS_ON()
-  EXPECT_FALSE(animations.BaseComputedStyle());
-#else
   ASSERT_TRUE(animations.BaseComputedStyle());
   EXPECT_EQ(20, animations.BaseComputedStyle()->FontSize());
-#endif
 
   // Getting style with customized parent style should not affect cached
   // animation base computed style.
@@ -66,12 +62,8 @@ TEST_F(StyleResolverTest, AnimationBaseComputedStyle) {
   EXPECT_EQ(
       10,
       resolver->StyleForElement(div, parent_style, parent_style)->FontSize());
-#if DCHECK_IS_ON()
-  EXPECT_FALSE(animations.BaseComputedStyle());
-#else
   ASSERT_TRUE(animations.BaseComputedStyle());
   EXPECT_EQ(20, animations.BaseComputedStyle()->FontSize());
-#endif
   EXPECT_EQ(20, resolver->StyleForElement(div)->FontSize());
 }