[webkit-reviews] review denied: [Bug 10526] embedded SVG object doesn't scale right : [Attachment 95165] Patch v3

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 10526: embedded SVG object doesn't scale right
https://bugs.webkit.org/show_bug.cgi?id=10526

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

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=95165&action=review

This is really a great area of SVG to fix! r- because of some minor things,
also I need to get the overall picture before r+.

> Source/WebCore/rendering/RenderPart.cpp:151
> +    if (widthIsAuto) {

Could move the above bools (apart from widthIsAuto obviously) into the block.

> Source/WebCore/rendering/RenderPart.cpp:172
> +	       if (heightIsAuto && !hasIntrinsicWidth && !hasIntrinsicHeight &&
containingBlock)

Isn't heightIsAuto && hasIntrinsicWidth already tested above?

> Source/WebCore/rendering/RenderPart.cpp:199
> +    if (heightIsAuto) {

Could move the above bools (apart from heightIsAuto obviously) into the block.

> Source/WebCore/rendering/RenderPart.cpp:210
> +	       return logicalHeight;

Could omit the variable, but I can see it is clearer to use it.

> Source/WebCore/rendering/RenderReplaced.h:35
> +    virtual void computePreferredLogicalWidths();

Should no be needed to make public.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:87
> +    if (svg->useCurrentView()) {

Getting a viewBox from the svg could be put into SVGSVGElement helper function
; internally we already have that code.