Closed Bug 457864 Opened 12 years ago Closed 11 years ago

Make wheel scrolling asynchronous

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: mstange, Assigned: mstange)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
This makes diagonal pixel scrolling bearable by avoiding the "stairstep" effect.

It doesn't fix bug 418351, but it might make fixing it easier.
Attachment #341113 - Flags: review?(roc)
Fixing bug 418351 requires that we wait for all pending native scroll events to be dispatched before firing the nsRunnable. However, at least on Mac OS X, that doesn't happen:

NS_DispatchToCurrentThread(ev) appends the nsRunnable to Gecko's internal event queue. The pending native scroll events, however, are waiting in the OS's native event queue.
When the nsRunnable is appended to Gecko's internal event queue, a new native event callback is scheduled: nsThread::Dispatch -> nsThread::PutEvent -> nsBaseAppShell::OnDispatchedEvent -> nsAppShell::ScheduleNativeEventCallback.
This "native event callback" fires the events in Gecko's internal event queue.

On Mac OS X, this callback happens immediately; it doesn't wait for the pending native events. (This might be different on other platforms.) So the nsRunnable ends up being run before the pending native scroll events.
Did you try to add a flag to the first mousescroll event that there is still another coming (horizontal and vertical events), and make ESM to scroll only
after latter one?
(In reply to comment #2)
> Did you try to add a flag to the first mousescroll event that there is still
> another coming (horizontal and vertical events), and make ESM to scroll only
> after latter one?

I didn't, and with this patch it's not necessary - the nsRunnable is only dispatched after both Gecko scroll events of a diagonal scroll gesture have been processed. (The native Cocoa mouse scroll event contains information about both axis in the same event. We split it in two Gecko events.)

When I said "this callback happens immediately", I didn't mean that it fires synchronously - it fires immediately after the current native event has been processed. In the diagonal scrolling case, that's after both Gecko scroll events have been processed.
(In reply to comment #3)
> I didn't, and with this patch it's not necessary 
Right, but if the other approach works well, then this patch isn't needed.
Why would you prefer the other approach? It sounds rather ugly to me, to be honest.
That would change only mousescroll handling, and only in case there is both
horizontal and vertical scrolling. The patch changes scroll handling in many
other cases too (if I read it correctly), so it is more regression-risky.
True. And I just realized that this patch probably breaks my tests for bug 350471; they assume that scrollTop/Left changes synchronously after a scroll event.
+  mView->ExecuteAsyncScroll();

I think mView could be destroyed at that point.
Unfortunately I think scrollTop/scrollLeft really do have to change synchronously after a scroll request. Certainly it would be confusing if setting scrollTop did not immediately return the same value from scrollTop.

The plan for compositor is to make the actual rendering of scrolling asynchronous. I guess that would fix this bug.
(In reply to comment #9)
> Unfortunately I think scrollTop/scrollLeft really do have to change
> synchronously after a scroll request. Certainly it would be confusing if
> setting scrollTop did not immediately return the same value from scrollTop.

Good point.

> The plan for compositor is to make the actual rendering of scrolling
> asynchronous. I guess that would fix this bug.

OK, that sounds like the right thing to do.
Attachment #341113 - Flags: review?(roc) → review-
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
I just realized that this approach might work after all: Smooth scrolling is asynchronous, too, so we could just tweak the patch to only use the nsRunnable in those cases where smooth scrolling would be appropriate, i.e. only when ::ScrollTo is caused by a mouse wheel event.
Status: REOPENED → ASSIGNED
Summary: Make nsScrollableView use an nsRunnable for asynchronous scrolling → Make wheel scrolling asynchronous
Attached patch v2 (obsolete) — Splinter Review
This patch addresses both comment 8 and comment 9 by unifying smooth scrolling and asynchronous scrolling:
 - I now use a timer instead of an nsRunnable; the timer is stopped when the
   view is destroyed, so the problem in comment 8 can't happen.
 - I use the NS_VMREFRESH_DEFERRED flag in the update flags to indicate that
   scrolling should be handled asynchronously. This flag is only set in the
   ::ScrollByXXX methods. This should avoid the scrollTop problem of comment 9.

I haven't run mochitests yet. I also need to update the test for bug 350471 since it assumes that the scroll position changes synchronously after a scroll event.

And now the good news: Combined with the patch in bug 459319, this patch fixes bug 418351! I've created a build that contains both patches: http://tests.themasta.com/fastscrolling_firefox-3.1b2pre.en-US.mac.dmg
Attachment #341113 - Attachment is obsolete: true
Attachment #343390 - Flags: review?(roc)
Instead of making nsChildView::Update a no-op (which is what bug 459319 does), we could also remove the call to ForceViewUpdate in nsEventStateManager::DoScrollText and the call to nsViewManager::UpdateViewAfterScroll in nsScrollPortView::Scroll - these are the only callers of nsChildView::Update while scrolling. But that makes me wonder why these calls were introduced in the first place.
The problem is that if the repaint of the window is delayed too much after the bitblit phase of the scroll, you get a nasty effect where one part of the window has been copied but the old copy is still visible.
Your patch looks good, and it's a great idea, but I don't like the fact that it makes scriptable methods like window.scrollByLines asynchronous. I think you should add flags parameters to the nsIScrollableView methods, and then just pass DEFERRED for the scrolling triggered by mousewheel in nsEventStateManager.
Attached patch v3 [checked in]Splinter Review
Attachment #343390 - Attachment is obsolete: true
Attachment #343656 - Flags: review?(roc)
Attachment #343390 - Flags: review?(roc)
Tested v3, "stairstep" is gone and at least here bug 418351 is pretty much gone.
Maybe not quite as fast as Safari, but pretty close.
bug 459319 would make things even a bit faster.

/me trying to get used to OSX.
I wonder if removing nsEventStateManager::ForceViewUpdate could still make any difference.
Would have to debug if it is ever called when nsChildView::mView::needsDisplay returns true, or something.
Seems like it doesn't save anything, at least not when scrolling tbox page. 
It is just useless call to update, when there isn't anything to update.
Comment on attachment 343656 [details] [diff] [review]
v3 [checked in]

>     else
>-      scrollView->ScrollByLines(scrollX, scrollY);
>+      scrollView->ScrollByLines(scrollX, scrollY,
>+                                NS_VMREFRESH_SMOOTHSCROLL | NS_VMREFRESH_DEFERRED);
Does this mean that smoothscroll is always used for scrollbylines?
Comment on attachment 343656 [details] [diff] [review]
v3 [checked in]


>+  PRBool isSmoothScroll = (aUpdateFlags & NS_VMREFRESH_SMOOTHSCROLL) &&
>+                          IsSmoothScrollingEnabled();
OK, it doesn't mean that.
Attachment #344464 - Flags: review?(Olli.Pettay)
There are many places where ScrollTo gets called with NS_SCROLL_PROPERTY_ALWAYS_BLIT, although it's ignored completely. The only place where NS_SCROLL_PROPERTY_ALWAYS_BLIT makes a difference is when calling SetScrollProperties (and there's only one line that does that).

In fact, with my patch NS_SCROLL_PROPERTY_ALWAYS_BLIT is misinterpreted as NS_VMREFRESH_DEFERRED (they're both 1), which is bad.

In this patch I make sure that NS_SCROLL_PROPERTY_ALWAYS_BLIT is never passed to ScrollTo.
Attachment #344465 - Flags: review?
Attachment #344465 - Flags: review? → review?(roc)
Comment on attachment 344464 [details] [diff] [review]
fix tests [checked in]

>+    setTimeout(function() {
Would it be possible to use "scroll" event? It is dispatched after scrolling.
Timeouts may in some cases cause random tbox orange.
(In reply to comment #24)
> (From update of attachment 344464 [details] [diff] [review])
> >+    setTimeout(function() {
> Would it be possible to use "scroll" event? It is dispatched after scrolling.
I guess that would work when there isn't any scrolling happening.
er, wouldn't work
Attachment #344464 - Flags: review?(Olli.Pettay) → review+
pushed:
http://hg.mozilla.org/mozilla-central/rev/cd77e64a8aea
http://hg.mozilla.org/mozilla-central/rev/296089ec0903
http://hg.mozilla.org/mozilla-central/rev/1622ca12ba25
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
I backed out because there are still two tests that need fixing:
- /tests/docshell/test/navigation/test_bug430723.html
- /tests/editor/libeditor/base/tests/test_selection_move_commands.xul

http://hg.mozilla.org/mozilla-central/rev/1e477902f74d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #343656 - Attachment description: v3 → v3 [backed out]
Attachment #344464 - Attachment description: fix tests → fix tests [checked in]
Attachment #344465 - Attachment description: remove NS_SCROLL_PROPERTY_ALWAYS_BLIT from ScrollTo callers → remove NS_SCROLL_PROPERTY_ALWAYS_BLIT from ScrollTo callers [backed out]
I was wrong. The test failures were caused by the fact that I landed the wrong version of the patch (v2 instead of v3). This is quite embarassing.

I relanded, this time hopefully the right ones.
http://hg.mozilla.org/mozilla-central/rev/89b91ae29a3d
http://hg.mozilla.org/mozilla-central/rev/6ea413efbe9a
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Attachment #343656 - Attachment description: v3 [backed out] → v3 [checked in]
Attachment #344465 - Attachment description: remove NS_SCROLL_PROPERTY_ALWAYS_BLIT from ScrollTo callers [backed out] → remove NS_SCROLL_PROPERTY_ALWAYS_BLIT from ScrollTo callers [checked in]
> -  if (mSmoothScroll) {
> +  if (mAsyncScroll && mAsyncScroll->mIsSmoothScroll) {
>      ...
>    } else {
>      ...
> +    mAsyncScroll = new AsyncScroll;
This caused a leak.

With smaug's help I landed this fix:
http://hg.mozilla.org/mozilla-central/rev/9f4dddb8d5f3
Could this have caused bug 462793?
(In reply to comment #14)
> The problem is that if the repaint of the window is delayed too much after the
> bitblit phase of the scroll, you get a nasty effect where one part of the
> window has been copied but the old copy is still visible.
Shouldn't it be enough if we mark all the relevant (OSX) widgets with needsDisplay YES and let OS to handle painting? Then whenever OS decides to
paint something, all the dirty widgets get redrawn. Or am I missing something here?
Maybe.

Scrolling has two parts: bitblitting part of the window from one location to another, and repainting the rest. Those two parts need to happen close together in time or you get obvious transient glitches. That's why we force an update right after blitting instead of waiting for the OS to get around to doing it.

Does that make sense?
Makes sense, but what doesn't make sense is that with the patch for bug 459319, 
there are no glitches. I wonder if nsChildView does the right thing with bitblitting, maybe it repaints too much.
There is probably too many updates or too large repainting happening,
because bug 459319 doesn't cause problems.
Disabling bitblitting on OSX certainly doesn't make scrolling slower, but
I think it actually makes it faster, at least on flash-heavy pages.
It certainly can be faster on some pages, but what about just a big window with a plain text document in it?
or a big window with lots of big images in it. And scroll by lines with the down-arrow.
(In reply to comment #36)
> but what about just a big window with a plain text document in it?
Couldn't see any difference.

(In reply to comment #37)
> or a big window with lots of big images in it. And scroll by lines with the
> down-arrow.
nor here.
Even with
PR_Sleep(PR_MillisecondsToInterval(200));
at the beginning of the drawRect method in nsChildView.mm I can't see any glitches (with the patch for bug 459319).
I'm confused. I've fixed bugs in the past on Mac where we were scrolling slowly because we were repainting the whole window by mistake. So how can it not matter now?
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.