Closed
Bug 702412
Opened 12 years ago
Closed 12 years ago
[layers] float comparisons should use an epsilon
Categories
(Firefox for Android Graveyard :: General, defect, P2)
Tracking
(firefox11 fixed, fennec11+)
RESOLVED
FIXED
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(2 files, 1 obsolete file)
2.28 KB,
patch
|
pcwalton
:
review+
|
Details | Diff | Splinter Review |
423 bytes,
text/plain
|
Details |
Floating point comparisons in java should generally use an epsilon rather than being compared to each other directly. Doing (foo == 0.0f) may fail when even though (Math.abs(foo) < 1e-7) passes, due to floating point rounding issues. Code that uses == for float comparisons exists in PanZoomPosition.java and a few other classes in the layers code.
Updated•12 years ago
|
Blocks: native_droid_panning
Priority: -- → P2
Assignee | ||
Comment 1•12 years ago
|
||
Turns out the other places this happened were in PanZoomPosition, which existed only briefly in the patch queue.
Attachment #574962 -
Flags: review?(pwalton)
Assignee | ||
Comment 2•12 years ago
|
||
Oops, wrong version of patch
Attachment #574962 -
Attachment is obsolete: true
Attachment #574962 -
Flags: review?(pwalton)
Attachment #574963 -
Flags: review?(pwalton)
Comment 3•12 years ago
|
||
Comment on attachment 574963 [details] [diff] [review] Add epsilon to float compares Review of attachment 574963 [details] [diff] [review]: ----------------------------------------------------------------- getExcess() actually explicitly returns 0.0f, so I'd have thought the comparison would be ok (I assume that 0.0f == 0.0f holds - but perhaps it doesn't after assignment?). Does this fix the crash you were seeing?
Assignee | ||
Comment 4•12 years ago
|
||
getExcess() explicitly returns 0.0f, but it could also return things like viewportPos or getViewportEnd() - mPageLength, and those values might be really really close to zero without actually being zero. I've attached a test case that shows how rounding error can result in values just slightly off because of floating point representation. I thought that this might have caused the crash in bug 700559 by sending the state into WAITING_TO_SNAP instead of STOPPED, but after looking through the logic again I don't think that's possible.
Assignee | ||
Comment 5•12 years ago
|
||
Comment 6•12 years ago
|
||
Comment on attachment 574963 [details] [diff] [review] Add epsilon to float compares Review of attachment 574963 [details] [diff] [review]: ----------------------------------------------------------------- Fine by me, with the method name renamed. ::: embedding/android/ui/PanZoomController.java @@ +333,5 @@ > private float computeElasticity(float excess, float viewportLength) { > return 1.0f - excess / (viewportLength * SNAP_LIMIT); > } > > + private static boolean floatsEqual(float a, float b) { I'd call this floatsNearlyEqual() or something -- floatsEqual() isn't accurate.
Attachment #574963 -
Flags: review?(pwalton) → review+
Assignee | ||
Comment 7•12 years ago
|
||
http://hg.mozilla.org/projects/birch/rev/e056708c00aa
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
tracking-fennec: --- → 11+
Updated•12 years ago
|
status-firefox11:
--- → fixed
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•