Avoid unnecessary full style resolution in getComputedStyle for non-inherited properties
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Oct 2016 09:50:47 +0000 (09:50 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Oct 2016 09:50:47 +0000 (09:50 +0000)
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
LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSComputedStyleDeclaration.cpp
Source/WebCore/css/StyleResolver.cpp

index 7f7ddc7..2e44b4a 100644 (file)
@@ -1,3 +1,13 @@
+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>
 
         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 (file)
index 0000000..711e245
--- /dev/null
@@ -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 (file)
index 0000000..c021286
--- /dev/null
@@ -0,0 +1,45 @@
+<script src="../../../resources/testharness.js"></script>
+<script src="../../../resources/testharnessreport.js"></script>
+<container>
+<subcontainer>
+<target>Text</target>
+</subcontainer>
+</container>
+<script>
+var target = document.querySelector("target");
+var container = document.querySelector("container");
+var subcontainer = document.querySelector("subcontainer");
+test(() => {
+     target.offsetLeft;
+     internals.startTrackingStyleRecalcs();
+     assert_equals(getComputedStyle(target).backgroundColor, "rgba(0, 0, 0, 0)", "getComputedStyle color is correct");
+     assert_equals(internals.styleRecalcCount(), 0, "getComputedStyle didn't trigger style resolution");
+}, "No style resolution when style is valid.");
+
+test(() => {
+     target.offsetLeft;
+     internals.startTrackingStyleRecalcs();
+     container.style.backgroundColor = "blue";
+     assert_equals(getComputedStyle(target).backgroundColor, "rgba(0, 0, 0, 0)", "getComputedStyle color is correct");
+     assert_equals(internals.styleRecalcCount(), 0, "getComputedStyle didn't trigger style resolution");
+ }, "No style resolution when parent style is invalid and querying non-inherited property.");
+
+ test(() => {
+     target.style.backgroundColor = "inherit";
+     target.offsetLeft;
+     internals.startTrackingStyleRecalcs();
+     container.style.backgroundColor = "red";
+     assert_equals(getComputedStyle(target).backgroundColor, "rgba(0, 0, 0, 0)", "getComputedStyle color is correct");
+     assert_equals(internals.styleRecalcCount(), 0, "getComputedStyle didn't trigger style resolution");
+ }, "This still works with 'inherit'");
+
+test(() => {
+     target.style.backgroundColor = "inherit";
+     subcontainer.style.backgroundColor = "inherit";
+     target.offsetLeft;
+     internals.startTrackingStyleRecalcs();
+     container.style.backgroundColor = "green";
+     assert_equals(getComputedStyle(target).backgroundColor, "rgb(0, 128, 0)", "getComputedStyle color is correct");
+     assert_equals(internals.styleRecalcCount(), 1, "getComputedStyle did trigger style resolution");
+}, "Explicit 'inherit' chain triggers style resolution");
+</script>
index e2359c9..326feb5 100644 (file)
@@ -1,3 +1,33 @@
+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>
 
         ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString()
index 7a4a51f..7844cc5 100644 (file)
@@ -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<CSSValue> CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropertyID propertyID, EUpdateLayout updateLayout) const
 {
     return ComputedStyleExtractor(m_element.ptr(), m_allowVisitedStyle, m_pseudoElementSpecifier).propertyValue(propertyID, updateLayout);
@@ -2347,35 +2365,47 @@ Ref<MutableStyleProperties> 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<CSSValue> 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<CSSValue> 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)
index b41738f..de294da 100644 (file)
@@ -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);