Closed Bug 856039 Opened 7 years ago Closed 7 years ago

Don't let bounce animations clobber viewport size updates

Categories

(Firefox for Android :: Toolbar, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files, 2 obsolete files)

See my explanation at https://bugzilla.mozilla.org/show_bug.cgi?id=814282#c100. Basically, the bounce animation that we have in JavaPanZoomController can clobber a viewport size update that comes in between, because it uses the interpolation of mBounceStartMetrics and mBounceEndMetrics which always have the viewport size as it was at the start of the animation.
Attached patch Part 1 - cleanup (obsolete) — Splinter Review
Attachment #731347 - Flags: review?(chrislord.net)
Try run is clean at https://tbpl.mozilla.org/?tree=Try&rev=33a9cf855ae9

I also realized I can do something very similar to get rid of the "keepFixedMargins" flag - i.e. keep it by default, and in the one place that calls setViewportMetrics with keepFixedMargins=false, we can set the margins beforehand, or just modify mViewportMetrics directly. Let me know if you want me to do that.
Attachment #731348 - Flags: review?(chrislord.net)
Comment on attachment 731347 [details] [diff] [review]
Part 1 - cleanup

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

LGTM.

::: mobile/android/base/gfx/JavaPanZoomController.java
@@ +66,5 @@
>      // The maximum amount we would like to scroll with the mouse
>      private static final float MAX_SCROLL = 0.075f * GeckoAppShell.getDpi();
>  
> +    // Length of the bounce animation in ms
> +    private static final int BOUNCE_ANIMATION_DURATION = 256;

256ms is a really weird timing for an animation... I wonder why we picked this? If you wanted to sneak in changing this to 250, for consistency (all our other animation timings are multiples of 50ms, I think), I wouldn't complain :)
Attachment #731347 - Flags: review?(chrislord.net) → review+
Comment on attachment 731348 [details] [diff] [review]
Part 2 - Don't let other code clobber GeckoLayerClient's viewport size

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

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> I also realized I can do something very similar to get rid of the
> "keepFixedMargins" flag - i.e. keep it by default, and in the one place that
> calls setViewportMetrics with keepFixedMargins=false, we can set the margins
> beforehand, or just modify mViewportMetrics directly. Let me know if you
> want me to do that.

Yes, please do, that'd be a lot cleaner than what it is currently.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +801,5 @@
>      @Override
>      public void setViewportMetrics(ImmutableViewportMetrics metrics) {
> +        // This class owns the viewport size; don't let other pieces of code clobber our notion
> +        // of the viewport size. The only place the viewport size should ever be updated is in
> +        // the GeckoLayerClient.setViewportSize function, and there mViewportMetrics is updated

nit: comma after 'there', or s/and there/where/
Attachment #731348 - Flags: review?(chrislord.net) → review+
Updated animation duration to 250ms, carrying r+
Attachment #731347 - Attachment is obsolete: true
Attachment #731720 - Flags: review+
I moved the code into the other setViewportMetrics function because that fits better with the next patch. No functional difference but re-requesting review anyway just to be safe.
Attachment #731348 - Attachment is obsolete: true
Attachment #731721 - Flags: review?(chrislord.net)
Comment on attachment 731721 [details] [diff] [review]
Part 2 - Don't let other code clobber GeckoLayerClient's viewport size (v2)

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

LGTM.
Attachment #731721 - Flags: review?(chrislord.net) → review+
Comment on attachment 731723 [details] [diff] [review]
Part 3 - Don't let other code clobber the margins

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

LGTM, nice work.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +809,5 @@
> +     * You must hold the monitor while calling this.
> +     */
> +    private void setViewportMetrics(ImmutableViewportMetrics metrics, boolean notifyGecko) {
> +        // This class owns the viewport size and the fixed layer margins; don't let other pieces
> +        // of code clobber either of them. The only place the viewport size should never be

s/never/ever/
Attachment #731723 - Flags: review?(chrislord.net) → review+
You need to log in before you can comment on or make changes to this bug.