Closed
Bug 903526
Opened 11 years ago
Closed 11 years ago
BaseRect should have IsFinite() function
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: milan, Assigned: milan)
References
Details
Attachments
(1 file, 1 obsolete file)
We have BaseRect::IsEmpty(), we could use BaseRect::IsFinite() as well.
Assignee | ||
Comment 1•11 years ago
|
||
Jeff, any objections to a BaseRect::IsFinite() function in principle?
Assignee: nobody → milan
Flags: needinfo?(jmuizelaar)
Comment 2•11 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #1) > Jeff, any objections to a BaseRect::IsFinite() function in principle? No.
Flags: needinfo?(jmuizelaar)
Assignee | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #788361 -
Flags: review?(bas) → review+
Comment 7•11 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsMathUtils.h#92 suggests that we don't really want to use isfinite
Assignee | ||
Comment 8•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
Carry r+ from Bas.
Attachment #788361 -
Attachment is obsolete: true
Attachment #789746 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/71632b7625e8
Flags: in-testsuite+
Keywords: checkin-needed
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/71632b7625e8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 12•11 years ago
|
||
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.
Assignee | ||
Comment 13•11 years ago
|
||
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.
Assignee | ||
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
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+
Comment 16•11 years ago
|
||
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.
Description
•