[webkit-reviews] review denied: [Bug 64823] <svg> fails to use explicit width and height inside <html> inside IFRAME : [Attachment 111666] Patch for landing

Thu Oct 20 00:56:22 PDT 2011

Nikolas Zimmermann <zimmermann at kde.org> has denied  review:
Bug 64823: <svg> fails to use explicit width and height inside <html> inside
IFRAME
https://bugs.webkit.org/show_bug.cgi?id=64823

Attachment 111666: Patch for landing
https://bugs.webkit.org/attachment.cgi?id=111666&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=111666&action=review

I forgot to mention another possible change, given that the commit failed, I'm
now doing it :-) r- because of the location of the pixel test, I didn't spot it
before, I guess I was too tired :(

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:139
> +static bool isEmbeddedThroughFrameContainingSVGDocument(const Frame* frame,
RenderPart* ownerRenderer)
> +{
> +    ASSERT(frame);
> +    // If our frame has an owner renderer, we're embedded through eg.
object/embed/iframe,
> +    // but we only negotiate if we're in an SVG document.
> +    return !ownerRenderer || !frame->document()->isSVGDocument();
> +}

Just realized that passing around ownerRenderer is unnecessary.

static inline bool isEmbeddedThroughFrameContainingSVGDocument(const Frame*
frame)
{
    ASSERT(frame);
    ASSERT(frame->document());
    // If our frame has an owner renderer, we're embedded through eg.
object/embed/iframe,
    // but we only negotiate if we're in an SVG document.
    return !frame->ownerRenderer() || !frame->document()->isSVGDocument();
}

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:152
> -    // If our frame has an owner renderer, we're embedded through eg.
object/embed.
>      RenderPart* ownerRenderer = frame->ownerRenderer();
> -    if (!ownerRenderer)
> +    if (isEmbeddedThroughFrameContainingSVGDocument(frame, ownerRenderer))
>	   return computeIntrinsicWidth(replacedWidth);
>  
>      RenderStyle* ownerRendererStyle = ownerRenderer->style();

if (isEmbeddedThroughFrameContainingSVGDocument(frame))
	return computeIntrinsicWidth(replacedWidth);
    RenderPart* ownerRenderer = frame->ownerRenderer();
    RenderStyle* ownerRendererStyle = ownerRenderer->style();
...

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:193
> -    // If our frame has an owner renderer, we're embedded through eg.
object/embed.
>      RenderPart* ownerRenderer = frame->ownerRenderer();
> -    if (!ownerRenderer)
> +    if (isEmbeddedThroughFrameContainingSVGDocument(frame, ownerRenderer))
>	   return computeIntrinsicHeight(replacedHeight);
>  
>      RenderStyle* ownerRendererStyle = ownerRenderer->style();

Ditto.

> LayoutTests/ChangeLog:11
> +	   * svg/as-object/svg-embedded-in-html-in-iframe-expected.png: Added.
> +	   * svg/as-object/svg-embedded-in-html-in-iframe-expected.txt: Added.

In theory this could be a cross-platform test, but it creates problems - see
the commit queue failure.
Currently we don't have any pixel test results (expected.png) or render tree
dumps (expected.txt) in svg/as-object. So you better move these results to
platform/mac/svg/as-object, in case you generated them on a mac, which I
assume.