Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add a 'fuzzyEquals' function to browser.js to do float comparisons

RESOLVED FIXED in Firefox 23

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cwiiis, Assigned: David Hsu)

Tracking

Trunk
Firefox 23
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=kats][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
To compare a float for equality, we often do Math.abs(a - b) > 1e-6 - This is done a few times in browser.js, and it'd be good to factor it out into a utility function to make the code more readable. We do the same thing in Java, and the same thing is done in native Gecko in various places too.
This would be a good first bug for somebody who knows JS.
Whiteboard: [mentor=kats][lang=js]
(Assignee)

Comment 2

4 years ago
I've taken a look at browser.js, and there are only nine Math.abs calls. Out of the nine, only three actually have an epsilon of 1e-6. 

I would love to help out with Firefox, and this seems like a great first bug, but I'm not sure it's worth refactoring a new function just to use it three times. Thoughts?
Thanks for taking a look! In addition to the three that you identified, there is an if condition that compares displayPort to _oldDisplayPort that uses an epsilon of 0.001 - I think those compares should also be converted to use the new fuzzyEquals with the smaller epsilon.

If you're interested in taking on this bug please don't hesitate to ask any questions either here or in #mobile on IRC. The first step would be to get your build environment set up as described at https://wiki.mozilla.org/Mobile/Fennec/Android - once you have that, the code changes should be fairly straightforward.
(Assignee)

Comment 4

4 years ago
Created attachment 737372 [details] [diff] [review]
Added fuzzyEquals method
Attachment #737372 - Flags: feedback+
(Assignee)

Comment 5

4 years ago
I was getting impatient with hg's cloning, so I just went ahead and grabbed browser.js from http://hg.mozilla.org/mozilla-central/annotate/7143c88f59dc/mobile/android/chrome/content/browser.js and changed that. I haven't had a chance to test my code yet, but the changes seem minimal:

1. Added fuzzyEquals method inside Tab.prototype. Everything that uses fuzzyEquals is in there.
2. Changed the first six occurrences of Math.abs. The other three use larger values of epsilon (.5, .5, and .9, respectively).
(Assignee)

Comment 6

4 years ago
Comment on attachment 737372 [details] [diff] [review]
Added fuzzyEquals method

2583,2586d2582
<   fuzzyEquals: function(diff) {
<       return (diff < 1e-6);
<   },
<
2617c2613
<     } else if (!fuzzyEquals(resolution - zoom)) {
---
>     } else if (Math.abs(resolution - zoom) >= 1e-6) {
2638a2635
>     let epsilon = 0.001;
2640,2643c2637,2640
<         !fuzzyEquals(displayPort.x - this._oldDisplayPort.x) ||
<         !fuzzyEquals(displayPort.y - this._oldDisplayPort.y) ||
<         !fuzzyEquals(displayPort.width - this._oldDisplayPort.width) ||
<         !fuzzyEquals(displayPort.height - this._oldDisplayPort.height)) {
---
>         Math.abs(displayPort.x - this._oldDisplayPort.x) > epsilon ||
>         Math.abs(displayPort.y - this._oldDisplayPort.y) > epsilon ||
>         Math.abs(displayPort.width - this._oldDisplayPort.width) > epsilon ||
>         Math.abs(displayPort.height - this._oldDisplayPort.height) > epsilon) {
2878c2875
<     if (aForce || !fuzzyEquals(aZoom - this._zoom)) {
---
>     if (aForce || Math.abs(aZoom - this._zoom) >= 1e-6) {
Attachment #737372 - Flags: feedback+ → feedback?(dvdhsu)
(Assignee)

Comment 7

4 years ago
Ick! I'm probably submitting my file wrongly; I'll read up the documentation again and resubmit correctly tomorrow morning.
(Assignee)

Updated

4 years ago
Attachment #737372 - Attachment is obsolete: true
Attachment #737372 - Flags: feedback?(dvdhsu)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F describes how to create a patch for submission. https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch describes the general process for submitting patches.

That being said, rather than define the fuzzyEquals function to take a diff and drop the Math.abs call, it would be better if your fuzzyEquals took two parameters and did the Math.abs(x - y) call directly. Taking two parameters instead of one makes it more readable at call sites - the intent of fuzzyEquals(a, b) is much more obvious than the intent of fuzzyEquals(a - b).
Assignee: nobody → dvdhsu
(Assignee)

Comment 9

4 years ago
Created attachment 737613 [details] [diff] [review]
Adds fuzzyEquals method in Tab.prototype

Sorry about that; I lost the equal in fuzzyEquals! I've fixed it now.
Attachment #737613 - Flags: feedback?
Comment on attachment 737613 [details] [diff] [review]
Adds fuzzyEquals method in Tab.prototype

Cool, this looks much better :)

A couple of things:
1) In browser.js we usually indent by two spaces. I realize the getActive function just above your new fuzzyEquals function is indented by 4 spaces but for all new code we should try to keep it to two spaces.
2) Your patch is missing the commit message and author info. The instructions at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F describe how to generate a patch with this information. (I really need to get around to merging that page with the other 5 million pages on MDN that give conflicting instructions...)

And one final note, I just landed this patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7f985396eee which adds a couple more comparisons that should be using fuzzyEquals. Updating your patch to include that code is not going to be easy right now since that patch is still on the inbound tree and not yet in mozilla-central. If you want you can wait until that gets merged to mozilla-central, and rebase your patch to include that, or you can leave it and I can merge in those changes when I land your patch. Either is fine by me.
Attachment #737613 - Flags: feedback? → feedback+
(Assignee)

Comment 11

4 years ago
1) Got it! I've changed the getActive() function to two spaces as well. 

2) Ah, sorry; I'll fix that and upload it again. 

I'd love to change take care of that as well! If you have an ETA as to when that'll get merged, I'll rebase my patch on that.
Awesome, thanks!

Usually inbound gets merged to central about once a day. You can cc yourself on bug 748495 if you want to know exactly when it gets merged - the status will be changed to "RESOLVED FIXED".
(Assignee)

Comment 13

4 years ago
In the new browser.js, there's an extra Math.abs call on line 3926 that could be changed to a fuzzyEquals call. The problem, though, is that 3926 is not part of Tab.prototype, so if I wanted to change the call, I would have to move fuzzyEquals such that it had file scope. Thoughts?

Other than that, I'm done; I'll upload once I hear back. Cheers!
Yup, moving it to have file scope sounds good. You can put it up near the dump(), getBridge(), and sendMessageToJava() function definitions which are similarly file-scoped utility functions.
(Assignee)

Comment 15

4 years ago
Created attachment 738205 [details] [diff] [review]
Adds fuzzyEquals method in browser.js

Here it is. Please let me know of any problems.

Thanks!
Attachment #737613 - Attachment is obsolete: true
Attachment #738205 - Flags: review?
Comment on attachment 738205 [details] [diff] [review]
Adds fuzzyEquals method in browser.js

Review of attachment 738205 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect, thanks!
Attachment #738205 - Flags: review? → review+
Looks like inbound is closed again, so flagging for checkin-needed.
Keywords: checkin-needed
This doesn't apply to inbound at all. Please post a rebased patch.
Keywords: checkin-needed
Applied fine when I tried it yesterday and just now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b44beaa36f3e
https://hg.mozilla.org/mozilla-central/rev/b44beaa36f3e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.