Android: Factor SadTab showing logic out of Tab

This CL aims to remove all the references to SadTab (thus slim it down)
by factoring the related methods out to a separate class SadTab. It is
managed as Tab UserData, and takes care of creating/showing sad tab view
on ContentView. Also got SadTabFactory folded into it.

Note: Tab still has references to SadTab (isShowingSadTab) from
handleTabCrash/notifyContentChanged. Removing them involves somewhat
side-tracked changes as big as  the main one. Will handle it in
a follow-up CL.

Bug: 877878
Change-Id: I94a03e8f9999809cf690339a16570cc538e0e18c
Reviewed-on: https://chromium-review.googlesource.com/1235490
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594950}

chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java → chrome/android/java/src/org/chromium/chrome/browser/tab/SadTab.java

@@ -4,6 +4,7 @@
 
 package org.chromium.chrome.browser.tab;
 
+import android.app.Activity;
 import android.content.Context;
 import android.graphics.Canvas;
 import android.graphics.Paint;
@@ -16,35 +17,170 @@ import android.view.LayoutInflater;
 import android.view.View;
 import android.view.View.OnClickListener;
 import android.widget.Button;
+import android.widget.FrameLayout;
+import android.widget.FrameLayout.LayoutParams;
 import android.widget.TextView;
 
+import org.chromium.base.UserData;
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.metrics.RecordHistogram;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeFeatureList;
+import org.chromium.chrome.browser.help.HelpAndFeedback;
+import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.components.ui_metrics.SadTabEvent;
+import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.ui.text.NoUnderlineClickableSpan;
 import org.chromium.ui.text.SpanApplier;
 import org.chromium.ui.text.SpanApplier.SpanInfo;
 
 /**
- * A factory class for creating the "Sad Tab" view, which is shown in place of a crashed renderer.
+ * Represent the sad tab displayed in place of a crashed renderer. Instantiated on the first
+ * |show()| request from a Tab, and destroyed together with it.
  */
-public class SadTabViewFactory {
+public class SadTab extends EmptyTabObserver implements UserData {
+    private static final Class<SadTab> USER_DATA_KEY = SadTab.class;
+
+    private final Tab mTab;
+
+    private View mView;
+
+    /**
+     * Counts the number of successive refreshes on the sad tab page. The count is is reset after a
+     * successful page load.
+     */
+    private int mSadTabSuccessiveRefreshCounter;
+
+    public static SadTab from(Tab tab) {
+        SadTab sadTab = get(tab);
+        if (sadTab == null) {
+            sadTab = tab.getUserDataHost().setUserData(USER_DATA_KEY, new SadTab(tab));
+        }
+        return sadTab;
+    }
+
+    public static SadTab get(Tab tab) {
+        return tab.getUserDataHost().getUserData(USER_DATA_KEY);
+    }
+
+    public static boolean isShowing(Tab tab) {
+        SadTab sadTab = get(tab);
+        return sadTab != null ? sadTab.isShowing() : false;
+    }
+
+    @VisibleForTesting
+    public SadTab(Tab tab) {
+        mTab = tab;
+        mTab.addObserver(this);
+    }
+
+    /**
+     * Constructs and shows a sad tab (Aw, Snap!).
+     */
+    public void show() {
+        if (mTab.getWebContents() == null) return;
+
+        // Make sure we are not adding the "Aw, snap" view over an existing one.
+        assert mView == null;
+
+        // If the tab has crashed twice in a row change the sad tab view to the "Send Feedback"
+        // version and change the onClickListener.
+        final boolean showSendFeedbackView = mSadTabSuccessiveRefreshCounter >= 1;
+
+        Runnable suggestionAction = new Runnable() {
+            @Override
+            public void run() {
+                Activity activity = mTab.getWindowAndroid().getActivity().get();
+                assert activity != null;
+                HelpAndFeedback.getInstance(activity).show(activity,
+                        activity.getString(R.string.help_context_sad_tab),
+                        Profile.getLastUsedProfile(), null);
+            }
+        };
+
+        Runnable buttonAction = new Runnable() {
+            @Override
+            public void run() {
+                if (showSendFeedbackView) {
+                    mTab.getActivity().startHelpAndFeedback(
+                            mTab.getUrl(), "MobileSadTabFeedback", mTab.getProfile());
+                } else {
+                    mTab.reload();
+                }
+            }
+        };
+
+        mView = createView(
+                suggestionAction, buttonAction, showSendFeedbackView, mTab.isIncognito());
+        mSadTabSuccessiveRefreshCounter++;
+
+        // Show the sad tab inside ContentView.
+        mTab.getContentView().addView(mView,
+                new FrameLayout.LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT));
+        mTab.notifyContentChanged();
+    }
+
+    /**
+     * Removes the sad tab view if present.
+     */
+    @VisibleForTesting
+    void removeIfPresent() {
+        if (isShowing()) {
+            mTab.getContentView().removeView(mView);
+            mTab.notifyContentChanged();
+        }
+        mView = null;
+    }
+
+    /**
+     * @return Whether or not the sad tab is showing.
+     */
+    public boolean isShowing() {
+        return mView != null && mView.getParent() == mTab.getContentView();
+    }
+
+    // TabObserver
+
+    @Override
+    public void onLoadUrl(Tab tab, LoadUrlParams params, int loadType) {
+        removeIfPresent();
+    }
+
+    @Override
+    public void onPageLoadStarted(Tab tab, String url) {
+        removeIfPresent();
+    }
+
+    @Override
+    public void onPageLoadFinished(Tab tab) {
+        // Reset the succressiveRefresh counter after successfully loading a page.
+        mSadTabSuccessiveRefreshCounter = 0;
+        removeIfPresent();
+    }
+
+    // UserData
+
+    @Override
+    public void destroy() {
+        mTab.removeObserver(this);
+    }
+
     /**
-     * @param context Context of the resulting Sad Tab view.
      * @param suggestionAction {@link Runnable} to be executed when user clicks "try these
      *                        suggestions".
      * @param buttonAction {@link Runnable} to be executed when the button is pressed.
      *                     (e.g., refreshing the page or sending feedback)
      * @param showSendFeedbackView Whether to show the "send feedback" version of the Sad Tab view.
      * @param isIncognito Whether the Sad Tab view is being showin in an incognito tab.
-     * @return A "Sad Tab" view instance which is used in place of a crashed renderer.
+     * @return A {@link View} instance which is used in place of a crashed renderer.
      */
-    public static View createSadTabView(Context context, final Runnable suggestionAction,
-            final Runnable buttonAction, final boolean showSendFeedbackView, boolean isIncognito) {
+    protected View createView(final Runnable suggestionAction, Runnable buttonAction,
+            boolean showSendFeedbackView, boolean isIncognito) {
+        Context context = mTab.getThemedApplicationContext();
+
         // Inflate Sad tab and initialize.
-        LayoutInflater inflater = (LayoutInflater) context.getSystemService(
-                Context.LAYOUT_INFLATER_SERVICE);
+        LayoutInflater inflater =
+                (LayoutInflater) context.getSystemService(Context.LAYOUT_INFLATER_SERVICE);
         View sadTabView = inflater.inflate(R.layout.sad_tab, null);
 
         TextView titleText = (TextView) sadTabView.findViewById(R.id.sad_tab_title);
@@ -65,12 +201,12 @@ public class SadTabViewFactory {
         button.setOnClickListener(new OnClickListener() {
             @Override
             public void onClick(View v) {
-                SadTabViewFactory.recordEvent(showSendFeedbackView, SadTabEvent.BUTTON_CLICKED);
+                recordEvent(showSendFeedbackView, SadTabEvent.BUTTON_CLICKED);
                 buttonAction.run();
             }
         });
 
-        SadTabViewFactory.recordEvent(showSendFeedbackView, SadTabEvent.DISPLAYED);
+        recordEvent(showSendFeedbackView, SadTabEvent.DISPLAYED);
 
         return sadTabView;
     }
@@ -85,7 +221,7 @@ public class SadTabViewFactory {
     private static CharSequence getHelpMessage(
             Context context, final Runnable suggestionAction, final boolean showSendFeedback) {
         NoUnderlineClickableSpan linkSpan = new NoUnderlineClickableSpan((view) -> {
-            SadTabViewFactory.recordEvent(showSendFeedback, SadTabEvent.HELP_LINK_CLICKED);
+            recordEvent(showSendFeedback, SadTabEvent.HELP_LINK_CLICKED);
             suggestionAction.run();
         });
 
@@ -179,4 +315,10 @@ public class SadTabViewFactory {
                     c, p, x + mXOffset, dir, top, baseline, bottom, text, start, end, first, l);
         }
     }
+
+    // Bare minimum set up so |isShowing| returns true.
+    @VisibleForTesting
+    public static void initForTesting(Tab tab, SadTab sadTab) {
+        tab.getUserDataHost().setUserData(USER_DATA_KEY, sadTab);
+    }
 }

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

@@ -27,7 +27,6 @@ import android.view.ViewGroup;
 import android.view.accessibility.AccessibilityEvent;
 import android.widget.Button;
 import android.widget.FrameLayout;
-import android.widget.FrameLayout.LayoutParams;
 import android.widget.PopupWindow;
 import android.widget.PopupWindow.OnDismissListener;
 
@@ -67,7 +66,6 @@ import org.chromium.chrome.browser.document.ChromeLauncherActivity;
 import org.chromium.chrome.browser.feature_engagement.TrackerFactory;
 import org.chromium.chrome.browser.fullscreen.FullscreenManager;
 import org.chromium.chrome.browser.fullscreen.FullscreenOptions;
-import org.chromium.chrome.browser.help.HelpAndFeedback;
 import org.chromium.chrome.browser.infobar.InfoBarContainer;
 import org.chromium.chrome.browser.media.ui.MediaSessionTabHelper;
 import org.chromium.chrome.browser.native_page.FrozenNativePage;
@@ -370,17 +368,6 @@ public class Tab
      */
     private boolean mIsDetached;
 
-    /**
-     * Reference to the current sadTabView if one is defined.
-     */
-    private View mSadTabView;
-
-    /**
-     * Counts the number of successive refreshes on the sad tab page. The count is is reset after a
-     * successful page load.
-     */
-    private int mSadTabSuccessiveRefreshCounter;
-
     /**
      * Stores total data saved at the start of a page load. Used to calculate delta at the end of
      * page load, which is just an estimate of the data saved for the current page load since there
@@ -714,8 +701,6 @@ public class Tab
                 mIsNativePageCommitPending = maybeShowNativePage(params.getUrl(), false);
             }
 
-            removeSadTabIfPresent();
-
             // Clear the app association if the user navigated to a different page from the omnibox.
             if ((params.getTransitionType() & PageTransition.FROM_ADDRESS_BAR)
                     == PageTransition.FROM_ADDRESS_BAR) {
@@ -1617,7 +1602,6 @@ public class Tab
      */
     protected void didStartPageLoad(String validatedUrl, boolean showingErrorPage) {
         updateTitle();
-        removeSadTabIfPresent();
         mDataSavedOnStartPageLoad =
                 DataReductionProxySettings.getInstance().getContentLengthSavedInHistorySummary();
 
@@ -1634,9 +1618,6 @@ public class Tab
         updateTitle();
         updateFullscreenEnabledState();
 
-        // Reset the succressiveRefresh counter after successfully loading a page.
-        mSadTabSuccessiveRefreshCounter = 0;
-
         for (TabObserver observer : mObservers) observer.onPageLoadFinished(this);
         mIsBeingRestored = false;
 
@@ -1899,72 +1880,6 @@ public class Tab
         }
     }
 
-    /**
-     * Constructs and shows a sad tab (Aw, Snap!).
-     */
-    protected void showSadTab() {
-        if (getWebContents() == null) return;
-
-        // If the tab has crashed twice in a row change the sad tab view to the "Send Feedback"
-        // version and change the onClickListener.
-        final boolean showSendFeedbackView = mSadTabSuccessiveRefreshCounter >= 1;
-
-        Runnable suggestionAction = new Runnable() {
-            @Override
-            public void run() {
-                Activity activity = mWindowAndroid.getActivity().get();
-                assert activity != null;
-                HelpAndFeedback.getInstance(activity).show(activity,
-                        activity.getString(R.string.help_context_sad_tab),
-                        Profile.getLastUsedProfile(), null);
-            }
-        };
-
-        Runnable buttonAction = new Runnable() {
-            @Override
-            public void run() {
-                if (showSendFeedbackView) {
-                    getActivity().startHelpAndFeedback(
-                            getUrl(), "MobileSadTabFeedback", getProfile());
-                } else {
-                    reload();
-                }
-            }
-        };
-
-        // Make sure we are not adding the "Aw, snap" view over an existing one.
-        assert mSadTabView == null;
-
-        mSadTabView = SadTabViewFactory.createSadTabView(mThemedApplicationContext,
-                suggestionAction, buttonAction, showSendFeedbackView, mIncognito);
-        mSadTabSuccessiveRefreshCounter++;
-        // Show the sad tab inside ContentView.
-        mContentView.addView(mSadTabView,
-                new FrameLayout.LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT));
-        notifyContentChanged();
-    }
-
-    /**
-     * Removes the sad tab view if present.
-     */
-    private void removeSadTabIfPresent() {
-        if (isShowingSadTab()) {
-            mContentView.removeView(mSadTabView);
-            notifyContentChanged();
-        }
-        mSadTabView = null;
-    }
-
-    /**
-     * Removes any existing sad tab view and shows it again. This "reloads" the tab without
-     * going through any formal loading logic.
-     */
-    @VisibleForTesting
-    public void reloadSadTabForTesting() {
-        removeSadTabIfPresent();
-        showSadTab();
-    }
-
     /**
      * @return The ID of the renderer process that backs this tab or
      *         {@link #INVALID_RENDER_PROCESS_PID} if there is none.
@@ -1979,13 +1894,13 @@ public class Tab
      * @return Whether or not the sad tab is showing.
      */
     public boolean isShowingSadTab() {
-        return mSadTabView != null && mSadTabView.getParent() == mContentView;
+        return SadTab.isShowing(this);
     }
 
     /**
      * Calls onContentChanged on all TabObservers and updates accessibility visibility.
      */
-    private void notifyContentChanged() {
+    void notifyContentChanged() {
         for (TabObserver observer : mObservers) observer.onContentChanged(this);
         updateAccessibilityVisibility();
     }

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

@@ -122,7 +122,7 @@ public class TabWebContentsObserver extends WebContentsObserver {
             }
         } else {
             rendererCrashStatus = TAB_RENDERER_CRASH_STATUS_SHOWN_IN_FOREGROUND_APP;
-            mTab.showSadTab();
+            SadTab.from(mTab).show();
             // This is necessary to correlate histogram data with stability counts.
             RecordHistogram.recordBooleanHistogram("Stability.Android.RendererCrash", true);
         }

chrome/android/java_sources.gni

@@ -1431,8 +1431,8 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/tab/ChildBackgroundTabShowObserver.java",
   "java/src/org/chromium/chrome/browser/tab/EmptyTabObserver.java",
   "java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java",
+  "java/src/org/chromium/chrome/browser/tab/SadTab.java",
   "java/src/org/chromium/chrome/browser/tab/SadTabView.java",
-  "java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java",
   "java/src/org/chromium/chrome/browser/tab/Tab.java",
   "java/src/org/chromium/chrome/browser/tab/TabBrowserControlsOffsetHelper.java",
   "java/src/org/chromium/chrome/browser/tab/TabContextMenuItemDelegate.java",

chrome/android/javatests/src/org/chromium/chrome/browser/TabTest.java

@@ -51,6 +51,10 @@ public class TabTest {
         }
     };
 
+    private boolean isShowingSadTab() throws Exception {
+        return ThreadUtils.runOnUiThreadBlocking(() -> mTab.isShowingSadTab());
+    }
+
     @Before
     public void setUp() throws Exception {
         mActivityTestRule.startMainActivityOnBlankPage();
@@ -101,7 +105,7 @@ public class TabTest {
 
         Assert.assertFalse(mTab.needsReload());
         Assert.assertFalse(mTab.isHidden());
-        Assert.assertFalse(mTab.isShowingSadTab());
+        Assert.assertFalse(isShowingSadTab());
 
         // Stop the activity and simulate a killed renderer.
         ApplicationTestUtils.fireHomeScreenIntent(InstrumentationRegistry.getTargetContext());
@@ -114,7 +118,7 @@ public class TabTest {
             }
         });
         Assert.assertTrue(mTab.needsReload());
-        Assert.assertFalse(mTab.isShowingSadTab());
+        Assert.assertFalse(isShowingSadTab());
 
         ApplicationTestUtils.launchChrome(InstrumentationRegistry.getTargetContext());
 
@@ -126,7 +130,7 @@ public class TabTest {
             }
         });
         Assert.assertFalse(mTab.needsReload());
-        Assert.assertFalse(mTab.isShowingSadTab());
+        Assert.assertFalse(isShowingSadTab());
     }
 
     @Test

chrome/android/javatests/src/org/chromium/chrome/browser/infobar/InfoBarTest.java

@@ -592,7 +592,7 @@ public class InfoBarTest {
         // The renderer should have been killed and the InfoBar removed.
         mListener.removeInfoBarAnimationFinished("InfoBar not removed.");
         Assert.assertTrue("Wrong infobar count", mActivityTestRule.getInfoBars().isEmpty());
-        CriteriaHelper.pollInstrumentationThread(new Criteria() {
+        CriteriaHelper.pollUiThread(new Criteria() {
             @Override
             public boolean isSatisfied() {
                 return mActivityTestRule.getActivity().getActivityTab().isShowingSadTab();

chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java

@@ -25,6 +25,8 @@ import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.net.test.util.TestWebServer;
 
+import java.util.concurrent.ExecutionException;
+
 /**
  * Tests related to the sad tab logic.
  */
@@ -41,6 +43,14 @@ public class SadTabTest {
         mActivityTestRule.startMainActivityOnBlankPage();
     }
 
+    private static boolean isShowingSadTab(Tab tab) {
+        try {
+            return ThreadUtils.runOnUiThreadBlocking(() -> tab.isShowingSadTab());
+        } catch (ExecutionException e) {
+            return false;
+        }
+    }
+
     /**
      * Verify that the sad tab is shown when the renderer crashes.
      */
@@ -50,9 +60,9 @@ public class SadTabTest {
     public void testSadTabShownWhenRendererProcessKilled() {
         final Tab tab = mActivityTestRule.getActivity().getActivityTab();
 
-        Assert.assertFalse(tab.isShowingSadTab());
+        Assert.assertFalse(isShowingSadTab(tab));
         simulateRendererKilled(tab, true);
-        Assert.assertTrue(tab.isShowingSadTab());
+        Assert.assertTrue(isShowingSadTab(tab));
     }
 
     /**
@@ -65,9 +75,9 @@ public class SadTabTest {
     public void testSadTabNotShownWhenRendererProcessKilledInBackround() {
         final Tab tab = mActivityTestRule.getActivity().getActivityTab();
 
-        Assert.assertFalse(tab.isShowingSadTab());
+        Assert.assertFalse(isShowingSadTab(tab));
         simulateRendererKilled(tab, false);
-        Assert.assertFalse(tab.isShowingSadTab());
+        Assert.assertFalse(isShowingSadTab(tab));
     }
 
     /**
@@ -127,16 +137,16 @@ public class SadTabTest {
     public void testSadTabPageButtonText() throws IllegalArgumentException, InterruptedException {
         final Tab tab = mActivityTestRule.getActivity().getActivityTab();
 
-        Assert.assertFalse(tab.isShowingSadTab());
+        Assert.assertFalse(isShowingSadTab(tab));
         simulateRendererKilled(tab, true);
-        Assert.assertTrue(tab.isShowingSadTab());
+        Assert.assertTrue(isShowingSadTab(tab));
         String actualText = getSadTabButton(tab).getText().toString();
         Assert.assertEquals("Expected the sad tab button to have the reload label",
                 mActivityTestRule.getActivity().getString(R.string.sad_tab_reload_label),
                 actualText);
 
         reloadSadTab(tab);
-        Assert.assertTrue(tab.isShowingSadTab());
+        Assert.assertTrue(isShowingSadTab(tab));
         actualText = getSadTabButton(tab).getText().toString();
         Assert.assertEquals(
                 "Expected the sad tab button to have the feedback label after the tab button "
@@ -147,7 +157,7 @@ public class SadTabTest {
         Assert.assertFalse(
                 "Expected about:blank to destroy the sad tab however the sad tab is still in "
                         + "view",
-                tab.isShowingSadTab());
+                isShowingSadTab(tab));
         simulateRendererKilled(tab, true);
         actualText = getSadTabButton(tab).getText().toString();
         Assert.assertEquals(
@@ -159,7 +169,7 @@ public class SadTabTest {
     /**
      * Helper method that kills the renderer on a UI thread.
      */
-    private void simulateRendererKilled(final Tab tab, final boolean visible) {
+    private static void simulateRendererKilled(final Tab tab, final boolean visible) {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
@@ -172,11 +182,13 @@ public class SadTabTest {
     /**
      * Helper method that reloads a tab with a SadTabView currently displayed.
      */
-    private void reloadSadTab(final Tab tab) {
+    private static void reloadSadTab(final Tab tab) {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
-                tab.reloadSadTabForTesting();
+                SadTab sadTab = SadTab.from(tab);
+                sadTab.removeIfPresent();
+                sadTab.show();
             }
         });
     }
@@ -187,7 +199,7 @@ public class SadTabTest {
      * @return Returns the button that is on the SadTabView, null if SadTabView.
      *         doesn't exist.
      */
-    private Button getSadTabButton(Tab tab) {
+    private static Button getSadTabButton(Tab tab) {
         return (Button) tab.getContentView().findViewById(R.id.sad_tab_button);
     }