Make danger zone percentage scale based on recent velocity

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jpr, Assigned: kats)

Tracking

Trunk
Firefox 14
x86
Linux
Points:
---

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 +)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Make danger zone percentage scale based on recent velocity.  ie the faster the velocity of the recent scroll, the great the danger zone percentage.  Could decay over time for "recent".

Comment 1

6 years ago
(In reply to JP Rosevear [:jpr] from comment #0)
> Make danger zone percentage scale based on recent velocity.  ie the faster
> the velocity of the recent scroll, the great the danger zone percentage. 
> Could decay over time for "recent".

This sounds like a good strategy. We should probably also make the danger-zone larger than it currently is by default too. For reference, xul-fennec redraws every 20 pixels of change (or somewhere thereabouts) - this is probably too extreme, but I often see a small sliver of checkerboarding when scrolling fast, which would suggest that our danger-zone isn't quite large enough.
Created attachment 614856 [details] [diff] [review]
Add danger zones to velocity bias, and prefs to control it

The default parameters are chosen such that by the behaviour doesn't change from the way it was before. (This is true when max(DANGER_X_BASE, DANGER_Y_BASE) + 1000 > MULTIPLIER).
Attachment #614856 - Flags: review?(chrislord.net)
blocking-fennec1.0: --- → ?

Comment 3

6 years ago
Comment on attachment 614856 [details] [diff] [review]
Add danger zones to velocity bias, and prefs to control it

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

Looks good, some nits below.

::: mobile/android/app/mobile.js
@@ +377,5 @@
>  pref("gfx.displayport.strategy_vb.reverse_buffer", -1); // fraction of buffer to keep in reverse direction from scroll
> +pref("gfx.displayport.strategy_vb.danger_x_base", -1); // base contribution for danger zone when multiplied by viewport width
> +pref("gfx.displayport.strategy_vb.danger_y_base", -1); // base contribution for danger zone when multiplied by viewport height
> +pref("gfx.displayport.strategy_vb.danger_x_incr", -1); // velocity contribution for danger zone when multiplied by viewport width and velocity
> +pref("gfx.displayport.strategy_vb.danger_y_incr", -1); // velocity contribution for danger zone when multiplied by viewport height and velocity

The comments for these variables don't feel descriptive enough to me. The idea of a 'base contribution' only makes sense when you've read the code. Either they should be shorter comments like the ones above, or they should be reworded to make it a bit more obvious what these do.

Something along the lines of:
// Danger-zone size on x-axis, as a fraction of viewport width
// Additional danger-zone size on x-axis when above velocity threshold

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +310,5 @@
> +        /**
> +         * Split the given amounts into margins based on the VELOCITY_THRESHOLD and REVERSE_BUFFER values.
> +         * If the velocity is above the VELOCITY_THRESHOLD on an axis, split the amount into REVERSE_BUFFER
> +         * and 1.0 - REVERSE_BUFFER in the opposite and same direction as velocity, respectively. If velocity
> +         * is lower, split the amount evenly.

I feel this could be rephrased to parse more easily.

@@ +321,5 @@
> +                margins.left = xAmount * (1.0f - REVERSE_BUFFER);
> +            } else {
> +                margins.left = xAmount / 2.0f;
> +            }
> +            margins.right = xAmount - margins.left;

A line-break after this line would be nice for readability.

@@ +329,5 @@
> +                margins.top = yAmount * (1.0f - REVERSE_BUFFER);
> +            } else {
> +                margins.top = yAmount / 2.0f;
> +            }
> +            margins.bottom = yAmount - margins.top;

Same here.
Attachment #614856 - Flags: review?(chrislord.net) → review+
Landed with nits addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/46c7419d4bb8
status-firefox14: --- → fixed
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/46c7419d4bb8
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
blocking-fennec1.0: ? → +
You need to log in before you can comment on or make changes to this bug.