856039 - Don't let bounce animations clobber viewport size updates

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

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

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.

Comment 1

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

Attachment: Part 1 - cleanup

Comment 2

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

Attachment: Part 2 - Don't let other code clobber GeckoLayerClient's viewport size

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.

Comment 3

[Chris Lord [:cwiiis]]

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

Comment 4

[Chris Lord [:cwiiis]]

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:[e-mail]) 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/

Comment 5

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

Attachment: Part 1 - cleanup (v2)

Updated animation duration to 250ms, carrying r+

Comment 6

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

Attachment: Part 2 - Don't let other code clobber GeckoLayerClient's viewport size (v2)

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.

Comment 7

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

Attachment: Part 3 - Don't let other code clobber the margins

Try run with all three patches in progress at https://tbpl.mozilla.org/?tree=Try&rev=895b567e8fe5

Comment 8

[Chris Lord [:cwiiis]]

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.

Comment 9

[Chris Lord [:cwiiis]]

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/

Comment 10

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

Fixed and landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/42912b8b82d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/be42980135f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/dccd23655513

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42912b8b82d4
https://hg.mozilla.org/mozilla-central/rev/be42980135f2
https://hg.mozilla.org/mozilla-central/rev/dccd23655513