Last Comment Bug 716673 - PanZoomController needs an in-depth review/clean up
: PanZoomController needs an in-depth review/clean up
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P4 normal (vote)
: Firefox 12
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
: 716325 (view as bug list)
Depends on: 714711 714874
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 14:01 PST by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2012-01-19 13:22 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
(1/17) Remove unnecessary finalize() (1.81 KB, patch)
2012-01-09 14:02 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(2/17) Refactor duplicate gesture listener code (4.03 KB, patch)
2012-01-09 14:03 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(3/17) Remove dead code for non-primary pointer events (2.26 KB, patch)
2012-01-09 14:03 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(4/17) Misc cleanups (mostly cosmetic) (16.50 KB, patch)
2012-01-09 14:06 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(5/17) Factor out a startTouch method (4.08 KB, patch)
2012-01-09 14:12 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(6/17) Refactor duplicated code in track() functions (7.25 KB, patch)
2012-01-09 14:13 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(7/17) Encapsulate Axis::firstTouchPos (5.43 KB, patch)
2012-01-09 14:14 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback-
Details | Diff | Splinter Review
(8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos (3.20 KB, patch)
2012-01-09 14:14 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback-
Details | Diff | Splinter Review
(9/17) Collapse unused FlingStates into STOPPED (4.36 KB, patch)
2012-01-09 14:15 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(10/17) Fix up the advanceFling() step (2.68 KB, patch)
2012-01-09 14:15 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback-
Details | Diff | Splinter Review
(11/17) Use locked velocity instead of unlocked velocity (2.68 KB, patch)
2012-01-09 14:17 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(12/17) Encapsulate more Axis variables (6.54 KB, patch)
2012-01-09 14:17 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback-
Details | Diff | Splinter Review
(14/17) Encapsulate fling state in Axis (19.59 KB, patch)
2012-01-09 14:18 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback-
Details | Diff | Splinter Review
(14/17) Split a SubwindowScrollHelper out of PZC (19.59 KB, patch)
2012-01-09 14:18 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(15/17) Split the Axis class out of PZC (21.26 KB, patch)
2012-01-09 14:19 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(16/17) Pull out hard-coded strings (3.43 KB, patch)
2012-01-09 14:19 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(17/17) Fix copyright/authors (1.55 KB, patch)
2012-01-09 14:20 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(10/17) Fix up the advanceFling() step (9.05 KB, patch)
2012-01-09 20:54 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(13/17) Encapsulate fling state in Axis (4.07 KB, patch)
2012-01-09 20:55 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: feedback+
Details | Diff | Splinter Review
(1/17) Remove unnecessary finalize() (1.82 KB, patch)
2012-01-10 10:27 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(2/17) Refactor duplicate gesture listener code (4.04 KB, patch)
2012-01-10 10:28 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(3/17) Remove dead code for non-primary pointer events (2.27 KB, patch)
2012-01-10 10:29 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(4/17) Misc cleanups (mostly cosmetic) (15.16 KB, patch)
2012-01-10 10:30 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(5/17) Factor out a startTouch method (4.09 KB, patch)
2012-01-10 10:31 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(6/17) Refactor duplicated code in track() functions (7.26 KB, patch)
2012-01-10 10:33 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(7/17) Encapsulate Axis::firstTouchPos (5.44 KB, patch)
2012-01-10 10:34 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos (3.21 KB, patch)
2012-01-10 10:35 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(9/17) Collapse unused FlingStates into STOPPED (4.37 KB, patch)
2012-01-10 10:41 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(10/17) Fix up the advanceFling() step (9.06 KB, patch)
2012-01-10 10:42 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(11/17) Use locked velocity instead of unlocked velocity (1.73 KB, patch)
2012-01-10 10:43 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(12/17) Encapsulate more Axis variables (6.53 KB, patch)
2012-01-10 10:45 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(13/17) Encapsulate fling state in Axis (4.08 KB, patch)
2012-01-10 10:46 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(14/17) Split a SubdocumentScrollHelper out of PZC (19.63 KB, patch)
2012-01-10 10:47 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(15/17) Split the Axis class out of PZC (21.53 KB, patch)
2012-01-10 10:48 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(16/17) Pull out hard-coded strings (3.45 KB, patch)
2012-01-10 10:50 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(17/17) Fix copyright/authors (1.56 KB, patch)
2012-01-10 10:51 PST, Kartikaya Gupta (email:kats@mozilla.com)
pwalton: review+
bugmail: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:01:34 PST
In an attempt to flush out any remaining bugs in PZC, I performed an in-depth review and refactoring of the code. Most of the effort ended up just being refactoring, but there were a few minor bugs/inconsistencies that I found and fixed along the way, but I didn't find any glaring problems. I'm going to attach all the patches I created here, even though most of them are just refactoring.

I'm not sure which of these patches should be landed - all, just the bugfix-type ones, or none. Feedback welcome.
Comment 1 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:02:19 PST
Created attachment 587122 [details] [diff] [review]
(1/17) Remove unnecessary finalize()
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:03:03 PST
Created attachment 587123 [details] [diff] [review]
(2/17) Refactor duplicate gesture listener code
Comment 3 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:03:38 PST
Created attachment 587124 [details] [diff] [review]
(3/17) Remove dead code for non-primary pointer events
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:06:55 PST
Created attachment 587127 [details] [diff] [review]
(4/17) Misc cleanups (mostly cosmetic)
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:12:04 PST
Created attachment 587131 [details] [diff] [review]
(5/17) Factor out a startTouch method
Comment 6 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:13:40 PST
Created attachment 587133 [details] [diff] [review]
(6/17) Refactor duplicated code in track() functions
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:14:13 PST
Created attachment 587134 [details] [diff] [review]
(7/17) Encapsulate Axis::firstTouchPos
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:14:44 PST
Created attachment 587135 [details] [diff] [review]
(8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos
Comment 9 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:15:10 PST
Created attachment 587136 [details] [diff] [review]
(9/17) Collapse unused FlingStates into STOPPED
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:15:54 PST
Created attachment 587137 [details] [diff] [review]
(10/17) Fix up the advanceFling() step
Comment 11 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:17:26 PST
Created attachment 587139 [details] [diff] [review]
(11/17) Use locked velocity instead of unlocked velocity
Comment 12 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:17:51 PST
Created attachment 587141 [details] [diff] [review]
(12/17) Encapsulate more Axis variables
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:18:17 PST
Created attachment 587142 [details] [diff] [review]
(14/17) Encapsulate fling state in Axis
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:18:41 PST
Created attachment 587143 [details] [diff] [review]
(14/17) Split a SubwindowScrollHelper out of PZC
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:19:09 PST
Created attachment 587145 [details] [diff] [review]
(15/17) Split the Axis class out of PZC
Comment 16 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:19:38 PST
Created attachment 587146 [details] [diff] [review]
(16/17) Pull out hard-coded strings
Comment 17 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:20:01 PST
Created attachment 587147 [details] [diff] [review]
(17/17) Fix copyright/authors
Comment 18 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 14:33:28 PST
The patches that weren't just cosmetic/refactoring:

Patch 2: Not strictly a bug, but there was the possibility of throwing RuntimeExceptions on the UI thread, so I replaced that with a more robust log-and-recover.

Patch 3: There was a typo in some code (mX.firstTouchPos = mY.touchPos = ...), but luckily that code is dead and so removing the entire chunk removed the incorrect code as well.

Patch 10: The velocity checking code was duplicated and inconsistent between the advanceFling() function and the FlingRunnable implementation. This cleans it up so that it's always consistent, and always uses the combined velocity from both axes to determine whether or not the fling has stopped.

Patch 11: A couple of places were using mX.velocity and mY.velocity where (I believe) using getRealVelocity() is more appropriate, since the flings should stop when the page stops visibly scrolling to the user.

Patch 14: There's a race condition in the subwindow scrolling because the message handler for Panning:Override can change mOverrideScrollAck while updatePosition() is running on the UI thread. Handling all messages on the UI thread should eliminate this race.
Comment 19 Patrick Walton (:pcwalton) 2012-01-09 16:13:53 PST
(In reply to Kartikaya Gupta (:kats) from comment #18)
> Patch 10: The velocity checking code was duplicated and inconsistent between
> the advanceFling() function and the FlingRunnable implementation. This
> cleans it up so that it's always consistent, and always uses the combined
> velocity from both axes to determine whether or not the fling has stopped.
> 
> Patch 11: A couple of places were using mX.velocity and mY.velocity where (I
> believe) using getRealVelocity() is more appropriate, since the flings
> should stop when the page stops visibly scrolling to the user.

I've seen cases in which we still stall on the edges for too long before bouncing back. It's pretty random and inconsistent. Have you seen this as well?

I guess I can play around with these patches and see if it fixes this issue. I would like to land these on Aurora if it does.
Comment 20 Patrick Walton (:pcwalton) 2012-01-09 16:19:34 PST
Comment on attachment 587124 [details] [diff] [review]
(3/17) Remove dead code for non-primary pointer events

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

Make sure to test this on several devices.
Comment 21 Patrick Walton (:pcwalton) 2012-01-09 16:22:24 PST
Comment on attachment 587127 [details] [diff] [review]
(4/17) Misc cleanups (mostly cosmetic)

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

::: mobile/android/base/ui/PanZoomController.java
@@ +893,5 @@
>          return viewportMetrics;
>      }
>  
>      private class AxisX extends Axis {
> +        @Override public float getOrigin() { return mController.getOrigin().x; }

nit: The usual style is to keep @Override on a separate line, as with all class attributes. I totally agree it looks weird, but that's how Java usually does it...
Comment 22 Patrick Walton (:pcwalton) 2012-01-09 16:26:29 PST
Comment on attachment 587133 [details] [diff] [review]
(6/17) Refactor duplicated code in track() functions

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

::: mobile/android/base/ui/PanZoomController.java
@@ +732,5 @@
>              locked = false;
>              firstTouchPos = touchPos = lastTouchPos = pos;
>          }
>  
> +        void updateWithTouchAt(float pos, float timeDelta) {

Package-protected method? I guess this is ok, but I'd probably say public...
Comment 23 Patrick Walton (:pcwalton) 2012-01-09 16:28:18 PST
Comment on attachment 587134 [details] [diff] [review]
(7/17) Encapsulate Axis::firstTouchPos

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

::: mobile/android/base/ui/PanZoomController.java
@@ +293,5 @@
>              // should never happen
>              Log.e(LOGTAG, "Received impossible touch move while in " + mState);
>              return false;
>          case TOUCHING:
> +            if (panDistance(event) < PAN_THRESHOLD * GeckoAppShell.getDpi()) {

remove {}

@@ +700,5 @@
>              PLUS,       // Overscrolled in the positive direction
>              BOTH,       // Overscrolled in both directions (page is zoomed to smaller than screen)
>          }
>  
> +        private float firstTouchPos;            /* Position of the first touch event on the current drag. */

Since this is private, should it be mFirstTouchPos instead?

@@ +734,5 @@
>              locked = false;
>              firstTouchPos = touchPos = lastTouchPos = pos;
>          }
>  
> +        float panDistance(float currentPos) {

Package protected again...
Comment 24 Patrick Walton (:pcwalton) 2012-01-09 16:29:06 PST
Comment on attachment 587135 [details] [diff] [review]
(8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos

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

::: mobile/android/base/ui/PanZoomController.java
@@ +702,5 @@
>          }
>  
>          private float firstTouchPos;            /* Position of the first touch event on the current drag. */
> +        private float touchPos;                 /* Position of the most recent touch event on the current drag. */
> +        private float lastTouchPos;             /* Position of the touch event before touchPos. */

Again, mTouchPos and mLastTouchPos.
Comment 25 Patrick Walton (:pcwalton) 2012-01-09 16:31:22 PST
Comment on attachment 587136 [details] [diff] [review]
(9/17) Collapse unused FlingStates into STOPPED

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

::: mobile/android/base/ui/PanZoomController.java
@@ +712,5 @@
>          protected abstract float getPageLength();
>  
>          public float displacement;
>  
> +        public Axis() {}

You can just remove this constructor entirely. Yay!

@@ +801,5 @@
>              return locked ? 0.0f : velocity;
>          }
>  
>          public void startFling(boolean stopped) {
> +            if (stopped) {

nit: no {}
Comment 26 Patrick Walton (:pcwalton) 2012-01-09 16:32:41 PST
Comment on attachment 587137 [details] [diff] [review]
(10/17) Fix up the advanceFling() step

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

::: mobile/android/base/ui/PanZoomController.java
@@ -507,5 @@
>          return FloatMath.sqrt(xvel * xvel + yvel * yvel);
>      }
>  
> -    private boolean stopped() {
> -        float absVelocity = (float)Math.sqrt(mX.velocity * mX.velocity +

Why remove this? I guess it'll turn into a trivial function, so I'm ok with removing it, but I'd probably have left it in for clarity.
Comment 27 Patrick Walton (:pcwalton) 2012-01-09 16:33:32 PST
Comment on attachment 587139 [details] [diff] [review]
(11/17) Use locked velocity instead of unlocked velocity

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

You uploaded the same patch twice. I think this is the right one, and 10/17 is the wrong one (a duplicate of this).
Comment 28 Patrick Walton (:pcwalton) 2012-01-09 16:33:49 PST
Comment on attachment 587137 [details] [diff] [review]
(10/17) Fix up the advanceFling() step

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

This is the wrong patch.
Comment 29 Patrick Walton (:pcwalton) 2012-01-09 16:34:31 PST
Comment on attachment 587141 [details] [diff] [review]
(12/17) Encapsulate more Axis variables

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

::: mobile/android/base/ui/PanZoomController.java
@@ +702,5 @@
>          private float touchPos;                 /* Position of the most recent touch event on the current drag. */
>          private float lastTouchPos;             /* Position of the touch event before touchPos. */
> +        private float velocity;                  /* Velocity in this direction. */
> +        private boolean locked;                  /* Whether movement on this axis is locked. */
> +        private boolean disableSnap;             /* Whether overscroll snapping is disabled. */

mVelocity, mLocked, mDisableSnap
Comment 30 Patrick Walton (:pcwalton) 2012-01-09 16:35:23 PST
Comment on attachment 587142 [details] [diff] [review]
(14/17) Encapsulate fling state in Axis

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

Wrong patch; this should be 13/17 but is a duplicate of 14/17.
Comment 31 Patrick Walton (:pcwalton) 2012-01-09 16:37:26 PST
Comment on attachment 587143 [details] [diff] [review]
(14/17) Split a SubwindowScrollHelper out of PZC

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

Thanks for doing this, this helps a lot.

One nit: I think we've been calling these "subdocument frames" instead of "subwindows".

::: mobile/android/base/ui/PanZoomController.java
@@ +872,5 @@
>          return viewportMetrics;
>      }
>  
>      private class AxisX extends Axis {
> +        AxisX(SubwindowScrollHelper subscroller) { super(subscroller); }

Private classes should be able to access private members of their enclosing classes, right?

In other words I don't think you need this constructor.
Comment 32 Patrick Walton (:pcwalton) 2012-01-09 16:39:57 PST
Comment on attachment 587145 [details] [diff] [review]
(15/17) Split the Axis class out of PZC

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

Oh, I see, you added the m prefixes here. Ignore the feedback-s above in those cases then.

::: mobile/android/base/ui/Axis.java
@@ +39,5 @@
> +package org.mozilla.gecko.ui;
> +
> +import org.mozilla.gecko.FloatUtils;
> +
> +abstract class Axis {

Add a Javadoc comment explaining what this is for; "Axis" is a very generic term.
Comment 33 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 20:46:55 PST
(In reply to Patrick Walton (:pcwalton) from comment #19)
> I've seen cases in which we still stall on the edges for too long before
> bouncing back. It's pretty random and inconsistent. Have you seen this as
> well?

The only behaviour related to this I recall seeing is the one where we wait for both axes to stop flinging before bouncing back.

> I guess I can play around with these patches and see if it fixes this issue.
> I would like to land these on Aurora if it does.

Yeah, let me know if this fixes the issue. I can provide you with a build that has these patches if don't want to build it yourself. Just ping me on IRC.

(In reply to Patrick Walton (:pcwalton) from comment #21)
> nit: The usual style is to keep @Override on a separate line, as with all
> class attributes. I totally agree it looks weird, but that's how Java
> usually does it...

I've seen it both ways, but sure, I can change that.

(In reply to Patrick Walton (:pcwalton) from comment #22)
> Package-protected method? I guess this is ok, but I'd probably say public...

I wanted to keep the public methods to a minimum. There's no reason to make them public, and it just increases the surface area of things that can go wrong. Particularly since this stuff is thread-sensitive and always needs to run on the UI thread. It's easier to ensure that within a package than it is over the entire codebase, and easier to tell that at a glance when looking over the code.

(In reply to Patrick Walton (:pcwalton) from comment #23)
> > +            if (panDistance(event) < PAN_THRESHOLD * GeckoAppShell.getDpi()) {
> 
> remove {}

I think the consensus is that for Java code we're keeping braces for one-liners. This will be flagged by the checkstyle config in bug 716137. If it's a big sticking point for you I can change it and hope it gets changed back later when checkstyle is enabled :)

(In reply to Patrick Walton (:pcwalton) from comment #26)
> > -    private boolean stopped() {
> > -        float absVelocity = (float)Math.sqrt(mX.velocity * mX.velocity +
> 
> Why remove this? I guess it'll turn into a trivial function, so I'm ok with
> removing it, but I'd probably have left it in for clarity.

Ok, I'll put it back in.

(In reply to Patrick Walton (:pcwalton) from comment #28)
> This is the wrong patch.

Whoops. Will re-upload correct one, same for 13/14.

(In reply to Patrick Walton (:pcwalton) from comment #31)
> Private classes should be able to access private members of their enclosing
> classes, right?
> 
> In other words I don't think you need this constructor.

In this and a few other cases (the mXXX variable naming and empty constructor) are a result of the way I did these changes - I made all the changes first and then went back and split them up into logically grouped patches. So in some cases things were left in a previous patch because I knew they would be needed in a future patch, and it resulted in more readable diffs that way.

(In reply to Patrick Walton (:pcwalton) from comment #32)
> Add a Javadoc comment explaining what this is for; "Axis" is a very generic
> term.

Will do.
Comment 34 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 20:54:32 PST
Created attachment 587239 [details] [diff] [review]
(10/17) Fix up the advanceFling() step

Update patch #10 to correct patch. Ironic I screwed this one up since it's probably the patch with the most actual change.
Comment 35 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-09 20:55:52 PST
Created attachment 587240 [details] [diff] [review]
(13/17) Encapsulate fling state in Axis

Update patch #13 (previously mislabeled as #14) to correct patch.
Comment 36 Patrick Walton (:pcwalton) 2012-01-09 21:53:41 PST
Comment on attachment 587239 [details] [diff] [review]
(10/17) Fix up the advanceFling() step

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

Looks good.
Comment 37 Patrick Walton (:pcwalton) 2012-01-09 21:56:54 PST
(In reply to Kartikaya Gupta (:kats) from comment #33)
> I wanted to keep the public methods to a minimum. There's no reason to make
> them public, and it just increases the surface area of things that can go
> wrong. Particularly since this stuff is thread-sensitive and always needs to
> run on the UI thread. It's easier to ensure that within a package than it is
> over the entire codebase, and easier to tell that at a glance when looking
> over the code.

Yeah, I didn't see that you had moved it out from being an inner class at that time, so package protected is fine there.

> I think the consensus is that for Java code we're keeping braces for
> one-liners. This will be flagged by the checkstyle config in bug 716137. If
> it's a big sticking point for you I can change it and hope it gets changed
> back later when checkstyle is enabled :)

Not a big sticking point for me, just wanted to ensure consistency. If we're going with braced ifs throughout the codebase then we can leave them.
Comment 38 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:26:32 PST
Pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=924e1fcea62c and it was green. I'm going to re-upload the patches with the fixes discussed above and updated commit messages. Sorry in advance for the bugspam..
Comment 39 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:27:48 PST
Created attachment 587366 [details] [diff] [review]
(1/17) Remove unnecessary finalize()

I'll leave feedback+ on the patches that I didn't really touch from last time.
Comment 40 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:28:41 PST
Created attachment 587368 [details] [diff] [review]
(2/17) Refactor duplicate gesture listener code
Comment 41 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:29:26 PST
Created attachment 587369 [details] [diff] [review]
(3/17) Remove dead code for non-primary pointer events
Comment 42 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:30:47 PST
Created attachment 587371 [details] [diff] [review]
(4/17) Misc cleanups (mostly cosmetic)
Comment 43 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:31:53 PST
Created attachment 587372 [details] [diff] [review]
(5/17) Factor out a startTouch method
Comment 44 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:33:33 PST
Created attachment 587373 [details] [diff] [review]
(6/17) Refactor duplicated code in track() functions
Comment 45 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:34:41 PST
Created attachment 587374 [details] [diff] [review]
(7/17) Encapsulate Axis::firstTouchPos
Comment 46 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:35:42 PST
Created attachment 587375 [details] [diff] [review]
(8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos
Comment 47 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:41:03 PST
Created attachment 587381 [details] [diff] [review]
(9/17) Collapse unused FlingStates into STOPPED
Comment 48 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:42:12 PST
Created attachment 587382 [details] [diff] [review]
(10/17) Fix up the advanceFling() step
Comment 49 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:43:54 PST
Created attachment 587383 [details] [diff] [review]
(11/17) Use locked velocity instead of unlocked velocity
Comment 50 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:45:25 PST
Created attachment 587384 [details] [diff] [review]
(12/17) Encapsulate more Axis variables
Comment 51 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:46:39 PST
Created attachment 587385 [details] [diff] [review]
(13/17) Encapsulate fling state in Axis
Comment 52 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:47:45 PST
Created attachment 587387 [details] [diff] [review]
(14/17) Split a SubdocumentScrollHelper out of PZC

Renamed this to SubdocumentScrollHelper
Comment 53 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:48:37 PST
Created attachment 587388 [details] [diff] [review]
(15/17) Split the Axis class out of PZC
Comment 54 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:50:03 PST
Created attachment 587389 [details] [diff] [review]
(16/17) Pull out hard-coded strings
Comment 55 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 10:51:06 PST
Created attachment 587390 [details] [diff] [review]
(17/17) Fix copyright/authors
Comment 56 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-10 11:20:23 PST
*** Bug 716325 has been marked as a duplicate of this bug. ***
Comment 57 Patrick Walton (:pcwalton) 2012-01-10 14:13:00 PST
Comment on attachment 587387 [details] [diff] [review]
(14/17) Split a SubdocumentScrollHelper out of PZC

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

::: mobile/android/base/ui/SubdocumentScrollHelper.java
@@ +108,5 @@
> +
> +    public void handleMessage(final String event, final JSONObject message) {
> +        // this comes in on the gecko thread; hand off the handling to the UI thread
> +        mUiHandler.post(new Runnable() {
> +            public void run() {

nit: @Override
Comment 58 Patrick Walton (:pcwalton) 2012-01-10 14:15:07 PST
Comment on attachment 587388 [details] [diff] [review]
(15/17) Split the Axis class out of PZC

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

::: mobile/android/base/ui/Axis.java
@@ +41,5 @@
> +import org.mozilla.gecko.FloatUtils;
> +
> +/**
> + * This class represents the physics for one axis of movement (i.e. either
> + * horizontal or vertical. It tracks the different properties of movement

nit: vertical).
Comment 61 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-12 09:26:06 PST
These patches will not apply cleanly on aurora without the patch from bug 714711, so adding that as a dependency.
Comment 62 Kartikaya Gupta (email:kats@mozilla.com) 2012-01-12 14:29:17 PST
Comment on attachment 587366 [details] [diff] [review]
(1/17) Remove unnecessary finalize()

[Approval Request Comment] (for all the patches combined)

Regression caused by (bug #): none
User impact if declined: Weird behaviour when finishing a fling, errors during iframe scrolling.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): possible regressions in pan/zoom behaviour
Comment 63 Alex Keybl [:akeybl] 2012-01-12 22:26:16 PST
Comment on attachment 587366 [details] [diff] [review]
(1/17) Remove unnecessary finalize()

[Triage Comment]
Mobile only - approving for Aurora.

Note You need to log in before you can comment on or make changes to this bug.