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

Wed Oct 19 01:39:22 PDT 2011

Nikolas Zimmermann <zimmermann at kde.org> has granted Levi Weintraub
<leviw at chromium.org>'s request for 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 111542: Patch
https://bugs.webkit.org/attachment.cgi?id=111542&action=review

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

Great patch Levi, r=me. Please fix following issues before landing:

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:133
> +static bool isEmbeddedFrameContainingSVGDocument(const Frame* frame,
RenderPart* ownerRenderer)

I'd name this "isEmbeddedThroughFrameContainingSVGDocument" - bug 47156 adds a
similar method "isEmbeddedThroughSVGImage" and i'd like to have it consistent
:-)

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:135
> +    // If our frame has an owner renderer, we're embedded through eg.
object/embed/iframe,

I'd ASSERT(frame) here to save the reader from looking at the call-site whether
this can be null.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:136
> +    // but we only negotiate if we're in an SVG document

Trailing period missing.

> LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:2
> +
> +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

I'd suggest to turn this into a HTML5 test, aka. <!DOCTYPE html> and leaving
out xmlns everywhere.

> LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:12
> +B+="<head>\r\n";
> +B+="<title>Circle</title>\r\n";
> +B+="</head><body>\r\n";

I'd remove the head alltogether, it has no benefit here. Also the \r\n are not
needed.

> LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:14
> +B+="<ellipse cx=\"50%\" cy=\"50%\" rx=\"50%\" ry=\"50%\" fill=\"blue\"
stroke=\"none\" />\r\n";

Make it a circle :-)

> LayoutTests/svg/as-object/svg-embedded-in-html-in-iframe.html:22
> +<iframe src="javascript:parent.CreateCircle();" width="100%" height="100%"
frameborder="0" scrolling="no"></iframe>

Neat trick Levi! :-)