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.

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.

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).

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) {

Ick! I'm probably submitting my file wrongly; I'll read up the documentation again and resubmit correctly tomorrow morning.

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).

Sorry about that; I lost the equal in fuzzyEquals! I've fixed it now.

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.

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".

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.

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

Comment on attachment 738205 [details] [diff] [review] Adds fuzzyEquals method in browser.js Review of attachment 738205 [details] [diff] [review]: ----------------------------------------------------------------- Perfect, thanks!

Looks like inbound is closed again, so flagging for checkin-needed.

This doesn't apply to inbound at all. Please post a rebased patch.

Applied fine when I tried it yesterday and just now. https://hg.mozilla.org/integration/mozilla-inbound/rev/b44beaa36f3e