[webkit-reviews] review denied: [Bug 64974] REGRESSION(87526): ASSERT(!needsLayout()) followed by graphical glitches on google charts (svg loaded in iframe) : [Attachment 102898] Patch v5

[bugzilla-daemon at webkit.org]

Zoltan Herczeg <zherczeg at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 64974: REGRESSION(87526): ASSERT(!needsLayout()) followed by graphical
glitches on google charts (svg loaded in iframe)
https://bugs.webkit.org/show_bug.cgi?id=64974

Attachment 102898: Patch v5
https://bugs.webkit.org/attachment.cgi?id=102898&action=review

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
The patch seems ok, couple of things:

View in context: https://bugs.webkit.org/attachment.cgi?id=102898&action=review


> Source/WebCore/page/FrameView.cpp:1123
> +    ASSERT(frameView);

This code could be moved down.

> Source/WebCore/page/FrameView.cpp:1131
> +    frameView->layout();

Before this line. Its purpose would be clearer.

> Source/WebCore/page/FrameView.cpp:2155
> +RenderBox* FrameView::embeddedContentBox() const
> +{
> +    RenderView* contentRenderer = m_frame->contentRenderer();
> +    if (!contentRenderer)
> +	   return 0;
> +
> +    RenderObject* rootChild = contentRenderer->firstChild();
> +    if (!rootChild || !rootChild->isBox())
> +	   return 0;
> +
> +#if ENABLE(SVG)
> +    // Curently only embedded SVG documents participate in the
size-negotiation logic.
> +    if (rootChild->isSVGRoot())
> +	   return toRenderBox(rootChild);
> +#endif
> +
> +    return 0;
> +}
> +

Am I see right that this code have any effect if SVG is enabled? Otherwise, it
just returns with 0. Maybe we could put an ENABLE(SVG) around the whole
function? (Including the header, call sites, etc.) At least it should be an
inline { return 0; } if SVG is disabled.

> Source/WebCore/page/FrameView.h:319
> +    void forceLayoutParentViewIfNeeded();

Make this function inline, please.

> Source/WebCore/rendering/svg/RenderSVGRoot.h:47
> +    void scheduledSizeNegotiationWithHostDocument() {
ASSERT(m_needsSizeNegotiationWithHostDocument);
m_needsSizeNegotiationWithHostDocument = false; }

WebKit has a one statement in every line policy, strange that the style bot did
not capture it. Fix it please.

> LayoutTests/ChangeLog:15
> +	   *
platform/mac/fast/block/positioning/rtl-fixed-positioning-expected.png:
> +	   *
platform/mac/fast/block/positioning/vertical-rl/fixed-positioning-expected.png:


Why are these changed?