SVG + <object> tests are flakey
authorzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jan 2012 15:04:55 +0000 (15:04 +0000)
committerzimmermann@webkit.org <zimmermann@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 26 Jan 2012 15:04:55 +0000 (15:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=77099

Reviewed by Andreas Kling.

Source/WebCore:

Bug 76447 changed the way RenderSVGRoot figures out its size. Previously RenderSVGRoot directly called out to the
ownerRenderer (RenderEmbeddedObject) to compute its replaced size when embedded through eg. <object> element,
which was quite hacky. It now relies on the ownerRenderers availableLogicalWidth/Height to be correctly set,
which requires that the ownerRenderer is always laid out before the RenderSVGRoot and not the other way round.

This is the source of current flakiness bugs.

In trunk FrameView contains several special hacks, to layout the ownerRenderers view, after the RenderSVGRoots view
finished layout. This worked without flakiness as RenderSVGRoot used to directly call computeReplacedLogicalWidth/Height
on the ownerRenderer, which is now gone. Fortunately we can keep the new design, and can remove all hacks out of
RenderSVGRoot/FrameView, if we can guarantee that the ownerRenderer FrameView is laid out before the RenderSVGRoot FrameView.

This is a much less error-prone approach as the previous one. This lets us run nrwt --tolerance 0 -p svg -g again,
without 100% reproducable failing svg/wicd tests. (There's still one unrelated error, before guard malloc mode passes fully).

Test: svg/wicd/sizing-flakiness.html (Adjusted version of the rightsizing test, made to fail with trunk w/o this patch.)

* page/FrameView.cpp: Remove m_inLayoutParentView.
(WebCore::FrameView::FrameView): Remove no longer needed m_inLayoutParentView.
(WebCore::FrameView::forceLayoutParentViewIfNeeded): Simplify, no need to call updateWidgetPositions anymore, nor to clear/query flags in RenderSVGRoot.
(WebCore::FrameView::layout): Call forceLayoutParentViewIfNeeded() before laying out the embedded document, to guarantee the correct order.
* page/FrameView.h:
(FrameView): Remove m_inLayoutParentView.
* rendering/svg/RenderSVGRoot.cpp:
(WebCore::RenderSVGRoot::RenderSVGRoot): Remove m_needsSizeNegotiationWithHostDocument.
(WebCore::resolveLengthAttributeForSVG): Remove outcommented code, that went in by accident.
(WebCore::RenderSVGRoot::layout): Remove m_needsSizeNegotiationWithHostDocument handling which is now incorrect and no longer needed.
* rendering/svg/RenderSVGRoot.h:
(RenderSVGRoot): Remove m_needsSizeNegotiationWithHostDocument + accessors.

LayoutTests:

Introduce a testcase that fails reproducibly w/o needed guard malloc, if we ever regress <object> sizing again.

* platform/mac/svg/wicd/sizing-flakiness-expected.png:
* platform/mac/svg/wicd/sizing-flakiness-expected.txt:
* svg/wicd/sizing-flakiness.html: Adjusted version of the rightsizing test, made to fail with trunk w/o this patch.

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

LayoutTests/ChangeLog
LayoutTests/platform/mac/svg/wicd/sizing-flakiness-expected.png [new file with mode: 0644]
LayoutTests/platform/mac/svg/wicd/sizing-flakiness-expected.txt [new file with mode: 0644]
LayoutTests/svg/wicd/sizing-flakiness.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/rendering/svg/RenderSVGRoot.cpp
Source/WebCore/rendering/svg/RenderSVGRoot.h

index 3d1fa0b..0273ecb 100644 (file)
@@ -1,3 +1,16 @@
+2012-01-26  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        SVG + <object> tests are flakey
+        https://bugs.webkit.org/show_bug.cgi?id=77099
+
+        Reviewed by Andreas Kling.
+
+        Introduce a testcase that fails reproducibly w/o needed guard malloc, if we ever regress <object> sizing again.
+
+        * platform/mac/svg/wicd/sizing-flakiness-expected.png:
+        * platform/mac/svg/wicd/sizing-flakiness-expected.txt:
+        * svg/wicd/sizing-flakiness.html: Adjusted version of the rightsizing test, made to fail with trunk w/o this patch.
+
 2012-01-26  Anton Muhin  <antonm@chromium.org>
 
         Another unreviewed rebaseline.
diff --git a/LayoutTests/platform/mac/svg/wicd/sizing-flakiness-expected.png b/LayoutTests/platform/mac/svg/wicd/sizing-flakiness-expected.png
new file mode 100644 (file)
index 0000000..b0bf4ee
Binary files /dev/null and b/LayoutTests/platform/mac/svg/wicd/sizing-flakiness-expected.png differ
diff --git a/LayoutTests/platform/mac/svg/wicd/sizing-flakiness-expected.txt b/LayoutTests/platform/mac/svg/wicd/sizing-flakiness-expected.txt
new file mode 100644 (file)
index 0000000..2798bfa
--- /dev/null
@@ -0,0 +1,19 @@
+layer at (0,0) size 800x600
+  RenderView at (0,0) size 800x600
+layer at (0,0) size 800x410
+  RenderBlock {HTML} at (0,0) size 800x410
+    RenderBody {BODY} at (8,8) size 784x0
+      RenderBlock (floating) {DIV} at (0,0) size 402x402 [border: (1px solid #FF0000)]
+        RenderEmbeddedObject {OBJECT} at (1,1) size 200x67
+          layer at (0,0) size 200x67
+            RenderView at (0,0) size 200x67
+          layer at (0,0) size 200x67
+            RenderSVGRoot {svg} at (0,0) size 200x67
+              RenderSVGHiddenContainer {defs} at (0,0) size 0x0
+                RenderSVGResourceLinearGradient {linearGradient} [id="surface"] [gradientUnits=objectBoundingBox] [start=(1,0)] [end=(1,1)]
+                  RenderSVGGradientStop {stop} [offset=0.00] [color=#FFFFFF]
+                  RenderSVGGradientStop {stop} [offset=1.00] [color=#FFEEAA]
+              RenderSVGRect {rect} at (0,0) size 200x67 [stroke={[type=SOLID] [color=#FFCC33] [stroke width=2.00]}] [fill={[type=LINEAR-GRADIENT] [id="surface"]}] [x=1.00] [y=1.00] [width=118.00] [height=38.00]
+              RenderSVGText {text} at (54,9) size 11x23 contains 1 chunk(s)
+                RenderSVGInlineText {#text} at (0,0) size 11x23
+                  chunk 1 (middle anchor) text run 1 at (54.60,27.00) startOffset 0 endOffset 1 width 10.80: "F"
diff --git a/LayoutTests/svg/wicd/sizing-flakiness.html b/LayoutTests/svg/wicd/sizing-flakiness.html
new file mode 100644 (file)
index 0000000..78fbe13
--- /dev/null
@@ -0,0 +1,23 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML Basic 1.1//EN" "http://www.w3.org/TR/xhtml-basic/xhtml-basic11.dtd">
+<html xmlns="http://www.w3.org/1999/xhtml">
+<head>
+<title>Passes, if the embedded SVG image is not cropped in the middle</title>
+<style type="text/css">
+div {
+    float: left;
+    width: 400px;
+    height: 400px;
+    border: 1px solid red;
+}
+object {
+    float: left;
+    width: 50%;
+}
+</style>
+</head>
+
+<body>
+<div><object type="image/svg+xml" data="resources/f.svg"/></div>
+</body>
+</html>
index bb26ebc..17fa621 100644 (file)
@@ -1,3 +1,40 @@
+2012-01-26  Nikolas Zimmermann  <nzimmermann@rim.com>
+
+        SVG + <object> tests are flakey
+        https://bugs.webkit.org/show_bug.cgi?id=77099
+
+        Reviewed by Andreas Kling.
+
+        Bug 76447 changed the way RenderSVGRoot figures out its size. Previously RenderSVGRoot directly called out to the
+        ownerRenderer (RenderEmbeddedObject) to compute its replaced size when embedded through eg. <object> element,
+        which was quite hacky. It now relies on the ownerRenderers availableLogicalWidth/Height to be correctly set,
+        which requires that the ownerRenderer is always laid out before the RenderSVGRoot and not the other way round.
+
+        This is the source of current flakiness bugs.
+
+        In trunk FrameView contains several special hacks, to layout the ownerRenderers view, after the RenderSVGRoots view
+        finished layout. This worked without flakiness as RenderSVGRoot used to directly call computeReplacedLogicalWidth/Height
+        on the ownerRenderer, which is now gone. Fortunately we can keep the new design, and can remove all hacks out of
+        RenderSVGRoot/FrameView, if we can guarantee that the ownerRenderer FrameView is laid out before the RenderSVGRoot FrameView.
+
+        This is a much less error-prone approach as the previous one. This lets us run nrwt --tolerance 0 -p svg -g again,
+        without 100% reproducable failing svg/wicd tests. (There's still one unrelated error, before guard malloc mode passes fully).
+
+        Test: svg/wicd/sizing-flakiness.html (Adjusted version of the rightsizing test, made to fail with trunk w/o this patch.)
+
+        * page/FrameView.cpp: Remove m_inLayoutParentView.
+        (WebCore::FrameView::FrameView): Remove no longer needed m_inLayoutParentView.
+        (WebCore::FrameView::forceLayoutParentViewIfNeeded): Simplify, no need to call updateWidgetPositions anymore, nor to clear/query flags in RenderSVGRoot.
+        (WebCore::FrameView::layout): Call forceLayoutParentViewIfNeeded() before laying out the embedded document, to guarantee the correct order.
+        * page/FrameView.h:
+        (FrameView): Remove m_inLayoutParentView.
+        * rendering/svg/RenderSVGRoot.cpp:
+        (WebCore::RenderSVGRoot::RenderSVGRoot): Remove m_needsSizeNegotiationWithHostDocument.
+        (WebCore::resolveLengthAttributeForSVG): Remove outcommented code, that went in by accident.
+        (WebCore::RenderSVGRoot::layout): Remove m_needsSizeNegotiationWithHostDocument handling which is now incorrect and no longer needed.
+        * rendering/svg/RenderSVGRoot.h:
+        (RenderSVGRoot): Remove m_needsSizeNegotiationWithHostDocument + accessors.
+
 2012-01-23  Andreas Kling  <awesomekling@apple.com>
 
         Make elements that don't have attributes smaller.
index 21948f3..65b483f 100644 (file)
@@ -131,9 +131,6 @@ FrameView::FrameView(Frame* frame)
     , m_fixedObjectCount(0)
     , m_layoutTimer(this, &FrameView::layoutTimerFired)
     , m_layoutRoot(0)
-#if ENABLE(SVG)
-    , m_inLayoutParentView(false)
-#endif
     , m_inSynchronousPostLayout(false)
     , m_postLayoutTasksTimer(this, &FrameView::postLayoutTimerFired)
     , m_isTransparent(false)
@@ -900,9 +897,6 @@ static inline void collectFrameViewChildren(FrameView* frameView, Vector<RefPtr<
 inline void FrameView::forceLayoutParentViewIfNeeded()
 {
 #if ENABLE(SVG)
-    if (m_inLayoutParentView)
-        return;
-
     RenderPart* ownerRenderer = m_frame->ownerRenderer();
     if (!ownerRenderer || !ownerRenderer->frame())
         return;
@@ -912,28 +906,21 @@ inline void FrameView::forceLayoutParentViewIfNeeded()
         return;
 
     RenderSVGRoot* svgRoot = toRenderSVGRoot(contentBox);
-    if (!svgRoot->needsSizeNegotiationWithHostDocument())
+    if (svgRoot->everHadLayout() && !svgRoot->needsLayout())
         return;
 
+    // If the embedded SVG document appears the first time, the ownerRenderer has already finished
+    // layout without knowing about the existence of the embedded SVG document, because RenderReplaced
+    // embeddedContentBox() returns 0, as long as the embedded document isn't loaded yet. Before
+    // bothering to lay out the SVG document, mark the ownerRenderer needing layout and ask its
+    // FrameView for a layout. After that the RenderEmbeddedObject (ownerRenderer) carries the
+    // correct size, which RenderSVGRoot::computeReplacedLogicalWidth/Height rely on, when laying
+    // out for the first time, or when the RenderSVGRoot size has changed dynamically (eg. via <script>).
     RefPtr<FrameView> frameView = ownerRenderer->frame()->view();
 
-    ASSERT(!m_inLayoutParentView);
-    TemporaryChange<bool> resetInLayoutParentView(m_inLayoutParentView, true);
-
-    // Clear needs-size-negotiation flag in RenderSVGRoot, so the next call to our
-    // layout() method won't fire the size negotiation logic again.
-    svgRoot->scheduledSizeNegotiationWithHostDocument();
-    ASSERT(!svgRoot->needsSizeNegotiationWithHostDocument());
-
     // Mark the owner renderer as needing layout.
     ownerRenderer->setNeedsLayoutAndPrefWidthsRecalc();
 
-    // Immediately relayout the child widgets, which can now calculate the SVG documents
-    // intrinsic size, negotiating with the parent object/embed/iframe.
-    RenderView* rootView = ownerRenderer->frame()->contentRenderer();
-    ASSERT(rootView);
-    rootView->updateWidgetPositions();
-
     // Synchronously enter layout, to layout the view containing the host object/embed/iframe.
     ASSERT(frameView);
     frameView->layout();
@@ -1121,6 +1108,7 @@ void FrameView::layout(bool allowSubtree)
 
             m_inLayout = true;
             beginDeferredRepaints();
+            forceLayoutParentViewIfNeeded();
             root->layout();
             endDeferredRepaints();
             m_inLayout = false;
@@ -1208,7 +1196,6 @@ void FrameView::layout(bool allowSubtree)
         return;
 
     page->chrome()->client()->layoutUpdated(frame());
-    forceLayoutParentViewIfNeeded();
 }
 
 RenderBox* FrameView::embeddedContentBox() const
index 12aa07a..1761a06 100644 (file)
@@ -422,9 +422,6 @@ private:
     
     bool m_layoutSchedulingEnabled;
     bool m_inLayout;
-#if ENABLE(SVG)
-    bool m_inLayoutParentView;
-#endif
     bool m_inSynchronousPostLayout;
     int m_layoutCount;
     unsigned m_nestedLayoutCount;
index d0fe7f1..8fc7afe 100644 (file)
@@ -58,7 +58,6 @@ RenderSVGRoot::RenderSVGRoot(SVGStyledElement* node)
     : RenderReplaced(node)
     , m_isLayoutSizeChanged(false)
     , m_needsBoundariesOrTransformUpdate(true)
-    , m_needsSizeNegotiationWithHostDocument(false)
 {
 }
 
@@ -155,11 +154,6 @@ bool RenderSVGRoot::isEmbeddedThroughFrameContainingSVGDocument() const
 static inline LayoutUnit resolveLengthAttributeForSVG(const Length& length, float scale, float maxSize)
 {
     return static_cast<LayoutUnit>(length.calcValue(maxSize) * (length.isFixed() ? scale : 1));
-/*
-    if (length.isFixed())
-        return static_cast<LayoutUnit>(length.calcFloatValue(maxSize) * scale);
-    return static_cast<LayoutUnit>(length.calcFloatValue(maxSize));
-*/
 }
 
 LayoutUnit RenderSVGRoot::computeReplacedLogicalWidth(bool includeMaxWidth) const
@@ -219,13 +213,6 @@ void RenderSVGRoot::layout()
 
     SVGSVGElement* svg = static_cast<SVGSVGElement*>(node());
     m_isLayoutSizeChanged = needsLayout || (svg->hasRelativeLengths() && oldSize != size());
-
-    if (view() && view()->frameView() && view()->frameView()->embeddedContentBox()) {
-        if (!m_needsSizeNegotiationWithHostDocument)
-            m_needsSizeNegotiationWithHostDocument = !everHadLayout() || oldSize != size();
-    } else
-        ASSERT(!m_needsSizeNegotiationWithHostDocument);
-
     SVGRenderSupport::layoutChildren(this, needsLayout || SVGRenderSupport::filtersForceContainerLayout(this));
     m_isLayoutSizeChanged = false;
 
index 7b55a28..cb7a1b2 100644 (file)
@@ -46,14 +46,6 @@ public:
     const RenderObjectChildList* children() const { return &m_children; }
     RenderObjectChildList* children() { return &m_children; }
 
-    bool needsSizeNegotiationWithHostDocument() const { return m_needsSizeNegotiationWithHostDocument; }
-
-    void scheduledSizeNegotiationWithHostDocument()
-    {
-        ASSERT(m_needsSizeNegotiationWithHostDocument);
-        m_needsSizeNegotiationWithHostDocument = false;
-    }
-
     bool isLayoutSizeChanged() const { return m_isLayoutSizeChanged; }
     virtual void setNeedsBoundariesUpdate() { m_needsBoundariesOrTransformUpdate = true; }
     virtual void setNeedsTransformUpdate() { m_needsBoundariesOrTransformUpdate = true; }
@@ -108,7 +100,6 @@ private:
     AffineTransform m_localToBorderBoxTransform;
     bool m_isLayoutSizeChanged : 1;
     bool m_needsBoundariesOrTransformUpdate : 1;
-    bool m_needsSizeNegotiationWithHostDocument : 1;
 };
 
 inline RenderSVGRoot* toRenderSVGRoot(RenderObject* object)