1257959 - Dynamic toolbar transition seems to slow down flings

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Markus Stange [:mstange]]

STR [Nightly with APZ on Android enabled]:
 1. Be on a page that's long enough for two flings down.
 2. In the initial state, with the toolbar visible, fling down, but do it very quickly, keeping your finger on the screen for only a very short time.
 3. Now that the toolbar is hidden, fling down again, in the same way.

The second fling feels much better than the first. It seems like the disappearance of the toolbar reduces the fling start velocity.

The same thing happens if you fling up while the toolbar is hidden.

Comment 1

[Jamie Nicol [:jnicol]]

I definitely see this too, and didn't with java APZ. My first fling is very juttery and doesn't go very far. But I see it even with browser.chrome.dynamictoolbar=false. Does disabling dynamic toolbar actually fix the issue for you?

Comment 2

[Markus Stange [:mstange]]

Setting browser.chrome.dynamictoolbar=false to false fixes it for me, yes. I don't see much jutter in either configuration. But it takes a long time for the lowres tiles to become highres.

Comment 3

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1257959-Dynamic-toolbar-transition-seems-to-slow-down-flings-16040515-775f03a.patch

Comment 4

[Randall Barker [:rbarker]]

Comment on attachment 8738297 [details] [diff] [review]
0001-Bug-1257959-Dynamic-toolbar-transition-seems-to-slow-down-flings-16040515-775f03a.patch

Git made a bit of a mess of Axis::UpdateWithTouchAtDevicePoint. I basically pulled the velocity curve code out into a separate function.

Comment 5

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

Comment on attachment 8738297 [details] [diff] [review]
0001-Bug-1257959-Dynamic-toolbar-transition-seems-to-slow-down-flings-16040515-775f03a.patch

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

Neat, this seems like a pretty good solution to this problem that I hadn't considered at all! Mostly nits below, overall it looks good.

::: gfx/layers/apz/src/APZCTreeManager.h
@@ +446,5 @@
> +  /**
> +   * Process touch velocity.
> +   * Some times the touch move event will have a velocity even though no scrolling
> +   * is occurring such as when the Toolbar is being hidden/shown in Fennec.
> +   * This allows the Y Axis to have its velocity queue updated.

s/Some times/Sometimes/
s/Toolbar/toolbar/
Also reword the last sentence to something like "This function can be called to have the y axis' velocity queue updated."

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +266,5 @@
>    /**
> +   * Handler for touch velocity.
> +   * Some times the touch move event will have a velocity even though no scrolling
> +   * is occurring such as when the Toolbar is being hidden/shown in Fennec.
> +   * This allows the Y Axis to have its velocity queue updated.

Ditto the changes in APZCTreeManager.h

::: gfx/layers/apz/src/Axis.cpp
@@ +70,5 @@
>        mAsyncPanZoomController->ToParentLayerCoordinates(velocity, panStart);
>    return localVelocity.Length();
>  }
>  
> +float Axis::ApplyVelocityCurve(float aVelocity) const {

The diff would probably be cleaner if you rearranged these method bodies to be in this order:

UpdateWithTouchAtDevicePoint
ApplyVelocityCurve
AddVelocityToQueue

Doesn't matter too much though. Personally I prefer if the order of implementations matches the order of declarations in the header but that ship sailed a long time ago.

::: gfx/layers/apz/src/Axis.h
@@ +300,5 @@
>  
>    // Convert a velocity from global inches/ms into ParentLayerCoords/ms.
>    float ToLocalVelocity(float aVelocityInchesPerMs) const;
> +
> +  float ApplyVelocityCurve(float aVelocity) const;

Rename this ApplyFlingCurveToVelocity. This feature is referred to as "fling curving" throughout the code and documentation so I think for consistency we should use that in the name.

::: mobile/android/base/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java
@@ +378,5 @@
>          return 0;
>      }
>  
> +    long mLastEventTime;
> +    float mVelocity;

Move these to be with the other class-level fields, add some comments on them, make them private.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cce22ba996a6

Comment 7

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1257959-Dynamic-toolbar-transition-seems-to-slow-down-flings-r-kats-16040614-7bd31e0.patch

Address review comments. Carry forward r+ from :kats

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cce22ba996a6