1139220 - Use a key spline animation for APZ wheel animations

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 2

[Botond Ballo [:botond]]

Taking with permission.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, move AsyncPanZoomAnimation into its own file

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, factor out AsyncScroll

This factors the guts of ScrollFrameHelper::AsyncScroll into its own class, AsyncScrollBase, so that we can use it in APZ as well.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3 WIP, create a WheelScrollAnimation

This creates a new APZ animation wrapping AsyncScrollBase. Not r?'ing yet since the velocity is messed up.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, remove dead code

After we have the new animation, some stuff in SmoothScrollAnimation becomes dead.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[:kip (Kearwood Gilbert)]

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.

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, create a WheelScrollAnimation

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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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_

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, create a WheelScrollAnimation

w/ comments addressed

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, remove dead code

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, stronger typing for GetScrollWheelDelta

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8586876 [details] [diff] [review]
part 3, create a WheelScrollAnimation

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

Thanks

Comment 18

[Kartikaya Gupta (email:[e-mail])]

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

Comment 19

[:kip (Kearwood Gilbert)]

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()

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b92f3ac380c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/15b7c48af4c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fce26111e6f0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2e519e843706
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32358e02e9ae

Comment 21

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=8391441&repo=mozilla-inbound

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

Take two w/ build bustage fix:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/408eb2514c7c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/182ea0cfe49c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c4f3a3a15b3b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e34a66a053da
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c982f0795de

Comment 23

[Tooru Fujisawa [:arai]]

Fixed static analysis build bustage :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1934d57a5233

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/408eb2514c7c
https://hg.mozilla.org/mozilla-central/rev/182ea0cfe49c
https://hg.mozilla.org/mozilla-central/rev/c4f3a3a15b3b
https://hg.mozilla.org/mozilla-central/rev/e34a66a053da
https://hg.mozilla.org/mozilla-central/rev/5c982f0795de
https://hg.mozilla.org/mozilla-central/rev/1934d57a5233