Commit 8a3ab164 authored by Anders Hartvoll Ruud's avatar Anders Hartvoll Ruud Committed by Commit Bot

[mpc] Set ChildHasExplicitInheritance also for cache hits

When a style resolve is a cache hit, we don't go through the normal
process of applying declarations via StyleBuilder::ApplyProperty.
We then also don't set the ChildHasExplicitInheritance flag on the
parent style for cache hits. This wasn't a problem a before, since
any style with a parent with this flag set could not enter the MPC.

However, with MPCDependencies enabled, such styles can now enter the
cache, and cache hits for those entries will then fail to set the flag
on the parent style.

To fix this, this CL adds a new flag HasExplicitInheritance, which
pertains to the _current_ ComputedStyle being computed (not the
parent). This way we can get the information we need into the MPC,
and perform the necessary propagation to the parent style when we
need to.

TEST=external/wpt/css/css-transitions/inherit-height-transition.html

Bug: 1057072
Change-Id: Ic03cdc6377ab5e58d6fe08a6fef2bd7505064f16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2228535
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: default avatarXiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775263}
parent 34f1b63d
......@@ -100,6 +100,7 @@ void StyleBuilder::ApplyProperty(const CSSProperty& property,
if (is_inherit && !is_inherited) {
state.MarkDependency(property);
state.Style()->SetHasExplicitInheritance();
state.ParentStyle()->SetChildHasExplicitInheritance();
} else if (value.IsUnsetValue()) {
DCHECK(!is_inherit && !is_initial);
......
......@@ -78,4 +78,22 @@ TEST_F(StyleBuilderTest, TextOrientationChangeDirtiesFont) {
}
}
TEST_F(StyleBuilderTest, HasExplicitInheritance) {
auto parent_style = ComputedStyle::Create();
auto style = ComputedStyle::Create();
StyleResolverState state(GetDocument(), *GetDocument().body(),
parent_style.get(), parent_style.get());
state.SetStyle(style);
EXPECT_FALSE(style->HasExplicitInheritance());
// Flag should not be set for properties which are inherited.
StyleBuilder::ApplyProperty(GetCSSPropertyColor(), state,
*CSSInheritedValue::Create());
EXPECT_FALSE(style->HasExplicitInheritance());
StyleBuilder::ApplyProperty(GetCSSPropertyBackgroundColor(), state,
*CSSInheritedValue::Create());
EXPECT_TRUE(style->HasExplicitInheritance());
}
} // namespace blink
......@@ -1903,6 +1903,10 @@ StyleResolver::CacheSuccess StyleResolver::ApplyMatchedCache(
if (!IsForcedColorsModeEnabled() || is_inherited_cache_hit) {
state.Style()->CopyNonInheritedFromCached(
*cached_matched_properties->computed_style);
// If the child style is a cache hit, we'll never reach StyleBuilder::
// ApplyProperty, hence we'll never set the flag on the parent.
if (state.Style()->HasExplicitInheritance())
state.ParentStyle()->SetChildHasExplicitInheritance();
is_non_inherited_cache_hit = true;
}
UpdateFont(state);
......
......@@ -12,6 +12,7 @@
#include "third_party/blink/renderer/core/css/css_value_list.h"
#include "third_party/blink/renderer/core/css/properties/computed_style_utils.h"
#include "third_party/blink/renderer/core/css/properties/css_property_ref.h"
#include "third_party/blink/renderer/core/css/style_change_reason.h"
#include "third_party/blink/renderer/core/css/style_engine.h"
#include "third_party/blink/renderer/core/css/style_sheet_contents.h"
#include "third_party/blink/renderer/core/dom/document.h"
......@@ -260,6 +261,34 @@ TEST_F(StyleResolverTest, AnimationMaskedByImportant) {
EXPECT_FALSE(div->GetElementAnimations()->BaseImportantSet());
}
TEST_F(StyleResolverTest, CachedExplicitInheritanceFlags) {
ScopedMPCDependenciesForTest scoped_feature(true);
GetDocument().documentElement()->setInnerHTML(R"HTML(
<style>
#outer { height: 10px; }
#inner { height: inherit; }
</style>
<div id=outer>
<div id=inner></div>
</div>
)HTML");
UpdateAllLifecyclePhasesForTest();
Element* outer = GetDocument().getElementById("outer");
ASSERT_TRUE(outer);
EXPECT_TRUE(outer->ComputedStyleRef().ChildHasExplicitInheritance());
auto recalc_reason = StyleChangeReasonForTracing::Create("test");
// This will hit the MatchedPropertiesCache for both #outer/#inner,
// which means special care must be taken for the ChildHasExplicit-
// Inheritance flag to persist.
GetStyleEngine().MarkAllElementsForStyleRecalc(recalc_reason);
UpdateAllLifecyclePhasesForTest();
EXPECT_TRUE(outer->ComputedStyleRef().ChildHasExplicitInheritance());
}
class StyleResolverFontRelativeUnitTest
: public testing::WithParamInterface<const char*>,
public StyleResolverTest {};
......
......@@ -200,6 +200,12 @@
custom_compare: true,
mutable: true,
},
// Explicitly inherits a non-inherited property
{
name: "HasExplicitInheritance",
field_template: "monotonic_flag",
default_value: "false",
},
// These are set if we used viewport or rem units when resolving a length.
// FIXME: HasViewportUnits should be a monotonic_flag.
{
......
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