Closed Bug 1257959 Opened 4 years ago Closed 4 years ago

Dynamic toolbar transition seems to slow down flings

Categories

(Core :: Panning and Zooming, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mstange, Assigned: rbarker)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 1 obsolete file)

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.
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?
Whiteboard: [gfx-noted]
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.
Assignee: nobody → 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.
Attachment #8738297 - Flags: review?(bugmail.mozilla)
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.
Attachment #8738297 - Flags: review?(bugmail.mozilla) → review+
Attachment #8738297 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/cce22ba996a6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.