Closed
Bug 716673
Opened 12 years ago
Closed 12 years ago
PanZoomController needs an in-depth review/clean up
Categories
(Firefox for Android Graveyard :: General, defect, P4)
Tracking
(firefox11 fixed, firefox12 fixed, fennec11+)
RESOLVED
FIXED
Firefox 12
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(17 files, 19 obsolete files)
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.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #587137 -
Attachment description: (11/17) [fix] Fix up the advanceFling() step → (10/17) Fix up the advanceFling() step
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Comment 12•12 years ago
|
||
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
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.
Updated•12 years ago
|
Priority: -- → P4
Comment 19•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #587122 -
Flags: feedback+
Updated•12 years ago
|
Attachment #587123 -
Flags: feedback+
Comment 20•12 years ago
|
||
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 21•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #587131 -
Flags: feedback+
Comment 22•12 years ago
|
||
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 23•12 years ago
|
||
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 24•12 years ago
|
||
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 25•12 years ago
|
||
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 26•12 years ago
|
||
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 27•12 years ago
|
||
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 28•12 years ago
|
||
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 29•12 years ago
|
||
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 30•12 years ago
|
||
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 31•12 years ago
|
||
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 32•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #587146 -
Flags: feedback+
Updated•12 years ago
|
Attachment #587147 -
Flags: feedback+
Assignee | ||
Comment 33•12 years ago
|
||
(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.
Assignee | ||
Comment 34•12 years ago
|
||
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
Assignee | ||
Comment 35•12 years ago
|
||
Update patch #13 (previously mislabeled as #14) to correct patch.
Attachment #587142 -
Attachment is obsolete: true
Comment 36•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #587240 -
Flags: feedback+
Comment 37•12 years ago
|
||
(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.
Assignee | ||
Comment 38•12 years ago
|
||
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..
Assignee | ||
Comment 39•12 years ago
|
||
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+
Assignee | ||
Comment 40•12 years ago
|
||
Attachment #587123 -
Attachment is obsolete: true
Attachment #587368 -
Flags: review?(pwalton)
Attachment #587368 -
Flags: feedback+
Assignee | ||
Comment 41•12 years ago
|
||
Attachment #587124 -
Attachment is obsolete: true
Attachment #587369 -
Flags: review?(pwalton)
Attachment #587369 -
Flags: feedback+
Assignee | ||
Comment 42•12 years ago
|
||
Attachment #587127 -
Attachment is obsolete: true
Attachment #587371 -
Flags: review?(pwalton)
Assignee | ||
Comment 43•12 years ago
|
||
Attachment #587131 -
Attachment is obsolete: true
Attachment #587372 -
Flags: review?(pwalton)
Attachment #587372 -
Flags: feedback+
Assignee | ||
Comment 44•12 years ago
|
||
Attachment #587133 -
Attachment is obsolete: true
Attachment #587373 -
Flags: review?(pwalton)
Attachment #587373 -
Flags: feedback+
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #587134 -
Attachment is obsolete: true
Attachment #587374 -
Flags: review?(pwalton)
Assignee | ||
Comment 46•12 years ago
|
||
Attachment #587135 -
Attachment is obsolete: true
Attachment #587375 -
Flags: review?(pwalton)
Assignee | ||
Comment 47•12 years ago
|
||
Attachment #587136 -
Attachment is obsolete: true
Attachment #587381 -
Flags: review?(pwalton)
Attachment #587381 -
Flags: feedback+
Assignee | ||
Comment 48•12 years ago
|
||
Attachment #587239 -
Attachment is obsolete: true
Attachment #587382 -
Flags: review?(pwalton)
Assignee | ||
Comment 49•12 years ago
|
||
Attachment #587139 -
Attachment is obsolete: true
Attachment #587383 -
Flags: review?(pwalton)
Assignee | ||
Comment 50•12 years ago
|
||
Attachment #587141 -
Attachment is obsolete: true
Attachment #587384 -
Flags: review?(pwalton)
Assignee | ||
Comment 51•12 years ago
|
||
Attachment #587240 -
Attachment is obsolete: true
Attachment #587385 -
Flags: review?(pwalton)
Attachment #587385 -
Flags: feedback+
Assignee | ||
Comment 52•12 years ago
|
||
Renamed this to SubdocumentScrollHelper
Attachment #587143 -
Attachment is obsolete: true
Attachment #587387 -
Flags: review?(pwalton)
Assignee | ||
Comment 53•12 years ago
|
||
Attachment #587145 -
Attachment is obsolete: true
Attachment #587388 -
Flags: review?(pwalton)
Assignee | ||
Comment 54•12 years ago
|
||
Attachment #587146 -
Attachment is obsolete: true
Attachment #587389 -
Flags: review?(pwalton)
Attachment #587389 -
Flags: feedback+
Updated•12 years ago
|
Attachment #587366 -
Flags: review?(pwalton) → review+
Assignee | ||
Comment 55•12 years ago
|
||
Attachment #587147 -
Attachment is obsolete: true
Attachment #587390 -
Flags: review?(pwalton)
Attachment #587390 -
Flags: feedback+
Updated•12 years ago
|
Attachment #587368 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587369 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587371 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587372 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587373 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587374 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587375 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587381 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587382 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587383 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587384 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587385 -
Flags: review?(pwalton) → review+
Comment 57•12 years ago
|
||
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 58•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #587389 -
Flags: review?(pwalton) → review+
Updated•12 years ago
|
Attachment #587390 -
Flags: review?(pwalton) → review+
Assignee | ||
Comment 59•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Comment 60•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Updated•12 years ago
|
tracking-fennec: ? → 11+
Assignee | ||
Comment 61•12 years ago
|
||
These patches will not apply cleanly on aurora without the patch from bug 714711, so adding that as a dependency.
Depends on: 714711
Assignee | ||
Comment 62•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Attachment #587368 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587369 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587371 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587372 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587373 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587374 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587375 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587381 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587382 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587383 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587384 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587385 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587387 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587388 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587389 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #587390 -
Flags: approval-mozilla-aurora?
Comment 63•12 years ago
|
||
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•12 years ago
|
Attachment #587368 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587369 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587371 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587372 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587373 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587374 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587375 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587381 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587382 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587383 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587384 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587385 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587387 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587388 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587389 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #587390 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 64•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•