Fix getBBox() returning (0,0) bug when width or height is zero

Fix getBBox() returning (0,0) bug when width or height is zero
    
For rects/ellipses with width or height of 0, getBBox() returns the
bounding box as being empty. This patch fixes this bug.
    
It ports and extends one initial patch, fixing the isEmpty() check,
by Changhun Kang <temoochin@company100.net>;
    
On top of Changhun's patch, this patch addresses the case where
rects/ellipses fall back to renderSVGShape and their fill bounding box
should not fall back.
    
In detail, this patch involves the following:
    
- Patch for the bug (RenderSVGEllipse, RenderSVGRect and SVGPathData)
- Three new test cases for rect, ellipse and circle
- Current layout tests updated under platform/linux
- Updated TestExpectations

BUG=140472
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170097

[Stephen Chennney, 2014-03-24 15:38:18]

You don't need all the linux baselines in the patch. It makes it harder to
review.

[Stephen Chennney, 2014-03-24 15:49:15]

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGEllipse.cpp (right):

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGEllipse.cpp:75: if
(style()->svgStyle()->hasStroke() && m_radii.width() && m_radii.height());
Typo? Does strokeWidth() depend on having non-zero radii? Otherwise, why do you
want the new check at all?

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGRect.cpp (right):

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGRect.cpp:80: if
(style()->svgStyle()->hasStroke() && boundingBoxSize.width() &&
boundingBoxSize.height()) {
Same here.

[pdr., 2014-03-24 17:17:56]

On 2014/03/24 15:38:18, Stephen Chenney wrote:
> You don't need all the linux baselines in the patch. It makes it harder to
> review.

This is jmunhoz's first patch and I asked him to include them so a test
regression wouldn't slip by unnoticed.

[pdr., 2014-03-24 17:39:31]

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGEllipse.cpp (right):

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGEllipse.cpp:60: m_usePathFallback = true;
I'm afraid this part isn't right.

Some background: RenderSVGShape is the superclass of the renderers for path,
rect, ellipse, etc. RenderSVGShape knows how to render a path but it's wasteful
in some cases (e.g., a rect) so we have specialized renderers for the simple
cases. When the specialized renderers can't handle some quirk such as non
scaling stroke, they fall back to the generic but slow RenderSVGShape.

With this change we are mixing the fallback and specialized cases.
calculateRadiiAndCenter is for the specialized ellipse code and shouldn't be
needed in the fallback case.



I suspect this explains why so many test results were affected. I don't think we
have that many shapes with width=0 or height=0 in our tests :)

[jmunhoz, 2014-03-24 18:42:18]

On 2014/03/24 15:49:15, Stephen Chenney wrote:
>
https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGEllipse.cpp:75: if
> (style()->svgStyle()->hasStroke() && m_radii.width() && m_radii.height());
> Typo? Does strokeWidth() depend on having non-zero radii? Otherwise, why do
you
> want the new check at all?
[...]
>
https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGRect.cpp:80: if
> (style()->svgStyle()->hasStroke() && boundingBoxSize.width() &&
> boundingBoxSize.height()) {
> Same here.

You are right. I will remove them. Thanks!

[jmunhoz, 2014-03-24 18:51:23]

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
File Source/core/rendering/svg/RenderSVGEllipse.cpp (right):

https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
Source/core/rendering/svg/RenderSVGEllipse.cpp:60: m_usePathFallback = true;
On 2014/03/24 17:39:32, pdr wrote:
[...] 
> With this change we are mixing the fallback and specialized cases.
> calculateRadiiAndCenter is for the specialized ellipse code and shouldn't be
> needed in the fallback case.

I understand. I will redo the patch. Thanks!

> I suspect this explains why so many test results were affected. I don't think
we
> have that many shapes with width=0 or height=0 in our tests :)

:)

[jmunhoz, 2014-03-26 00:40:06]

On 2014/03/24 17:39:31, pdr wrote:
>
https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
> File Source/core/rendering/svg/RenderSVGEllipse.cpp (right):
> 
>
https://codereview.chromium.org/208323007/diff/1/Source/core/rendering/svg/Re...
> Source/core/rendering/svg/RenderSVGEllipse.cpp:60: m_usePathFallback = true;
> I'm afraid this part isn't right.
> 
> Some background: RenderSVGShape is the superclass of the renderers for path,
> rect, ellipse, etc. RenderSVGShape knows how to render a path but it's
wasteful
> in some cases (e.g., a rect) so we have specialized renderers for the simple
> cases. When the specialized renderers can't handle some quirk such as non
> scaling stroke, they fall back to the generic but slow RenderSVGShape.
> 
> With this change we are mixing the fallback and specialized cases.
> calculateRadiiAndCenter is for the specialized ellipse code and shouldn't be
> needed in the fallback case.
> 
> I suspect this explains why so many test results were affected. I don't think
we
> have that many shapes with width=0 or height=0 in our tests :)

PTAL

[pdr., 2014-03-26 00:55:41]

Nice work!

LGTM with nits.

https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/SVGPathData.cpp (right):

https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGPathData.cpp:58: if (!(rx || ry))
It took me a minute to understand this codechange. Could we fold the negation in
like so:
if (!rx && !ry)
  return;

https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/SVGPathData.cpp:111: if (!(width || height))
Similarly here, can you fold the negation into the parens for readability?

[pdr., 2014-03-26 00:58:22]

Confusingly, the trybots instantly fail for non-committers. I've triggered some
for you.

[jmunhoz, 2014-03-26 01:26:22]

On 2014/03/26 00:55:41, pdr wrote:
> Nice work!
> 
> LGTM with nits.
> 
>
https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
> File Source/core/rendering/svg/SVGPathData.cpp (right):
> 
>
https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
> Source/core/rendering/svg/SVGPathData.cpp:58: if (!(rx || ry))
> It took me a minute to understand this codechange. Could we fold the negation
in
> like so:
> if (!rx && !ry)
>   return;
> 
>
https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
> Source/core/rendering/svg/SVGPathData.cpp:111: if (!(width || height))
> Similarly here, can you fold the negation into the parens for readability?

Done. PTAL Patch Set 3.

[pdr., 2014-03-26 01:27:48]

On 2014/03/26 01:26:22, jmunhoz wrote:
> On 2014/03/26 00:55:41, pdr wrote:
> > Nice work!
> > 
> > LGTM with nits.
> > 
> >
>
https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
> > File Source/core/rendering/svg/SVGPathData.cpp (right):
> > 
> >
>
https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
> > Source/core/rendering/svg/SVGPathData.cpp:58: if (!(rx || ry))
> > It took me a minute to understand this codechange. Could we fold the
negation
> in
> > like so:
> > if (!rx && !ry)
> >   return;
> > 
> >
>
https://codereview.chromium.org/208323007/diff/20001/Source/core/rendering/sv...
> > Source/core/rendering/svg/SVGPathData.cpp:111: if (!(width || height))
> > Similarly here, can you fold the negation into the parens for readability?
> 
> Done. PTAL Patch Set 3.

LGTM, off to the CQ

[pdr., 2014-03-26 01:27:53]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-26 01:28:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jmunhoz@igalia.com/208323007/100001

[commit-bot: I haz the power, 2014-03-26 01:28:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 01:28:13]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 947.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
bd0b0fcf82c01210409cf7e896983e547d36b899..d0cd8a7360d595c0f1a3aa8435cdaf416a59b194
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -947,4 +947,7 @@ crbug.com/354785 [ Linux Debug ]
svg/as-background-image/animated-svg-animation-
 crbug.com/354787 [ Mac Win ] plugins/touch-events-details.html [ Failure ]
 crbug.com/354787 [ Mac Win ] plugins/transformed-events.html [ Failure ]
 
-
+crbug.com/140472 svg/W3C-SVG-1.1/shapes-ellipse-02-t.svg [ NeedsRebaseline ]
+crbug.com/140472 svg/W3C-SVG-1.1/animate-elem-32-t.svg [ NeedsRebaseline ]
+crbug.com/140472 svg/W3C-SVG-1.1/shapes-intro-01-t.svg [ NeedsRebaseline ]
+crbug.com/140472 svg/W3C-SVG-1.1/shapes-rect-02-t.svg [ NeedsRebaseline ]

[pdr., 2014-03-26 01:30:58]

On 2014/03/26 01:28:13, I haz the power (commit-bot) wrote:
> Failed to apply patch for LayoutTests/TestExpectations:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file LayoutTests/TestExpectations
>   Hunk #1 FAILED at 947.
>   1 out of 1 hunk FAILED -- saving rejects to file
> LayoutTests/TestExpectations.rej
> 
> Patch:       LayoutTests/TestExpectations
> Index: LayoutTests/TestExpectations
> diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
> index
>
bd0b0fcf82c01210409cf7e896983e547d36b899..d0cd8a7360d595c0f1a3aa8435cdaf416a59b194
> 100644
> --- a/LayoutTests/TestExpectations
> +++ b/LayoutTests/TestExpectations
> @@ -947,4 +947,7 @@ crbug.com/354785 [ Linux Debug ]
> svg/as-background-image/animated-svg-animation-
>  crbug.com/354787 [ Mac Win ] plugins/touch-events-details.html [ Failure ]
>  crbug.com/354787 [ Mac Win ] plugins/transformed-events.html [ Failure ]
>  
> -
> +crbug.com/140472 svg/W3C-SVG-1.1/shapes-ellipse-02-t.svg [ NeedsRebaseline ]
> +crbug.com/140472 svg/W3C-SVG-1.1/animate-elem-32-t.svg [ NeedsRebaseline ]
> +crbug.com/140472 svg/W3C-SVG-1.1/shapes-intro-01-t.svg [ NeedsRebaseline ]
> +crbug.com/140472 svg/W3C-SVG-1.1/shapes-rect-02-t.svg [ NeedsRebaseline ]

Aww shucks, it looks like TestExpectations moved from under you.

Can you rebase and re-upload?

[jmunhoz, 2014-03-26 04:28:59]

On 2014/03/26 01:30:58, pdr wrote:
> On 2014/03/26 01:28:13, I haz the power (commit-bot) wrote:
> > Failed to apply patch for LayoutTests/TestExpectations:
> > While running patch -p1 --forward --force --no-backup-if-mismatch;
> >   patching file LayoutTests/TestExpectations
> >   Hunk #1 FAILED at 947.
> >   1 out of 1 hunk FAILED -- saving rejects to file
> > LayoutTests/TestExpectations.rej
> > 
> > Patch:       LayoutTests/TestExpectations
> > Index: LayoutTests/TestExpectations
> > diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
> > index
> >
>
bd0b0fcf82c01210409cf7e896983e547d36b899..d0cd8a7360d595c0f1a3aa8435cdaf416a59b194
> > 100644
> > --- a/LayoutTests/TestExpectations
> > +++ b/LayoutTests/TestExpectations
> > @@ -947,4 +947,7 @@ crbug.com/354785 [ Linux Debug ]
> > svg/as-background-image/animated-svg-animation-
> >  crbug.com/354787 [ Mac Win ] plugins/touch-events-details.html [ Failure ]
> >  crbug.com/354787 [ Mac Win ] plugins/transformed-events.html [ Failure ]
> >  
> > -
> > +crbug.com/140472 svg/W3C-SVG-1.1/shapes-ellipse-02-t.svg [ NeedsRebaseline
]
> > +crbug.com/140472 svg/W3C-SVG-1.1/animate-elem-32-t.svg [ NeedsRebaseline ]
> > +crbug.com/140472 svg/W3C-SVG-1.1/shapes-intro-01-t.svg [ NeedsRebaseline ]
> > +crbug.com/140472 svg/W3C-SVG-1.1/shapes-rect-02-t.svg [ NeedsRebaseline ]
> 
> Aww shucks, it looks like TestExpectations moved from under you.
> 
> Can you rebase and re-upload?

Rebased and uploaded.

[pdr, 2014-03-26 04:33:47]

The CQ bit was checked by pdr@google.com

[commit-bot: I haz the power, 2014-03-26 04:33:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jmunhoz@igalia.com/208323007/150003

[pdr, 2014-03-26 04:34:53]

On 2014/03/26 04:33:51, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/jmunhoz%40igalia.com/208323007/150003

ヾ(⌐■_■)ノOff to the CQ!

[commit-bot: I haz the power, 2014-03-26 05:39:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 05:39:50]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-03-26 14:25:04]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-26 14:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jmunhoz@igalia.com/208323007/150003

[commit-bot: I haz the power, 2014-03-26 15:35:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 15:35:41]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-03-26 15:59:00]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-26 15:59:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jmunhoz@igalia.com/208323007/150003

[commit-bot: I haz the power, 2014-03-26 17:03:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:03:14]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-03-26 17:03:34]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-26 17:03:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jmunhoz@igalia.com/208323007/150003

[commit-bot: I haz the power, 2014-03-26 18:11:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:11:45]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-03-26 18:14:15]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-26 18:14:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jmunhoz@igalia.com/208323007/150003

[commit-bot: I haz the power, 2014-03-26 19:22:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 19:22:32]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[pdr., 2014-03-26 19:51:06]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-26 19:51:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jmunhoz@igalia.com/208323007/150003

[commit-bot: I haz the power, 2014-03-26 20:55:16]

Change committed as 170097