Changeset 106001 in webkit


Ignore:
Timestamp:
Jan 26, 2012 7:04:55 AM (12 years ago)
Author:
Nikolas Zimmermann
Message:

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.
Location:
trunk
Files:
3 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r105998 r106001  
     12012-01-26  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        SVG + <object> tests are flakey
     4        https://bugs.webkit.org/show_bug.cgi?id=77099
     5
     6        Reviewed by Andreas Kling.
     7
     8        Introduce a testcase that fails reproducibly w/o needed guard malloc, if we ever regress <object> sizing again.
     9
     10        * platform/mac/svg/wicd/sizing-flakiness-expected.png:
     11        * platform/mac/svg/wicd/sizing-flakiness-expected.txt:
     12        * svg/wicd/sizing-flakiness.html: Adjusted version of the rightsizing test, made to fail with trunk w/o this patch.
     13
    1142012-01-26  Anton Muhin  <antonm@chromium.org>
    215

  • trunk/Source/WebCore/ChangeLog

    r105999 r106001  
     12012-01-26  Nikolas Zimmermann  <nzimmermann@rim.com>
     2
     3        SVG + <object> tests are flakey
     4        https://bugs.webkit.org/show_bug.cgi?id=77099
     5
     6        Reviewed by Andreas Kling.
     7
     8        Bug 76447 changed the way RenderSVGRoot figures out its size. Previously RenderSVGRoot directly called out to the
     9        ownerRenderer (RenderEmbeddedObject) to compute its replaced size when embedded through eg. <object> element,
     10        which was quite hacky. It now relies on the ownerRenderers availableLogicalWidth/Height to be correctly set,
     11        which requires that the ownerRenderer is always laid out before the RenderSVGRoot and not the other way round.
     12
     13        This is the source of current flakiness bugs.
     14
     15        In trunk FrameView contains several special hacks, to layout the ownerRenderers view, after the RenderSVGRoots view
     16        finished layout. This worked without flakiness as RenderSVGRoot used to directly call computeReplacedLogicalWidth/Height
     17        on the ownerRenderer, which is now gone. Fortunately we can keep the new design, and can remove all hacks out of
     18        RenderSVGRoot/FrameView, if we can guarantee that the ownerRenderer FrameView is laid out before the RenderSVGRoot FrameView.
     19
     20        This is a much less error-prone approach as the previous one. This lets us run nrwt --tolerance 0 -p svg -g again,
     21        without 100% reproducable failing svg/wicd tests. (There's still one unrelated error, before guard malloc mode passes fully).
     22
     23        Test: svg/wicd/sizing-flakiness.html (Adjusted version of the rightsizing test, made to fail with trunk w/o this patch.)
     24
     25        * page/FrameView.cpp: Remove m_inLayoutParentView.
     26        (WebCore::FrameView::FrameView): Remove no longer needed m_inLayoutParentView.
     27        (WebCore::FrameView::forceLayoutParentViewIfNeeded): Simplify, no need to call updateWidgetPositions anymore, nor to clear/query flags in RenderSVGRoot.
     28        (WebCore::FrameView::layout): Call forceLayoutParentViewIfNeeded() before laying out the embedded document, to guarantee the correct order.
     29        * page/FrameView.h:
     30        (FrameView): Remove m_inLayoutParentView.
     31        * rendering/svg/RenderSVGRoot.cpp:
     32        (WebCore::RenderSVGRoot::RenderSVGRoot): Remove m_needsSizeNegotiationWithHostDocument.
     33        (WebCore::resolveLengthAttributeForSVG): Remove outcommented code, that went in by accident.
     34        (WebCore::RenderSVGRoot::layout): Remove m_needsSizeNegotiationWithHostDocument handling which is now incorrect and no longer needed.
     35        * rendering/svg/RenderSVGRoot.h:
     36        (RenderSVGRoot): Remove m_needsSizeNegotiationWithHostDocument + accessors.
     37
    1382012-01-23  Andreas Kling  <awesomekling@apple.com>
    239

  • trunk/Source/WebCore/page/FrameView.cpp

    r105250 r106001  
    132132    , m_layoutTimer(this, &FrameView::layoutTimerFired)
    133133    , m_layoutRoot(0)
    134 #if ENABLE(SVG)
    135     , m_inLayoutParentView(false)
    136 #endif
    137134    , m_inSynchronousPostLayout(false)
    138135    , m_postLayoutTasksTimer(this, &FrameView::postLayoutTimerFired)
    … …  
    901898{
    902899#if ENABLE(SVG)
    903     if (m_inLayoutParentView)
    904         return;
    905 
    906900    RenderPart* ownerRenderer = m_frame->ownerRenderer();
    907901    if (!ownerRenderer || !ownerRenderer->frame())
    … …  
    913907
    914908    RenderSVGRoot* svgRoot = toRenderSVGRoot(contentBox);
    915     if (!svgRoot->needsSizeNegotiationWithHostDocument())
    916         return;
    917 
     909    if (svgRoot->everHadLayout() && !svgRoot->needsLayout())
     910        return;
     911
     912    // If the embedded SVG document appears the first time, the ownerRenderer has already finished
     913    // layout without knowing about the existence of the embedded SVG document, because RenderReplaced
     914    // embeddedContentBox() returns 0, as long as the embedded document isn't loaded yet. Before
     915    // bothering to lay out the SVG document, mark the ownerRenderer needing layout and ask its
     916    // FrameView for a layout. After that the RenderEmbeddedObject (ownerRenderer) carries the
     917    // correct size, which RenderSVGRoot::computeReplacedLogicalWidth/Height rely on, when laying
     918    // out for the first time, or when the RenderSVGRoot size has changed dynamically (eg. via <script>).
    918919    RefPtr<FrameView> frameView = ownerRenderer->frame()->view();
    919 
    920     ASSERT(!m_inLayoutParentView);
    921     TemporaryChange<bool> resetInLayoutParentView(m_inLayoutParentView, true);
    922 
    923     // Clear needs-size-negotiation flag in RenderSVGRoot, so the next call to our
    924     // layout() method won't fire the size negotiation logic again.
    925     svgRoot->scheduledSizeNegotiationWithHostDocument();
    926     ASSERT(!svgRoot->needsSizeNegotiationWithHostDocument());
    927920
    928921    // Mark the owner renderer as needing layout.
    929922    ownerRenderer->setNeedsLayoutAndPrefWidthsRecalc();
    930 
    931     // Immediately relayout the child widgets, which can now calculate the SVG documents
    932     // intrinsic size, negotiating with the parent object/embed/iframe.
    933     RenderView* rootView = ownerRenderer->frame()->contentRenderer();
    934     ASSERT(rootView);
    935     rootView->updateWidgetPositions();
    936923
    937924    // Synchronously enter layout, to layout the view containing the host object/embed/iframe.
    … …  
    11221109            m_inLayout = true;
    11231110            beginDeferredRepaints();
     1111            forceLayoutParentViewIfNeeded();
    11241112            root->layout();
    11251113            endDeferredRepaints();
    … …  
    12091197
    12101198    page->chrome()->client()->layoutUpdated(frame());
    1211     forceLayoutParentViewIfNeeded();
    12121199}
    12131200

  • trunk/Source/WebCore/page/FrameView.h

    r104879 r106001  
    423423    bool m_layoutSchedulingEnabled;
    424424    bool m_inLayout;
    425 #if ENABLE(SVG)
    426     bool m_inLayoutParentView;
    427 #endif
    428425    bool m_inSynchronousPostLayout;
    429426    int m_layoutCount;

  • trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp

    r105513 r106001  
    5959    , m_isLayoutSizeChanged(false)
    6060    , m_needsBoundariesOrTransformUpdate(true)
    61     , m_needsSizeNegotiationWithHostDocument(false)
    6261{
    6362}
    … …  
    156155{
    157156    return static_cast<LayoutUnit>(length.calcValue(maxSize) * (length.isFixed() ? scale : 1));
    158 /*
    159     if (length.isFixed())
    160         return static_cast<LayoutUnit>(length.calcFloatValue(maxSize) * scale);
    161     return static_cast<LayoutUnit>(length.calcFloatValue(maxSize));
    162 */
    163157}
    164158
    … …  
    220214    SVGSVGElement* svg = static_cast<SVGSVGElement*>(node());
    221215    m_isLayoutSizeChanged = needsLayout || (svg->hasRelativeLengths() && oldSize != size());
    222 
    223     if (view() && view()->frameView() && view()->frameView()->embeddedContentBox()) {
    224         if (!m_needsSizeNegotiationWithHostDocument)
    225             m_needsSizeNegotiationWithHostDocument = !everHadLayout() || oldSize != size();
    226     } else
    227         ASSERT(!m_needsSizeNegotiationWithHostDocument);
    228 
    229216    SVGRenderSupport::layoutChildren(this, needsLayout || SVGRenderSupport::filtersForceContainerLayout(this));
    230217    m_isLayoutSizeChanged = false;

  • trunk/Source/WebCore/rendering/svg/RenderSVGRoot.h

    r105513 r106001  
    4646    const RenderObjectChildList* children() const { return &m_children; }
    4747    RenderObjectChildList* children() { return &m_children; }
    48 
    49     bool needsSizeNegotiationWithHostDocument() const { return m_needsSizeNegotiationWithHostDocument; }
    50 
    51     void scheduledSizeNegotiationWithHostDocument()
    52     {
    53         ASSERT(m_needsSizeNegotiationWithHostDocument);
    54         m_needsSizeNegotiationWithHostDocument = false;
    55     }
    5648
    5749    bool isLayoutSizeChanged() const { return m_isLayoutSizeChanged; }
    … …  
    109101    bool m_isLayoutSizeChanged : 1;
    110102    bool m_needsBoundariesOrTransformUpdate : 1;
    111     bool m_needsSizeNegotiationWithHostDocument : 1;
    112103};
    113104
Note: See TracChangeset for help on using the changeset viewer.