<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[207755] trunk</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/207755">207755</a></dd>
<dt>Author</dt> <dd>antti@apple.com</dd>
<dt>Date</dt> <dd>2016-10-24 02:50:47 -0700 (Mon, 24 Oct 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorecssCSSComputedStyleDeclarationcpp">trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp</a></li>
<li><a href="#trunkSourceWebCorecssStyleResolvercpp">trunk/Source/WebCore/css/StyleResolver.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsfastcssgetComputedStylegetComputedStylestyleresolutionexpectedtxt">trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt</a></li>
<li><a href="#trunkLayoutTestsfastcssgetComputedStylegetComputedStylestyleresolutionhtml">trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (207754 => 207755)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog 2016-10-24 08:04:20 UTC (rev 207754)
+++ trunk/LayoutTests/ChangeLog 2016-10-24 09:50:47 UTC (rev 207755)
</span><span class="lines">@@ -1,3 +1,13 @@
</span><ins>+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.
+
</ins><span class="cx"> 2016-10-24 Youenn Fablet <youenn@apple.com>
</span><span class="cx">
</span><span class="cx"> ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString()
</span></span></pre></div>
<a id="trunkLayoutTestsfastcssgetComputedStylegetComputedStylestyleresolutionexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution-expected.txt (0 => 207755)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,7 @@
</span><ins>+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
+
</ins></span></pre></div>
<a id="trunkLayoutTestsfastcssgetComputedStylegetComputedStylestyleresolutionhtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/fast/css/getComputedStyle/getComputedStyle-style-resolution.html (0 => 207755)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -0,0 +1,45 @@
</span><ins>+<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>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (207754 => 207755)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,33 @@
</span><ins>+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.
+
</ins><span class="cx"> 2016-10-24 Youenn Fablet <youenn@apple.com>
</span><span class="cx">
</span><span class="cx"> ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString()
</span></span></pre></div>
<a id="trunkSourceWebCorecssCSSComputedStyleDeclarationcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/css/CSSComputedStyleDeclaration.cpp (207754 => 207755)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -57,6 +57,7 @@
</span><span class="cx"> #include "ExceptionCode.h"
</span><span class="cx"> #include "FontTaggedSettings.h"
</span><span class="cx"> #include "HTMLFrameOwnerElement.h"
</span><ins>+#include "NodeRenderStyle.h"
</ins><span class="cx"> #include "Pair.h"
</span><span class="cx"> #include "PseudoElement.h"
</span><span class="cx"> #include "Rect.h"
</span><span class="lines">@@ -2337,6 +2338,23 @@
</span><span class="cx"> return parent->computedStyle()->alignItems();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+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;
+ }
+}
+
</ins><span class="cx"> RefPtr<CSSValue> CSSComputedStyleDeclaration::getPropertyCSSValue(CSSPropertyID propertyID, EUpdateLayout updateLayout) const
</span><span class="cx"> {
</span><span class="cx"> return ComputedStyleExtractor(m_element.ptr(), m_allowVisitedStyle, m_pseudoElementSpecifier).propertyValue(propertyID, updateLayout);
</span><span class="lines">@@ -2347,35 +2365,47 @@
</span><span class="cx"> return ComputedStyleExtractor(m_element.ptr(), m_allowVisitedStyle, m_pseudoElementSpecifier).copyProperties();
</span><span class="cx"> }
</span><span class="cx">
</span><del>-static inline bool elementOrItsAncestorNeedsStyleRecalc(Element& element)
</del><ins>+static inline bool hasValidStyleForProperty(Element& element, CSSPropertyID propertyID)
</ins><span class="cx"> {
</span><del>- if (element.needsStyleRecalc())
- return true;
</del><ins>+ if (element.styleValidity() != Style::Validity::Valid)
+ return false;
</ins><span class="cx"> if (element.document().hasPendingForcedStyleRecalc())
</span><ins>+ return false;
+ if (!element.document().childNeedsStyleRecalc())
</ins><span class="cx"> return true;
</span><del>- if (!element.document().childNeedsStyleRecalc())
- return false;
</del><span class="cx">
</span><ins>+ bool isInherited = CSSProperty::isInheritedProperty(propertyID) || isImplicitlyInheritedGridOrFlexProperty(propertyID);
+ bool maybeExplicitlyInherited = !isInherited;
+
</ins><span class="cx"> const auto* currentElement = &element;
</span><span class="cx"> for (auto& ancestor : composedTreeAncestors(element)) {
</span><del>- if (ancestor.needsStyleRecalc())
- return true;
</del><ins>+ if (ancestor.styleValidity() >= Style::Validity::SubtreeInvalid)
+ return false;
</ins><span class="cx">
</span><ins>+ if (maybeExplicitlyInherited) {
+ auto* style = currentElement->renderStyle();
+ maybeExplicitlyInherited = !style || style->hasExplicitlyInheritedProperties();
+ }
+
+ if ((isInherited || maybeExplicitlyInherited) && ancestor.styleValidity() == Style::Validity::ElementInvalid)
+ return false;
+
</ins><span class="cx"> if (ancestor.directChildNeedsStyleRecalc() && currentElement->styleIsAffectedByPreviousSibling())
</span><del>- return true;
</del><ins>+ return false;
</ins><span class="cx">
</span><span class="cx"> currentElement = &ancestor;
</span><span class="cx"> }
</span><del>- return false;
</del><ins>+
+ return true;
</ins><span class="cx"> }
</span><span class="cx">
</span><del>-static bool updateStyleIfNeededForElement(Element& element)
</del><ins>+static bool updateStyleIfNeededForProperty(Element& element, CSSPropertyID propertyID)
</ins><span class="cx"> {
</span><span class="cx"> auto& document = element.document();
</span><span class="cx">
</span><span class="cx"> document.styleScope().flushPendingUpdate();
</span><span class="cx">
</span><del>- if (!elementOrItsAncestorNeedsStyleRecalc(element))
</del><ins>+ if (hasValidStyleForProperty(element, propertyID))
</ins><span class="cx"> return false;
</span><span class="cx">
</span><span class="cx"> document.updateStyleIfNeeded();
</span><span class="lines">@@ -2468,7 +2498,7 @@
</span><span class="cx"> if (!styledElement)
</span><span class="cx"> return nullptr;
</span><span class="cx">
</span><del>- if (updateStyleIfNeededForElement(*styledElement)) {
</del><ins>+ if (updateStyleIfNeededForProperty(*styledElement, CSSPropertyCustom)) {
</ins><span class="cx"> // Style update may change styledElement() to PseudoElement or back.
</span><span class="cx"> styledElement = this->styledElement();
</span><span class="cx"> }
</span><span class="lines">@@ -2500,7 +2530,7 @@
</span><span class="cx"> if (updateLayout) {
</span><span class="cx"> Document& document = m_element->document();
</span><span class="cx">
</span><del>- if (updateStyleIfNeededForElement(*styledElement)) {
</del><ins>+ if (updateStyleIfNeededForProperty(*styledElement, propertyID)) {
</ins><span class="cx"> // Style update may change styledElement() to PseudoElement or back.
</span><span class="cx"> styledElement = this->styledElement();
</span><span class="cx"> }
</span><span class="lines">@@ -3977,7 +4007,7 @@
</span><span class="cx">
</span><span class="cx"> unsigned CSSComputedStyleDeclaration::length() const
</span><span class="cx"> {
</span><del>- updateStyleIfNeededForElement(m_element.get());
</del><ins>+ updateStyleIfNeededForProperty(m_element.get(), CSSPropertyCustom);
</ins><span class="cx">
</span><span class="cx"> auto* style = m_element->computedStyle(m_pseudoElementSpecifier);
</span><span class="cx"> if (!style)
</span></span></pre></div>
<a id="trunkSourceWebCorecssStyleResolvercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/css/StyleResolver.cpp (207754 => 207755)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1817,6 +1817,8 @@
</span><span class="cx"> case CSSValueWebkitFocusRingColor:
</span><span class="cx"> return RenderTheme::focusRingColor();
</span><span class="cx"> case CSSValueCurrentcolor:
</span><ins>+ // Color is an inherited property so depending on it effectively makes the property inherited.
+ state.style()->setHasExplicitlyInheritedProperties();
</ins><span class="cx"> return state.style()->color();
</span><span class="cx"> default: {
</span><span class="cx"> return StyleColor::colorFromKeyword(ident);
</span></span></pre>
</div>
</div>
</body>
</html>