[webkit-reviews] review denied: [Bug 5793] HTML OBJECT without width/height attributes doesn't honor the size of the image : [Attachment 72495] Patch for Object and SVG size issue.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Julie Jeongeun Kim
<jiyuluna at gmail.com>'s request for review:
Bug 5793: HTML OBJECT without width/height attributes doesn't  honor the size
of the image
https://bugs.webkit.org/show_bug.cgi?id=5793

Attachment 72495: Patch for Object and SVG size issue.
https://bugs.webkit.org/attachment.cgi?id=72495&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72495&action=review

I'm glad you're taking a look at this super-old bug.   I think we need another
round here.

> WebCore/html/ImageDocument.cpp:312
> +    if (FrameView* view = frame()->view()) {
> +	   IntSize windowSize = IntSize(view->width(), view->height());
>      
> +	   if (imageSize != windowSize) {
> +	       if (HTMLFrameOwnerElement* ownerElement =
frame()->ownerElement()) {
> +		   RenderObject* renderer = ownerElement->renderer();
> +	       
> +		   if (renderer && renderer->isEmbeddedObject()) {
> +		       if (RenderEmbeddedObject* r =
toRenderEmbeddedObject(renderer)) 
> +			   r->updateIntrinsicSize(imageSize);
> +		   }
> +	       }
> +	   }
> +    }    

Seems this block may make sense as a helper function instead of inlined here.
(for easier readability and easy use of early-return).

> WebCore/rendering/RenderSVGRoot.cpp:335
> +void RenderSVGRoot::updateOwnerSize()
> +{
> +    if (HTMLFrameOwnerElement* ownerElement =
node()->document()->ownerElement()) {
> +	   RenderObject* render = ownerElement->renderer();
> +
> +	   if (render && render->isEmbeddedObject()) {
> +	       if (RenderEmbeddedObject* r = toRenderEmbeddedObject(render)) {
> +		   IntSize size = IntSize(width(), height());
> +		   r->updateIntrinsicSize(size);
> +	       }
> +	   }
> +    }

Generally we prefer early return to long indented if blocks.

I'm not sure why teh "size" local is useful.

> LayoutTests/svg/custom/svg-without-size-attr.html:36
> +  <object  id="svg1" type='image/svg+xml'
data='data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4wIiA/Pjxzdmcgd2lkdGg9IjU
wMCIgaGVpZ2h0PSI1MDAiIHhtbG5zPSJodHRwOi8vd3d3LnczLm9yZy8yMDAwL3N2ZyI+PGNpcmNsZS
BjeD0iMjUwIiBjeT0iMjUwIiByPSIyMDAiIGZpbGw9ImdyZWVuIiAvPjwvc3ZnPg=='></object>

Seems a little odd to use the same data url twice.  I guess I would have just
added a resources/test-name.svg file isntead.