Skip to content
Permalink
Browse files
SVG + <object> tests are flakey
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.

Canonical link: https://commits.webkit.org/93991@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@106001 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Nikolas Zimmermann committed Jan 26, 2012
1 parent 89676a5 commit a3f6fee245d6986441ebdf4e70e3e9dd2c103bf2
@@ -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.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
@@ -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"
@@ -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>
@@ -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.
@@ -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
@@ -422,9 +422,6 @@ class FrameView : public ScrollView {

bool m_layoutSchedulingEnabled;
bool m_inLayout;
#if ENABLE(SVG)
bool m_inLayoutParentView;
#endif
bool m_inSynchronousPostLayout;
int m_layoutCount;
unsigned m_nestedLayoutCount;
@@ -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;

@@ -46,14 +46,6 @@ class RenderSVGRoot : public RenderReplaced {
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 @@ class RenderSVGRoot : public RenderReplaced {
AffineTransform m_localToBorderBoxTransform;
bool m_isLayoutSizeChanged : 1;
bool m_needsBoundariesOrTransformUpdate : 1;
bool m_needsSizeNegotiationWithHostDocument : 1;
};

inline RenderSVGRoot* toRenderSVGRoot(RenderObject* object)

0 comments on commit a3f6fee

Please sign in to comment.