Closed Bug 744390 Opened 8 years ago Closed 8 years ago

Make danger zone percentage scale based on recent velocity

Categories

(Firefox for Android :: General, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
firefox14 --- fixed
blocking-fennec1.0 --- +

People

(Reporter: jpr, Assigned: kats)

Details

Attachments

(1 file)

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".
(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.
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 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+
https://hg.mozilla.org/mozilla-central/rev/46c7419d4bb8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
blocking-fennec1.0: ? → +
You need to log in before you can comment on or make changes to this bug.