1457586 - Fennec's initial fling velocity is lower than in Chrome, even when using Chrome's fling physics

Status: RESOLVED FIXED

Description

[Markus Stange [:mstange]]

Steps to reproduce:
 1. On Firefox for Android, go to about:config and set apz.android.chrome_fling_physics.enabled to true.
 2. Go to any scrollable page, for example https://en.m.wikipedia.org/wiki/Gomphus_clavatus
 3. Scroll up and down alternatingly, using short but quick finger motions.

Expected results:
Scrolling should feel the same in Firefox as it does in Chrome.

Actual results:
Firefox scrolls a much shorter distance based on the same touch input.

Firefox also reacts later than Chrome, or at least that's what it feels like. Maybe we ignore the first few touch moves because of some tolerance for bad touch screens?
The main difference seems to be the fling start velocity. Maybe we're averaging too many touch deltas when computing that velocity?

Comment 1

[Markus Stange [:mstange]]

Created attachment 8971714 [details]
video recording

I'm happy to demonstrate this in person. Here's a video I just took of it.

Comment 2

[Chris Peterson [:cpeterson]]

Tagging [geckoview:klar:p2] because this would be nice to fix for Klar+GeckoView, but is not a blocker.

Comment 3

[Botond Ballo [:botond]]

Yes, it's possible that there's still a difference in the starting fling velocity. How Chrome determines the initial fling velocity is not one of the things I investigated in bug 1448376; I sort of assumed it would correspond to the physical velocity of the page at the point of the fling's release, the way it does for us, but maybe it doesn't.

Comment 4

[Botond Ballo [:botond]]

I tried tracking down in the Chromium codebase where the fling's start velocity comes from on Android, but it's pretty convoluted. I think I traced it as far as the parameters to WebView.flingScroll(), which is an Android API, and so (I guess) has its callers in Android code rather than Chromium code?

Comment 5

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #4)
> I think I traced
> it as far as the parameters to WebView.flingScroll(), which is an Android
> API, and so (I guess) has its callers in Android code rather than Chromium
> code?

It looks like this was a false direction. WebView.flingScroll() is a way for apps using Android's WebView to start a fling animation with an initial velocity of their choosing, it's not the codepath by which the Chrome app itself initiates a fling animation. Markus helped me locate the relevant code [1].

(In reply to Botond Ballo [:botond] from comment #3)
> How Chrome determines the initial fling velocity is not one of the
> things I investigated in bug 1448376; I sort of assumed it would correspond
> to the physical velocity of the page at the point of the fling's release,
> the way it does for us, but maybe it doesn't.

I glossed over something here: we don't quite use the page's physical velocity at the point of the fling's release, but rather an average based on the last few touch events.

The difference seems to be how we compute this "average": in the Firefox code, it literally is just an average, whereas in the Chromium code, they use a more sophisticated model (second-degree least squares linear regression) to compute it [2].

[1] https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_detector.cc?l=376&rcl=85d94fd334e5df67195bbef8513b0df5bbb570b8
[2] https://cs.chromium.org/chromium/src/ui/events/gesture_detection/velocity_tracker.cc?l=101&rcl=9ea9a086d4f54c702ec9a38e55fb3eb8bbc2401b

Comment 6

[Botond Ballo [:botond]]

Going to start working on this.

Comment 7

[Chris Peterson [:cpeterson]]

Awesome! If you land the fix in Nightly 63, we should consider uplifting it to Beta 62 so we can ship it together with new fling physics (bug 1460206).

Comment 8

[rishel.nick]

The scroll flinging solution seem very similar to the solutions to throwing in VR - https://www.gamasutra.com/blogs/CharlieDeck/20161118/285808/Why_Throwing_in_VR_Sucksand_How_to_Make_it_Better.php

Extrapolating from that there might be a responsiveness win in tying either the fling velocity calculation or fling trigger to an apparent drop in touch pressure instead of touch release as it better signals the user's intended point of release.

I was thinking of opening an issue for this but I'm uncertain if it would just be unnecessary noise.

Comment 9

[Botond Ballo [:botond]]

(In reply to rishel.nick from comment #8)
> Extrapolating from that there might be a responsiveness win in tying either
> the fling velocity calculation or fling trigger to an apparent drop in touch
> pressure instead of touch release as it better signals the user's intended
> point of release.
> 
> I was thinking of opening an issue for this but I'm uncertain if it would
> just be unnecessary noise.

Please feel free to file an issue for this suggestion. We can discuss it further there.

Comment 10

[Chris Peterson [:cpeterson]]

glamon (UX) says he is able to reproduce this bug in Fennec 61 and it is fairly noticeable. The effect seems more pronounced while scrolling up than down. He would prioritize this below "scrolling hitch" bug 1425739 since this feels more like a "difference" than an "error".

Comment 11

[Botond Ballo [:botond]]

Created attachment 8997615 [details]
Bug 1457586 - Simplify Axis::UpdateWithTouchAtDevicePoint() by removing the aAdditionalDelta parameter.

Review commit: https://reviewboard.mozilla.org/r/261312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261312/

Comment 12

[Botond Ballo [:botond]]

Created attachment 8997616 [details]
Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity computation along an axis.

The current velocity computation code is factored out into an implementation
called SimpleVelocityTracker.

Review commit: https://reviewboard.mozilla.org/r/261314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261314/

Comment 13

[Botond Ballo [:botond]]

Created attachment 8997617 [details]
Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker implementation.

Review commit: https://reviewboard.mozilla.org/r/261316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261316/

Comment 14

[Botond Ballo [:botond]]

Created attachment 8997618 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy.

Review commit: https://reviewboard.mozilla.org/r/261318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261318/

Comment 15

[Botond Ballo [:botond]]

This is an initial patch series; it needs debugging and is not ready for review yet.

Comment 16

[Botond Ballo [:botond]]

Created attachment 8997757 [details]
Bug 1457586 - Refactor APZCTreeManager::ProcessTouchVelocity() and related functions to take the time and position deltas separately.

The motivation is to support velocity tracking implementations (added in a
later patch) that need the position delta rather than resulting velocity.

Also rename the functions to make it clearer that they have to do with
dynamic toolbar movement.

Review commit: https://reviewboard.mozilla.org/r/261492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261492/

Comment 17

[Botond Ballo [:botond]]

Created attachment 8997758 [details]
Bug 1457586 - Initialize AsyncPanZoomController::mPlatformSpecificState before the axes.

This allows the Axis constructors to use the platform-specific state.

Review commit: https://reviewboard.mozilla.org/r/261494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261494/

Comment 18

[Botond Ballo [:botond]]

Created attachment 8997759 [details]
Bug 1457586 - Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully.

Review commit: https://reviewboard.mozilla.org/r/261496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261496/

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8997616 [details]
Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity computation along an axis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261314/diff/1-2/

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8997617 [details]
Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261316/diff/1-2/

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8997618 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261318/diff/1-2/

Comment 22

[Botond Ballo [:botond]]

Markus, could you try the patches (you can use a Try build from [1]) and let me know if it fixes the problem as you originally reported it? Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=24ec0cece2e7e546ef8928e186b7dca1237450d5

Comment 23

[Botond Ballo [:botond]]

(In reply to Chris Peterson [:cpeterson] from comment #10)
> He would prioritize this below "scrolling hitch" bug 1425739 since
> this feels more like a "difference" than an "error".

I did see this comment, but I was already in the middle of working on this, and it seemed more realistic to finish this before going on PTO, than to change tack and work on bug 1425739.

I asked Markus if he could look at bug 1425739; if not, it'll be my first priority after returning from PTO.

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8997618 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261318/diff/2-3/

Comment 25

[Botond Ballo [:botond]]

Comment on attachment 8997759 [details]
Bug 1457586 - Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261496/diff/1-2/

Comment 26

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

Comment on attachment 8997757 [details]
Bug 1457586 - Refactor APZCTreeManager::ProcessTouchVelocity() and related functions to take the time and position deltas separately.

https://reviewboard.mozilla.org/r/261492/#review268594

Comment 27

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

Comment on attachment 8997616 [details]
Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity computation along an axis.

https://reviewboard.mozilla.org/r/261314/#review268596

::: gfx/layers/apz/src/SimpleVelocityTracker.cpp:7
(Diff revision 2)
> +#include <cstdint>
> +#include <utility>

nit: don't need these includes here, since they are already in the .h file

Comment 28

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

Comment on attachment 8997758 [details]
Bug 1457586 - Initialize AsyncPanZoomController::mPlatformSpecificState before the axes.

https://reviewboard.mozilla.org/r/261494/#review268602

Comment 29

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

Comment on attachment 8997617 [details]
Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker implementation.

https://reviewboard.mozilla.org/r/261316/#review268604

Somewhat convoluted, but ok. Personally I would have made the APZC constructor do something like mX.SetVelocityTracker(...), but I don't feel strongly about it.

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8997616 [details]
Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity computation along an axis.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261314/diff/2-3/

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8997758 [details]
Bug 1457586 - Initialize AsyncPanZoomController::mPlatformSpecificState before the axes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261494/diff/1-2/

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8997617 [details]
Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker implementation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261316/diff/2-3/

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8997618 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261318/diff/3-4/

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8997759 [details]
Bug 1457586 - Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261496/diff/2-3/

Comment 35

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave soon) from comment #27)
> Comment on attachment 8997616 [details]
> Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity
> computation along an axis.
> 
> https://reviewboard.mozilla.org/r/261314/#review268596
> 
> ::: gfx/layers/apz/src/SimpleVelocityTracker.cpp:7
> (Diff revision 2)
> > +#include <cstdint>
> > +#include <utility>
> 
> nit: don't need these includes here, since they are already in the .h file

Fixed.

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave soon) from comment #29)
> Comment on attachment 8997617 [details]
> Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker
> implementation.
> 
> https://reviewboard.mozilla.org/r/261316/#review268604
> 
> Somewhat convoluted, but ok. Personally I would have made the APZC
> constructor do something like mX.SetVelocityTracker(...), but I don't feel
> strongly about it.

In defense of this approach, it avoids repeating that setup code for mX and mY.

Comment 36

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

Comment on attachment 8997618 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy.

https://reviewboard.mozilla.org/r/261318/#review268606

Minor comments below, but looks good in general.

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:139
(Diff revision 3)
> + * goodness of fit of the model for the given data.  It is a value between 0
> + * and 1, where 1 indicates perfect correspondence.
> + *
> + * This function first expands the X vector to a m by n matrix A such that
> + * A[i][0] = 1, A[i][1] = X[i], A[i][2] = X[i]^2, ..., A[i][n] = X[i]^n, then
> + * multiplies it by w[i]./

nit: trailing slash

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:164
(Diff revision 3)
> +                              uint32_t n,
> +                              float* out_b,
> +                              float* out_det) {
> +  // MSVC does not support variable-length arrays (used by the original Android
> +  // implementation of this function).
> +#if defined(COMPILER_MSVC)

This file should never be getting compiled with MSVC. I don't mind if you leave this in though

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:274
(Diff revision 3)
> +  for (size_t i = 0; i <= kMaxDegree; i++) {
> +    xcoeff[i] = 0;
> +  }
> +
> +  // Iterate over movement samples in reverse time order and collect samples.
> +  float x[kHistorySize];

"pos" might be a better than than "x" since this code is used for both axes

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:304
(Diff revision 3)
> +
> +  // Calculate a least squares polynomial fit.
> +
> +  // Polynomial degree (number of coefficients), or zero if no information is
> +  // available.
> +  uint32_t degree = 2;

Let's use "kMaxDegree - 1" instead of hard-coding "2" here. I'm assuming that would be semantically correct, given the comment for kMaxDegree says ".. degree of the approximation plus 1"

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:305
(Diff revision 3)
> +  if (degree > m - 1)
> +    degree = m - 1;

nit: braces

::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:311
(Diff revision 3)
> +    degree = m - 1;
> +
> +  if (degree >= 1) {  // otherwise, no velocity data available
> +    float xdet;
> +    uint32_t n = degree + 1;
> +    if (SolveLeastSquares(time, x, w, m, n, xcoeff, &xdet)) {

Since we're not using xdet, maybe we should just rip that out along with the code that computes it? Or allow passing a null for that arg and put the code that computes it behind a null guard (if you want to keep the code). As-is it's doing a bunch of easily-avoidable wasted computation.

Comment 37

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

Comment on attachment 8997759 [details]
Bug 1457586 - Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully.

https://reviewboard.mozilla.org/r/261496/#review268718

Comment 38

[Markus Stange [:mstange]]

(In reply to Botond Ballo [:botond] [on PTO, back Aug 27] from comment #22)
> Markus, could you try the patches (you can use a Try build from [1]) and let
> me know if it fixes the problem as you originally reported it? Thanks!
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=24ec0cece2e7e546ef8928e186b7dca1237450d5

This try build is showing rather erratic behavior unfortunately: If I keep flinging up and down alternatingly, I can get into a state where the fling velocity and direction is completely arbitrary. For example, up/down/up may end up doing up/down/down, or it may just halt the fling entirely, or it may teleport me to the end of the page.

Comment 39

[Botond Ballo [:botond]]

Comment on attachment 8997618 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261318/diff/4-5/

Comment 40

[Botond Ballo [:botond]]

Comment on attachment 8997759 [details]
Bug 1457586 - Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261496/diff/3-4/

Comment 41

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave) from comment #36)
> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:139
> (Diff revision 3)
> > + * goodness of fit of the model for the given data.  It is a value between 0
> > + * and 1, where 1 indicates perfect correspondence.
> > + *
> > + * This function first expands the X vector to a m by n matrix A such that
> > + * A[i][0] = 1, A[i][1] = X[i], A[i][2] = X[i]^2, ..., A[i][n] = X[i]^n, then
> > + * multiplies it by w[i]./
> 
> nit: trailing slash

Fixed.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:164
> (Diff revision 3)
> > +                              uint32_t n,
> > +                              float* out_b,
> > +                              float* out_det) {
> > +  // MSVC does not support variable-length arrays (used by the original Android
> > +  // implementation of this function).
> > +#if defined(COMPILER_MSVC)
> 
> This file should never be getting compiled with MSVC. I don't mind if you
> leave this in though

I left it in, in case we like this physics model enough that we want to use it on desktop as well.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:274
> (Diff revision 3)
> > +  for (size_t i = 0; i <= kMaxDegree; i++) {
> > +    xcoeff[i] = 0;
> > +  }
> > +
> > +  // Iterate over movement samples in reverse time order and collect samples.
> > +  float x[kHistorySize];
> 
> "pos" might be a better than than "x" since this code is used for both axes

Fixed.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:304
> (Diff revision 3)
> > +
> > +  // Calculate a least squares polynomial fit.
> > +
> > +  // Polynomial degree (number of coefficients), or zero if no information is
> > +  // available.
> > +  uint32_t degree = 2;
> 
> Let's use "kMaxDegree - 1" instead of hard-coding "2" here. I'm assuming
> that would be semantically correct, given the comment for kMaxDegree says
> ".. degree of the approximation plus 1"

I cleaned this up a bit, including renaming "kMaxDegree" (the "max" came from the Chromium codebase where the same data structures were used for approximations of different degrees).

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:305
> (Diff revision 3)
> > +  if (degree > m - 1)
> > +    degree = m - 1;
> 
> nit: braces

Fixed.

> ::: gfx/layers/apz/src/AndroidVelocityTracker.cpp:311
> (Diff revision 3)
> > +    degree = m - 1;
> > +
> > +  if (degree >= 1) {  // otherwise, no velocity data available
> > +    float xdet;
> > +    uint32_t n = degree + 1;
> > +    if (SolveLeastSquares(time, x, w, m, n, xcoeff, &xdet)) {
> 
> Since we're not using xdet, maybe we should just rip that out along with the
> code that computes it? Or allow passing a null for that arg and put the code
> that computes it behind a null guard (if you want to keep the code). As-is
> it's doing a bunch of easily-avoidable wasted computation.

I removed the parameter and the code that computes it. The Chromium code used it to compute a "confidence" value, but then didn't actually use that confidence value for anything.

Comment 42

[Markus Stange [:mstange]]

Comment on attachment 8997615 [details]
Bug 1457586 - Simplify Axis::UpdateWithTouchAtDevicePoint() by removing the aAdditionalDelta parameter.

> diff --git a/gfx/layers/apz/src/AsyncPanZoomController.cpp b/gfx/layers/apz/src/AsyncPanZoomController.cpp

> @@ -2448,18 +2448,18 @@ nsEventStatus AsyncPanZoomController::On
>    MOZ_ASSERT(GetCurrentPanGestureBlock());
>    AdjustDeltaForAllowedScrollDirections(logicalPanDisplacement,
>        GetCurrentPanGestureBlock()->GetAllowedScrollDirections());
>  
>    // We need to update the axis velocity in order to get a useful display port
>    // size and position. We need to do so even if this is a momentum pan (i.e.
>    // aFingersOnTouchpad == false); in that case the "with touch" part is not
>    // really appropriate, so we may want to rethink this at some point.
> -  mX.UpdateWithTouchAtDevicePoint(aEvent.mLocalPanStartPoint.x, logicalPanDisplacement.x, aEvent.mTime);
> -  mY.UpdateWithTouchAtDevicePoint(aEvent.mLocalPanStartPoint.y, logicalPanDisplacement.y, aEvent.mTime);
> +  mX.UpdateWithTouchAtDevicePoint(mX.GetPos() + logicalPanDisplacement.x, aEvent.mTime);
> +  mY.UpdateWithTouchAtDevicePoint(mY.GetPos() + logicalPanDisplacement.y, aEvent.mTime);

Please add a comment here saying that, during a pan, we have to make all simulated positions relative to Axis::GetPos(), because the current position is an invented position, and because resetting the position to the mouse position (e.g. aEvent.mLocalPanStartPoint) would mess up velocity calculation.
Maybe we should also mention that other callers of UpdateWithTouchAtDevicePoint can't reset the position because this is the only caller of UpdateWithTouchAtDevicePoint for pan events.

Comment 43

[Chris Peterson [:cpeterson]]

status-firefox63=fix-optional because it would be nice to uplift scrolling improvements to GeckoView 63 because scrolling issues are a common complaint for Fennec users.

Comment 44

[Botond Ballo [:botond]]

Apologies for the delay here; I've been dealing with regressions from bug 656036 work which is riding 63. I will try to get this landed ASAP.

Comment 45

[Botond Ballo [:botond]]

Created attachment 9014180 [details]
Bug 1457586 - Fix unified build bustage from bug 1420996. r?kats

Comment 46

[Botond Ballo [:botond]]

Created attachment 9014181 [details]
Bug 1457586 - Simplify Axis::UpdateWithTouchAtDevicePoint() by removing the aAdditionalDelta parameter. r?mstange

MozReview-Commit-ID: CBBPhfRkl97

Depends on D7653

Comment 47

[Botond Ballo [:botond]]

Created attachment 9014182 [details]
Bug 1457586 - Refactor APZCTreeManager::ProcessTouchVelocity() and related functions to take the time and position deltas separately. r?kats

The motivation is to support velocity tracking implementations (added in a
later patch) that need the position delta rather than resulting velocity.

Also rename the functions to make it clearer that they have to do with
dynamic toolbar movement.

MozReview-Commit-ID: G0IVJHYTurB

Depends on D7654

Comment 48

[Botond Ballo [:botond]]

Created attachment 9014183 [details]
Bug 1457586 - Introduce a VelocityTracker abstraction for handling velocity computation along an axis. r?kats

The current velocity computation code is factored out into an implementation
called SimpleVelocityTracker.

MozReview-Commit-ID: G0VnvREdIX3

Depends on D7655

Comment 49

[Botond Ballo [:botond]]

Created attachment 9014184 [details]
Bug 1457586 - Initialize AsyncPanZoomController::mPlatformSpecificState before the axes. r?kats

This allows the Axis constructors to use the platform-specific state.

MozReview-Commit-ID: KWtDX4XVpjF

Depends on D7656

Comment 50

[Botond Ballo [:botond]]

Created attachment 9014186 [details]
Bug 1457586 - Allow PlatformSpecificState to choose the VelocityTracker implementation. r?kats

MozReview-Commit-ID: LyW9N2H7fv7

Depends on D7657

Comment 51

[Botond Ballo [:botond]]

Created attachment 9014187 [details]
Bug 1457586 - Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy. r?kats

MozReview-Commit-ID: 6kteQv1KHDN

Depends on D7658

Comment 52

[Botond Ballo [:botond]]

Created attachment 9014189 [details]
Bug 1457586 - Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully. r?kats

MozReview-Commit-ID: JYqiViaucmY

Depends on D7659

Comment 53

[Botond Ballo [:botond]]

I addressed comment 42, and reuploaded the patch series to Phabricator. The "unified build bustage" patch is new, the others are just "carry the r+ from Mozreview".

I haven't had any luck reproducing the issue described in comment 38 with any reliability so far.

Comment 54

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6b9404970a457ae5094ef41c4a293ce2c9ee65

Comment 55

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #53)
> I haven't had any luck reproducing the issue described in comment 38 with
> any reliability so far.

Discussed this with Markus; with the Try build from comment 54, he can also only reproduce the issue very intermittently. The behaviour in question may be an existing issue with the dynamic toolbar which has improved due to recent dynamic toolbar related fixes.

As this was the last outstanding issue, and we have a green Try push, I believe this should be good to land.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65403cf5da8a
Fix unified build bustage from bug 1420996. r=kats
https://hg.mozilla.org/integration/autoland/rev/664a7c0f8183
Simplify Axis::UpdateWithTouchAtDevicePoint() by removing the aAdditionalDelta parameter. r=mstange
https://hg.mozilla.org/integration/autoland/rev/cd68d246a34d
Refactor APZCTreeManager::ProcessTouchVelocity() and related functions to take the time and position deltas separately. r=kats
https://hg.mozilla.org/integration/autoland/rev/5d40107ce896
Introduce a VelocityTracker abstraction for handling velocity computation along an axis. r=kats
https://hg.mozilla.org/integration/autoland/rev/2ab8498738f4
Initialize AsyncPanZoomController::mPlatformSpecificState before the axes. r=kats
https://hg.mozilla.org/integration/autoland/rev/1507f6511c87
Allow PlatformSpecificState to choose the VelocityTracker implementation. r=kats
https://hg.mozilla.org/integration/autoland/rev/f266ca6d096a
Add an AndroidVelocityTracker class that implement Chrome's default velocity tracking strategy. r=kats
https://hg.mozilla.org/integration/autoland/rev/77ace67c85de
Implement AndroidVelocityTrakcer::HandleDynamicToolbarMovement() usefully. r=kats

Comment 57

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/65403cf5da8a
https://hg.mozilla.org/mozilla-central/rev/664a7c0f8183
https://hg.mozilla.org/mozilla-central/rev/cd68d246a34d
https://hg.mozilla.org/mozilla-central/rev/5d40107ce896
https://hg.mozilla.org/mozilla-central/rev/2ab8498738f4
https://hg.mozilla.org/mozilla-central/rev/1507f6511c87
https://hg.mozilla.org/mozilla-central/rev/f266ca6d096a
https://hg.mozilla.org/mozilla-central/rev/77ace67c85de

Comment 58

[Chris Peterson [:cpeterson]]

Thanks, Botond!

63=wontfix because this is probably too much code to uplift to 63 Beta.

Comment 60

[Chris Lord [:cwiiis]]

I haven't filed a bug yet, but I've noticed a distinct increase in 'weird' flings since this landed - flings where it'll start scrolling in the opposite direction of my swipe, or at an odd speed. Note, however, that the feel of things is otherwise much improved and feels far more like the rest of Android now.