Android: Single TabUma instance per tab

TabUma is an optional Tab UserData object that keeps track of tab
restoring/state transition event, and collects relevant UMA
stats. It's instantiated only for the Tab with a notable
creation state or restoration results. And a new TabUma object
is created every time tab restoration fails, and replaces the old one.

With this CL, TabUma is always instantiated upon Tab creation and
lasts till the tab goes away. This comes at a cost of having an
instance doing potentially no actual work but has a few benefits:

- TabCreationState doesn't need to be passed to Tab any
  more. TabBuilder instantiates TabUma and does the required
  initialization.
- TabUma-related tasks can be moved out of Tab and
  confined within TabUma class itself using TabObserver events.

Now the restoration result is passed to a single TabUma per tab through
TabObserver.

A new test was added to make sure tab state transition works as expected
before and after the CL.

Bug: 995903
Change-Id: I8f59bf52542325da06129f76a4a32229cdd9dbb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2087861Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750865}

chrome/android/java/src/org/chromium/chrome/browser/tab/EmptyTabObserver.java

@@ -54,6 +54,9 @@ public class EmptyTabObserver implements TabObserver {
     @Override
     public void onRestoreStarted(Tab tab) {}
 
+    @Override
+    public void onRestoreFailed(Tab tab) {}
+
     @Override
     public void onFaviconUpdated(Tab tab, Bitmap icon) { }
 

chrome/android/java/src/org/chromium/chrome/browser/tab/TabBuilder.java

@@ -26,7 +26,6 @@ public class TabBuilder {
     private TabDelegateFactory mDelegateFactory;
     private boolean mInitiallyHidden;
     private TabState mTabState;
-    private boolean mUnfreeze;
 
     /**
      * Sets the id with which the Tab to create should be identified.
@@ -120,17 +119,6 @@ public class TabBuilder {
         return this;
     }
 
-    /**
-     * Sets a flag indicating if there should be an attempt to restore state at the end of
-     *        the initialization.
-     * @param unfreeze {@code true} if WebContents needs restoring from its saved state.
-     * @return {@link TabBuilder} creating the Tab.
-     */
-    public TabBuilder setUnfreeze(boolean unfreeze) {
-        mUnfreeze = unfreeze;
-        return this;
-    }
-
     public Tab build() {
         // Pre-condition check
         if (mCreationType != null) {
@@ -154,7 +142,7 @@ public class TabBuilder {
         // Initializes Tab. Its user data objects are also initialized through the event
         // |onInitialized| of TabObserver they register.
         tab.initialize(mParent, mCreationType, mLoadUrlParams, mWebContents, mDelegateFactory,
-                mInitiallyHidden, mTabState, mUnfreeze);
+                mInitiallyHidden, mTabState);
         return tab;
     }
 

chrome/android/java/src/org/chromium/chrome/browser/tab/TabCreationState.java

@@ -14,13 +14,11 @@ import java.lang.annotation.RetentionPolicy;
  * distinguish reasons for a tab to be restored upon first display.
  */
 @IntDef({TabCreationState.LIVE_IN_FOREGROUND, TabCreationState.LIVE_IN_BACKGROUND,
-        TabCreationState.FROZEN_ON_RESTORE, TabCreationState.FROZEN_FOR_LAZY_LOAD,
-        TabCreationState.FROZEN_ON_RESTORE_FAILED})
+        TabCreationState.FROZEN_ON_RESTORE, TabCreationState.FROZEN_FOR_LAZY_LOAD})
 @Retention(RetentionPolicy.SOURCE)
 public @interface TabCreationState {
     int LIVE_IN_FOREGROUND = 0;
     int LIVE_IN_BACKGROUND = 1;
     int FROZEN_ON_RESTORE = 2;
     int FROZEN_FOR_LAZY_LOAD = 3;
-    int FROZEN_ON_RESTORE_FAILED = 4;
 }

chrome/android/java/src/org/chromium/chrome/browser/tab/TabHelpers.java

@@ -29,10 +29,9 @@ public final class TabHelpers {
      * Creates Tab helper objects upon Tab creation.
      * @param tab {@link Tab} to create helpers for.
      * @param parentTab {@link Tab} parent tab
-     * @param creationState State in which the tab is created.
      */
-    static void initTabHelpers(Tab tab, Tab parentTab, @TabCreationState Integer creationState) {
-        if (creationState != null) TabUma.create(tab, creationState);
+    static void initTabHelpers(Tab tab, Tab parentTab) {
+        TabUma.createForTab(tab);
         TabDistillabilityProvider.createForTab(tab);
         TabThemeColorHelper.createForTab(tab);
         InterceptNavigationDelegateImpl.createForTab(tab);

chrome/android/java/src/org/chromium/chrome/browser/tab/TabImpl.java

@@ -140,6 +140,8 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
      */
     private @Nullable @TabLaunchType Integer mLaunchTypeAtCreation;
 
+    private @Nullable @TabCreationState Integer mCreationState;
+
     /**
      * Navigation state of the WebContents as returned by nativeGetContentsStateAsByteBuffer(),
      * stored to be inflated on demand using unfreezeContents(). If this is not null, there is no
@@ -814,21 +816,20 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
      * @param initiallyHidden Only used if {@code webContents} is {@code null}.  Determines
      *        whether or not the newly created {@link WebContents} will be hidden or not.
      * @param tabState State containing information about this Tab, if it was persisted.
-     * @param unfreeze Whether there should be an attempt to restore state at the end of
-     *        the initialization.
      */
     void initialize(Tab parent, @Nullable @TabCreationState Integer creationState,
             LoadUrlParams loadUrlParams, WebContents webContents,
             @Nullable TabDelegateFactory delegateFactory, boolean initiallyHidden,
-            TabState tabState, boolean unfreeze) {
+            TabState tabState) {
         try {
             TraceEvent.begin("Tab.initialize");
 
             mLaunchTypeAtCreation = mLaunchType;
+            mCreationState = creationState;
             mPendingLoadParams = loadUrlParams;
             if (loadUrlParams != null) mUrl = new GURL(loadUrlParams.getUrl());
 
-            TabHelpers.initTabHelpers(this, parent, creationState);
+            TabHelpers.initTabHelpers(this, parent);
 
             if (tabState != null) restoreFieldsFromState(tabState);
 
@@ -838,11 +839,8 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
             RevenueStats.getInstance().tabCreated(this);
 
             // If there is a frozen WebContents state or a pending lazy load, don't create a new
-            // WebContents.
-            if (getFrozenContentsState() != null || getPendingLoadParams() != null) {
-                if (unfreeze) unfreezeContents();
-                return;
-            }
+            // WebContents. Restoring will be done when showing the tab in the foreground.
+            if (getFrozenContentsState() != null || getPendingLoadParams() != null) return;
 
             boolean creatingWebContents = webContents == null;
             if (creatingWebContents) {
@@ -869,6 +867,12 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
         }
     }
 
+    @Nullable
+    @TabCreationState
+    Integer getCreationState() {
+        return mCreationState;
+    }
+
     /**
      * Restores member fields from the given TabState.
      * @param state TabState containing information about this Tab.
@@ -1391,11 +1395,10 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
     private final void restoreIfNeeded() {
         try {
             TraceEvent.begin("Tab.restoreIfNeeded");
-            if (isFrozen() && mFrozenContentsState != null) {
-                // Restore is needed for a tab that is loaded for the first time. WebContents will
-                // be restored from a saved state.
-                unfreezeContents();
-            } else if (!needsReload()) {
+            // Restore is needed for a tab that is loaded for the first time. WebContents will
+            // be restored from a saved state.
+            if ((isFrozen() && mFrozenContentsState != null && !unfreezeContents())
+                    || !needsReload()) {
                 return;
             }
 
@@ -1412,21 +1415,21 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
      * frozen with a saved TabState, and NOT if it was frozen for a lazy load.
      * @return Whether or not the restoration was successful.
      */
-    private void unfreezeContents() {
+    private boolean unfreezeContents() {
+        boolean restored = true;
         try {
             TraceEvent.begin("Tab.unfreezeContents");
             assert mFrozenContentsState != null;
 
             WebContents webContents =
                     mFrozenContentsState.restoreContentsFromByteBuffer(isHidden());
-            boolean failedToRestore = false;
             if (webContents == null) {
                 // State restore failed, just create a new empty web contents as that is the best
                 // that can be done at this point. TODO(jcivelli) http://b/5910521 - we should show
                 // an error page instead of a blank page in that case (and the last loaded URL).
                 webContents = WebContentsFactory.createWebContents(isIncognito(), isHidden());
-                TabUma.create(this, TabCreationState.FROZEN_ON_RESTORE_FAILED);
-                failedToRestore = true;
+                for (TabObserver observer : mObservers) observer.onRestoreFailed(this);
+                restored = false;
             }
             View compositorView = getActivity().getCompositorViewHolder();
             webContents.setSize(compositorView.getWidth(), compositorView.getHeight());
@@ -1434,13 +1437,14 @@ public class TabImpl implements Tab, TabObscuringHandler.Observer {
             mFrozenContentsState = null;
             initWebContents(webContents);
 
-            if (failedToRestore) {
+            if (!restored) {
                 String url = mUrl.getSpec().isEmpty() ? UrlConstants.NTP_URL : mUrl.getSpec();
                 loadUrl(new LoadUrlParams(url, PageTransition.GENERATED));
             }
         } finally {
             TraceEvent.end("Tab.unfreezeContents");
         }
+        return restored;
     }
 
     private boolean isCustomTab() {

chrome/android/java/src/org/chromium/chrome/browser/tab/TabObserver.java

@@ -141,6 +141,12 @@ public interface TabObserver {
      */
     void onRestoreStarted(Tab tab);
 
+    /**
+     * Called when restoration of the corresponding tab failed.
+     * @param tab The notifying {@link Tab}.
+     */
+    void onRestoreFailed(Tab tab);
+
     /**
      * Called when the WebContents of a {@link Tab} have been swapped.
      * @param tab The notifying {@link Tab}.

chrome/android/java/src/org/chromium/chrome/browser/tab/TabUma.java

@@ -53,7 +53,7 @@ public class TabUma extends EmptyTabObserver implements UserData {
     // Counter of tab shows (as per onShow()) for all tabs.
     private static long sAllTabsShowCount;
 
-    private final @TabCreationState int mTabCreationState;
+    private @TabCreationState int mTabCreationState;
 
     // Timestamp when this tab was last shown.
     private long mLastShownTimestamp = -1;
@@ -70,28 +70,28 @@ public class TabUma extends EmptyTabObserver implements UserData {
 
     private TabModelSelectorObserver mNewTabObserver;
 
-    static TabUma create(Tab tab, @TabCreationState int creationState) {
-        TabUma tabUma = tab.getUserDataHost().getUserData(USER_DATA_KEY);
-        if (tabUma != null) tabUma.removeObservers(tab);
-
-        return tab.getUserDataHost().setUserData(USER_DATA_KEY, new TabUma(tab, creationState));
+    /**
+     * Creates {@link TabUma} instance optionally. Creates one only when tab creation type
+     * is non-null.
+     */
+    static void createForTab(Tab tab) {
+        assert tab.getUserDataHost().getUserData(USER_DATA_KEY) == null;
+        @TabCreationState
+        Integer creationState = ((TabImpl) tab).getCreationState();
+        if (creationState != null) {
+            tab.getUserDataHost().setUserData(USER_DATA_KEY, new TabUma(tab, creationState));
+        }
     }
 
     /**
      * Constructs a new UMA tracker for a specific tab.
-     * @param Tab The Tab being monitored for stats.
+     * @param tab Tab this UMA tracker is created for.
      * @param creationState In what state the tab was created.
      */
     private TabUma(Tab tab, @TabCreationState int creationState) {
-        mTabCreationState = creationState;
-
         mLastTabStateChangeMillis = System.currentTimeMillis();
+        mTabCreationState = creationState;
         switch (mTabCreationState) {
-            case TabCreationState.FROZEN_ON_RESTORE_FAILED:
-                // A previous TabUma should have reported an active tab state. Initialize but avoid
-                // recording this as a state change.
-                mLastTabState = TAB_STATE_ACTIVE;
-            // Fall through
             case TabCreationState.LIVE_IN_FOREGROUND:
                 updateTabState(TAB_STATE_ACTIVE);
                 break;
@@ -178,6 +178,8 @@ public class TabUma extends EmptyTabObserver implements UserData {
         mLastTabState = newState;
     }
 
+    // TabObserver
+
     @Override
     public void onShown(Tab tab, @TabSelectionType int selectionType) {
         long previousTimestampMillis = tab.getTimestampMillis();
@@ -264,8 +266,6 @@ public class TabUma extends EmptyTabObserver implements UserData {
         updateTabState(TAB_STATE_ACTIVE);
     }
 
-    // TabObserver
-
     @Override
     public void onHidden(Tab tab, @TabHidingType int type) {
         if (type == TabHidingType.ACTIVITY_HIDDEN) {
@@ -286,11 +286,10 @@ public class TabUma extends EmptyTabObserver implements UserData {
         }
 
         recordNumBackgroundTabsOpened();
-        removeObservers(tab);
-    }
-
-    private void removeObservers(Tab tab) {
-        if (mNewTabObserver != null) TabModelSelector.from(tab).removeObserver(mNewTabObserver);
+        if (mNewTabObserver != null) {
+            TabModelSelector.from(tab).removeObserver(mNewTabObserver);
+            mNewTabObserver = null;
+        }
         tab.removeObserver(this);
     }
 
@@ -299,13 +298,23 @@ public class TabUma extends EmptyTabObserver implements UserData {
         mRestoreStartedAtMillis = SystemClock.elapsedRealtime();
     }
 
+    @Override
+    public void onRestoreFailed(Tab tab) {
+        assert mRestoreStartedAtMillis == -1;
+        if (mLastTabState == TAB_STATE_ACTIVE) {
+            mTabCreationState = TabCreationState.LIVE_IN_FOREGROUND;
+        } else {
+            mTabCreationState = TabCreationState.LIVE_IN_BACKGROUND;
+        }
+    }
+
     /** Called when the corresponding tab starts a page load. */
     @Override
     public void onPageLoadStarted(Tab tab, String url) {
         recordNumBackgroundTabsOpened();
     }
 
-    /** Called when the correspoding tab completes a page load. */
+    /** Called when the corresponding tab completes a page load. */
     @Override
     public void onPageLoadFinished(Tab tab, String url) {
         // Record only tab restores that the user became aware of. If the restore is triggered
@@ -320,7 +329,7 @@ public class TabUma extends EmptyTabObserver implements UserData {
         mRestoreStartedAtMillis = -1;
     }
 
-    /** Called when the correspoding tab fails a page load. */
+    /** Called when the corresponding tab fails a page load. */
     @Override
     public void onPageLoadFailed(Tab tab, int errorCode) {
         if (mRestoreStartedAtMillis != -1 && mLastShownTimestamp >= mRestoreStartedAtMillis) {
@@ -330,7 +339,7 @@ public class TabUma extends EmptyTabObserver implements UserData {
         mRestoreStartedAtMillis = -1;
     }
 
-    /** Called when the renderer of the correspoding tab crashes. */
+    /** Called when the renderer of the corresponding tab crashes. */
     @Override
     public void onCrash(Tab tab) {
         if (mRestoreStartedAtMillis != -1) {

chrome/test/android/javatests/src/org/chromium/chrome/browser/tab/MockTab.java

@@ -18,7 +18,7 @@ public class MockTab extends TabImpl {
      */
     public static Tab createAndInitialize(int id, boolean incognito) {
         TabImpl tab = new MockTab(id, incognito);
-        tab.initialize(null, null, null, null, null, false, null, false);
+        tab.initialize(null, null, null, null, null, false, null);
         return tab;
     }
 
@@ -38,7 +38,7 @@ public class MockTab extends TabImpl {
     public void initialize(Tab parent, @Nullable @TabCreationState Integer creationState,
             LoadUrlParams loadUrlParams, WebContents webContents,
             @Nullable TabDelegateFactory delegateFactory, boolean initiallyHidden,
-            TabState tabState, boolean unfreeze) {
-        TabHelpers.initTabHelpers(this, parent, creationState);
+            TabState tabState) {
+        TabHelpers.initTabHelpers(this, parent);
     }
 }