903526 - BaseRect should have IsFinite() function

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We have BaseRect::IsEmpty(), we could use BaseRect::IsFinite() as well.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Jeff, any objections to a BaseRect::IsFinite() function in principle?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to Milan Sreckovic [:milan] from comment #1)
> Jeff, any objections to a BaseRect::IsFinite() function in principle?

No.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

A few obvious options:

1) return (std::isfinite(x) && std::isfinite(y) && std::isfinite(width) && std::isfinite(height));

2) return (x == x && y == y && width == width && height == height);

3) float prod = x; prod *= y; prod *= width; prod *= height; return prod == prod;

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Milan Sreckovic [:milan] from comment #3)
> A few obvious options:
> 
> 1) return (std::isfinite(x) && std::isfinite(y) && std::isfinite(width) &&
> std::isfinite(height));
> 
> 2) return (x == x && y == y && width == width && height == height);
> 
> 3) float prod = x; prod *= y; prod *= width; prod *= height; return prod ==
> prod;

My preference is in the order you've written them.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

And I just realized we also have mozilla:IsFinite() method, which would be option 4.  I'll check if std::isfinite() works on all platforms.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Add BaseRect::IsFinite() method, add unit tests (including for gfxRect)

gfxRect wasn't unit tested in TestRect.cpp, so I enabled those for the most part.  The specific "infinite" test is only there for the gfxRect.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsMathUtils.h#92 suggests that we don't really want to use isfinite

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to :Ms2ger from comment #7)
> http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsMathUtils.h#92
> suggests that we don't really want to use isfinite
You wrote the code so I'll trust your research.  I'll modify the patch to use NS_finite() instead.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Add BaseRect::IsFinite() method, add unit tests (including for gfxRect). Use NS_finite from nsMathUtils.h.

Carry r+ from Bas.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71632b7625e8

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/71632b7625e8

Comment 12

[Jeff Muizelaar [:jrmuizel]]

This should be backed out. We don't want to be using nsMathUtils.h from gfx/2d. To keep gfx/2d build able standalone we can only depend on things in mfbt.

http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsMathUtils.h#92 does not seem to suggest that we can't use std::isfinite it just doesn't on platforms other than OS X with no comment suggesting why. If anything, it seems like NS_finite should be changed to use std::isfinite everywhere.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Based on Comment 12 and a conversation with Jeff, I'm going to open another bug and change the call to std::isfinite, rather than messing with this bug.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 789746 [details] [diff] [review]
Add BaseRect::IsFinite() method, add unit tests (including for gfxRect).  Use NS_finite from nsMathUtils.h.

[Approval Request Comment]
Another one blocking aurora approved bug 913614

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 789746 [details] [diff] [review]
Add BaseRect::IsFinite() method, add unit tests (including for gfxRect).  Use NS_finite from nsMathUtils.h.

Approving this as this needed for https://bugzilla.mozilla.org/show_bug.cgi?id=913614

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2e303957d50