Closed Bug 206438 Opened 21 years ago Closed 12 years ago

Smooth scrolling should use the 'smoothwheel' algorithm

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla13
Tracking Flags:
Tracking Status
firefox13 - ---

People

(Reporter: jrs_66, Assigned: avih)

References

(
URL
)

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 file, 8 obsolete files)

V5: Apply dynamic duration to mouse wheel, Configurable for the rest
18.19 KB, patch
roc
: review+
Details | Diff | Splinter Review 
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.
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.
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.
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.
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.
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.
Status: UNCONFIRMED → RESOLVED
Closed: 21 years ago
Resolution: --- → WONTFIX
Ok.
Thank you.
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
*** Bug 239531 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
Reopening, I think many people will benefit from this feature and maybe extension author (avih) is still willing to help.
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
URL: http://drudgereport.com → http://smoothwheel.mozdev.org/
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Depends on: smoothscroll
Ever confirmed: true
OS: Windows 2000 → All
Product: Mozilla Application Suite → Core
Hardware: PC → All
Assignee: general → nobody
QA Contact: general → general
Is this still wanted?
Whiteboard: [Snappy]
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?
Blocks: 710372
No longer blocks: 675866
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
(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.
There's also this other extension ( https://addons.mozilla.org/it/firefox/addon/yet-another-smooth-scrolling/ ) that could be investigated.
Jared will look into using this extension if our built-in physics can't be sufficiently tweaked.
Whiteboard: [Snappy] → [Snappy:p3]
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..
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.
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").
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.
No problem, and note taken.
Assignee: nobody → chuku_regs
Pushed to UX Nightly branch: https://hg.mozilla.org/projects/ux/rev/3ed31af179c2
Status: NEW → ASSIGNED
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.
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'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...
You can get the hg revision from about:buildconfig, and punch them into http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=&tochange=
With s/tracemonkey/mozilla-central/ of course.
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).
(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)?
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.
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.
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.
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.
I filed bug 728738 for this.
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?
Attachment #598361 - Flags: feedback?(ux-review)
Whiteboard: [Snappy:p3] → [Snappy:p1]
This moves the algorithm variables to about:config.

Pushed to UX Nightly branch: https://hg.mozilla.org/projects/ux/rev/7565525d9138
Attachment #598361 - Attachment is obsolete: true
Attachment #599369 - Flags: ui-review?(ux-review)
Attachment #598361 - Flags: feedback?(ux-review)
Whiteboard: [Snappy:p1] → [Snappy:P1][autoland-try]
Whiteboard: [Snappy:P1][autoland-try] → [Snappy:P1][autoland-in-queue]
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
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
Whiteboard: [Snappy:P1][autoland-in-queue] → [Snappy:P1]
(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.
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 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 :(
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).
Summary: Smoothscroll should be replaced by 'smoothwheel' extension → Smooth scrolling should use the 'smoothwheel' algorithm
The initialization would hopefully satisfy the try server.
The averaging (over last 3 events deltas) should moderate fluctuations of the dynamic duration.
P.S. The patch from comment 43 is against current mozilla-central (and I forgot to mark it as a diff file... sorry).
Attachment #599369 - Flags: ui-review?(ux-review)
Attachment #599369 - Attachment is obsolete: true
Attachment #601668 - Attachment is patch: true
Attachment #601668 - Flags: ui-review?(ux-review)
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);
(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
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.
Attachment #601668 - Attachment is obsolete: true
Attachment #601668 - Flags: ui-review?(ux-review)
Attachment #603092 - Attachment is obsolete: true
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
Attachment #603596 - Flags: ui-review?(ux-review)
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.
Attachment #603596 - Flags: ui-review?(ux-review) → ui-review+
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?
(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 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?
(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.
(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.
Also, where did we put the results of these discussions? I think I missed reading the results.
(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.
(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.
(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.
(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.
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".
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.
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).
(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.
(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.
(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.
(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.
(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.
(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 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 (
Address roc's comments.
Attachment #603596 - Attachment is obsolete: true
Attachment #604597 - Attachment is obsolete: true
Attachment #604706 - Flags: review?(roc)
Attachment #603596 - Flags: review?(roc)
Attachment #604597 - Flags: review?(roc)
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).
Attachment #604706 - Attachment is obsolete: true
Attachment #604731 - Flags: review?(roc)
Attachment #604706 - Flags: review?(roc)
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.
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.
Attachment #604731 - Attachment is obsolete: true
Attachment #604788 - Flags: review?(roc)
Attachment #604731 - Flags: review?(roc)
Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d676d694f99
Flags: in-testsuite-
Flags: in-litmus-
https://hg.mozilla.org/mozilla-central/rev/0d676d694f99
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
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.
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!
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.
(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.
Let's track any fallout from this smooth scrolling change, but not the change itself.
tracking-firefox13: ? → -
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.
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.
Depends on: 737758
Component: General → Layout
QA Contact: general → layout
Depends on: 736251
(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".
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.
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 :)
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
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!
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!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: