Last Comment Bug 206438 - Smooth scrolling should use the 'smoothwheel' algorithm
: Smooth scrolling should use the 'smoothwheel' algorithm
Status: VERIFIED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: mozilla13
Assigned To: Avi Halachmi (:avih)
:
Mentors:
http://smoothwheel.mozdev.org/
: 239531 (view as bug list)
Depends on: 728738 1066480 smoothscroll 736251 737758
Blocks: 100951 710372
  Show dependency treegraph
 
Reported: 2003-05-20 07:54 PDT by JimS
Modified: 2014-11-12 13:48 PST (History)
43 users (show)
jaws: in‑testsuite-
jaws: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Increased duration, adds adaptive-duration (1.43 KB, patch)
2012-02-17 13:55 PST, Avi Halachmi (:avih)
no flags Details | Diff | Splinter Review
Avi's patch but with the algorithm variables configurable in about:config now (6.63 KB, patch)
2012-02-21 14:23 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Adds initialization, averaging of events deltas. (7.63 KB, patch)
2012-02-29 10:43 PST, Avi Halachmi (:avih)
no flags Details | Diff | Splinter Review
Additional patch for bug (fixes division by zero) (4.32 KB, patch)
2012-03-05 16:05 PST, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Apply dynamic duration to mouse wheel, Configurable for the rest (16.51 KB, patch)
2012-03-06 21:51 PST, Avi Halachmi (:avih)
limi: ui‑review+
Details | Diff | Splinter Review
V2: Apply dynamic duration to mouse wheel, Configurable for the rest (16.69 KB, patch)
2012-03-09 20:00 PST, Avi Halachmi (:avih)
no flags Details | Diff | Splinter Review
V3: Apply dynamic duration to mouse wheel, Configurable for the rest (16.49 KB, patch)
2012-03-10 18:29 PST, Avi Halachmi (:avih)
no flags Details | Diff | Splinter Review
V4: Apply dynamic duration to mouse wheel, Configurable for the rest (17.09 KB, patch)
2012-03-10 21:30 PST, Avi Halachmi (:avih)
no flags Details | Diff | Splinter Review
V5: Apply dynamic duration to mouse wheel, Configurable for the rest (18.19 KB, patch)
2012-03-11 13:55 PDT, Avi Halachmi (:avih)
roc: review+
Details | Diff | Splinter Review

Description JimS 2003-05-20 07:54:12 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030518
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4b) Gecko/20030518

The 'smoothwheel' extension located at http://smoothwheel.mozdev.org/ is
superior to smoothscroll in every way.  Should it be included in the tree in
place of the deficient existing 'smoothscroll'?

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 Boris Zbarsky [:bz] 2003-05-20 08:47:14 PDT
One key question is, "Do the authors of the extension want it to be included in
the tree?"  Because if they don't, it can't very well be included.
Comment 2 Avi Halachmi (:avih) 2003-05-20 10:43:06 PDT
hmmm...
1. I'm flattered that you find my extension usefull. thank you.

2. It's open source (triple licensed), so there's nothing stopping anyone from
using my code/concept as long as the license terms are kept.

3. however, as i noted here:
http://www.mozillazine.org/forums/viewtopic.php?p=84408#84408 , i want to put a
donation link at my page. unfortunately, my current financial situation is
rather bad, and i'm unemployed for some time now. tough times. So, at least at
the moment, i'd prefere that my code stays as an extension, and hope that some
donations flow in.

but then again, it's your decision, as it IS licensed under open source license.

thank you for the consideration.

best regards,
Avi Halachmi.
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-05-20 11:59:21 PDT
Smoothwheel doesn't handle scrollling with the keyboard or scrollbars, does it?
Only the wheel. So I don't think we can rip out what we've already got.

I'd be happy to help Avi integrate his superior algorithms into the base
smoothscroll support, if he wanted that. I don't much care either way.
Comment 4 Neil Cronin 2003-05-20 12:12:33 PDT
Smoothwheel seems fine, but doesn't work at all with the keyboard or scrollbar.
 The animation algorithm is nice (if slow), but the extension is totally useless
unless you have a wheel mouse.
Comment 5 Neil Cronin 2003-05-20 22:33:56 PDT
resolving WONTFIX since the author of the extension doesn't want it included,
and it doesn't do everything that the base feature does anyway.  

Avi, if you want to improve the existing algorithm, please file another bug.
Comment 6 Avi Halachmi (:avih) 2003-05-21 03:44:34 PDT
Ok.
Thank you.
Comment 7 Avi Halachmi (:avih) 2004-04-03 13:29:01 PST
hi.

after much time with my donation link, and $0 donated, i've given up on that
subject. i'm willing to help implementing my algorithms instead of the current
smooth scroll implementation.

i don't think i have enough time to set up a compilation environment though, or
to learn the gecko code. therefore, if someone wishes to try it himself, just
email me to avihpit (*at*) yahoo (*dot*) com and i'll help as much as i can.

cheers
avih
Comment 8 Mike Connor [:mconnor] 2004-04-17 14:48:21 PDT
*** Bug 239531 has been marked as a duplicate of this bug. ***
Comment 9 Piotr Komoda 2006-07-22 00:39:12 PDT
Reopening, I think many people will benefit from this feature and maybe extension author (avih) is still willing to help.
Comment 10 Marco Castelluccio [:marco] (PTO until August 24/25) 2011-12-02 14:50:46 PST
Is this still wanted?
Comment 11 (dormant account) 2011-12-13 18:52:34 PST
Limi, this feels atleast as good as Opera to me(minus usual cycle collector/etc pauses). Can you try this out?

Jared, how much would would it be to port this into Firefox?
Comment 12 Markus Stange [:mstange] 2011-12-14 00:56:12 PST
Note that our smooth scroll implementation was made a little smoother in bug 627651.
I haven't tried Smoothwheel recently, but we might come closer to its feel by simply tweaking the constants here: http://hg.mozilla.org/mozilla-central/annotate/221eccfa6a3f/layout/generic/nsGfxScrollFrame.cpp#l1306
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2011-12-14 01:18:35 PST
(In reply to Taras Glek (:taras) from comment #11)
> Limi, this feels atleast as good as Opera to me(minus usual cycle
> collector/etc pauses). Can you try this out?

This feels very good. Great job Avih!

> Jared, how much would would it be to port this into Firefox?

I've just started to look over the source code, so I'm not sure. I'm also curious to see how much we can gain by tweaking the variables that mstange mentioned.

I can try to spend some time on this on Thursday or Friday of this week to see how much code would need to be ported.
Comment 14 Marco Castelluccio [:marco] (PTO until August 24/25) 2011-12-15 15:54:41 PST
There's also this other extension ( https://addons.mozilla.org/it/firefox/addon/yet-another-smooth-scrolling/ ) that could be investigated.
Comment 15 (dormant account) 2012-01-19 15:29:35 PST
Jared will look into using this extension if our built-in physics can't be sufficiently tweaked.
Comment 16 Avi Halachmi (:avih) 2012-02-17 03:31:31 PST
I'm SmoothWheel's author. I've exchanged few email with Jared, and I'm posting my thoughts here to allow some public discussion:

I've looked at the sources of current Mozilla's implementation (nsGfxScrollFrame.cpp::AsyncScroll), and they seem very very similar to SmoothWheel's. Pretty much identical systems, with derivative-velocities, etc.

There are two differences I can see with the "physics" algo (which isn't real physics on your version too AFAICT):

1. You use splines instead of my "transition". While the technique is different, the outcome should be quite comparable, and definitely tunable to look even more similar, if so you wish.

2. The actual "profile function" which I use might be a bit different than your spline params. I believe you should be able to tune your spline to behave closer to SW's 1-(1-p)^2 (or just outright replace it with this function, and derivative), and anyway, it's probably not THAT different even as it stands.

I also noticed that when I set SW's param similar to yours (150ms, no acceleration and similar step-distance), it doesn't look too different from your implementation. Different, but not by much at all.

Which brings me to think that the main difference isn't really the physics or "system", but rather the parameters, and the adaptivity of SW.

SmoothWheel uses defaults as follows:
1. Average step of 1/5 height (works great when scrolling small areas). This is a major difference from line-height based distance, especially when the scroll is applied to different sized areas.
2. Adaptive-step (acceleration) of x3. This makes the actual steps about 0.1-0.3 of the scrollable-area-height.
3. Average duration of 400ms.
4. Adaptive-duration of x2.5. Which results in actual durations of about 250ms-600ms (600ms when events arrive slowly, 250ms when scrolling fast).

#3 and #4 are probably the more important differences.

The overall effect of the above is that when scrolling a single time, you get a relatively very short distance over a relatively long duration, which is very easy to follow while reading.

The other result of relatively longer durations (compared to Mozilla's fixed 150ms) is that you allow continuous and smooth scroll, and let the transition/spline actually "kick in" and connect two scroll commands into a single flow of the page, and so it doesn't scroll in 150ms "bursts", which while better than discrete scroll, don't allow reading while scrolling.

The downsize of longer durations, however, is that it's definitely not unanimously liked...

my $0.031415..
Comment 17 Avi Halachmi (:avih) 2012-02-17 06:33:38 PST
So, I've just compiled Firefox, and then changed the duration from 150ms to 500ms. To my eyes, it's definitely at least on-par with SmoothWheel's algorithm. So IMO the "physics" not where the main difference is.

I'm just posting this to confirm my guess from the previous post: The major difference is longer duration by default, adaptive-duration, and acceleration (Adaptive-step).

FYI.
Comment 18 Avi Halachmi (:avih) 2012-02-17 13:55:27 PST
Created attachment 598361 [details] [diff] [review]
Increased duration, adds adaptive-duration

Attaching a patch (for layout/generic/nsGfxScrollFrame.cpp) which increases duration and adds adaptive-duration. The result is extremely similar to SmoothWheel at default settings with Adaptive-Step (Acceleration) off.

Duration was 150ms, now it's 250ms-800ms, depending on the rate at which the events arrive (slow rate -> 800ms, rapid rate -> 250ms, progressively in between).

Step-acceleration is a different matter, and may currently depend on system settings, Firefox settings, etc. I think that coding it is much less local than this patch (though it shouldn't be more code than this patch).

This is a proof-of-concept, and I don't recommend committing this change into the tree before some UI-config considerations for the amount of adaptivity and duration (again, it seems not everyone likes it, and some find it "laggy").
Comment 19 (dormant account) 2012-02-17 14:57:15 PST
Jared, lets lend this on the UX branch and expose the constants as prefs so people can experiment with tweaking them.

Avi, really appreciate your analysis + code. Please submit unified patches(diff -u) in the future, those are much easier to read.
Comment 20 Avi Halachmi (:avih) 2012-02-17 15:53:18 PST
No problem, and note taken.
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2012-02-17 18:25:11 PST
Pushed to UX Nightly branch: https://hg.mozilla.org/projects/ux/rev/3ed31af179c2
Comment 22 Jared Wein [:jaws] (please needinfo? me) 2012-02-17 19:11:38 PST
Avi, Here's information on the UX Nightly branch: https://wiki.mozilla.org/UX_Branch

Just so you know, this does not feed in to mozilla-central. It is used for testing out and experimenting with UX changes.
Comment 23 Avi Halachmi (:avih) 2012-02-18 03:21:41 PST
It should be noted that since recently (Firefox 13?) there's a small jitter once a second - probably some recurring maintenance, but it seems to stall the main thread for <100ms (probably 20-40ms), repeatedly, and it's noticed when the refresh rate is supposedly 60 -> 17ms per frame. I noticed it with SmoothWheel on Aurora, and now it's also noticed with this patch.

FYI.
Comment 24 Avi Halachmi (:avih) 2012-02-18 04:58:27 PST
I've tried few mozilla-central Firefox nightly builds, and found that 2012-01-15 is OK, but the jitter appears on the 2012-01-16 nightly build (testing using SmoothWheel). Not sure how to find the changelog/commits for that build...
Comment 25 Josh Matthews [:jdm] 2012-02-18 05:07:28 PST
You can get the hg revision from about:buildconfig, and punch them into http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=&tochange=
Comment 26 Josh Matthews [:jdm] 2012-02-18 05:07:55 PST
With s/tracemonkey/mozilla-central/ of course.
Comment 27 Timothy Nikkel (:tnikkel) 2012-02-18 15:02:20 PST
I think that would give us http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=21c84409902e&tochange=047c8ba7d2e4  which makes me think bug 598482.
Comment 28 Timothy Nikkel (:tnikkel) 2012-02-18 15:10:24 PST
The smooth scroll timer is 1000 / 60 = 16 ms, the refresh driver is NSToIntRound(1000.0 / DEFAULT_FRAME_RATE), where DEFAULT_FRAME_RATE is 60, so 17 ms. This would probably make it look smooth except for the time when we fire the smooth scroll timer twice instead of once between refresh driver ticks. If this is the case then it's another reason to put smooth scrolling on the refresh driver (bug 702463).
Comment 29 (dormant account) 2012-02-18 17:15:40 PST
(In reply to Timothy Nikkel (:tn) from comment #28)
> The smooth scroll timer is 1000 / 60 = 16 ms, the refresh driver is
> NSToIntRound(1000.0 / DEFAULT_FRAME_RATE), where DEFAULT_FRAME_RATE is 60,
> so 17 ms. This would probably make it look smooth except for the time when
> we fire the smooth scroll timer twice instead of once between refresh driver
> ticks. If this is the case then it's another reason to put smooth scrolling
> on the refresh driver (bug 702463).

Ehsan looked at 702463 and said it wont help, can you be more specific on why you think it will help? Are you saying that this divergence between the two timers means that a single extra paint is causing lag(ie one extra paint every 256ms)?
Comment 30 Timothy Nikkel (:tnikkel) 2012-02-18 17:19:36 PST
Ehsan and I discussed 702463 and we agreed then, but this is a new observation. If the smooth scroll timer fires twice between refresh driver ticks we will miss a frame of the smooth scroll animation. It will appear jumpy, as if we skipped a frame in a video. That's my theory anyway, haven't proved it. We get the same number of paints (one approximately every 17ms) its just that sometimes we jump more than expected in the animation between them.
Comment 31 Avi Halachmi (:avih) 2012-02-19 04:05:51 PST
I would think that if the reason is timers convergence, then decreasing the intervals to way below the monitor refresh rate should solve it (as long as the CPU isn't choked).

So I've changed the default refresh rate (at nsRefreshDriver.cpp) to 250, which didn't help, and then I also changed the smooth scroll interval to the same value (1000/250), which also didn't help. The jitter is still there (on a "light" page which doesn't use much CPU when scrolling). 

I also printed the delta between smooth-scroll iterations (supposedly 4ms at my settings), and I got this:

 -6- 10 4 3 2 3 -6- 3 2 -4- 6 3 3 4 5 4 4 4 4 4 4 4 4 4 3 4 4 4 4 3 4 4 4 4 4 4
4 3 4 4 4 4 4 4 4 4 3 4 4 3 5 4 4 3 5 4 3 3 4 4 5 5 4 3 4 4 4 4 4 4 4 4 4 4 4 4
3 4 4 3 4 4 4 3 3 4 4 3 4 4 4 4 4 4 4 3 3 4 4 4 7 4 3 4 4 4 3 4 4 4 3 3 4 4 4 3
4 4 3 4 4 4 3 4 4 4 4 3 4 4 3 4 4 4 3 3 4 4 3 4 5 4 3 -7- 4 4 3 4 4 3 3 4 4 3 4
4 4 3 4 4 4 3 3 4 4 4 7 6 3 4 -8- 4 4 -8- 3 4 -9- 3 3 -7- 4 3 -8- 3 2 3 5 3 3 4
5 3 4 3 5 4 4 -8- 3 4 -8- 4 3 -8- 4 3 -8- 4 4 -8- 3 4 2 -6- 3 3 4 5 3 3 4 5 5 4
-14- 3 3 -6- 3 4 3 5 4 4 3 4 4 4 4 4 4 4 3 5 3 3 4 4 4 3 3 4 4 4 3 4 4 3 4 4 4 4
 4 4 5 4 6 4 3 3 4 4 3 4 4 4 4 4 4 4 3 3 4 4 3 4 4 4 3 3 3 4 4 3 4 4 3 3 4 4 3 4
 4 4 4 3 4 4 3 4 4 4 4 3 4 4 4 -8- 3 3 3 5 4 4 -8- 3 4 3 4 4 3 4 4 4 3 4 4 4 3 3
 4 4 3 4 4 4 3 4 4 4 3 4 4 4 4 4 4 4 4 3 4 4 4 4 4 4 3 4 4 4 3 4 4 4 3 4 4 4 3 4
 5 4 4 -13- 3 3 3 5 4 3 -8- 4 3 -8- 3 3 3 5 4 3 -9- 3 3 -8- 3 3 2 -6- 4 4 -8- 4
4 -9- 3 4 -9- 4 3 -8- 4 4 7 3 3 3 -6- 4 4 -9- 3 2 3 5 3 4 -8- 4 3 -8- 4 4 -9- 8
7 3 4 6 4 4 3 4 4 4 3 4 4 4 3 3 3 3 3 5 4 3 4 -9- 3 4 5 3 4 4 5 4 4 -8- 3 4 -8-
4 4 7 3 3 3 -7- 3 4 -8- 3 4 -8- 5 4 -8- 3 3 5 3 4 3 5 3 3 4 5 4 3 4 4 3 4 4 5 4
3 4 4 3 3 4 5 4 4 3 4 4 4 7 4 3 4 4 4 3 4 5 4 4 7 4 3 3 4 4 3 3 4 4 4 3 4 4 3 4
4 4 4 4 4 4 3 3 4 4 4 3 4 4 4 7 4 3 4 4 4 3 3 4 4 4 3 4 4 3 3 4 4 3 4 4 4 3 4 4


Values with -x- are at least twice longer than the previous interval, and may indicate jitter. However, all the intervals are below 16.67 (60 hz, as my monitor is), yet a jitter is still visible.

Maybe someone can have some insight from this info.
Comment 32 Avi Halachmi (:avih) 2012-02-19 06:45:21 PST
Also, I've built Firefox 10.02 and applied the longer duration and 4ms scroll intervals, and the smooth scroll iterations intervals, as a reference:

 4 4 4 4 4 4 4 4 4 4 4 5 5 5 5 4 4 4 5 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 5 4 5 4 5 4 4 5
 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 5 5 5 5 4 4 4 4 5 4 4 4 4 4 4 4
 4 5 4 4 4 4 4 4 5 4 4 -11- 5 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
5 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
4 5 4 5 5 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 5 -12- 5 4 5 4 4 4 4 4 4 4 4 4 4 4 4 4 4
 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 5 4 4 -10- 5 4
5 4 4 5 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
4 4 4 4 4 4 4 4 4 4 4 4 5 4 4 4 4 4 5 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4


As you can tell, while the occasional glitches do exist, it's overall much more consistent.

Now, I know Firefox isn't a game engine and wasn't developed to account for sub 10ms stalls, however, with smooth scrolling which can last for few hundreds of ms, it become noticeable. So I don't know if removing these jitters should be a priority, but I'm posting it here for the record.
Comment 33 Timothy Nikkel (:tnikkel) 2012-02-19 14:41:25 PST
This is good investigation but we are getting off track from the real purpose of this bug. We should file a new one and continue there.
Comment 34 Timothy Nikkel (:tnikkel) 2012-02-19 14:50:59 PST
I filed bug 728738 for this.
Comment 35 Jared Wein [:jaws] (please needinfo? me) 2012-02-21 12:02:11 PST
Comment on attachment 598361 [details] [diff] [review]
Increased duration, adds adaptive-duration

Can someone from the UX team take a look at the smooth scrolling changes, now in the UX nightly builds, and see if we should take these changes or if any tweaks should be made?
Comment 36 Jared Wein [:jaws] (please needinfo? me) 2012-02-21 14:23:43 PST
Created attachment 599369 [details] [diff] [review]
Avi's patch but with the algorithm variables configurable in about:config now

This moves the algorithm variables to about:config.

Pushed to UX Nightly branch: https://hg.mozilla.org/projects/ux/rev/7565525d9138
Comment 37 Mozilla RelEng Bot 2012-02-21 14:27:45 PST
Autoland Patchset:
	Patches: 599369
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=64beb7f9e0e7
Try run started, revision 64beb7f9e0e7. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=64beb7f9e0e7
Comment 38 Mozilla RelEng Bot 2012-02-22 02:01:33 PST
Try run for 64beb7f9e0e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=64beb7f9e0e7
Results (out of 218 total builds):
    exception: 4
    success: 160
    warnings: 39
    failure: 14
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-64beb7f9e0e7
Comment 39 Sean Newman 2012-02-24 04:22:39 PST
(In reply to Avi Halachmi (:avih) from comment #23)
> It should be noted that since recently (Firefox 13?) there's a small jitter
> once a second - probably some recurring maintenance, but it seems to stall
> the main thread for <100ms (probably 20-40ms), repeatedly, and it's noticed
> when the refresh rate is supposedly 60 -> 17ms per frame. I noticed it with
> SmoothWheel on Aurora, and now it's also noticed with this patch.
> 
> FYI.

I'm not sure if this helps, but I think I can explain the cause of this small glitch: it happens when CC works out.

How did I notice this?
1. I opened error console (and selected to show "messages").
2. I checked that AutoScrolling is enabled ("general.autoScroll" is set to "true" by default, so I just did a check).
3. Middle click somewhere on the page and move cursor just A BIT lower/higher from where you've middle clicked, so autoscrolling starts working and the page is getting scrolled very slowly.
4. watch both windows: error console and the page you scroll.

The noticed result: every time you see the message in the error console that CC has just worked out - you'll also see the small glitch at scrolling.

I hope that it's a helpful observation.

(In reply to Timothy Nikkel (:tn) from comment #34)
> I filed bug 728738 for this.

I will post my observation into that bug too.
Comment 40 Sean Newman 2012-02-24 04:37:59 PST
Oh, maybe (just maybe, not sure) the CC messages are not shown by default, so you might also need to set "browser.dom.window.dump.enabled" to "true", not sure though.
Comment 41 Jared Wein [:jaws] (please needinfo? me) 2012-02-24 08:11:10 PST
I haven't looked deeper in to this yet, but on IRCCloud keyboard scrolling using the up/down keys is extremely slow, moving about 1 pixel per second, and sometimes speeding up.

It has been noted before that IRCCloud is the source of many issues, but there weren't any issues with keyboard scrolling before this patch :(
Comment 42 Avi Halachmi (:avih) 2012-02-24 09:06:48 PST
jaws, the trybuild also reported an error with the uninitialized TimeStamp (null TimeStamp being used), though it compiles and works as expected here. I'd say first initialize it, then see how the longer duration affects overall scrolled distance (if indeed unrelated to the initialization).
Comment 43 Avi Halachmi (:avih) 2012-02-29 10:43:33 PST
Created attachment 601668 [details] [diff] [review]
Adds initialization, averaging of events deltas.

The initialization would hopefully satisfy the try server.
The averaging (over last 3 events deltas) should moderate fluctuations of the dynamic duration.
Comment 44 Avi Halachmi (:avih) 2012-02-29 10:47:49 PST
P.S. The patch from comment 43 is against current mozilla-central (and I forgot to mark it as a diff file... sorry).
Comment 45 Jared Wein [:jaws] (please needinfo? me) 2012-02-29 11:07:21 PST
Pushed to ux-branch: https://tbpl.mozilla.org/?tree=UX&rev=b48e5d3f52e6
Comment 46 Avi Halachmi (:avih) 2012-03-01 03:04:59 PST
Current patch can cause division by zero when duration is equal to DurationMax (doesn't on windows, but probably can on other platforms/compilers).

I suggest adding the following lines as the first lines of AsyncScroll::AsyncScroll() (also enforces sanity - max 10s - on user prefs):

    // Clamp user prefs to valid and sane (up to 10 seconds).
    kSmoothScrollAnimationDurationMS    = clamped(kSmoothScrollAnimationDurationMS,    0.0, 10000.0);
    kSmoothScrollAnimationDurationMaxMS = clamped(kSmoothScrollAnimationDurationMaxMS, kSmoothScrollAnimationDurationMS + 1, 10001.0);
Comment 47 Jared Wein [:jaws] (please needinfo? me) 2012-03-01 07:54:06 PST
(In reply to Avi Halachmi (:avih) from comment #46)
> Current patch can cause division by zero when duration is equal to
> DurationMax (doesn't on windows, but probably can on other
> platforms/compilers).

I've pushed a fix to the UX branch:
https://hg.mozilla.org/projects/ux/rev/b6b8aea8bf38
Comment 48 Jared Wein [:jaws] (please needinfo? me) 2012-03-05 16:05:15 PST
Created attachment 603092 [details] [diff] [review]
Additional patch for bug (fixes division by zero)
Comment 49 Avi Halachmi (:avih) 2012-03-06 21:51:25 PST
Created attachment 603596 [details] [diff] [review]
Apply dynamic duration to mouse wheel, Configurable for the rest

Addons experience (Yet another smooth scrolling, SmoothWheel) show that users value and use smooth scrolling differently for different triggers (KB, mouse wheel, etc). This patch allows different durations for different scroll triggers. It also has cleaner code compared to previous patches.

Default duration is now longer and dynamic for mouse wheel (200ms-800ms), and unchanged (but now configurable) for the rest (150ms). Configuration is the same for all (min ms, max ms).

Classification was decided according to what had minimal impact on the code around, and is not necessarily "correct". It's enough for current requirements (change mouse wheel duration only), and may be expanded/changed in the future.

This classification is currently not exposed for usage outside of nsGfxScrollFrameInner. We might want to expose it at a later stage if such need arise.

Current classification and some triggers:
lines      - KB arrows
pages      - PgUp/PgDn/Space
pixels     - Mouse wheel, Windows Synaptic touchpad (generates wheel events)
scrollbars - Clicking scrollbars arrows, clicking scrollbars empty areas
other      - Anything else (tested it a bit, couldn't find any)

Triggers which currently bypass smooth scrolling (unchanged by this patch):
- Home/End
- OS X trackpad two fingers scroll*
- Dragging a scrollbar**

* Important: OS X touchpad - behavior may degrade if we apply smooth scrolling to it. limi and Jared observed yesterday that it's currently unaffected by smooth scroll. Please raise a flag if you know of cases where this is incorrect.

** While outside the scope of this bug, I've experimented a bit with applying smooth scrolling when dragging a scrollbar. It shows an interesting potential. FYI.

Possible future enhancements which SmoothWheel already deploys:
- Use distance relative to client area size (instead of by "lines").
- Implement our own acceleration, possibly overriding the OS/drivers.
Comment 50 Jared Wein [:jaws] (please needinfo? me) 2012-03-06 23:27:10 PST
Comment on attachment 603596 [details] [diff] [review]
Apply dynamic duration to mouse wheel, Configurable for the rest

I updated the UX branch with this new patch: https://hg.mozilla.org/projects/ux/rev/26272dc66219
Comment 51 Alex Limi (:limi) — Firefox UX Team 2012-03-07 15:39:45 PST
Comment on attachment 603596 [details] [diff] [review]
Apply dynamic duration to mouse wheel, Configurable for the rest

This seems to work great for me. Tested the various settings in OS X, and with a USB mouse in Windows 8.
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-07 17:52:29 PST
Comment on attachment 603596 [details] [diff] [review]
Apply dynamic duration to mouse wheel, Configurable for the rest

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

How about instead of SmoothProfile, we just pass an extra optional nsIAtom* parameter to ScrollTo (default to null), letting various callers of ScrollTo specify their own smoothness profile and prefs without changing nsGfxScrollFrame? Then you can just get the string for the pref instead of having a big switch statement.

::: browser/app/profile/firefox.js
@@ +238,5 @@
>  pref("general.useragent.locale", "@AB_CD@");
>  pref("general.skins.selectedSkin", "classic/1.0");
>  
>  pref("general.smoothScroll", true);
> +// These values are duplicated in layout/generic/nsGfxScrollFrame.cpp

Please add more comments explaining what these prefs do.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1334,5 @@
>    nsCOMPtr<nsITimer> mScrollTimer;
>    TimeStamp mStartTime;
> +  TimeStamp mPrevStartTime1;
> +  TimeStamp mPrevStartTime2;
> +  TimeStamp mPrevStartTime3;

Document what these are for? especially why we need 3?

@@ +1361,5 @@
> +  
> +  void InitDuration(nsGfxScrollFrameInner::SmoothProfile aProfile);
> +  
> +  // Linear transform a value from one scale into another (opposite polarities are allowed).
> +  double linearTransform(double inVal, double inRef1, double inRef2, double outRef1, double outRef2){

LinearTransform

@@ +1441,5 @@
> +  if(!isSmooth)
> +    minMS = maxMS = 0;
> +  
> +  minMS = clamped(minMS, 0.0,   10000.0);
> +  maxMS = clamped(maxMS, minMS, 10000.0);

Name this magic constant :-).

@@ +1462,5 @@
> +
> +  mDuration = TimeDuration::FromMilliseconds(
> +                clamped( 
> +                  linearTransform(deltaMs,
> +                                  minMS / 1.25, maxMS / 1.25,  // Manually tuned correlation

Name this magic constant.

::: layout/generic/nsGfxScrollFrame.h
@@ +348,5 @@
>    // Used for asynchronous scrolling.
>    bool mShouldBuildLayer:1;
> +  
> +protected:
> +  void _ScrollTo(nsPoint aScrollPosition, nsIScrollableFrame::ScrollMode aMode, SmoothProfile aProfile);

We don't use underscores in method names. Call this ScrollWithSmoothnessProfile?
Comment 53 Avi Halachmi (:avih) 2012-03-07 23:09:42 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #52)
> 
> How about instead of SmoothProfile, we just pass an extra optional nsIAtom*
> parameter to ScrollTo (default to null), letting various callers of ScrollTo
> specify their own smoothness profile and prefs without changing
> nsGfxScrollFrame? Then you can just get the string for the pref instead of
> having a big switch statement.
> ...

I'll address all the issues, and while I agree the switch may look cumbersome, I'd like to state my reasons for the switch/emun approach (in a nutshell: manageability, easier grasp of the "big picture", and possibly not final classification), and let you decide if they're valid/strong enough:

1. The default values (3 per profile - 15 overall) are currently duplicated between firefox.js and that big switch statement. I'd consider the ones on firefox.js to be the redundant ones, as we might decide not to expose them. So all 15 default values must remain at the c++ code too, preferably grouped - such that it's easier to see the big picture.

2. When passed as an argument, a profile should be identified by a "code name" and not as an explicit group of arguments, because the latter imposes the caller to know the implementation details of smooth scrolling (e.g. the fact that it uses min and max durations), which might change in the future, just as it has changed with this patch (adding dynamic duration).

combining 1 and 2 means that at some place, a profile name should be associated with its 3 default values (maybe also with its prefs name differentiator), possibly with a switch statement (or a struct which holds them, but it adds more code).

3. Current classification (pixels/lines/pages/scrollbars) was chosen opportunistically and ad-hoc because it seems enough for task, and because it only required minimal and local code changes. It's possible that this classification will change*, thus requiring components outside of nsGfxScrollFrame to be able to choose profiles. The SmoothProfile enum is already in place for such case, and will also enable easier backtracking when changing related code IMHO.

* The reason I'm not fully happy with current classification is that the main requirement (differentiating mouse wheel events from the rest) is satisfied implicitly and not explicitly. I.e. we count on the fact that currently only mouse wheel events generate "pixels" scroll, which is far from perfect IMO. When I discussed it with taras, he suggested that we'll keep it this way for now, and see if it works. But I still suspect we might need to change it at some stage.

So, 15 default values at the code - preferably grouped, profile which is passed as a name and not as its arguments/details - but still needs association with different defaults according to different names, and the fact that we might change current classification and need it accessible from the outside. With those in mind, I'm not sure other approaches will result in less code or better readability.

A reasonable alternative approach (to remove that switch) might be to define a SmoothProfile struct with prefName/DefaultEnabled/DefMin/DefMax, define SmoothProfile constants (possibly static public at nsGfxScrollFrame) for each profile with their defaults, and use those instead of the enum. It will remove that switch, and add a little code elsewhere.

The rest of your comments are good and I'll address them. Thanks.
Let me know what you think of the switch/enum thingy.
Comment 54 Timothy Nikkel (:tnikkel) 2012-03-07 23:42:50 PST
Comment on attachment 603596 [details] [diff] [review]
Apply dynamic duration to mouse wheel, Configurable for the rest

>   if (isSmoothScroll) {
>     mAsyncScroll->InitSmoothScroll(now, currentPosition, currentVelocity,
>-                                   aScrollPosition);
>+                                   mDestination, aProfile);

In bug 728135, comment 2 Markus said that he used aScrollPosition instead of mDestination here purposefully. Do you feel differently?
Comment 55 Jared Wein [:jaws] (please needinfo? me) 2012-03-07 23:53:40 PST
(In reply to Timothy Nikkel (:tn) from comment #54)
> Comment on attachment 603596 [details] [diff] [review]
> Apply dynamic duration to mouse wheel, Configurable for the rest
> 
> >   if (isSmoothScroll) {
> >     mAsyncScroll->InitSmoothScroll(now, currentPosition, currentVelocity,
> >-                                   aScrollPosition);
> >+                                   mDestination, aProfile);
> 
> In bug 728135, comment 2 Markus said that he used aScrollPosition instead of
> mDestination here purposefully. Do you feel differently?

This specifically came up in our discussion of the metrics that we wanted to put in to the smooth scrolling algorithm. We should use the clamped scroll position since it gives users a feel that the edge of the page is near and the scrolling doesn't stop so abruptly at the edge of the page.
Comment 56 Timothy Nikkel (:tnikkel) 2012-03-08 00:01:59 PST
(In reply to Jared Wein [:jaws] from comment #55)
> This specifically came up in our discussion of the metrics that we wanted to
> put in to the smooth scrolling algorithm. We should use the clamped scroll
> position since it gives users a feel that the edge of the page is near and
> the scrolling doesn't stop so abruptly at the edge of the page.

We should probably test out both approaches then because Markus said the exact opposite feels better.
Comment 57 Timothy Nikkel (:tnikkel) 2012-03-08 00:08:13 PST
Also, where did we put the results of these discussions? I think I missed reading the results.
Comment 58 Markus Stange [:mstange] 2012-03-08 00:09:43 PST
(In reply to Timothy Nikkel (:tn) from comment #56)
> (In reply to Jared Wein [:jaws] from comment #55)
> > This specifically came up in our discussion of the metrics that we wanted to
> > put in to the smooth scrolling algorithm. We should use the clamped scroll
> > position since it gives users a feel that the edge of the page is near and
> > the scrolling doesn't stop so abruptly at the edge of the page.
> 
> We should probably test out both approaches then because Markus said the
> exact opposite feels better.

Maybe it's a platform-specific thing. On OS X Lion, native applications have a bounce effect when they reach the end of a scroll area, and this effect makes it really clear where the "springs" are: They're *past* the end of the scroll area, and not before it.
The right feel may also be mouse specific. My external mouse has a free-spin wheel without any friction at the scroll "steps", and for such a wheel it makes even less sense to have scrolling slow down before the page end.
Comment 59 Jared Wein [:jaws] (please needinfo? me) 2012-03-08 00:11:26 PST
(In reply to Timothy Nikkel (:tn) from comment #57)
> Also, where did we put the results of these discussions? I think I missed
> reading the results.

Sorry, we did a quick Google Hangout between myself, Avi, Taras, and Limi, and I don't think anybody took any notes. Limi may be able to provide more details on the reasoning behind this though.
Comment 60 Jared Wein [:jaws] (please needinfo? me) 2012-03-08 00:15:30 PST
(In reply to Markus Stange from comment #58)
> (In reply to Timothy Nikkel (:tn) from comment #56)
> > (In reply to Jared Wein [:jaws] from comment #55)
> > > This specifically came up in our discussion of the metrics that we wanted to
> > > put in to the smooth scrolling algorithm. We should use the clamped scroll
> > > position since it gives users a feel that the edge of the page is near and
> > > the scrolling doesn't stop so abruptly at the edge of the page.
> > 
> > We should probably test out both approaches then because Markus said the
> > exact opposite feels better.
> 
> Maybe it's a platform-specific thing. On OS X Lion, native applications have
> a bounce effect when they reach the end of a scroll area, and this effect
> makes it really clear where the "springs" are: They're *past* the end of the
> scroll area, and not before it.
> The right feel may also be mouse specific. My external mouse has a free-spin
> wheel without any friction at the scroll "steps", and for such a wheel it
> makes even less sense to have scrolling slow down before the page end.

The bounce is a nice effect, and one that we should implement. This is tracked by bug 673875.
Comment 61 Timothy Nikkel (:tnikkel) 2012-03-08 00:19:12 PST
(In reply to Jared Wein [:jaws] from comment #59)
> Sorry, we did a quick Google Hangout between myself, Avi, Taras, and Limi,
> and I don't think anybody took any notes. Limi may be able to provide more
> details on the reasoning behind this though.

I'd appreciate if someone could provide a summary of the decisions made.
Comment 62 Avi Halachmi (:avih) 2012-03-08 00:57:16 PST
Summary of the google hangout from 2012-03-06 (taras, jared, limi, avih):

Agenda: discuss changes to previous patch (longer/dynamic durations, identical values for all smooth scroll triggers) to make limi happy.

Observations:
1. limi liked the longer/dynamic durations of the previous patch.
2. limi said it feels unnaturally slow for all except for mouse wheel scroll.
3. On OSX - smooth scroll is bypassed, and is unaffected by our changes.

Decisions:
1. Longer smooth scroll duration should be applied to mouse wheel only (and touchpads which generate discrete wheel events). Other triggers (page up/down, scrollbar clicks, etc) should stay at the current value of 150ms.

2. I noted that currently we can distinguish mouse wheel events from the rest by identifying "pixels" scroll requests. None of us knew of other triggers which generate pixels scrolls. We agreed to try it as a differentiator and "See if it sticks".
Comment 63 Avi Halachmi (:avih) 2012-03-08 01:01:19 PST
The preference for slowing down vs "hitting the wall" vs OSX "bounce" is indeed subjective (SmoothWheel calls it "Soft-edge", which is on by default), and the effect is enhanced when the animation lasts longer, which is what happens here.

Also, currently, there's a bug where AsyncScroll can't reliably report IsFinished() when the scroll aims past the edge, which sometimes results in the page "sticking" to the edge until the full duration has elapsed, even after "hitting the wall". Here too, the effect is enhanced when the animation lasts longer.

I still think that it's more correct to request a scroll to the known limit, instead of aiming past it for the reason of achieving an abrupt stop. We might need to consider user preference for edge behavior.

I also agree with Jared that OSX bounce should be handled (natively?) at bug 673875, and is not directly related to current bug.
Comment 64 Markus Stange [:mstange] 2012-03-08 01:25:24 PST
Note that even on OS X you can use an external mouse which scrolls in lines, not pixels, and those line scrolls are affected by smooth scrolling.
On OS X, the only things that send pixel scrolls are touchpads and the Magic Mouse (which doesn't have a wheel but a built-in touchpad for scrolling).
Comment 65 Avi Halachmi (:avih) 2012-03-08 01:36:48 PST
(In reply to Markus Stange from comment #64)
> Note that even on OS X you can use an external mouse which scrolls in lines,
> not pixels, and those line scrolls are affected by smooth scrolling.
> On OS X, the only things that send pixel scrolls are touchpads and the Magic
> Mouse (which doesn't have a wheel but a built-in touchpad for scrolling).

Correct. However, as observed by limi/jaws, OS X touchpads bypass our smooth scroll, so currently smooth+pixels is only requested for "normal" mouse wheels which originally request discrete scrolls in lines (which are then converted to pixels before the smooth scroll call, yes, a bit of a mess there).

As I noted on comment 49, if someone knows otherwise, do tell.
Comment 66 Markus Stange [:mstange] 2012-03-08 02:11:12 PST
(In reply to Avi Halachmi (:avih) from comment #65)
> (In reply to Markus Stange from comment #64)
> > Note that even on OS X you can use an external mouse which scrolls in lines,
> > not pixels, and those line scrolls are affected by smooth scrolling.
> > On OS X, the only things that send pixel scrolls are touchpads and the Magic
> > Mouse (which doesn't have a wheel but a built-in touchpad for scrolling).
> 
> Correct. However, as observed by limi/jaws, OS X touchpads bypass our smooth
> scroll,

Right, that's good and should stay that way. If you want some history, there's bug 607464 which restored things last time they broke.

(In reply to Avi Halachmi (:avih) from comment #63)
> Also, currently, there's a bug where AsyncScroll can't reliably report
> IsFinished() when the scroll aims past the edge, which sometimes results in
> the page "sticking" to the edge until the full duration has elapsed, even
> after "hitting the wall".

Sure, that would need to be fixed. Note that I wrote both the code and the XXX comment.
Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-08 14:39:32 PST
(In reply to Avi Halachmi (:avih) from comment #53)
> 1. The default values (3 per profile - 15 overall) are currently duplicated
> between firefox.js and that big switch statement. I'd consider the ones on
> firefox.js to be the redundant ones, as we might decide not to expose them.
> So all 15 default values must remain at the c++ code too, preferably grouped
> - such that it's easier to see the big picture.
> 
> 2. When passed as an argument, a profile should be identified by a "code
> name" and not as an explicit group of arguments, because the latter imposes
> the caller to know the implementation details of smooth scrolling (e.g. the
> fact that it uses min and max durations), which might change in the future,
> just as it has changed with this patch (adding dynamic duration).

I intended the atom to just be "lines" or "pixels" or whatever and that would automatically be munged to generate the right pref names. If we add more parameters per smoothness profile, these atoms would not be affected.
Comment 68 Avi Halachmi (:avih) 2012-03-08 16:34:23 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #67)
> 
> I intended the atom to just be "lines" or "pixels" or whatever and that
> would automatically be munged to generate the right pref names. If we add
> more parameters per smoothness profile, these atoms would not be affected.

My fear here is that if we use arbitrary atoms instead of predefined values (constants, enum, etc) it will be harder to backtrack the code at a later stage, and easier to introduce bugs if the string literals are not confined to a known set (e.g. someone uses the non existing "aProfileWhichProbablyExist").

Also, at some stage the atoms should be associated with different default values according to the different atom values, which can imply a similar big switch.
Comment 69 Avi Halachmi (:avih) 2012-03-08 17:49:09 PST
(In reply to Markus Stange from comment #66)
> (In reply to Avi Halachmi (:avih) from comment #65)
> > Also, currently, there's a bug where AsyncScroll can't reliably report
> > IsFinished() when the scroll aims past the edge, which sometimes results in
> > the page "sticking" to the edge until the full duration has elapsed, even
> > after "hitting the wall".
> 
> Sure, that would need to be fixed. Note that I wrote both the code and the
> XXX comment.

If we keep the current patch as is, then the IsFinished fix is not required, as we'll never "hit the wall", and the scroll will never "stick" to the edge.

However, if the current behavior is not good enough because it slows down too much towards the edge, we can do one of the following:

1. Revert that change, keep aiming past the edge and hit the wall, and fix IsFinished.

Or 2. Change the behavior such that it doesn't slow down too much towards the edge (reducing duration instead of aiming past the edge, never hitting the wall).

Or 3. Implement both, and add a user pref for choosing between them.

Both approaches include passing both mDestination (clamped) and aScrollPosition (unclamped) to InitSmoothScroll. The IsFinished fix adds some logic to IsFinished, and the reduced duration solution adds some logic at InitDuration(..).

I have local implementation for all these approaches, I think #2 works very well. Let me know which one we should take.
Comment 70 Avi Halachmi (:avih) 2012-03-09 20:00:44 PST
Created attachment 604597 [details] [diff] [review]
V2: Apply dynamic duration to mouse wheel, Configurable for the rest

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #67)
> I intended the atom to just be "lines" or "pixels" or whatever and that
> would automatically be munged to generate the right pref names. If we add
> more parameters per smoothness profile, these atoms would not be affected.

I've asked quite a bit about how to use the atoms; that's the best I could come up with. Also, not sure how to get rid of that big if-else without removing the default values from the cpp file.

This patch also uses less code instead of the LinearTransform function (identical result).
Comment 71 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-10 13:47:18 PST
Comment on attachment 604597 [details] [diff] [review]
V2: Apply dynamic duration to mouse wheel, Configurable for the rest

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

::: browser/app/profile/firefox.js
@@ +239,5 @@
>  pref("general.skins.selectedSkin", "classic/1.0");
>  
>  pref("general.smoothScroll", true);
> +// These values are duplicated in layout/generic/nsGfxScrollFrame.cpp
> +// They define the smooth scroll behavior (min ms, max ms) for different triggers

Document what the "pixels", "lines", "pages", "scrollbars" and "other" triggers are used for.

::: content/base/src/nsGkAtomList.h
@@ +1937,5 @@
> +GK_ATOM(SmoothProfilePixels,      "pixels")
> +GK_ATOM(SmoothProfileLines,       "lines")
> +GK_ATOM(SmoothProfilePages,       "pages")
> +GK_ATOM(SmoothProfileScrollbars,  "scrollbars")
> +GK_ATOM(SmoothProfileOther,       "other")

Just call these None, Pixels, Lines, Pages, Scrollbars, Other.

And actually just use null instead of None.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1333,5 @@
>    }
>  
>    nsCOMPtr<nsITimer> mScrollTimer;
>    TimeStamp mStartTime;
> +  TimeStamp mPrevStartTime[3]; // Store previous 3 timestamps to allow delta averaging (reduce duration fluctuations)

Put comment on its own line (otherwise line is too long)

@@ +1340,5 @@
>    nsPoint mDestination;
>    nsSMILKeySpline mTimingFunctionX;
>    nsSMILKeySpline mTimingFunctionY;
>    bool mIsSmoothScroll;
> +  bool mIsInitializedDuration;

Document this in a comment

@@ +1360,5 @@
> +  
> +  void InitDuration(nsIAtom *aProfile);
> +
> +  // Stores the default values for a single smooth scroll profile.
> +  struct ProfileDefaults{

space before {

@@ +1366,5 @@
> +      enabled(enabled), minMS(minMS), maxMS(maxMS)
> +      {};
> +    bool enabled;
> +    int minMS;
> +    int maxMS;

Field names must start with "m" prefix

@@ +1393,5 @@
> + * Calculate/update mDuration, possibly dynamically according to events rate and smooth profile.
> + * (also maintain previous timestamps - which are only used here).
> + */
> +void
> +nsGfxScrollFrameInner::AsyncScroll::InitDuration(nsIAtom *aProfile){

Space before {

@@ +1405,5 @@
> +  else if(aProfile == nsGkAtoms::SmoothProfileLines)      defaults = ProfileDefaults(true, 150, 150);
> +  else if(aProfile == nsGkAtoms::SmoothProfilePages)      defaults = ProfileDefaults(true, 150, 150);
> +  else if(aProfile == nsGkAtoms::SmoothProfileScrollbars) defaults = ProfileDefaults(true, 150, 150);
> +  else if(aProfile == nsGkAtoms::SmoothProfileOther)      defaults = ProfileDefaults(true, 150, 150);
> +  else NS_ERROR("Invalid smooth profile");

Why not just make all the defaults 150,150, and put the prefs in all.js instead of firefox.js so all Gecko users get them? Seems like that shouldn't be a problem.

@@ +1409,5 @@
> +  else NS_ERROR("Invalid smooth profile");
> +  
> +  nsCString profileName;
> +  aProfile->ToUTF8String(profileName);
> +  nsCString prefBase = nsCString("general.smoothScroll.") + profileName;

Use NS_LITERAL_CSTRING here instead of the nsCString constructor.

@@ +1411,5 @@
> +  nsCString profileName;
> +  aProfile->ToUTF8String(profileName);
> +  nsCString prefBase = nsCString("general.smoothScroll.") + profileName;
> +  nsCString prefMin = prefBase + nsCString(".durationMinMS");
> +  nsCString prefMax = prefBase + nsCString(".durationMaxMS");

Here too.

@@ +1417,5 @@
> +  isSmooth = Preferences::GetBool(prefBase.get(), defaults.enabled);
> +  minMS =    Preferences::GetInt (prefMin.get(),  defaults.minMS);
> +  maxMS =    Preferences::GetInt (prefMax.get(),  defaults.maxMS);
> +
> +  if(!isSmooth)

if (
Comment 72 Avi Halachmi (:avih) 2012-03-10 18:29:01 PST
Created attachment 604706 [details] [diff] [review]
V3: Apply dynamic duration to mouse wheel, Configurable for the rest

Address roc's comments.
Comment 73 Avi Halachmi (:avih) 2012-03-10 21:30:20 PST
Created attachment 604731 [details] [diff] [review]
V4: Apply dynamic duration to mouse wheel, Configurable for the rest

Avoid re-reading preferences when extending an existing animation for the same profile (can be as frequent as every 10ms (!) for a quick roll of the mouse wheel).
Comment 74 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-11 10:41:34 PDT
Comment on attachment 604731 [details] [diff] [review]
V4: Apply dynamic duration to mouse wheel, Configurable for the rest

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

::: content/base/src/nsGkAtomList.h
@@ +1936,5 @@
> +GK_ATOM(Pixels,     "pixels")
> +GK_ATOM(Lines,      "lines")
> +GK_ATOM(Pages,      "pages")
> +GK_ATOM(Scrollbars, "scrollbars")
> +GK_ATOM(Other,      "other")

The atom names should be "pixels", "lines" etc

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1345,5 @@
> +  // animation for the same profile (can be as frequent as every 10(!)ms for a quick
> +  // roll of the mouse wheel).
> +  nsIAtom* mProfile;
> +  double mMinMS;
> +  double mMaxMS;

Seems like these can be PRInt32?

@@ +1346,5 @@
> +  // roll of the mouse wheel).
> +  nsIAtom* mProfile;
> +  double mMinMS;
> +  double mMaxMS;
> +  double mIsProfileSmoothEnabled;

This should be bool?

Also should be called mIsSmoothnessProfileEnabled.

@@ +1347,5 @@
> +  nsIAtom* mProfile;
> +  double mMinMS;
> +  double mMaxMS;
> +  double mIsProfileSmoothEnabled;
> +  double mIntervalRatio;

You need to document all these.

@@ +1392,5 @@
>                                    mStartPos.y, mDestination.y));
>  }
>  
> +/*
> + * Calculate/update mDuration, possibly dynamically according to events rate and smooth profile.

"smoothness profile"

@@ +1397,5 @@
> + * (also maintain previous timestamps - which are only used here).
> + */
> +void
> +nsGfxScrollFrameInner::AsyncScroll::InitDuration(nsIAtom *aProfile) {
> +

No blank line here

@@ +1405,5 @@
> +    mMinMS = mMaxMS = 0;
> +    mIsProfileSmoothEnabled = false;
> +    mIntervalRatio = 1;
> +    
> +    // Default smooth scroll values for different triggers are defined at /modules/libpref/src/init/all.js

You don't need to say this. Default values for all preferences are defined in all.js.

@@ +1408,5 @@
> +    
> +    // Default smooth scroll values for different triggers are defined at /modules/libpref/src/init/all.js
> +    if (aProfile) {
> +      const double kDefaultMinMS = 150, kDefaultMaxMS = 150;
> +      const bool kDefaultIsSmoothEnabled = true;

these can be static.

@@ +1410,5 @@
> +    if (aProfile) {
> +      const double kDefaultMinMS = 150, kDefaultMaxMS = 150;
> +      const bool kDefaultIsSmoothEnabled = true;
> +
> +      nsCString profileName;

nsAutoCString

@@ +1421,5 @@
> +        nsCString prefMax = prefBase + NS_LITERAL_CSTRING(".durationMaxMS");
> +        mMinMS = Preferences::GetInt(prefMin.get(), kDefaultMinMS);
> +        mMaxMS = Preferences::GetInt(prefMax.get(), kDefaultMaxMS);
> +
> +        const double kSmoothScrollMaxAllowedAnimationDurationMS = 10000;

static

@@ +1428,5 @@
> +      }
> +      
> +      // Keep the animation duration longer than the average event intervals
> +      //   (to "connect" consecutive scroll animations before the scroll comes to a stop).
> +      const double kDurationToIntervalRatio = 2;

static

@@ +1430,5 @@
> +      // Keep the animation duration longer than the average event intervals
> +      //   (to "connect" consecutive scroll animations before the scroll comes to a stop).
> +      const double kDurationToIntervalRatio = 2;
> +      mIntervalRatio = (double)Preferences::GetInt("general.smoothScroll.durationToIntervalRatio",
> +                                                   kDurationToIntervalRatio * 100) / 100;

Instead of casting to (double) you can just divide by 100.0

@@ +1432,5 @@
> +      const double kDurationToIntervalRatio = 2;
> +      mIntervalRatio = (double)Preferences::GetInt("general.smoothScroll.durationToIntervalRatio",
> +                                                   kDurationToIntervalRatio * 100) / 100;
> +      if (mIntervalRatio <= 0)
> +        mIntervalRatio = 1;

mIntervalRatio = NS_MAX<double>(1, mIntervalRatio).

@@ +1642,5 @@
>   * this method wraps calls to ScrollToImpl(), either in one shot or incrementally,
>   *  based on the setting of the smooth scroll pref
>   */
>  void
> +nsGfxScrollFrameInner::ScrollToWithSmoothProfile(nsPoint aScrollPosition,

SmoothnessProfile

::: layout/generic/nsGfxScrollFrame.h
@@ +179,5 @@
>  
>    nsPoint RestrictToDevPixels(const nsPoint& aPt, nsIntPoint* aPtDevPx, bool aShouldClamp) const;
>    nsPoint ClampScrollPosition(const nsPoint& aPt) const;
>    static void AsyncScrollCallback(nsITimer *aTimer, void* anInstance);
> +  void ScrollTo(nsPoint aScrollPosition, nsIScrollableFrame::ScrollMode aMode){

) {

::: modules/libpref/src/init/all.js
@@ +1329,5 @@
> +// Some triggers:
> +// Pixels: Discrete mouse wheel events, Synaptics touchpads on windows (generate wheel events)
> +// Lines:  Up/Down/Left/Right KB arrows
> +// Pages:  Page up/down, Space
> +// Scrollbars: Clicking scrollbars arrows, clicking scrollbars empty areas

We call the latter case the "scrollbar track" rather than "empty areas"

@@ +1347,5 @@
> +pref("general.smoothScroll.lines", true);
> +pref("general.smoothScroll.pages", true);
> +pref("general.smoothScroll.scrollbars", true);
> +pref("general.smoothScroll.other", true);
> +// Animation lasts longer than events intervals. This defines by how much (percentage)

This comment is unclear.
Comment 75 Avi Halachmi (:avih) 2012-03-11 13:55:38 PDT
Created attachment 604788 [details] [diff] [review]
V5: Apply dynamic duration to mouse wheel, Configurable for the rest

Address roc's comments.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #74)
> Comment on attachment 604731 [details] [diff] [review]
> V4: Apply dynamic duration to mouse wheel, Configurable for the rest
> @@ +1346,5 @@
> > +  // roll of the mouse wheel).
> > +  nsIAtom* mProfile;
> > +  double mMinMS;
> > +  double mMaxMS;
> > +  double mIsProfileSmoothEnabled;
...
> Also should be called mIsSmoothnessProfileEnabled.
> 

This is the "isEnabled" value of this profile, so I think not.
Comment 76 Jared Wein [:jaws] (please needinfo? me) 2012-03-12 19:13:22 PDT
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d676d694f99
Comment 77 Marco Bonardo [::mak] 2012-03-13 05:56:34 PDT
https://hg.mozilla.org/mozilla-central/rev/0d676d694f99
Comment 78 Avi Halachmi (:avih) 2012-03-13 09:48:12 PDT
As minor as it is, since it's my first committed patch, I just wanted to thank the helpful guys on IRC #introduction/#perf (Ms2ger, khuey, Yoric, Mook, taras, ... and firebot!) and especially jaws who probably put as much time into it as myself. Also, I can really appreciate such attention to code quality. So cheers :)

Summary of potential improvements/new-options:
1. Edge behavior - use less "resistance" (see comment 69).
2. Distance relative to client area size instead of by "lines".
3. Implement our own distance-acceleration.
Comment 79 Danial Horton 2012-03-15 04:26:45 PDT
This algorithm is a joke! who's foolish idea was it to add it as default.

it was bad enough when i tried it with the "yet another smooth scrolling" extension.

For goodness sakes, give the users the ability to revert to the original method (and make it possible from the GUI), this new algorithm isn't going to go down well with most users!
Comment 80 Danial Horton 2012-03-15 06:26:42 PDT
I'll apologise to Avih, its not his algorithm thats the problem, its the people that didn't listen when he said max 800 may be laggy to some people.

Does it really seem all that snappy when it takes a full second to scroll 3 lines instead of barely half of one.
Comment 81 Jared Wein [:jaws] (please needinfo? me) 2012-03-15 13:31:22 PDT
(In reply to Danial Horton from comment #80)
> I'll apologise to Avih, its not his algorithm thats the problem, its the
> people that didn't listen when he said max 800 may be laggy to some people.
> 
> Does it really seem all that snappy when it takes a full second to scroll 3
> lines instead of barely half of one.

This bug is resolved and so discussion should move to a new bug. Can you file one for adjusting the defaults? I should add that we did include the ability to change the durations through general.smoothScroll.pixels.durationMaxMS and general.smoothScroll.pixels.durationMinMS in about:config.
Comment 82 Alex Keybl [:akeybl] 2012-03-16 13:08:29 PDT
Let's track any fallout from this smooth scrolling change, but not the change itself.
Comment 83 Avi Halachmi (:avih) 2012-03-16 15:11:22 PDT
Bug 736251 adds UI to enable legacy smooth scrolling behavior, currently with a V1 patch, which is already pushed to UX. FYI, if you wish to follow it up.
Comment 84 Avi Halachmi (:avih) 2012-03-20 23:07:56 PDT
According to bug 736251 comment 20, the last patch submitted here (use longer+dynamic duration for mouse wheel scrolls) only works on Windows.

I've followed tn's suggestion and instead of reopening this bug, I've opened bug 737758 to make it work on OSX+Linux too. FYI.
Comment 85 Sean Newman 2012-03-25 11:28:06 PDT
(In reply to Sean Newman from comment #40)
> Oh, maybe (just maybe, not sure) the CC messages are not shown by default,
> so you might also need to set "browser.dom.window.dump.enabled" to "true",
> not sure though.

I got tired of those messages and at last I found out what pref did turn on that kind of debug information.

"javascript.options.mem.log" - set it to "true".
Comment 86 Avi Halachmi (:avih) 2012-04-09 05:01:21 PDT
FYI, we might want to add some info to the release notes:
- To restore previous behavior, set general.smoothScroll.mouseWheel.durationMaxMS to 150.
- You might want to experiment with higher values for general.smoothScroll.mouseWheel.durationMaxMS, such as 1000 - easier to read, but as snappy as before when rolling the mouse wheel quickly.
Comment 87 CruNcher 2012-04-12 09:31:01 PDT
Does this take Aero and non Aero and Direct2D into account or is there no real difference ?
And did someone ever investigated the Google part of this with a heavy graphical page and saw that the Pictures don't smear when scrolling with the keyboard ? 
I found that pretty amazing compared to how Firefox looks in that situation (it doesn't stress they eyes as much, decodeondraw was off).
cant wait to try avihs direct implementation now i used his extension @ first but then changed to Yet Another Smooth scroll because of its sheer configuration options to adjust it graphical to your needs for different scroll ways :)
Comment 88 CruNcher 2012-04-12 10:22:53 PDT
Also i found interesting what was wrote about CC causing these stalls could maybe those same be responsible for the frame drops in this bug when only the plain CPU is used instead of Direct2D there are less frame drops without Direct2D but still they happen https://bugzilla.mozilla.org/show_bug.cgi?id=702485
Comment 89 Mihaela Velimiroviciu (:mihaelav) 2012-05-28 05:51:35 PDT
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0 (beta 5)

Verified that smooth scroll is applied to KB arrows, mouse scroll, PgUp/PgDn, Space, scrollbar buttons/empty space.

Is there anything else to check before marking this verified?

Thank you!
Comment 90 Jared Wein [:jaws] (please needinfo? me) 2012-05-28 09:06:21 PDT
The way that the animation is done is a little different than the previous animation for smooth scrolling.

There's nothing more that needs to be verified though. Thanks!

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