716673 - PanZoomController needs an in-depth review/clean up

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P4)

Description

[Kartikaya Gupta (email:[e-mail])]

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:[e-mail])]

Attachment: (1/17) Remove unnecessary finalize()

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/17) Refactor duplicate gesture listener code

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/17) Remove dead code for non-primary pointer events

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: (4/17) Misc cleanups (mostly cosmetic)

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: (5/17) Factor out a startTouch method

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: (6/17) Refactor duplicated code in track() functions

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: (7/17) Encapsulate Axis::firstTouchPos

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: (8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: (9/17) Collapse unused FlingStates into STOPPED

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: (10/17) Fix up the advanceFling() step

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: (11/17) Use locked velocity instead of unlocked velocity

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: (12/17) Encapsulate more Axis variables

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: (14/17) Encapsulate fling state in Axis

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: (14/17) Split a SubwindowScrollHelper out of PZC

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: (15/17) Split the Axis class out of PZC

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: (16/17) Pull out hard-coded strings

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Attachment: (17/17) Fix copyright/authors

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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

(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)]

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

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

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

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

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

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

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

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

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

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

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

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

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:[e-mail])]

(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:[e-mail])]

Attachment: (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:[e-mail])]

Attachment: (13/17) Encapsulate fling state in Axis

Update patch #13 (previously mislabeled as #14) to correct patch.

Comment 36

[Patrick Walton (:pcwalton)]

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

(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:[e-mail])]

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:[e-mail])]

Attachment: (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:[e-mail])]

Attachment: (2/17) Refactor duplicate gesture listener code

Comment 41

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/17) Remove dead code for non-primary pointer events

Comment 42

[Kartikaya Gupta (email:[e-mail])]

Attachment: (4/17) Misc cleanups (mostly cosmetic)

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Attachment: (5/17) Factor out a startTouch method

Comment 44

[Kartikaya Gupta (email:[e-mail])]

Attachment: (6/17) Refactor duplicated code in track() functions

Comment 45

[Kartikaya Gupta (email:[e-mail])]

Attachment: (7/17) Encapsulate Axis::firstTouchPos

Comment 46

[Kartikaya Gupta (email:[e-mail])]

Attachment: (8/17) Encapsulate Axis::lastTouchPos and Axis::touchPos

Comment 47

[Kartikaya Gupta (email:[e-mail])]

Attachment: (9/17) Collapse unused FlingStates into STOPPED

Comment 48

[Kartikaya Gupta (email:[e-mail])]

Attachment: (10/17) Fix up the advanceFling() step

Comment 49

[Kartikaya Gupta (email:[e-mail])]

Attachment: (11/17) Use locked velocity instead of unlocked velocity

Comment 50

[Kartikaya Gupta (email:[e-mail])]

Attachment: (12/17) Encapsulate more Axis variables

Comment 51

[Kartikaya Gupta (email:[e-mail])]

Attachment: (13/17) Encapsulate fling state in Axis

Comment 52

[Kartikaya Gupta (email:[e-mail])]

Attachment: (14/17) Split a SubdocumentScrollHelper out of PZC

Renamed this to SubdocumentScrollHelper

Comment 53

[Kartikaya Gupta (email:[e-mail])]

Attachment: (15/17) Split the Axis class out of PZC

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Attachment: (16/17) Pull out hard-coded strings

Comment 55

[Kartikaya Gupta (email:[e-mail])]

Attachment: (17/17) Fix copyright/authors

Comment 57

[Patrick Walton (:pcwalton)]

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

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 59

[Kartikaya Gupta (email:[e-mail])]

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

Comment 60

[Ed Morley [:emorley]]

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

Comment 61

[Kartikaya Gupta (email:[e-mail])]

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:[e-mail])]

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]]

Comment on attachment 587366 [details] [diff] [review]
(1/17) Remove unnecessary finalize()

[Triage Comment]
Mobile only - approving for Aurora.

Comment 64

[Kartikaya Gupta (email:[e-mail])]

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