[webkit-reviews] review granted: [Bug 93290] getBBox() returns (0, 0) when width or height is zero. : [Attachment 232191] Patch

[bugzilla-daemon at webkit.org]

Philip Rogers <pdr at google.com> has granted Nikos Andronikos
<nikos.andronikos-webkit at cisra.canon.com.au>'s request for review:
Bug 93290: getBBox() returns (0,0) when width or height is zero.
https://bugs.webkit.org/show_bug.cgi?id=93290

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

------- Additional Comments from Philip Rogers <pdr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=232191&action=review


r+ with some minor nits.

> Source/WebCore/ChangeLog:7
> +

Please update this change log to describe the problem being solved.

> Source/WebCore/ChangeLog:12
> +	   * rendering/svg/RenderSVGEllipse.cpp:

Please add a line or so about what is changed in each of these function.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:61
> +    // Based on geometry, should we render?

Nit: the geometry comment doesn't really help, lets just remove it.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:63
> +    if (m_radii.width() > 0 && m_radii.height() > 0) {

Nit: isn't this if (!m_radii.isEmpty()) {

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:50
> +

Nit: extra newline

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:67
> +    if (boundingBoxSize.width() > 0 && boundingBoxSize.height() > 0) {

Nit: if (!boundingBoxSize.isEmpty()) {

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:69
> +	       // fall back to RenderSVGShape

Nit: // Fall back to RenderSVGShape.

> LayoutTests/svg/custom/getBBox-js-circle-zerodimension.html:9
> +	   {			       

Nit: { on the if line, and change the else line to } else {
(here, and elsewhere)