Changeset 207755 in webkit

Timestamp:

Oct 24, 2016, 2:50:47 AM (8 years ago)

Author:

Antti Koivisto

Message:

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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt (added)
• LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/css/CSSComputedStyleDeclaration.cpp (modified) (7 diffs)
• Source/WebCore/css/StyleResolver.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-10-23  Antti Koivisto  <antti@apple.com>
+
+        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  <youenn@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-10-23  Antti Koivisto  <antti@apple.com>
+
+        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  <youenn@apple.com>
 

trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp

@@ -58 +58 @@
 #include "FontTaggedSettings.h"
 #include "HTMLFrameOwnerElement.h"
+#include "NodeRenderStyle.h"
 #include "Pair.h"
 #include "PseudoElement.h"
@@ -2338 +2339 @@
 }
 
+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<CSSValue> CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropertyID propertyID, EUpdateLayout updateLayout) const
 {
@@ -2348 +2366 @@
 }
 
-static inline bool elementOrItsAncestorNeedsStyleRecalc(Element& element)
-{
-    if (element.needsStyleRecalc())
+static inline bool hasValidStyleForProperty(Element& element, CSSPropertyID propertyID)
+{
+    if (element.styleValidity() != Style::Validity::Valid)
+        return false;
+    if (element.document().hasPendingForcedStyleRecalc())
+        return false;
+    if (!element.document().childNeedsStyleRecalc())
         return true;
-    if (element.document().hasPendingForcedStyleRecalc())
-        return true;
-    if (!element.document().childNeedsStyleRecalc())
-        return false;
+
+    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;
-}
-
-static bool updateStyleIfNeededForElement(Element& element)
+
+    return true;
+}
+
+static bool updateStyleIfNeededForProperty(Element& element, CSSPropertyID propertyID)
 {
     auto& document = element.document();
@@ -2376 +2406 @@
     document.styleScope().flushPendingUpdate();
 
-    if (!elementOrItsAncestorNeedsStyleRecalc(element))
+    if (hasValidStyleForProperty(element, propertyID))
         return false;
 
@@ -2469 +2499 @@
         return nullptr;
     
-    if (updateStyleIfNeededForElement(*styledElement)) {
+    if (updateStyleIfNeededForProperty(*styledElement, CSSPropertyCustom)) {
         // Style update may change styledElement() to PseudoElement or back.
         styledElement = this->styledElement();
@@ -2501 +2531 @@
         Document& document = m_element->document();
 
-        if (updateStyleIfNeededForElement(*styledElement)) {
+        if (updateStyleIfNeededForProperty(*styledElement, propertyID)) {
             // Style update may change styledElement() to PseudoElement or back.
             styledElement = this->styledElement();
@@ -3978 +4008 @@
 unsigned CSSComputedStyleDeclaration::length() const
 {
-    updateStyleIfNeededForElement(m_element.get());
+    updateStyleIfNeededForProperty(m_element.get(), CSSPropertyCustom);
 
     auto* style = m_element->computedStyle(m_pseudoElementSpecifier);

trunk/Source/WebCore/css/StyleResolver.cpp

@@ -1818 +1818 @@
         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: {