- Revision
- 207755
- Author
- an...@apple.com
- Date
- 2016-10-24 02:50:47 -0700 (Mon, 24 Oct 2016)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (207754 => 207755)
--- trunk/LayoutTests/ChangeLog 2016-10-24 08:04:20 UTC (rev 207754)
+++ trunk/LayoutTests/ChangeLog 2016-10-24 09:50:47 UTC (rev 207755)
@@ -1,3 +1,13 @@
+2016-10-23 Antti Koivisto <an...@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 <you...@apple.com>
ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString()
Added: trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt (0 => 207755)
--- trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt 2016-10-24 09:50:47 UTC (rev 207755)
@@ -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
+
Added: trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html (0 => 207755)
--- trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html (rev 0)
+++ trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html 2016-10-24 09:50:47 UTC (rev 207755)
@@ -0,0 +1,45 @@
+<script src=""
+<script src=""
+<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>
Modified: trunk/Source/WebCore/ChangeLog (207754 => 207755)
--- trunk/Source/WebCore/ChangeLog 2016-10-24 08:04:20 UTC (rev 207754)
+++ trunk/Source/WebCore/ChangeLog 2016-10-24 09:50:47 UTC (rev 207755)
@@ -1,3 +1,33 @@
+2016-10-23 Antti Koivisto <an...@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 <you...@apple.com>
ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString()
Modified: trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp (207754 => 207755)
--- trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp 2016-10-24 08:04:20 UTC (rev 207754)
+++ trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp 2016-10-24 09:50:47 UTC (rev 207755)
@@ -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 @@
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 @@
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 false;
+ if (!element.document().childNeedsStyleRecalc())
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;
+
+ 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 @@
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 @@
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 @@
unsigned CSSComputedStyleDeclaration::length() const
{
- updateStyleIfNeededForElement(m_element.get());
+ updateStyleIfNeededForProperty(m_element.get(), CSSPropertyCustom);
auto* style = m_element->computedStyle(m_pseudoElementSpecifier);
if (!style)
Modified: trunk/Source/WebCore/css/StyleResolver.cpp (207754 => 207755)
--- trunk/Source/WebCore/css/StyleResolver.cpp 2016-10-24 08:04:20 UTC (rev 207754)
+++ trunk/Source/WebCore/css/StyleResolver.cpp 2016-10-24 09:50:47 UTC (rev 207755)
@@ -1817,6 +1817,8 @@
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);
