The default bug view has changed. See this FAQ.

PanZoomController needs an in-depth review/clean up

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
P4
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
Firefox 12
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

Attachments

(17 attachments, 19 obsolete attachments)

1.82 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
4.04 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
2.27 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
15.16 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
4.09 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
7.26 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
5.44 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
3.21 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
4.37 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
9.06 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
1.73 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
6.53 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
4.08 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
19.63 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
21.53 KB, patch
pcwalton
: review+
Details | Diff | Splinter Review
3.45 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
1.56 KB, patch
pcwalton
: review+
kats
: feedback+
Details | Diff | Splinter Review
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.
Created attachment 587122 [details] [diff] [review]
(1/17) Remove unnecessary finalize()
Created attachment 587123 [details] [diff] [review]
(2/17) Refactor duplicate gesture listener code
Created attachment 587124 [details] [diff] [review]
(3/17) Remove dead code for non-primary pointer events
Created attachment 587127 [details] [diff] [review]
(4/17) Misc cleanups (mostly cosmetic)
Created attachment 587131 [details] [diff] [review]
(5/17) Factor out a startTouch method
Created attachment 587133 [details] [diff] [review]
(6/17) Refactor duplicated code in track() functions
Created attachment 587134 [details] [diff] [review]
(7/17) Encapsulate Axis::firstTouchPos
Created attachment 587135 [details] [diff] [review]
(8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos
Created attachment 587136 [details] [diff] [review]
(9/17) Collapse unused FlingStates into STOPPED
Created attachment 587137 [details] [diff] [review]
(10/17) Fix up the advanceFling() step
Attachment #587137 - Attachment description: (11/17) [fix] Fix up the advanceFling() step → (10/17) Fix up the advanceFling() step
Created attachment 587139 [details] [diff] [review]
(11/17) Use locked velocity instead of unlocked velocity
Created attachment 587141 [details] [diff] [review]
(12/17) Encapsulate more Axis variables
Created attachment 587142 [details] [diff] [review]
(14/17) Encapsulate fling state in Axis
Created attachment 587143 [details] [diff] [review]
(14/17) Split a SubwindowScrollHelper out of PZC
Created attachment 587145 [details] [diff] [review]
(15/17) Split the Axis class out of PZC
Created attachment 587146 [details] [diff] [review]
(16/17) Pull out hard-coded strings
Created attachment 587147 [details] [diff] [review]
(17/17) Fix copyright/authors
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.
Blocks: 716325
Priority: -- → P4
(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.
Attachment #587122 - Flags: feedback+
Attachment #587123 - Flags: feedback+
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.
Attachment #587124 - Flags: feedback+
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...
Attachment #587127 - Flags: feedback+
Attachment #587131 - Flags: feedback+
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...
Attachment #587133 - Flags: feedback+
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...
Attachment #587134 - Flags: feedback-
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.
Attachment #587135 - Flags: feedback-
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 {}
Attachment #587136 - Flags: feedback+
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.
Attachment #587137 - Flags: feedback+
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).
Attachment #587139 - Flags: feedback+
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.
Attachment #587137 - Flags: feedback+ → feedback-
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
Attachment #587141 - Flags: feedback-
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.
Attachment #587142 - Flags: feedback-
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.
Attachment #587143 - Flags: feedback+
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.
Attachment #587145 - Flags: feedback+
Attachment #587146 - Flags: feedback+
Attachment #587147 - Flags: feedback+
(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.
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.
Attachment #587137 - Attachment is obsolete: true
Created attachment 587240 [details] [diff] [review]
(13/17) Encapsulate fling state in Axis

Update patch #13 (previously mislabeled as #14) to correct patch.
Attachment #587142 - Attachment is obsolete: true
Comment on attachment 587239 [details] [diff] [review]
(10/17) Fix up the advanceFling() step

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

Looks good.
Attachment #587239 - Flags: feedback+
Attachment #587240 - Flags: feedback+
(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.
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..
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.
Attachment #587122 - Attachment is obsolete: true
Attachment #587366 - Flags: review?(pwalton)
Attachment #587366 - Flags: feedback+
Created attachment 587368 [details] [diff] [review]
(2/17) Refactor duplicate gesture listener code
Attachment #587123 - Attachment is obsolete: true
Attachment #587368 - Flags: review?(pwalton)
Attachment #587368 - Flags: feedback+
Created attachment 587369 [details] [diff] [review]
(3/17) Remove dead code for non-primary pointer events
Attachment #587124 - Attachment is obsolete: true
Attachment #587369 - Flags: review?(pwalton)
Attachment #587369 - Flags: feedback+
Created attachment 587371 [details] [diff] [review]
(4/17) Misc cleanups (mostly cosmetic)
Attachment #587127 - Attachment is obsolete: true
Attachment #587371 - Flags: review?(pwalton)
Created attachment 587372 [details] [diff] [review]
(5/17) Factor out a startTouch method
Attachment #587131 - Attachment is obsolete: true
Attachment #587372 - Flags: review?(pwalton)
Attachment #587372 - Flags: feedback+
Created attachment 587373 [details] [diff] [review]
(6/17) Refactor duplicated code in track() functions
Attachment #587133 - Attachment is obsolete: true
Attachment #587373 - Flags: review?(pwalton)
Attachment #587373 - Flags: feedback+
Created attachment 587374 [details] [diff] [review]
(7/17) Encapsulate Axis::firstTouchPos
Attachment #587134 - Attachment is obsolete: true
Attachment #587374 - Flags: review?(pwalton)
Created attachment 587375 [details] [diff] [review]
(8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos
Attachment #587135 - Attachment is obsolete: true
Attachment #587375 - Flags: review?(pwalton)
Created attachment 587381 [details] [diff] [review]
(9/17) Collapse unused FlingStates into STOPPED
Attachment #587136 - Attachment is obsolete: true
Attachment #587381 - Flags: review?(pwalton)
Attachment #587381 - Flags: feedback+
Created attachment 587382 [details] [diff] [review]
(10/17) Fix up the advanceFling() step
Attachment #587239 - Attachment is obsolete: true
Attachment #587382 - Flags: review?(pwalton)
Created attachment 587383 [details] [diff] [review]
(11/17) Use locked velocity instead of unlocked velocity
Attachment #587139 - Attachment is obsolete: true
Attachment #587383 - Flags: review?(pwalton)
Created attachment 587384 [details] [diff] [review]
(12/17) Encapsulate more Axis variables
Attachment #587141 - Attachment is obsolete: true
Attachment #587384 - Flags: review?(pwalton)
Created attachment 587385 [details] [diff] [review]
(13/17) Encapsulate fling state in Axis
Attachment #587240 - Attachment is obsolete: true
Attachment #587385 - Flags: review?(pwalton)
Attachment #587385 - Flags: feedback+
Created attachment 587387 [details] [diff] [review]
(14/17) Split a SubdocumentScrollHelper out of PZC

Renamed this to SubdocumentScrollHelper
Attachment #587143 - Attachment is obsolete: true
Attachment #587387 - Flags: review?(pwalton)
Created attachment 587388 [details] [diff] [review]
(15/17) Split the Axis class out of PZC
Attachment #587145 - Attachment is obsolete: true
Attachment #587388 - Flags: review?(pwalton)
Created attachment 587389 [details] [diff] [review]
(16/17) Pull out hard-coded strings
Attachment #587146 - Attachment is obsolete: true
Attachment #587389 - Flags: review?(pwalton)
Attachment #587389 - Flags: feedback+
Attachment #587366 - Flags: review?(pwalton) → review+
Created attachment 587390 [details] [diff] [review]
(17/17) Fix copyright/authors
Attachment #587147 - Attachment is obsolete: true
Attachment #587390 - Flags: review?(pwalton)
Attachment #587390 - Flags: feedback+
Duplicate of this bug: 716325
Attachment #587368 - Flags: review?(pwalton) → review+
Attachment #587369 - Flags: review?(pwalton) → review+
Attachment #587371 - Flags: review?(pwalton) → review+
Attachment #587372 - Flags: review?(pwalton) → review+
Attachment #587373 - Flags: review?(pwalton) → review+
Attachment #587374 - Flags: review?(pwalton) → review+
Attachment #587375 - Flags: review?(pwalton) → review+
Attachment #587381 - Flags: review?(pwalton) → review+
Attachment #587382 - Flags: review?(pwalton) → review+
Attachment #587383 - Flags: review?(pwalton) → review+
Attachment #587384 - Flags: review?(pwalton) → review+
Attachment #587385 - Flags: review?(pwalton) → review+
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
Attachment #587387 - Flags: review?(pwalton) → review+
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).
Attachment #587388 - Flags: review?(pwalton) → review+
Attachment #587389 - Flags: review?(pwalton) → review+
Attachment #587390 - Flags: review?(pwalton) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8974d08b3e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/d742721fc832
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcf520615ac2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b10d778278b
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f670580388f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7eb5f95bb59
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f876211d7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3130c6f285
https://hg.mozilla.org/integration/mozilla-inbound/rev/82efff89bd76
https://hg.mozilla.org/integration/mozilla-inbound/rev/72528d442842
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee48470bdaf3
https://hg.mozilla.org/integration/mozilla-inbound/rev/821ffb38adf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/260031a8af9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/66766a647921
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e0ff7b92eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c6c054efeb2
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb912e0dba67
tracking-fennec: --- → ?
status-firefox11: --- → affected
status-firefox12: --- → affected
https://hg.mozilla.org/mozilla-central/rev/f8974d08b3e5
https://hg.mozilla.org/mozilla-central/rev/d742721fc832
https://hg.mozilla.org/mozilla-central/rev/dcf520615ac2
https://hg.mozilla.org/mozilla-central/rev/9b10d778278b
https://hg.mozilla.org/mozilla-central/rev/4f670580388f
https://hg.mozilla.org/mozilla-central/rev/c7eb5f95bb59
https://hg.mozilla.org/mozilla-central/rev/b4f876211d7e
https://hg.mozilla.org/mozilla-central/rev/9f3130c6f285
https://hg.mozilla.org/mozilla-central/rev/82efff89bd76
https://hg.mozilla.org/mozilla-central/rev/72528d442842
https://hg.mozilla.org/mozilla-central/rev/ee48470bdaf3
https://hg.mozilla.org/mozilla-central/rev/821ffb38adf8
https://hg.mozilla.org/mozilla-central/rev/260031a8af9a
https://hg.mozilla.org/mozilla-central/rev/66766a647921
https://hg.mozilla.org/mozilla-central/rev/c9e0ff7b92eb
https://hg.mozilla.org/mozilla-central/rev/2c6c054efeb2
https://hg.mozilla.org/mozilla-central/rev/cb912e0dba67
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
No longer blocks: 716325
tracking-fennec: ? → 11+
These patches will not apply cleanly on aurora without the patch from bug 714711, so adding that as a dependency.
Depends on: 714711
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
Attachment #587366 - Flags: approval-mozilla-aurora?
Attachment #587368 - Flags: approval-mozilla-aurora?
Attachment #587369 - Flags: approval-mozilla-aurora?
Attachment #587371 - Flags: approval-mozilla-aurora?
Attachment #587372 - Flags: approval-mozilla-aurora?
Attachment #587373 - Flags: approval-mozilla-aurora?
Attachment #587374 - Flags: approval-mozilla-aurora?
Attachment #587375 - Flags: approval-mozilla-aurora?
Attachment #587381 - Flags: approval-mozilla-aurora?
Attachment #587382 - Flags: approval-mozilla-aurora?
Attachment #587383 - Flags: approval-mozilla-aurora?
Attachment #587384 - Flags: approval-mozilla-aurora?
Attachment #587385 - Flags: approval-mozilla-aurora?
Attachment #587387 - Flags: approval-mozilla-aurora?
Attachment #587388 - Flags: approval-mozilla-aurora?
Attachment #587389 - Flags: approval-mozilla-aurora?
Attachment #587390 - Flags: approval-mozilla-aurora?
Depends on: 714874
Comment on attachment 587366 [details] [diff] [review]
(1/17) Remove unnecessary finalize()

[Triage Comment]
Mobile only - approving for Aurora.
Attachment #587366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587368 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587369 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587371 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587372 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587373 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587374 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587375 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587381 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587382 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587383 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587384 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587385 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587387 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587388 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587389 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #587390 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b196abd31d1d
https://hg.mozilla.org/releases/mozilla-aurora/rev/412fc777469b
https://hg.mozilla.org/releases/mozilla-aurora/rev/142e0e5764d3
https://hg.mozilla.org/releases/mozilla-aurora/rev/ec0d8180ab04
https://hg.mozilla.org/releases/mozilla-aurora/rev/13263db01912
https://hg.mozilla.org/releases/mozilla-aurora/rev/c13147bd3526
https://hg.mozilla.org/releases/mozilla-aurora/rev/0594ad266dfa
https://hg.mozilla.org/releases/mozilla-aurora/rev/131dac883ec6
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d2cccb4d3eb
https://hg.mozilla.org/releases/mozilla-aurora/rev/4212d78cbee7
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ce2cc8bfbea
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d9ada0705df
https://hg.mozilla.org/releases/mozilla-aurora/rev/b4ec0b5d9b5a
https://hg.mozilla.org/releases/mozilla-aurora/rev/7e3b3f7bfc23
https://hg.mozilla.org/releases/mozilla-aurora/rev/639872e98bbc
https://hg.mozilla.org/releases/mozilla-aurora/rev/24ca2613ebfa
https://hg.mozilla.org/releases/mozilla-aurora/rev/579583350bf3
status-firefox11: affected → fixed
status-firefox12: affected → fixed
You need to log in before you can comment on or make changes to this bug.