Closed
Bug 1100904
Opened 10 years ago
Closed 10 years ago
Implement UI transitions tracker
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(5 files)
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
liuche
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8524556 -
Flags: review?(michael.l.comella) → review+
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
(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.
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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.
Assignee | ||
Comment 19•10 years ago
|
||
(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.
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/33c8749d384b https://hg.mozilla.org/integration/fx-team/rev/37c61cbcb970 https://hg.mozilla.org/integration/fx-team/rev/daf6d7046496 https://hg.mozilla.org/integration/fx-team/rev/4c73bc9c874e https://hg.mozilla.org/integration/fx-team/rev/cbda40262967
Comment 21•10 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1274148&repo=fx-team
Assignee | ||
Comment 22•10 years ago
|
||
Oops, forgot to update build files correctly. Done. https://hg.mozilla.org/integration/fx-team/rev/0294c87bc9d6 https://hg.mozilla.org/integration/fx-team/rev/961b9650ce75 https://hg.mozilla.org/integration/fx-team/rev/963f53325f24 https://hg.mozilla.org/integration/fx-team/rev/d8116e25b771 https://hg.mozilla.org/integration/fx-team/rev/83b257f7766b
https://hg.mozilla.org/mozilla-central/rev/0294c87bc9d6 https://hg.mozilla.org/mozilla-central/rev/961b9650ce75 https://hg.mozilla.org/mozilla-central/rev/963f53325f24 https://hg.mozilla.org/mozilla-central/rev/d8116e25b771 https://hg.mozilla.org/mozilla-central/rev/83b257f7766b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•