Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret
authorMichael Comella <michael.l.comella@gmail.com>
Mon, 28 Mar 2016 18:50:52 -0700
changeset 295514 5459831e5a2746943a29a4ea74a5593d93d22526
parent 295513 94af575a59a5e5ef4ae2ce072c874e4727884325
child 295515 3b7b5c87cf2b319aa751a5903f42ee2e5883749a
push id19007
push usermichael.l.comella@gmail.com
push dateFri, 29 Apr 2016 23:45:52 +0000
treeherderfx-team@5459831e5a27 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmargaret
bugs1245493, 1269041, 1245523
milestone49.0a1
Bug 1245493 - Don't animate when showing toolbar when FF is first unhidden. r=margaret After this patch, I still occasionally see the toolbar positioned part way down from the top of the screen. However, this state looks slightly less janky without the animation I removed and I can't consistently reproduce it anymore. Given the DynamicToolbar.setVisible calls I make, I'd guess this is likely to be a bug caused by BrowserApp.onTabChanged (and thus DynamicToolbar.setVisible) not getting called instantly and so the DynamicToolbar is initialized to a different location on screen. I'd guess it's a bug in DynamicToolbar as to why it's positioned partially off-screen. There is a little bit of code duplication, but that is because the code to load a url on a new intent is duplicated (i.e. once from GeckoApp.initialize - the initial load - and once from GeckoApp.onNewIntent). This could potentially be cleaned up if we moved the app loading code into onResume, but that may not be possible since we need to wait for Gecko to start up. Additionally, this patch adds a lot of hard-to-follow global state, which is also not good. Preferred solution (bug 1269041): show the toolbar each time onStart is called (i.e. FF is unhidden). This is good for the user - they'll be more aware which page they're on - but it's janky with the current implementation, where the page content does not scroll when the toolbar is shown so previously visible content is hidden. Thus, I went with the other approach. fwiw, Chrome does this behavior, but scrolls the toolbar offscreen shortly after it is shown. This solution is blocked on bug 1245523. MozReview-Commit-ID: 7JjCrIf4KTm
mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
mobile/android/base/java/org/mozilla/gecko/Tab.java
mobile/android/base/java/org/mozilla/gecko/Tabs.java
--- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -336,17 +336,22 @@ public class BrowserApp extends GeckoApp
             }
             return;
         }
 
         Log.d(LOGTAG, "BrowserApp.onTabChanged: " + tab.getId() + ": " + msg);
         switch (msg) {
             case SELECTED:
                 if (Tabs.getInstance().isSelectedTab(tab) && mDynamicToolbar.isEnabled()) {
-                    mDynamicToolbar.setVisible(true, VisibilityTransition.ANIMATE);
+                    final VisibilityTransition transition = (tab.getShouldShowToolbarWithoutAnimationOnFirstSelection()) ?
+                            VisibilityTransition.IMMEDIATE : VisibilityTransition.ANIMATE;
+                    mDynamicToolbar.setVisible(true, transition);
+
+                    // The first selection has happened - reset the state.
+                    tab.setShouldShowToolbarWithoutAnimationOnFirstSelection(false);
                 }
                 // fall through
             case LOCATION_CHANGE:
                 if (mZoomedView != null) {
                     mZoomedView.stopZoomDisplay(false);
                 }
                 if (Tabs.getInstance().isSelectedTab(tab)) {
                     updateHomePagerForTab(tab);
--- a/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
+++ b/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ -203,16 +203,18 @@ public abstract class GeckoApp
 
     private String mPrivateBrowsingSession;
 
     private volatile HealthRecorder mHealthRecorder;
     private volatile Locale mLastLocale;
 
     private Intent mRestartIntent;
 
+    private boolean mWasFirstTabShownAfterActivityUnhidden;
+
     abstract public int getLayout();
 
     protected void processTabQueue() {};
 
     protected void openQueuedTabs() {};
 
     @SuppressWarnings("serial")
     class SessionRestoreException extends Exception {
@@ -1338,16 +1340,22 @@ public abstract class GeckoApp
                 BrowserLocaleManager.storeAndNotifyOSLocale(GeckoSharedPrefs.forProfile(GeckoApp.this), osLocale);
             }
         });
 
         GeckoAppShell.setNotificationClient(makeNotificationClient());
         IntentHelper.init(this);
     }
 
+    @Override
+    public void onStart() {
+        super.onStart();
+        mWasFirstTabShownAfterActivityUnhidden = false; // onStart indicates we were hidden.
+    }
+
     /**
      * At this point, the resource system and the rest of the browser are
      * aware of the locale.
      *
      * Now we can display strings!
      *
      * You can think of this as being something like a second phase of onCreate,
      * where you can do string-related operations. Use this in place of embedding
@@ -1429,16 +1437,19 @@ public abstract class GeckoApp
 
     public String getHomepage() {
         return null;
     }
 
     private void initialize() {
         mInitialized = true;
 
+        final boolean isFirstTab = !mWasFirstTabShownAfterActivityUnhidden;
+        mWasFirstTabShownAfterActivityUnhidden = true; // Reset since we'll be loading a tab.
+
         final SafeIntent intent = new SafeIntent(getIntent());
         final String action = intent.getAction();
 
         final String uri = getURIFromIntent(intent);
 
         final String passedUri;
         if (!TextUtils.isEmpty(uri)) {
             passedUri = uri;
@@ -1486,16 +1497,19 @@ public abstract class GeckoApp
             Tabs.getInstance().notifyListeners(null, Tabs.TabEvents.RESTORED);
             processActionViewIntent(new Runnable() {
                 @Override
                 public void run() {
                     int flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_EXTERNAL;
                     if (ACTION_HOMESCREEN_SHORTCUT.equals(action)) {
                         flags |= Tabs.LOADURL_PINNED;
                     }
+                    if (isFirstTab) {
+                        flags |= Tabs.LOADURL_FIRST_AFTER_ACTIVITY_UNHIDDEN;
+                    }
                     loadStartupTab(passedUri, intent, flags);
                 }
             });
         } else {
             if (!mIsRestoringActivity) {
                 loadStartupTab(Tabs.LOADURL_NEW_TAB);
             }
 
@@ -1881,16 +1895,19 @@ public abstract class GeckoApp
         }
         handleNotification(ACTION_ALERT_CALLBACK, alertName, alertCookie);
     }
 
     @Override
     protected void onNewIntent(Intent externalIntent) {
         final SafeIntent intent = new SafeIntent(externalIntent);
 
+        final boolean isFirstTab = !mWasFirstTabShownAfterActivityUnhidden;
+        mWasFirstTabShownAfterActivityUnhidden = true; // Reset since we'll be loading a tab.
+
         // if we were previously OOM killed, we can end up here when launching
         // from external shortcuts, so set this as the intent for initialization
         if (!mInitialized) {
             setIntent(externalIntent);
             return;
         }
 
         final String action = intent.getAction();
@@ -1905,19 +1922,21 @@ public abstract class GeckoApp
 
         if (ACTION_LOAD.equals(action)) {
             Tabs.getInstance().loadUrl(intent.getDataString());
         } else if (Intent.ACTION_VIEW.equals(action)) {
             processActionViewIntent(new Runnable() {
                 @Override
                 public void run() {
                     final String url = intent.getDataString();
-                    Tabs.getInstance().loadUrlWithIntentExtras(url, intent, Tabs.LOADURL_NEW_TAB |
-                                                                                    Tabs.LOADURL_USER_ENTERED |
-                                                                                    Tabs.LOADURL_EXTERNAL);
+                    int flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_USER_ENTERED | Tabs.LOADURL_EXTERNAL;
+                    if (isFirstTab) {
+                        flags |= Tabs.LOADURL_FIRST_AFTER_ACTIVITY_UNHIDDEN;
+                    }
+                    Tabs.getInstance().loadUrlWithIntentExtras(url, intent, flags);
                 }
             });
         } else if (ACTION_HOMESCREEN_SHORTCUT.equals(action)) {
             GeckoAppShell.sendEventToGecko(GeckoEvent.createBookmarkLoadEvent(uri));
         } else if (Intent.ACTION_SEARCH.equals(action)) {
             GeckoAppShell.sendEventToGecko(GeckoEvent.createURILoadEvent(uri));
         } else if (ACTION_ALERT_CALLBACK.equals(action)) {
             processAlertCallback(intent);
--- a/mobile/android/base/java/org/mozilla/gecko/Tab.java
+++ b/mobile/android/base/java/org/mozilla/gecko/Tab.java
@@ -81,16 +81,17 @@ public class Tab {
     private boolean mDesktopMode;
     private boolean mEnteringReaderMode;
     private final Context mAppContext;
     private ErrorType mErrorType = ErrorType.NONE;
     private volatile int mLoadProgress;
     private volatile int mRecordingCount;
     private volatile boolean mIsAudioPlaying;
     private String mMostRecentHomePanel;
+    private boolean mShouldShowToolbarWithoutAnimationOnFirstSelection;
 
     private int mHistoryIndex;
     private int mHistorySize;
     private boolean mCanDoBack;
     private boolean mCanDoForward;
 
     private boolean mIsEditing;
     private final TabEditingState mEditingState = new TabEditingState();
@@ -894,9 +895,17 @@ public class Tab {
 
     public void setIsEditing(final boolean isEditing) {
         this.mIsEditing = isEditing;
     }
 
     public TabEditingState getEditingState() {
         return mEditingState;
     }
+
+    public void setShouldShowToolbarWithoutAnimationOnFirstSelection(final boolean shouldShowWithoutAnimation) {
+        mShouldShowToolbarWithoutAnimationOnFirstSelection = shouldShowWithoutAnimation;
+    }
+
+    public boolean getShouldShowToolbarWithoutAnimationOnFirstSelection() {
+        return mShouldShowToolbarWithoutAnimationOnFirstSelection;
+    }
 }
--- a/mobile/android/base/java/org/mozilla/gecko/Tabs.java
+++ b/mobile/android/base/java/org/mozilla/gecko/Tabs.java
@@ -60,16 +60,18 @@ public class Tabs implements GeckoEventL
     public static final int LOADURL_USER_ENTERED = 1 << 1;
     public static final int LOADURL_PRIVATE      = 1 << 2;
     public static final int LOADURL_PINNED       = 1 << 3;
     public static final int LOADURL_DELAY_LOAD   = 1 << 4;
     public static final int LOADURL_DESKTOP      = 1 << 5;
     public static final int LOADURL_BACKGROUND   = 1 << 6;
     /** Indicates the url has been specified by a source external to the app. */
     public static final int LOADURL_EXTERNAL     = 1 << 7;
+    /** Indicates the tab is the first shown after Firefox is hidden and restored. */
+    public static final int LOADURL_FIRST_AFTER_ACTIVITY_UNHIDDEN = 1 << 8;
 
     private static final long PERSIST_TABS_AFTER_MILLISECONDS = 1000 * 2;
 
     public static final int INVALID_TAB_ID = -1;
 
     private static final AtomicInteger sTabId = new AtomicInteger(0);
     private volatile boolean mInitialTabsAdded;
 
@@ -829,16 +831,17 @@ public class Tabs implements GeckoEventL
         // delayLoad implies background tab
         boolean background = delayLoad || (flags & LOADURL_BACKGROUND) != 0;
 
         try {
             boolean isPrivate = (flags & LOADURL_PRIVATE) != 0;
             boolean userEntered = (flags & LOADURL_USER_ENTERED) != 0;
             boolean desktopMode = (flags & LOADURL_DESKTOP) != 0;
             boolean external = (flags & LOADURL_EXTERNAL) != 0;
+            final boolean isFirstShownAfterActivityUnhidden = (flags & LOADURL_FIRST_AFTER_ACTIVITY_UNHIDDEN) != 0;
 
             args.put("url", url);
             args.put("engine", searchEngine);
             args.put("parentId", parentId);
             args.put("userEntered", userEntered);
             args.put("isPrivate", isPrivate);
             args.put("pinned", (flags & LOADURL_PINNED) != 0);
             args.put("desktopMode", desktopMode);
@@ -889,16 +892,21 @@ public class Tabs implements GeckoEventL
                 String tabUrl = (url != null && Uri.parse(url).getScheme() != null) ? url : null;
 
                 // Add the new tab to the end of the tab order.
                 final int tabIndex = -1;
 
                 tabToSelect = addTab(tabId, tabUrl, external, parentId, url, isPrivate, tabIndex);
                 tabToSelect.setDesktopMode(desktopMode);
                 tabToSelect.setApplicationId(applicationId);
+                if (isFirstShownAfterActivityUnhidden) {
+                    // We just opened Firefox so we want to show
+                    // the toolbar but not animate it to avoid jank.
+                    tabToSelect.setShouldShowToolbarWithoutAnimationOnFirstSelection(true);
+                }
             }
         } catch (Exception e) {
             Log.w(LOGTAG, "Error building JSON arguments for loadUrl.", e);
         }
 
         GeckoAppShell.notifyObservers("Tab:Load", args.toString());
 
         if (tabToSelect == null) {