456621 - Kinetic properties should be configurable

Status: VERIFIED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Stuff like the threshold to start a kinetic pan, the rate it slows down and the number of events to track should be kept in prefs.  I don't think we need UI to adjust them though, this is the sort of thing about:config was made for.

Comment 1

[Mike Kristoffersen (:MikeK)]

Attachment: Proposed fix to this bug

Comment 2

[Mike Kristoffersen (:MikeK)]

Attachment: A better fix for the bug

Comment 3

[Mike Kristoffersen (:MikeK)]

Attachment: Now with a typo less :)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 361886 [details] [diff] [review]
Now with a typo less :)

>diff --git a/app/mobile.js b/app/mobile.js

>+pref("browser.ui.panning.deadtime", 200);  // A panning gesture longer than this in millisecs will be accepted 

How about "amount of time to wait after a click until a panning gesture is detected."?

>+pref("browser.ui.panning.deadzone", 100);  // A panning gesture longer than the square root of this in pixels will be accepted

Seems like we should move the square root calculation into the code, and change this to 10.

>+pref("browser.ui.panning.acceleration", -4);  // The acceleration the screen will expirience after the mouse let go of it (/1000)

s/expirience/experience/

>+pref("browser.ui.panning.buffersize", 3);  // The number of samples to detect gesture speed

nit: bufferSize

"The number of past mouse events used to determine gesture speed"?

nit: s/buffersize/bufferSize/

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

> function KineticPanningModule(owner) {
>   this._owner = owner;
>+  this._kineticData.reset();
> }

> function PanningModule(owner) {
>   this._owner = owner;
>+  this._dragData.reset();
> }

I wonder if it makes sense to have these initialize lazily to avoid impacting startup... probably won't make much of a difference.

>     reset: function() {
>+      let prefsvc = Components.classes["@mozilla.org/preferences-service;1"].
>+      getService(Components.interfaces.nsIPrefBranch2);

nit: this indentation is weird, add some leading spaces before the getService call on the second line (also applies to the other identical method).

>+      this.kineticDecelloration = prefsvc.getIntPref("browser.ui.panning.acceleration")/-1000;

I suppose this was here before, but this should be "kineticDeceleration".

r- because the patch needs updating, but looks OK otherwise.

Comment 5

[Mike Kristoffersen (:MikeK)]

Fair comments, if there is a spelling mistake in kineticDecelloration, how about we then fix it properly and change it to kineticAcceleration and put a "-" in front of it in the places it is used?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Sure, either way.

Comment 7

[Mike Kristoffersen (:MikeK)]

Attachment: Updated with comments

Comment 8

[Mike Kristoffersen (:MikeK)]

Note, after making the different "constants" for the kinetic scrolling dynamic, I have played around with it a little, you can get some strange behavior, by e.g. setting the deadtime and deadzone to 1, you won't be able to select bookmarks from the bookmark menu????

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 365867 [details] [diff] [review]
Updated with comments

>diff --git a/app/mobile.js b/app/mobile.js

>+// amount of time a click should last before a panning gesture is detected.
>+pref("browser.ui.panning.deadtime", 200);

(in milliseconds)

>diff --git a/chrome/content/InputHandler.js b/chrome/content/InputHandler.js

> ChromeInputModule.prototype = {

>     reset: function reset() {

>+      this.deadZone = prefsvc.getIntPref("browser.ui.panning.deadzone")^2;

^ is XOR, you want Math.pow().

> function ContentPanningModule(owner, useKinetic) {

>     reset: function reset() {

>+                    prefsvc.getIntPref("browser.ui.panning.deadzone")^2;
>+      this.kineticAccelleration = 

kineticAccelleration -> kineticAcceleration

You also didn't remove the default value for kineticDecelloration.

I've noticed some trailing whitespace on some of the lines changed by this patch, should get rid of those too.

Comment 10

[Mike Kristoffersen (:MikeK)]

Attachment: Updated with Gavins latest comments

Comment 11

[Mike Kristoffersen (:MikeK)]

Attachment: Moved configuration load out of reset function

The loading of preferences was done in the frequently called reset functions, it has now moved to the initialisation code.

Comment 12

[Mike Kristoffersen (:MikeK)]

Comment on attachment 367104 [details] [diff] [review]
Moved configuration load out of reset function

Merge error found in the proposed patch

Comment 13

[Mike Kristoffersen (:MikeK)]

Attachment: Lets see if it got correct this time

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 367210 [details] [diff] [review]
Lets see if it got correct this time

Missing one replacement of kineticDecelloration, and you're not making use of the timeoutValueForStartDetection in the ContentPanningModule. Looks good otherwise...

Comment 15

[Mike Kristoffersen (:MikeK)]

Attachment: Updated with the issues found in the previous review, I think I must owe a beer or two to the reviewer at this point ;)

Comment 16

[Mike Kristoffersen (:MikeK)]

Comment on attachment 367301 [details] [diff] [review]
Updated with the issues found in the previous review, I think I must owe a beer or two to the reviewer at this point ;)

Never mind reviewing this anymore, the tree has moved far, far away from what this patch was made to fit.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 503362 adds some configuration prefs

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

verified with beta3

Comment 19

[Biju]

sorry for bug spam.
Many of the bugs which are marked invalid, I see comments telling it occurred in one version or other. But later it was fixed due to 1) by backingout the patch which made regression or 2) by fixing some other bug.
So if we can identify the bug/patch/reason then we should state that and mark those as FIXED. If not mark as WORKSFORME in that case.