Don't force layout when querying a fixed or non-box margin/padding property
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jul 2013 21:40:55 +0000 (21:40 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jul 2013 21:40:55 +0000 (21:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=118032

Reviewed by David Hyatt.

Source/WebCore:

Merge https://chromium.googlesource.com/chromium/blink/+/66427d0825fcc2975bd50220cdcaa2504d6f36e5.

This patch avoids layout in ComputedStyleExtractor::propertyValue for margin and padding properties
when they are of fixed length. According to the Blink patch's author, this improves the page load
time of economist.com by 27%.

The actual code change is significantly different from the original Blink patch since we've done
some refactorins in r152938 and r153067 to make this change more self-contained.

Test: fast/css/computed-width-without-renderer.html

* css/CSSComputedStyleDeclaration.cpp:
(WebCore::zoomAdjustedPaddingOrMarginPixelValue):
(WebCore::paddingOrMarginIsRendererDependent):
(WebCore::isLayoutDependent):
(WebCore::ComputedStyleExtractor::propertyValue):

LayoutTests:

Add a regression test inspired by the one added in
https://chromium.googlesource.com/chromium/blink/+/ff234b1593b2b493d47f38f687d09a87bc42c9eb.

* fast/css/computed-width-without-renderer-expected.txt: Added.
* fast/css/computed-width-without-renderer.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@153347 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css/computed-width-without-renderer-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/computed-width-without-renderer.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/CSSComputedStyleDeclaration.cpp

index 0bc2f92c4907dcd36a98a6020b3a9d0df157a86a..714f9794f8af187d45ae0ad5d1dc862bea15f0b8 100644 (file)
@@ -1,3 +1,16 @@
+2013-07-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Don't force layout when querying a fixed or non-box margin/padding property
+        https://bugs.webkit.org/show_bug.cgi?id=118032
+
+        Reviewed by David Hyatt.
+
+        Add a regression test inspired by the one added in
+        https://chromium.googlesource.com/chromium/blink/+/ff234b1593b2b493d47f38f687d09a87bc42c9eb.
+
+        * fast/css/computed-width-without-renderer-expected.txt: Added.
+        * fast/css/computed-width-without-renderer.html: Added.
+
 2013-07-25  Bear Travis  <betravis@adobe.com>
 
         [CSS Shapes] Fix typo in simple-polygon.js
diff --git a/LayoutTests/fast/css/computed-width-without-renderer-expected.txt b/LayoutTests/fast/css/computed-width-without-renderer-expected.txt
new file mode 100644 (file)
index 0000000..7cda2bc
--- /dev/null
@@ -0,0 +1,7 @@
+This tests obtaining widths of the same CSSComputedStyleDeclaration twice immediately after inserting a stylesheet. They should match.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.body.appendChild(div); style = getComputedStyle(div); style.width is style.width
+
diff --git a/LayoutTests/fast/css/computed-width-without-renderer.html b/LayoutTests/fast/css/computed-width-without-renderer.html
new file mode 100644 (file)
index 0000000..32e4ac0
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../js/resources/js-test-pre.js"></script>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+var div;
+
+function runTest() {
+    description("This tests obtaining widths of the same CSSComputedStyleDeclaration twice immediately after inserting a stylesheet. They should match.");
+
+    var link = document.createElement('link');
+    link.rel = 'stylesheet';
+    link.href = "doesnotexist.css";
+    document.head.appendChild(link);
+
+    div = document.createElement('div');
+    shouldBe("document.body.appendChild(div); style = getComputedStyle(div); style.width", "style.width");
+}
+</script>
+</head>
+<body onload="runTest()">
+</body>
+</html>
index c16116b3c08e911730af4037902cebcc03bc4876..b1b32f96b9c4a0f8a4f2e307664a05b578defba0 100644 (file)
@@ -1,3 +1,27 @@
+2013-07-25  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Don't force layout when querying a fixed or non-box margin/padding property
+        https://bugs.webkit.org/show_bug.cgi?id=118032
+
+        Reviewed by David Hyatt.
+
+        Merge https://chromium.googlesource.com/chromium/blink/+/66427d0825fcc2975bd50220cdcaa2504d6f36e5.
+
+        This patch avoids layout in ComputedStyleExtractor::propertyValue for margin and padding properties
+        when they are of fixed length. According to the Blink patch's author, this improves the page load
+        time of economist.com by 27%.
+
+        The actual code change is significantly different from the original Blink patch since we've done
+        some refactorins in r152938 and r153067 to make this change more self-contained.
+
+        Test: fast/css/computed-width-without-renderer.html
+
+        * css/CSSComputedStyleDeclaration.cpp:
+        (WebCore::zoomAdjustedPaddingOrMarginPixelValue):
+        (WebCore::paddingOrMarginIsRendererDependent):
+        (WebCore::isLayoutDependent):
+        (WebCore::ComputedStyleExtractor::propertyValue):
+
 2013-07-25  Sam Weinig  <sam@webkit.org>
 
         -[WebHTMLView attributedSubstringForProposedRange:actualRange:] does not include strikethrough attribute in the returned attributed string
index cfdda392e490f0a900d629628f850af1f8dfe2ab..f645ace2c1b360370c53b1fcbea7ea7a1fbe141e 100644 (file)
@@ -1520,21 +1520,31 @@ static PassRefPtr<CSSPrimitiveValue> fontWeightFromStyle(RenderStyle* style)
     return cssValuePool().createIdentifierValue(CSSValueNormal);
 }
 
-static bool isLayoutDependentProperty(CSSPropertyID propertyID)
+typedef Length (RenderStyle::*RenderStyleLengthGetter)() const;
+typedef LayoutUnit (RenderBoxModelObject::*RenderBoxComputedCSSValueGetter)() const;
+
+template<RenderStyleLengthGetter lengthGetter, RenderBoxComputedCSSValueGetter computedCSSValueGetter>
+inline PassRefPtr<CSSValue> zoomAdjustedPaddingOrMarginPixelValue(RenderStyle* style, RenderObject* renderer)
+{
+    Length unzoomzedLength = (style->*lengthGetter)();
+    if (!renderer || !renderer->isBox() || unzoomzedLength.isFixed())
+        return zoomAdjustedPixelValueForLength(unzoomzedLength, style);
+    return zoomAdjustedPixelValue((toRenderBox(renderer)->*computedCSSValueGetter)(), style);
+}
+
+template<RenderStyleLengthGetter lengthGetter>
+inline bool paddingOrMarginIsRendererDependent(RenderStyle* style, RenderObject* renderer)
+{
+    if (!renderer || !renderer->isBox())
+        return false;
+    return !(style && (style->*lengthGetter)().isFixed());
+}
+
+static bool isLayoutDependent(CSSPropertyID propertyID, RenderStyle* style, RenderObject* renderer)
 {
     switch (propertyID) {
     case CSSPropertyWidth:
     case CSSPropertyHeight:
-    case CSSPropertyMargin:
-    case CSSPropertyMarginTop:
-    case CSSPropertyMarginBottom:
-    case CSSPropertyMarginLeft:
-    case CSSPropertyMarginRight:
-    case CSSPropertyPadding:
-    case CSSPropertyPaddingTop:
-    case CSSPropertyPaddingBottom:
-    case CSSPropertyPaddingLeft:
-    case CSSPropertyPaddingRight:
     case CSSPropertyWebkitPerspectiveOrigin:
     case CSSPropertyWebkitTransformOrigin:
     case CSSPropertyWebkitTransform:
@@ -1542,6 +1552,34 @@ static bool isLayoutDependentProperty(CSSPropertyID propertyID)
     case CSSPropertyWebkitFilter:
 #endif
         return true;
+    case CSSPropertyMargin: {
+        if (!renderer || !renderer->isBox())
+            return false;
+        return !(style && style->marginTop().isFixed() && style->marginRight().isFixed()
+            && style->marginBottom().isFixed() && style->marginLeft().isFixed());
+    }
+    case CSSPropertyMarginTop:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::marginTop>(style, renderer);
+    case CSSPropertyMarginRight:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::marginRight>(style, renderer);
+    case CSSPropertyMarginBottom:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::marginBottom>(style, renderer);
+    case CSSPropertyMarginLeft:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::marginLeft>(style, renderer);
+    case CSSPropertyPadding: {
+        if (!renderer || !renderer->isBox())
+            return false;
+        return !(style && style->paddingTop().isFixed() && style->paddingRight().isFixed()
+            && style->paddingBottom().isFixed() && style->paddingLeft().isFixed());
+    }
+    case CSSPropertyPaddingTop:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::paddingTop>(style, renderer);
+    case CSSPropertyPaddingRight:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::paddingRight>(style, renderer);
+    case CSSPropertyPaddingBottom:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::paddingBottom>(style, renderer);
+    case CSSPropertyPaddingLeft:
+        return paddingOrMarginIsRendererDependent<&RenderStyle::paddingLeft>(style, renderer); 
     default:
         return false;
     }
@@ -1596,47 +1634,48 @@ static inline PassRefPtr<RenderStyle> computeRenderStyleForProperty(Node* styled
     return styledNode->computedStyle(styledNode->isPseudoElement() ? NOPSEUDO : pseudoElementSpecifier);
 }
 
-typedef Length (RenderStyle::*RenderStyleLengthGetter)() const;
-typedef LayoutUnit (RenderBoxModelObject::*RenderBoxComputedCSSValueGetter)() const;
-
-template<RenderStyleLengthGetter lengthGetter, RenderBoxComputedCSSValueGetter computedCSSValueGetter>
-inline PassRefPtr<CSSValue> zoomAdjustedPaddingOrMarginPixelValue(RenderStyle* style, RenderObject* renderer)
-{
-    Length unzoomedLength = (style->*lengthGetter)();
-    if (unzoomedLength.isFixed() || !renderer || !renderer->isBox())
-        return zoomAdjustedPixelValueForLength(unzoomedLength, style);
-    return zoomAdjustedPixelValue((toRenderBox(renderer)->*computedCSSValueGetter)(), style);
-}
-
 PassRefPtr<CSSValue> ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID, EUpdateLayout updateLayout) const
 {
     Node* styledNode = this->styledNode();
     if (!styledNode)
         return 0;
 
+    RefPtr<RenderStyle> style;
+    RenderObject* renderer;
+    bool forceFullLayout = false;
     if (updateLayout) {
         Document* document = styledNode->document();
+
+        if (nodeOrItsAncestorNeedsStyleRecalc(styledNode)) {
+            document->updateStyleIfNeeded();
+            // The style recalc could have caused the styled node to be discarded or replaced
+            // if it was a PseudoElement so we need to update it.
+            styledNode = this->styledNode();
+        }
+
+        style = computeRenderStyleForProperty(styledNode, m_pseudoElementSpecifier, propertyID);
+        renderer = styledNode->renderer();
+
         // FIXME: Some of these cases could be narrowed down or optimized better.
-        bool forceFullLayout = isLayoutDependentProperty(propertyID)
+        forceFullLayout = isLayoutDependent(propertyID, style.get(), renderer)
             || styledNode->isInShadowTree()
             || (document->styleResolverIfExists() && document->styleResolverIfExists()->hasViewportDependentMediaQueries() && document->ownerElement())
             || document->seamlessParentIFrame();
 
-        if (forceFullLayout)
+        if (forceFullLayout) {
             document->updateLayoutIgnorePendingStylesheets();
-        else if (nodeOrItsAncestorNeedsStyleRecalc(styledNode))
-            document->updateStyleIfNeeded();
+            styledNode = this->styledNode();
+        }
+    }
 
-        // The style recalc could have caused the styled node to be discarded or replaced
-        // if it was a PseudoElement so we need to update it.
-        styledNode = this->styledNode();
+    if (!updateLayout || forceFullLayout) {
+        style = computeRenderStyleForProperty(styledNode, m_pseudoElementSpecifier, propertyID);
+        renderer = styledNode->renderer();
     }
 
-    RefPtr<RenderStyle> style = computeRenderStyleForProperty(styledNode, m_pseudoElementSpecifier, propertyID);
     if (!style)
         return 0;
 
-    RenderObject* renderer = styledNode->renderer();
     propertyID = CSSProperty::resolveDirectionAwareProperty(propertyID, style->direction(), style->writingMode());
 
     switch (propertyID) {