Closed Bug 1100904 Opened 5 years ago Closed 5 years ago

Implement UI transitions tracker

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(5 files)

A simple API to allow us to query the UI about on-going transitions and avoid running layout passes that will probably cause UI jank during animations.

The main use case for it now is about:home. We want to avoid updating the panels with new content during UI transitions.
Comment on attachment 8524555 [details] [diff] [review]
Add TransitionsTracker API (r=mcomella)

Just a super simple mechanism for avoid running code that will trigger layout changes during transitions. It's up to the caller to properly manage its life-cycle when actions are called after destruction or something.
Attachment #8524555 - Flags: review?(michael.l.comella)
Comment on attachment 8524556 [details] [diff] [review]
Track transitions in tab strip (r=mcomella)

So that tab strip animations are consistently smooth. This will only make a different with the about:home changes in the next patches.
Attachment #8524556 - Flags: review?(michael.l.comella)
Comment on attachment 8524557 [details] [diff] [review]
Ensure panel updates don't happen during transitions (r=margaret)

Here's the real meat. This changes the cursor loader callbacks on all about:home panels to account for on-going transitions. They will only update the UI with new content if/once there are no on-going transitions in the UI e.g. tab strip animations, about:home strip's bouncy animation, toolbar entry animation, etc.
Attachment #8524557 - Flags: review?(margaret.leibovic)
Comment on attachment 8524558 [details] [diff] [review]
Don't block panel loading in HomeLoader.load() (r=margaret)

And now that we don't update the UI during transitions anymore, we can just trigger the cursor loaders immediately, no need to use the 'canLoadHint' when about:home is about to be displayed because the transition-aware callbacks take care of that for us now.
Attachment #8524558 - Flags: review?(margaret.leibovic)
Comment on attachment 8524564 [details] [diff] [review]
Track about:home strip bouncy animation (r=liuche)

So that about:home doesn't trigger layout changes during the bouncy animation on startup.
Attachment #8524564 - Flags: review?(liuche)
Probably worth mentioning: the changes in the last patch make about:home content show up a bit quicker because we start the cursor query earlier.
Comment on attachment 8524555 [details] [diff] [review]
Add TransitionsTracker API (r=mcomella)

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

Looks functionally correct, just some nits.

::: mobile/android/base/TransitionsTracker.java
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;

nit: Should this be in the animation package?

@@ +11,5 @@
> +
> +import org.mozilla.gecko.animation.PropertyAnimator;
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +public class TransitionsTracker {

Add a class comment

@@ +13,5 @@
> +import org.mozilla.gecko.util.ThreadUtils;
> +
> +public class TransitionsTracker {
> +    private static List<Runnable> sPendingActions;
> +    private static int sTransitionCount;

nit: I thought we decided not to use prefixes on variable names.

@@ +15,5 @@
> +public class TransitionsTracker {
> +    private static List<Runnable> sPendingActions;
> +    private static int sTransitionCount;
> +
> +    private static void runPendingActions() {

Add a note (or another assertion) that this should only be run on the UI thread

@@ +22,5 @@
> +        }
> +
> +        final int size = sPendingActions.size();
> +        for (int i = 0; i < size; i++) {
> +            sPendingActions.get(i).run();

We don't know what kind of List this is so for efficiency, it'd be better to use an iterator.

But the memory allocation/free sucks so it'd be even better to change the declaration to ArrayList (as per the constructor).

@@ +26,5 @@
> +            sPendingActions.get(i).run();
> +        }
> +
> +        if (size > 0) {
> +            sPendingActions.clear();

Why is this size check here? It looks like the same size check is done in ArrayList.clear() and can probably be removed.

@@ +56,5 @@
> +
> +    public static void track(PropertyAnimator animator) {
> +        ThreadUtils.assertOnUiThread();
> +
> +        animator.addPropertyAnimationListener(new PropertyAnimator.PropertyAnimationListener() {

nit: Use a static listener here to avoid creating and destroying numerous Listeners.

@@ +72,5 @@
> +
> +    public static void track(Animator animator) {
> +        ThreadUtils.assertOnUiThread();
> +
> +        animator.addListener(new Animator.AnimatorListener() {

nit: Same - static.

@@ +99,5 @@
> +        if (sPendingActions == null) {
> +            return;
> +        }
> +
> +        sPendingActions.remove(action);

nit: For thoroughness, `remove` returns a boolean so we can return that value as well.

@@ +106,5 @@
> +    public static void runAfterTransition(Runnable action) {
> +        ThreadUtils.assertOnUiThread();
> +
> +        if (sPendingActions == null) {
> +            sPendingActions = new ArrayList<>();

Why not avoid the null checks and just construct this in the declaration?

@@ +111,5 @@
> +        }
> +
> +        if (sTransitionCount == 0) {
> +            action.run();
> +        } else if (!sPendingActions.contains(action)) {

Why do we need this check? It seems possible that this check will negatively affect the calling code, (e.g. the action is not idempotent and needs to occur twice). On the other hand, running an action twice that is called twice is typically inefficient, but harmless.
Attachment #8524555 - Flags: review?(michael.l.comella) → review+
Attachment #8524556 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8524557 [details] [diff] [review]
Ensure panel updates don't happen during transitions (r=margaret)

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

::: mobile/android/base/home/TopSitesPanel.java
@@ +703,5 @@
>           * sometimes (e.g., returning to the activity) you'll *not* be called
>           * twice, and thus you'll never draw thumbnails.
>           *
>           * The root cause is TopSitesLoader.loadCursor being called twice.
>           * Why that is... dunno.

Is the comment up here still correct?

::: mobile/android/base/home/TransitionAwareCursorLoaderCallbacks.java
@@ +9,5 @@
> +import android.support.v4.content.Loader;
> +
> +import org.mozilla.gecko.TransitionsTracker;
> +
> +public abstract class TransitionAwareCursorLoaderCallbacks implements LoaderCallbacks<Cursor> {

This class is a bit difficult to follow. Could you add some comments with a high-level summary of what this aims to do?

@@ +32,5 @@
> +            cursorSwapRunnable = null;
> +        }
> +    }
> +
> +    private class CursorSwapRunnable implements Runnable {

It's a bit confusing that this is called CursorSwapRunnable, but it doesn't actually swap the cursors (but maybe that's okay because we always expect the implementation of onLoadFinishedAfterTransitions to do a cursor swap?).
Attachment #8524557 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8524558 [details] [diff] [review]
Don't block panel loading in HomeLoader.load() (r=margaret)

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

This looks okay, but I'm not 100% confident about the changes. r+ with questions answered :)

::: mobile/android/base/home/HomePager.java
@@ -242,5 @@
>  
>                  @Override
>                  public void onPropertyAnimationEnd() {
>                      setLayerType(View.LAYER_TYPE_NONE, null);
> -                    adapter.setCanLoadHint(true);

Can you explain a bit about why it's okay to remove this bit of logic? Does this transition always happen before the transition we're tracking is finished? Or should we track this transition as well?
Attachment #8524558 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8524564 [details] [diff] [review]
Track about:home strip bouncy animation (r=liuche)

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

::: mobile/android/base/home/HomePagerTabStrip.java
@@ +120,5 @@
>          nextBounceAnimator.queue(new Attributes(bounceDistance/4, BOUNCE2_MS));
>          nextBounceAnimator.queue(new Attributes(0, BOUNCE4_MS));
>          nextBounceAnimator.setStartDelay(ANIMATION_DELAY_MS);
>  
> +        TransitionsTracker.track(alphaAnimatorSet);

I would track nextBounceAnimator instead of the alphaAnimator because nextBounceAnimator is the last animation in this series to happen.
Attachment #8524564 - Flags: review?(liuche) → review+
(In reply to Michael Comella (:mcomella) from comment #12)
> Comment on attachment 8524555 [details] [diff] [review]
> Add TransitionsTracker API (r=mcomella)
> 
> Review of attachment 8524555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks functionally correct, just some nits.
> 
> ::: mobile/android/base/TransitionsTracker.java
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko;
> 
> nit: Should this be in the animation package?

Good point, done.

> @@ +11,5 @@
> > +
> > +import org.mozilla.gecko.animation.PropertyAnimator;
> > +import org.mozilla.gecko.util.ThreadUtils;
> > +
> > +public class TransitionsTracker {
> 
> Add a class comment

Done.

> @@ +13,5 @@
> > +import org.mozilla.gecko.util.ThreadUtils;
> > +
> > +public class TransitionsTracker {
> > +    private static List<Runnable> sPendingActions;
> > +    private static int sTransitionCount;
> 
> nit: I thought we decided not to use prefixes on variable names.

Oops, removed prefixes.

> @@ +15,5 @@
> > +public class TransitionsTracker {
> > +    private static List<Runnable> sPendingActions;
> > +    private static int sTransitionCount;
> > +
> > +    private static void runPendingActions() {
> 
> Add a note (or another assertion) that this should only be run on the UI
> thread

Added assertion just to be safe.

> @@ +22,5 @@
> > +        }
> > +
> > +        final int size = sPendingActions.size();
> > +        for (int i = 0; i < size; i++) {
> > +            sPendingActions.get(i).run();
> 
> We don't know what kind of List this is so for efficiency, it'd be better to
> use an iterator.
> 
> But the memory allocation/free sucks so it'd be even better to change the
> declaration to ArrayList (as per the constructor).

Done.

> @@ +26,5 @@
> > +            sPendingActions.get(i).run();
> > +        }
> > +
> > +        if (size > 0) {
> > +            sPendingActions.clear();
> 
> Why is this size check here? It looks like the same size check is done in
> ArrayList.clear() and can probably be removed.

Leftover, removed.

> @@ +56,5 @@
> > +
> > +    public static void track(PropertyAnimator animator) {
> > +        ThreadUtils.assertOnUiThread();
> > +
> > +        animator.addPropertyAnimationListener(new PropertyAnimator.PropertyAnimationListener() {
> 
> nit: Use a static listener here to avoid creating and destroying numerous
> Listeners.

Nice catch, done.

> @@ +72,5 @@
> > +
> > +    public static void track(Animator animator) {
> > +        ThreadUtils.assertOnUiThread();
> > +
> > +        animator.addListener(new Animator.AnimatorListener() {
> 
> nit: Same - static.

Done.

> @@ +99,5 @@
> > +        if (sPendingActions == null) {
> > +            return;
> > +        }
> > +
> > +        sPendingActions.remove(action);
> 
> nit: For thoroughness, `remove` returns a boolean so we can return that
> value as well.

Fair enough, done.

> @@ +106,5 @@
> > +    public static void runAfterTransition(Runnable action) {
> > +        ThreadUtils.assertOnUiThread();
> > +
> > +        if (sPendingActions == null) {
> > +            sPendingActions = new ArrayList<>();
> 
> Why not avoid the null checks and just construct this in the declaration?

Done.

> @@ +111,5 @@
> > +        }
> > +
> > +        if (sTransitionCount == 0) {
> > +            action.run();
> > +        } else if (!sPendingActions.contains(action)) {
> 
> Why do we need this check? It seems possible that this check will negatively
> affect the calling code, (e.g. the action is not idempotent and needs to
> occur twice). On the other hand, running an action twice that is called
> twice is typically inefficient, but harmless.

Hmm, yeah. I'll remove this check to avoid surprises on the caller side.
(In reply to :Margaret Leibovic from comment #13)
> Comment on attachment 8524557 [details] [diff] [review]
> Ensure panel updates don't happen during transitions (r=margaret)
> 
> Review of attachment 8524557 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/TopSitesPanel.java
> @@ +703,5 @@
> >           * sometimes (e.g., returning to the activity) you'll *not* be called
> >           * twice, and thus you'll never draw thumbnails.
> >           *
> >           * The root cause is TopSitesLoader.loadCursor being called twice.
> >           * Why that is... dunno.
> 
> Is the comment up here still correct?

No idea, lacks context ;-)

> ::: mobile/android/base/home/TransitionAwareCursorLoaderCallbacks.java
> @@ +9,5 @@
> > +import android.support.v4.content.Loader;
> > +
> > +import org.mozilla.gecko.TransitionsTracker;
> > +
> > +public abstract class TransitionAwareCursorLoaderCallbacks implements LoaderCallbacks<Cursor> {
> 
> This class is a bit difficult to follow. Could you add some comments with a
> high-level summary of what this aims to do?

Sure, done.

> @@ +32,5 @@
> > +            cursorSwapRunnable = null;
> > +        }
> > +    }
> > +
> > +    private class CursorSwapRunnable implements Runnable {
> 
> It's a bit confusing that this is called CursorSwapRunnable, but it doesn't
> actually swap the cursors (but maybe that's okay because we always expect
> the implementation of onLoadFinishedAfterTransitions to do a cursor swap?).

True. Renamed it to onLoadFinishedRunnable for clarity.
(In reply to :Margaret Leibovic from comment #14)
> Comment on attachment 8524558 [details] [diff] [review]
> Don't block panel loading in HomeLoader.load() (r=margaret)
> 
> Review of attachment 8524558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks okay, but I'm not 100% confident about the changes. r+ with
> questions answered :)
> 
> ::: mobile/android/base/home/HomePager.java
> @@ -242,5 @@
> >  
> >                  @Override
> >                  public void onPropertyAnimationEnd() {
> >                      setLayerType(View.LAYER_TYPE_NONE, null);
> > -                    adapter.setCanLoadHint(true);
> 
> Can you explain a bit about why it's okay to remove this bit of logic? Does
> this transition always happen before the transition we're tracking is
> finished? Or should we track this transition as well?

The about:home transition is always bound to an existing PropertyAnimator that is passed as argument in the load(...) method. This PropertyAnimator is always tracked by the TransitionTracker (see changes to BrowserApp.

The change here is basically that instead of only loading the cursor after the about:home transition (i.e. when you tap the URL bar), we start loading the cursor immediately and only post-pone the code that swaps the cursor in the adapter.
(In reply to Chenxia Liu [:liuche] from comment #15)
> Comment on attachment 8524564 [details] [diff] [review]
> Track about:home strip bouncy animation (r=liuche)
> 
> Review of attachment 8524564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePagerTabStrip.java
> @@ +120,5 @@
> >          nextBounceAnimator.queue(new Attributes(bounceDistance/4, BOUNCE2_MS));
> >          nextBounceAnimator.queue(new Attributes(0, BOUNCE4_MS));
> >          nextBounceAnimator.setStartDelay(ANIMATION_DELAY_MS);
> >  
> > +        TransitionsTracker.track(alphaAnimatorSet);
> 
> I would track nextBounceAnimator instead of the alphaAnimator because
> nextBounceAnimator is the last animation in this series to happen.

Good point, done.
You need to log in before you can comment on or make changes to this bug.