David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 9 years ago

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.

	Botond Ballo [:botond]
	Comment 2 • 9 years ago

Taking with permission.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 3 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 5 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 7 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 8 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 9 • 9 years ago

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.

	:kip (Kearwood Gilbert)
	Comment 10 • 9 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 11 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 9 years ago

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_

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 13 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 14 • 9 years ago

w/ comments addressed

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 17 • 9 years ago

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

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

Thanks

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 18 • 9 years ago

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

	:kip (Kearwood Gilbert)
	Comment 19 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 20 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 21 • 9 years ago

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

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 22 • 9 years ago

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

	Tooru Fujisawa [:arai]
	Comment 23 • 9 years ago

Fixed static analysis build bustage :)

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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 9 years ago

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