744390 - Make danger zone percentage scale based on recent velocity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[JP Rosevear [:jpr]]

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

[Chris Lord [:cwiiis]]

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

Comment 2

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

Attachment: 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).

Comment 3

[Chris Lord [:cwiiis]]

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.

Comment 4

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

Landed with nits addressed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/46c7419d4bb8

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/46c7419d4bb8