BaseRect should have IsFinite() function

RESOLVED FIXED in Firefox 25

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: milan, Assigned: milan)

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 fixed, firefox26 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

We have BaseRect::IsEmpty(), we could use BaseRect::IsFinite() as well.
Jeff, any objections to a BaseRect::IsFinite() function in principle?
Assignee: nobody → milan
Flags: needinfo?(jmuizelaar)
(In reply to Milan Sreckovic [:milan] from comment #1)
> Jeff, any objections to a BaseRect::IsFinite() function in principle?

No.
Flags: needinfo?(jmuizelaar)
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;
(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.
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.
Created attachment 788361 [details] [diff] [review]
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.
Attachment #788361 - Flags: review?(bas)
Attachment #788361 - Flags: review?(bas) → review+
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsMathUtils.h#92 suggests that we don't really want to use isfinite
(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.
Created attachment 789746 [details] [diff] [review]
Add BaseRect::IsFinite() method, add unit tests (including for gfxRect).  Use NS_finite from nsMathUtils.h.

Carry r+ from Bas.
Attachment #788361 - Attachment is obsolete: true
Attachment #789746 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/71632b7625e8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
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.
Blocks: 893572
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
Attachment #789746 - Flags: approval-mozilla-aurora?
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
Attachment #789746 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e2e303957d50
status-firefox25: --- → fixed
status-firefox26: --- → fixed
You need to log in before you can comment on or make changes to this bug.