Closed Bug 1139220 Opened 5 years ago Closed 5 years ago

Use a key spline animation for APZ wheel animations

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(5 files, 3 obsolete files)

Currently, wheel events in APZ do not always maintain their scroll velocity in between scroll animations. Since wheel deltas are very small the animation often completes before the next one comes in (even with very fast scrolling), so the scroll speed appears very slow.

All the code to make this work is already there, it just need to be hooked up to wheel events.
Duplicate of this bug: 1140903
Taking with permission.
Assignee: dvander → botond
Blocks: 1143883
Stealing back w/ permission... after testing this again lately, the scrolling animation fields weird in general for Desktop. It's also not compatible with scroll snapping which just landed.

Re-purposing this bug to implement the main thread wheel animation in APZ and hopefully just address all the problems at once.
Assignee: botond → dvander
Summary: Maintain scroll velocity in between scroll wheel animations → Use a key spline animation for APZ wheel animations
This factors the guts of ScrollFrameHelper::AsyncScroll into its own class, AsyncScrollBase, so that we can use it in APZ as well.
Attachment #8585911 - Flags: review?(kgilbert)
This creates a new APZ animation wrapping AsyncScrollBase. Not r?'ing yet since the velocity is messed up.
Attached patch part 4, remove dead code (obsolete) — Splinter Review
After we have the new animation, some stuff in SmoothScrollAnimation becomes dead.
Attachment #8585914 - Flags: review?(bugmail.mozilla)
Comment on attachment 8585909 [details] [diff] [review]
part 1, move AsyncPanZoomAnimation into its own file

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

::: gfx/layers/apz/src/AsyncPanZoomAnimation.h
@@ +10,5 @@
> +#include "base/message_loop.h"
> +#include "mozilla/RefPtr.h"
> +#include "FrameMetrics.h"
> +#include "nsISupportsImpl.h"
> +#include "mozilla/TimeStamp.h"

Sort headers
Attachment #8585909 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8585914 [details] [diff] [review]
part 4, remove dead code

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

Looks ok but I'd rather review it after you've finalized part 3. It looks like you can also remove SmoothScrollAnimation::mSource, unless you end up using it in part 3.
Attachment #8585914 - Flags: review?(bugmail.mozilla)
Comment on attachment 8585911 [details] [diff] [review]
part 2, factor out AsyncScroll

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

Looks great, just a couple of tiny nits.

::: layout/generic/AsyncScrollBase.cpp
@@ +67,5 @@
> +void
> +AsyncScrollBase::InitializeHistory(TimeStamp aTime)
> +{
> +  // Starting a new scroll (i.e. not when extending an existing scroll animation),
> +  //   create imaginary prev timestamps with maximum relevant intervals between them.

nit: extraneous white-space before "create".

::: layout/generic/AsyncScrollBase.h
@@ +19,5 @@
> +public:
> +  typedef mozilla::TimeStamp TimeStamp;
> +  typedef mozilla::TimeDuration TimeDuration;
> +
> +public:

Already public, don't need to repeat.

@@ +29,5 @@
> +
> +  // Get the velocity at a point in time in nscoords/sec.
> +  nsSize VelocityAt(TimeStamp aTime) const;
> +
> +  // Returns the expected scroll position at a given point in time.

Might be useful to document the coordinate system of the return value in the comment.

@@ +55,5 @@
> +  void InitTimingFunction(nsSMILKeySpline& aTimingFunction,
> +                          nscoord aCurrentPos, nscoord aCurrentVelocity,
> +                          nscoord aDestination);
> +
> +protected:

Already protected, don't need to repeat.
Attachment #8585911 - Flags: review?(kgilbert) → review+
WheelScrollAnimation simply wraps the nsSMILKeySpline logic that we use on the main thread. It seems to work fine, though I don't know if I'm potentially missing any state I should be giving APZ.
Attachment #8585913 - Attachment is obsolete: true
Attachment #8586607 - Flags: review?(bugmail.mozilla)
Comment on attachment 8586607 [details] [diff] [review]
part 3, create a WheelScrollAnimation

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

Looks mostly good, a few things I'd like to see addressed though.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1154,5 @@
>        // The scale gesture listener should have handled this.
>        NS_WARNING("Gesture listener should have handled pinching in OnTouchMove.");
>        return nsEventStatus_eIgnore;
>  
> +    case WHEEL_SCROLL:

You need to handle this state in OnTouchStart also

@@ +1561,4 @@
>        }
> +
> +      nsPoint delta =
> +        CSSPoint::ToAppUnits(LayoutDevicePoint(deltaX, deltaY) / mFrameMetrics.GetDevPixelsPerCSSPixel());

At some point we should change GetScrollWheelDelta to be more like this:

LayoutDevicePoint GetScrollWheelDelta(const ScrollWheelInput& aInput);

so that it's strongly typed. Should be a pretty trivial change and will help with readability. Can do this in a followup or a separate patch on this bug.

@@ +1562,5 @@
> +
> +      nsPoint delta =
> +        CSSPoint::ToAppUnits(LayoutDevicePoint(deltaX, deltaY) / mFrameMetrics.GetDevPixelsPerCSSPixel());
> +      nsPoint velocity =
> +        CSSPoint::ToAppUnits(CSSPoint(mX.GetVelocity(), mY.GetVelocity())) * 1000.0f;

Comment this conversion. Just copy the comment from StartSmoothScroll. Without it this code doesn't make much sense.

::: gfx/layers/apz/src/WheelScrollAnimation.cpp
@@ +6,5 @@
> +
> +#include "WheelScrollAnimation.h"
> +
> +using namespace mozilla;
> +using namespace mozilla::layers;

I'd rather do

namespace mozilla {
namespace layers {
...
}
}

and use fully-qualified class names for anything else as needed (either in using statements or in the code). In general "using namespace" is not nice with unified builds because it can affect other files and break builds in strange ways.

@@ +34,5 @@
> +
> +  CSSToParentLayerScale2D zoom = aFrameMetrics.GetZoom();
> +
> +  nsPoint position = PositionAt(now);
> +  ParentLayerPoint displacement = 

nit: trailing ws

::: gfx/layers/apz/src/WheelScrollAnimation.h
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_layers_ScrollAnimation_h_
> +#define mozilla_layers_ScrollAnimation_h_

_WheelScrollAnimation_
Attachment #8586607 - Flags: review?(bugmail.mozilla) → review-
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12)
>
> and use fully-qualified class names for anything else as needed (either in
> using statements or in the code). In general "using namespace" is not nice
> with unified builds because it can affect other files and break builds in
> strange ways.

Interesting, good point.
w/ comments addressed
Attachment #8586607 - Attachment is obsolete: true
Attachment #8586607 - Flags: review?(kgilbert)
Attachment #8586876 - Flags: review?(bugmail.mozilla)
Attachment #8585914 - Attachment is obsolete: true
Attachment #8586877 - Flags: review?(bugmail.mozilla)
Comment on attachment 8586876 [details] [diff] [review]
part 3, create a WheelScrollAnimation

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

Thanks
Attachment #8586876 - Flags: review?(bugmail.mozilla) → review+
Attachment #8586877 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8586889 [details] [diff] [review]
part 5, stronger typing for GetScrollWheelDelta

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

Great, thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1450,2 @@
>                   ? pageScrollSize.width
>                   : -pageScrollSize.width;

fix indentation, or just collapse it all onto one line. ditto for the .y case below
Attachment #8586889 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8586876 [details] [diff] [review]
part 3, create a WheelScrollAnimation

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

Looks good, r=me with the TimeStamp function change.

::: gfx/layers/apz/src/WheelScrollAnimation.cpp
@@ +26,5 @@
> +
> +bool
> +WheelScrollAnimation::DoSample(FrameMetrics& aFrameMetrics, const TimeDuration& aDelta)
> +{
> +  TimeStamp now = TimeStamp::Now();

Should use AsyncPanZoomController::GetFrameTime() instead of TimeStamp::Now()
Attachment #8586876 - Flags: review?(kgilbert) → review+
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=8391441&repo=mozilla-inbound
Flags: needinfo?(dvander)
Depends on: 1207656
You need to log in before you can comment on or make changes to this bug.