When there are more than two touches, the PanZoomController gets very confused, and the end result is frequently that your page turns into a single pixel and you have to restart the browser. This is rather suboptimal.

I think I've seen this. I got "11-30 15:51:38.949: E/GeckoPanZoomController(13127): Received impossible touch end while in NOTHING"

I see this at least once daily and I have to restart the browser when it happens, because I lose the page. Requesting that this be bumped to a P1.

As far as I can tell this is some sort of issue in the ScaleGestureDetector. I thought it might be that we aren't passing all touch events to the gesture detector, but changing LayerView.onTouchEvent() so that it unconditionally passes touch events through doesn't help. We may just need to reinvent the scale gesture detector...

I've seen some of this when playing with multitouch (i.e. resting your hand can result in multiple touches. They're usually pretty large touch surfaces though. Maybe we can try just not send touches with a size greater than .25 to the gesture detector? http://developer.android.com/reference/android/view/MotionEvent.html#getSize%28int%29

(In reply to Wesley Johnston (:wesj) from comment #4) > I've seen some of this when playing with multitouch (i.e. resting your hand > can result in multiple touches. They're usually pretty large touch surfaces > though. Maybe we can try just not send touches with a size greater than .25 > to the gesture detector? > > http://developer.android.com/reference/android/view/MotionEvent. > html#getSize%28int%29 Instead of trying to hack around the GestureDetector's limitations I'd rather just reinvent the ScaleGestureDetector. Trying hacks like that is really relying on the internal implementation of the gesture detector, which Google (or the manufacturers) could change under our feet.

I'll be testing this more thoroughly on various devices tomorrow, but I'd like to go ahead and get review in case I don't find any major issues. Preliminary testing suggests that this bug doesn't happen anymore on a Droid X, a Droid RAZR, or a Galaxy Tab. This patch implements the "SimpleScaleGestureDetector", a hopefully more reliable replacement for the built-in Android scale gesture detector. Copy-and-pasting from the comments at the top of the file, I believe it's more reliable because: * It doesn't assume that pointer IDs are numbered 0 and 1. * It doesn't attempt to correct for "slop" when resting one's hand on the device. On some devices (e.g. the Droid X) this can cause the ScaleGestureDetector to lose track of how many pointers are down, with disastrous results (this bug). * Cancelling a zoom into a pan is handled correctly. * It doesn't take pressure into account, which results in smoother scaling.

Comment on attachment 588311 [details] [diff] [review] Proposed patch. Review of attachment 588311 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks pretty good, and I like how you implemented the SimpleScaleGestureDetector - it's very clean and easy to follow. The one thing I was thinking about - do we want to handle more than two touch points? With the current code, if you put down fingers 1, 2, and 3, and then lift finger 1, you won't be able to pinch with fingers 2 and 3. I haven't checked if this works with the system detector, but if it works there then we should probably add support for it. If it doesn't work there then I could go either way; it would be nice but I think would complicate the code more. Thoughts? (Also I'm PTO for the next few days and may not be online, so if you want to get Cwiiis to steal the review that's fine by me, I don't see any problems with the code other than the few nits.) ::: mobile/android/base/ui/PanZoomController.java @@ +359,5 @@ > return FloatMath.sqrt(dx * dx + dy * dy); > } > > private void track(float x, float y, long time) { > + Log.e(LOGTAG, "### track " + x + " " + y); Do you want the log lines checked in, or were they debugging-only? There's this one and a few in onScale/onScaleEnd. @@ +403,4 @@ > track(event.getHistoricalX(0, i), > event.getHistoricalY(0, i), > event.getHistoricalEventTime(i)); > + }*/ Why take these out? Are they messing with things? ::: mobile/android/base/ui/SimpleScaleGestureDetector.java @@ +14,5 @@ > + * > + * The Original Code is Mozilla Android code. > + * > + * The Initial Developer of the Original Code is Mozilla Foundation. > + * Portions created by the Initial Developer are Copyright (C) 2009-2010 Fix copyright year.

Here's a new version that removes the debug logging that crept in. It also supports starting a gesture by placing >2 fingers on the device and lifting fingers until 2 remain. It didn't make the code significantly more complex; in fact, it arguably made it simpler. Thanks for the catch.

Comment on attachment 588509 [details] [diff] [review] Proposed patch, version 2. Review of attachment 588509 [details] [diff] [review]: ----------------------------------------------------------------- Like it - r+ with either using PointF.length or an explanation for the addition of PointUtils.distance, and removal of toJSON from PointerInfo.toString. ::: mobile/android/base/gfx/PointUtils.java @@ +81,5 @@ > + /** Computes the scalar distance between two points. */ > + public static float distance(PointF one, PointF two) { > + float deltaX = one.x - two.x, deltaY = one.y - two.y; > + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY); > + } Is there a reason not to use PointF.length(float x, float y) for this? ::: mobile/android/base/ui/SimpleScaleGestureDetector.java @@ +305,5 @@ > + String prevString; > + if (mPrevious == null) { > + prevString = "n/a"; > + } else { > + prevString = PointUtils.toJSON(mPrevious).toString(); It's a bit heavy-weight to use toJSON here isn't it? Why not just use the PointF.toString()? @@ +309,5 @@ > + prevString = PointUtils.toJSON(mPrevious).toString(); > + } > + > + // The current position should always be non-null. > + String currentString = PointUtils.toJSON(mCurrent).toString(); Same here.

> ::: mobile/android/base/gfx/PointUtils.java > @@ +81,5 @@ > > + /** Computes the scalar distance between two points. */ > > + public static float distance(PointF one, PointF two) { > > + float deltaX = one.x - two.x, deltaY = one.y - two.y; > > + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY); > > + } > > Is there a reason not to use PointF.length(float x, float y) for this? No legitimate reason. I didn't know about that function :) Google didn't use it themselves in ScaleGestureDetector, another strike against that class... > > ::: mobile/android/base/ui/SimpleScaleGestureDetector.java > @@ +305,5 @@ > > + String prevString; > > + if (mPrevious == null) { > > + prevString = "n/a"; > > + } else { > > + prevString = PointUtils.toJSON(mPrevious).toString(); > > It's a bit heavy-weight to use toJSON here isn't it? Why not just use the > PointF.toString()? > > @@ +309,5 @@ > > + prevString = PointUtils.toJSON(mPrevious).toString(); > > + } > > + > > + // The current position should always be non-null. > > + String currentString = PointUtils.toJSON(mCurrent).toString(); > > Same here. Sadly PointF doesn't have a reasonable implementation for toString(). (Point does IIRC. It's an oversight in the Android libraries.) But I can write one in PointUtils if you'd like.

(In reply to Patrick Walton (:pcwalton) from comment #10) > Sadly PointF doesn't have a reasonable implementation for toString(). (Point > does IIRC. It's an oversight in the Android libraries.) But I can write one > in PointUtils if you'd like. heh, typical Android :) If we ever have debugging that calls this a lot, I think we'll want to not take such a round-about route to get a string, so it's worth doing. Would remove the need for the try/catch block there too. Your call though, I'm easy either way.

(In reply to Patrick Walton (:pcwalton) from comment #10) > > ::: mobile/android/base/gfx/PointUtils.java > > @@ +81,5 @@ > > > + /** Computes the scalar distance between two points. */ > > > + public static float distance(PointF one, PointF two) { > > > + float deltaX = one.x - two.x, deltaY = one.y - two.y; > > > + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY); > > > + } > > > > Is there a reason not to use PointF.length(float x, float y) for this? > > No legitimate reason. I didn't know about that function :) Google didn't use > it themselves in ScaleGestureDetector, another strike against that class... > Despite what you'd expect, PointF.length(float x, float y) is a static method and returns sqrt(x*x + y*y), so you can't actually use that for this purpose. At least not without creating a new PointF(one.x - two.x, one.y - two.y) which seems silly.

Comment on attachment 588509 [details] [diff] [review] Proposed patch, version 2. Your updated patch looks fine, btw, although I would have used an ArrayList instead of a LinkedList since I don't think there's much advantage to using LinkedList, and it will be marginally slower. Also I would wrap the sPointerInfoFreeList in a SoftReference so the VM can reclaim the space if it wants to; however the space will be small enough that it won't make a significant difference either way. Really I would like to see a generalized ObjectCache class to handle recycling different object types, particularly ViewportMetrics and point/rect classes, since we create a metric-ton of those on any UI event.

(In reply to Kartikaya Gupta (:kats) from comment #12) > (In reply to Patrick Walton (:pcwalton) from comment #10) > > > ::: mobile/android/base/gfx/PointUtils.java > > > @@ +81,5 @@ > > > > + /** Computes the scalar distance between two points. */ > > > > + public static float distance(PointF one, PointF two) { > > > > + float deltaX = one.x - two.x, deltaY = one.y - two.y; > > > > + return FloatMath.sqrt(deltaX * deltaX + deltaY * deltaY); > > > > + } > > > > > > Is there a reason not to use PointF.length(float x, float y) for this? > > > > No legitimate reason. I didn't know about that function :) Google didn't use > > it themselves in ScaleGestureDetector, another strike against that class... > > > > Despite what you'd expect, PointF.length(float x, float y) is a static > method and returns sqrt(x*x + y*y), so you can't actually use that for this > purpose. At least not without creating a new PointF(one.x - two.x, one.y - > two.y) which seems silly. You could do PointF.length(one.x - two.x, one.y - two.y)? Why would you need to create a new PointF?

(In reply to Chris Lord [:cwiiis] from comment #14) > You could do PointF.length(one.x - two.x, one.y - two.y)? Why would you need > to create a new PointF? Oh, true. I think I still prefer PointUtils.distance for being more readable, but you're right, it could be done with PointF.length as well.

(In reply to Kartikaya Gupta (:kats) from comment #15) > (In reply to Chris Lord [:cwiiis] from comment #14) > > You could do PointF.length(one.x - two.x, one.y - two.y)? Why would you need > > to create a new PointF? > > Oh, true. I think I still prefer PointUtils.distance for being more > readable, but you're right, it could be done with PointF.length as well. I don't mind it being there as a utility function that uses PointF.length internally I suppose, though I think it'd be a bit extravagant. Where we can, we should try and stick to the stock class libraries.

This crashes on Froyo because LinkedList.push does not exist. The attached patch makes it use LinkedList.add instead which is equivalent.

Patrick, please request aurora approval

Comment on attachment 588509 [details] [diff] [review] Proposed patch, version 2. [Triage Comment]

Comment on attachment 591093 [details] [diff] [review] I'm a moron, it should be addFirst(). [Triage Comment]

Zoom is still going wild when touching the screen on a larger area than the touch of a finger, please see video: http://www.youtube.com/watch?v=1vGEBn251I4&feature=plcp&context=C3b4f1d5UDOEgsToPDskJ4vODsGN4DS13W6dxwebXG. This is reproducible on: Firefox 11 (tinderbox build): 1328844077/09-Feb-2012 20:42 20120209192117 Firefox 12: Firefox 12.0a2 (2012-02-09) Firefox 13: Firefox 13.0a1 (2012-02-09) Device: LG Optimus 2X (Android 2.2.2) Reopening the bug

This behaviour isn't the same as that described in comment #0. In fact it is a much more "expected" behaviour in that it leaves the browser looking normal and still usable. If you agree, please close this bug and file a new one for this behaviour.

per kats please open a new bug with this new behavior.

Verified fixed on: Firefox 13.0a1 (2012-03-01) 20120301031135 http://hg.mozilla.org/mozilla-central/rev/1c3b291d0830 -- Device: Samsung Galaxy S2 OS: Android 2.3.4