From 94e2f725462c926354a664f84518797f5ad987f3 Mon Sep 17 00:00:00 2001 From: "antti@apple.com" Date: Mon, 24 Oct 2016 09:50:47 +0000 Subject: [PATCH] Avoid unnecessary full style resolution in getComputedStyle for non-inherited properties https://bugs.webkit.org/show_bug.cgi?id=163875 Reviewed by Andreas Kling. Source/WebCore: Test: fast/css/getComputedStyle/getComputedStyle-style-resolution.html * css/CSSComputedStyleDeclaration.cpp: (WebCore::hasValidStyleForProperty): For non-inherited properties we don't need to update style even if some ancestor style is invalid as long as explicit 'inherit' is not being used. We still need to update if we find out that the whole subtree we are in is invalid. (WebCore::updateStyleIfNeededForProperty): Pass the property. (WebCore::ComputedStyleExtractor::customPropertyValue): (WebCore::ComputedStyleExtractor::propertyValue): (WebCore::CSSComputedStyleDeclaration::length): (WebCore::elementOrItsAncestorNeedsStyleRecalc): Deleted. (WebCore::updateStyleIfNeededForElement): Deleted. * css/StyleResolver.cpp: (WebCore::StyleResolver::colorFromPrimitiveValue): Mark style as using explicit inheritance if 'currentcolor' value is used. LayoutTests: * fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt: Added. * fast/css/getComputedStyle/getComputedStyle-style-resolution.html: Added. git-svn-id: https://svn.webkit.org/repository/webkit/trunk@207755 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- LayoutTests/ChangeLog | 10 ++++ .../getComputedStyle-style-resolution-expected.txt | 7 +++ .../getComputedStyle-style-resolution.html | 45 +++++++++++++++++ Source/WebCore/ChangeLog | 30 +++++++++++ Source/WebCore/css/CSSComputedStyleDeclaration.cpp | 58 ++++++++++++++++------ Source/WebCore/css/StyleResolver.cpp | 2 + 6 files changed, 138 insertions(+), 14 deletions(-) create mode 100644 LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt create mode 100644 LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 7f7ddc7..2e44b4a 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,13 @@ +2016-10-23 Antti Koivisto + + Avoid unnecessary full style resolution in getComputedStyle for non-inherited properties + https://bugs.webkit.org/show_bug.cgi?id=163875 + + Reviewed by Andreas Kling. + + * fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt: Added. + * fast/css/getComputedStyle/getComputedStyle-style-resolution.html: Added. + 2016-10-24 Youenn Fablet ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString() diff --git a/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt b/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt new file mode 100644 index 0000000..711e245 --- /dev/null +++ b/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt @@ -0,0 +1,7 @@ +Text + +PASS No style resolution when style is valid. +PASS No style resolution when parent style is invalid and querying non-inherited property. +PASS This still works with 'inherit' +PASS Explicit 'inherit' chain triggers style resolution + diff --git a/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html b/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html new file mode 100644 index 0000000..c021286 --- /dev/null +++ b/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html @@ -0,0 +1,45 @@ + + + + +Text + + + diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index e2359c9..326feb5 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,33 @@ +2016-10-23 Antti Koivisto + + Avoid unnecessary full style resolution in getComputedStyle for non-inherited properties + https://bugs.webkit.org/show_bug.cgi?id=163875 + + Reviewed by Andreas Kling. + + Test: fast/css/getComputedStyle/getComputedStyle-style-resolution.html + + * css/CSSComputedStyleDeclaration.cpp: + (WebCore::hasValidStyleForProperty): + + For non-inherited properties we don't need to update style even if some ancestor style is invalid + as long as explicit 'inherit' is not being used. + We still need to update if we find out that the whole subtree we are in is invalid. + + (WebCore::updateStyleIfNeededForProperty): + + Pass the property. + + (WebCore::ComputedStyleExtractor::customPropertyValue): + (WebCore::ComputedStyleExtractor::propertyValue): + (WebCore::CSSComputedStyleDeclaration::length): + (WebCore::elementOrItsAncestorNeedsStyleRecalc): Deleted. + (WebCore::updateStyleIfNeededForElement): Deleted. + * css/StyleResolver.cpp: + (WebCore::StyleResolver::colorFromPrimitiveValue): + + Mark style as using explicit inheritance if 'currentcolor' value is used. + 2016-10-24 Youenn Fablet ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString() diff --git a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp index 7a4a51f..7844cc5 100644 --- a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp +++ b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp @@ -57,6 +57,7 @@ #include "ExceptionCode.h" #include "FontTaggedSettings.h" #include "HTMLFrameOwnerElement.h" +#include "NodeRenderStyle.h" #include "Pair.h" #include "PseudoElement.h" #include "Rect.h" @@ -2337,6 +2338,23 @@ static StyleSelfAlignmentData resolveAlignSelfAuto(const StyleSelfAlignmentData& return parent->computedStyle()->alignItems(); } +static bool isImplicitlyInheritedGridOrFlexProperty(CSSPropertyID propertyID) +{ + // It would be nice if grid and flex worked within normal CSS mechanisms and not invented their own inheritance system. + switch (propertyID) { + case CSSPropertyAlignSelf: +#if ENABLE(CSS_GRID_LAYOUT) + case CSSPropertyJustifySelf: + case CSSPropertyJustifyItems: +#endif + // FIXME: In StyleResolver::adjustRenderStyle z-index is adjusted based on the parent display property for grid/flex. + case CSSPropertyZIndex: + return true; + default: + return false; + } +} + RefPtr CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropertyID propertyID, EUpdateLayout updateLayout) const { return ComputedStyleExtractor(m_element.ptr(), m_allowVisitedStyle, m_pseudoElementSpecifier).propertyValue(propertyID, updateLayout); @@ -2347,35 +2365,47 @@ Ref CSSComputedStyleDeclaration::copyProperties() const return ComputedStyleExtractor(m_element.ptr(), m_allowVisitedStyle, m_pseudoElementSpecifier).copyProperties(); } -static inline bool elementOrItsAncestorNeedsStyleRecalc(Element& element) +static inline bool hasValidStyleForProperty(Element& element, CSSPropertyID propertyID) { - if (element.needsStyleRecalc()) - return true; + if (element.styleValidity() != Style::Validity::Valid) + return false; if (element.document().hasPendingForcedStyleRecalc()) - return true; - if (!element.document().childNeedsStyleRecalc()) return false; + if (!element.document().childNeedsStyleRecalc()) + return true; + + bool isInherited = CSSProperty::isInheritedProperty(propertyID) || isImplicitlyInheritedGridOrFlexProperty(propertyID); + bool maybeExplicitlyInherited = !isInherited; const auto* currentElement = &element; for (auto& ancestor : composedTreeAncestors(element)) { - if (ancestor.needsStyleRecalc()) - return true; + if (ancestor.styleValidity() >= Style::Validity::SubtreeInvalid) + return false; + + if (maybeExplicitlyInherited) { + auto* style = currentElement->renderStyle(); + maybeExplicitlyInherited = !style || style->hasExplicitlyInheritedProperties(); + } + + if ((isInherited || maybeExplicitlyInherited) && ancestor.styleValidity() == Style::Validity::ElementInvalid) + return false; if (ancestor.directChildNeedsStyleRecalc() && currentElement->styleIsAffectedByPreviousSibling()) - return true; + return false; currentElement = &ancestor; } - return false; + + return true; } -static bool updateStyleIfNeededForElement(Element& element) +static bool updateStyleIfNeededForProperty(Element& element, CSSPropertyID propertyID) { auto& document = element.document(); document.styleScope().flushPendingUpdate(); - if (!elementOrItsAncestorNeedsStyleRecalc(element)) + if (hasValidStyleForProperty(element, propertyID)) return false; document.updateStyleIfNeeded(); @@ -2468,7 +2498,7 @@ RefPtr ComputedStyleExtractor::customPropertyValue(const String& prope if (!styledElement) return nullptr; - if (updateStyleIfNeededForElement(*styledElement)) { + if (updateStyleIfNeededForProperty(*styledElement, CSSPropertyCustom)) { // Style update may change styledElement() to PseudoElement or back. styledElement = this->styledElement(); } @@ -2500,7 +2530,7 @@ RefPtr ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID, if (updateLayout) { Document& document = m_element->document(); - if (updateStyleIfNeededForElement(*styledElement)) { + if (updateStyleIfNeededForProperty(*styledElement, propertyID)) { // Style update may change styledElement() to PseudoElement or back. styledElement = this->styledElement(); } @@ -3977,7 +4007,7 @@ String CSSComputedStyleDeclaration::getPropertyValue(CSSPropertyID propertyID) c unsigned CSSComputedStyleDeclaration::length() const { - updateStyleIfNeededForElement(m_element.get()); + updateStyleIfNeededForProperty(m_element.get(), CSSPropertyCustom); auto* style = m_element->computedStyle(m_pseudoElementSpecifier); if (!style) diff --git a/Source/WebCore/css/StyleResolver.cpp b/Source/WebCore/css/StyleResolver.cpp index b41738f..de294da 100644 --- a/Source/WebCore/css/StyleResolver.cpp +++ b/Source/WebCore/css/StyleResolver.cpp @@ -1817,6 +1817,8 @@ Color StyleResolver::colorFromPrimitiveValue(const CSSPrimitiveValue& value, boo case CSSValueWebkitFocusRingColor: return RenderTheme::focusRingColor(); case CSSValueCurrentcolor: + // Color is an inherited property so depending on it effectively makes the property inherited. + state.style()->setHasExplicitlyInheritedProperties(); return state.style()->color(); default: { return StyleColor::colorFromKeyword(ident); -- 1.8.3.1