Commit cbe520c0 authored by Xianzhu Wang's avatar Xianzhu Wang Committed by Commit Bot

Reland "Cache animation base computed style only on default StyleForElement()"

This reverts commit 0bc5ba70.

Reason for revert: Not the cause of crbug.com/996037.

Original change's description:
> Revert "Cache animation base computed style only on default StyleForElement()"
> 
> This reverts commit a1fe850a.
> 
> Reason for revert: speculative revert for crbug.com/996037.
> 
> Original change's description:
> > Cache animation base computed style only on default StyleForElement()
> > 
> > StyleForElement() can be used to get the default style (i.e. the
> > style that will be applied on the element), or style derived from
> > custom parent style for e.g. inherited first line style. We should
> > cache animation base computed style in the first case only.
> > 
> > Bug: 989490
> > Change-Id: Ia30b3e7bcd65cdb9456eb925e5c24f156353bff9
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1753030
> > Reviewed-by: Rune Lillesveen <futhark@chromium.org>
> > Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#686915}
> 
> TBR=wangxianzhu@chromium.org,futhark@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 989490,996037
> Change-Id: Ib12e4e55e70eaeb2c82ca173c145c225c1d79802
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1763343
> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#689165}

TBR=wangxianzhu@chromium.org,futhark@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 989490, 996037
Change-Id: I32dc5509075cfae90721dadc49144a309b40b901
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1776380Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691678}
parent 78aa43b2
...@@ -670,7 +670,7 @@ void StyleResolver::LoadPendingResources(StyleResolverState& state) { ...@@ -670,7 +670,7 @@ void StyleResolver::LoadPendingResources(StyleResolverState& state) {
state.GetElementStyleResources().LoadPendingResources(state.Style()); state.GetElementStyleResources().LoadPendingResources(state.Style());
} }
static const ComputedStyle* CalculateBaseComputedStyle( static const ComputedStyle* CachedAnimationBaseComputedStyle(
StyleResolverState& state) { StyleResolverState& state) {
if (!state.GetAnimatingElement()) if (!state.GetAnimatingElement())
return nullptr; return nullptr;
...@@ -691,7 +691,7 @@ static const ComputedStyle* CalculateBaseComputedStyle( ...@@ -691,7 +691,7 @@ static const ComputedStyle* CalculateBaseComputedStyle(
return element_animations->BaseComputedStyle(); return element_animations->BaseComputedStyle();
} }
static void UpdateBaseComputedStyle(StyleResolverState& state) { static void UpdateAnimationBaseComputedStyle(StyleResolverState& state) {
if (!state.GetAnimatingElement()) if (!state.GetAnimatingElement())
return; return;
...@@ -723,10 +723,19 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement( ...@@ -723,10 +723,19 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement(
StyleResolverState state(GetDocument(), *element, default_parent, StyleResolverState state(GetDocument(), *element, default_parent,
default_layout_parent); default_layout_parent);
const ComputedStyle* base_computed_style = CalculateBaseComputedStyle(state); // This function can return different results with different last 3 params,
// but we can only cache one base computed style for animations, thus we cache
if (base_computed_style) { // only when this function is called with default last 3 params.
state.SetStyle(ComputedStyle::Clone(*base_computed_style)); bool can_cache_animation_base_computed_style =
!default_parent && !default_layout_parent &&
matching_behavior == kMatchAllRules;
const ComputedStyle* animation_base_computed_style =
can_cache_animation_base_computed_style
? CachedAnimationBaseComputedStyle(state)
: nullptr;
if (animation_base_computed_style) {
state.SetStyle(ComputedStyle::Clone(*animation_base_computed_style));
if (!state.ParentStyle()) { if (!state.ParentStyle()) {
state.SetParentStyle(InitialStyleForElement(GetDocument())); state.SetParentStyle(InitialStyleForElement(GetDocument()));
state.SetLayoutParentStyle(state.ParentStyle()); state.SetLayoutParentStyle(state.ParentStyle());
...@@ -770,7 +779,7 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement( ...@@ -770,7 +779,7 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement(
state.Style()->SetInsideLink(link_state); state.Style()->SetInsideLink(link_state);
} }
if (!base_computed_style) { if (!animation_base_computed_style) {
GetDocument().GetStyleEngine().EnsureUAStyleForElement(*element); GetDocument().GetStyleEngine().EnsureUAStyleForElement(*element);
ElementRuleCollector collector(state.ElementContext(), selector_filter_, ElementRuleCollector collector(state.ElementContext(), selector_filter_,
...@@ -821,7 +830,8 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement( ...@@ -821,7 +830,8 @@ scoped_refptr<ComputedStyle> StyleResolver::StyleForElement(
StyleAdjuster::AdjustComputedStyle(state, element); StyleAdjuster::AdjustComputedStyle(state, element);
UpdateBaseComputedStyle(state); if (can_cache_animation_base_computed_style)
UpdateAnimationBaseComputedStyle(state);
} else { } else {
INCREMENT_STYLE_STATS_COUNTER(GetDocument().GetStyleEngine(), INCREMENT_STYLE_STATS_COUNTER(GetDocument().GetStyleEngine(),
base_styles_used, 1); base_styles_used, 1);
...@@ -891,10 +901,11 @@ bool StyleResolver::PseudoStyleForElementInternal( ...@@ -891,10 +901,11 @@ bool StyleResolver::PseudoStyleForElementInternal(
SelectorFilterParentScope::EnsureParentStackIsPushed(); SelectorFilterParentScope::EnsureParentStackIsPushed();
const ComputedStyle* base_computed_style = CalculateBaseComputedStyle(state); const ComputedStyle* animation_base_computed_style =
CachedAnimationBaseComputedStyle(state);
if (base_computed_style) { if (animation_base_computed_style) {
state.SetStyle(ComputedStyle::Clone(*base_computed_style)); state.SetStyle(ComputedStyle::Clone(*animation_base_computed_style));
} else if (pseudo_style_request.AllowsInheritance(state.ParentStyle())) { } else if (pseudo_style_request.AllowsInheritance(state.ParentStyle())) {
scoped_refptr<ComputedStyle> style = ComputedStyle::Create(); scoped_refptr<ComputedStyle> style = ComputedStyle::Create();
style->InheritFrom(*state.ParentStyle()); style->InheritFrom(*state.ParentStyle());
...@@ -909,7 +920,7 @@ bool StyleResolver::PseudoStyleForElementInternal( ...@@ -909,7 +920,7 @@ bool StyleResolver::PseudoStyleForElementInternal(
// Since we don't use pseudo-elements in any of our quirk/print // Since we don't use pseudo-elements in any of our quirk/print
// user agent rules, don't waste time walking those rules. // user agent rules, don't waste time walking those rules.
if (!base_computed_style) { if (!animation_base_computed_style) {
// Check UA, user and author rules. // Check UA, user and author rules.
ElementRuleCollector collector(state.ElementContext(), selector_filter_, ElementRuleCollector collector(state.ElementContext(), selector_filter_,
state.Style()); state.Style());
...@@ -936,7 +947,7 @@ bool StyleResolver::PseudoStyleForElementInternal( ...@@ -936,7 +947,7 @@ bool StyleResolver::PseudoStyleForElementInternal(
// in the StyleAdjuster::AdjustComputedStyle code. // in the StyleAdjuster::AdjustComputedStyle code.
StyleAdjuster::AdjustComputedStyle(state, nullptr); StyleAdjuster::AdjustComputedStyle(state, nullptr);
UpdateBaseComputedStyle(state); UpdateAnimationBaseComputedStyle(state);
} }
// FIXME: The CSSWG wants to specify that the effects of animations are // FIXME: The CSSWG wants to specify that the effects of animations are
......
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
#include "third_party/blink/renderer/core/css/resolver/style_resolver.h" #include "third_party/blink/renderer/core/css/resolver/style_resolver.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/renderer/core/animation/element_animations.h"
#include "third_party/blink/renderer/core/css/style_engine.h" #include "third_party/blink/renderer/core/css/style_engine.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/dom/node_computed_style.h" #include "third_party/blink/renderer/core/dom/node_computed_style.h"
...@@ -33,4 +34,45 @@ TEST_F(StyleResolverTest, StyleForTextInDisplayNone) { ...@@ -33,4 +34,45 @@ TEST_F(StyleResolverTest, StyleForTextInDisplayNone) {
To<Text>(GetDocument().body()->firstChild()))); To<Text>(GetDocument().body()->firstChild())));
} }
TEST_F(StyleResolverTest, AnimationBaseComputedStyle) {
GetDocument().documentElement()->SetInnerHTMLFromString(R"HTML(
<style>
html { font-size: 10px; }
body { font-size: 20px; }
</style>
<div id="div">Test</div>
)HTML");
UpdateAllLifecyclePhasesForTest();
Element* div = GetDocument().getElementById("div");
StyleResolver* resolver = GetStyleEngine().Resolver();
ASSERT_TRUE(resolver);
ElementAnimations& animations = div->EnsureElementAnimations();
animations.SetAnimationStyleChange(true);
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.
const ComputedStyle* parent_style =
GetDocument().documentElement()->GetComputedStyle();
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());
}
} // namespace blink } // namespace blink
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