Show 'Send Feedback' button on the sad tab page on multiple failures

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

Index: chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java b/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
index fac8ca7c579645b06b5cf89d3461077a8bb47539..1abc21d26f7b01e739df8cc315c40c7e96f7a186 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
@@ -365,6 +365,13 @@ public class Tab implements ViewGroup.OnHierarchyChangeListener,
      */
     private View mSadTabView;
 
+    /**
+     * When we refresh the page we want to know if its the first time that they've refreshed the
+     * page. We use this instead of a boolean so we can keep track of how often the user refreshes
+     * the page.
+     */
+    private int mSadTabSuccessiveRefreshCounter;

[Theresa, 2016/11/30 19:42:38]

nit: "Counts the number of successive refreshes on the sad tab page. This is
reset after a successful page load." or something like that.

[JJ, 2016/11/30 23:55:53]

Done.

+
     private final int mDefaultThemeColor;
     private int mThemeColor;
 
@@ -1659,6 +1666,10 @@ public class Tab implements ViewGroup.OnHierarchyChangeListener,
                     "Navigation.IsMobileOptimized", mContentViewCore.getIsMobileOptimizedHint());
         }
 
+        // We need to reset the succressiveRefresh counter since we were able to successfully load
+        // a page.

[Theresa, 2016/11/30 19:42:38]

nit: "Reset the sad tab refresh counter since the page successfully loaded."

[JJ, 2016/11/30 23:55:53]

Done.

+        mSadTabSuccessiveRefreshCounter = 0;
+
         if (mTabUma != null) mTabUma.onPageLoadFinished();
 
         for (TabObserver observer : mObservers) observer.onPageLoadFinished(this);
@@ -1866,6 +1877,7 @@ public class Tab implements ViewGroup.OnHierarchyChangeListener,
                             Profile.getLastUsedProfile(), null);
                 }
             };
+
             OnClickListener reloadButtonAction = new OnClickListener() {
                 @Override
                 public void onClick(View view) {
@@ -1873,11 +1885,29 @@ public class Tab implements ViewGroup.OnHierarchyChangeListener,
                 }
             };
 
+            String buttonText = getActivity().getString(R.string.sad_tab_reload_label);
+            // If we have crashed twice in a row we need to change the button to "Send Feedback"
+            // and change the onClickListener on the off

[Theresa, 2016/11/30 19:42:38]

The end of this comment doesn't make sense. I think it should just have a period
after onClickListener and drop the rest.

[JJ, 2016/11/30 23:55:53]

Done.

+            if (mSadTabSuccessiveRefreshCounter >= 1) {
+                reloadButtonAction = new OnClickListener() {
+
+                    @Override
+                    public void onClick(View v) {
+                        // We use the onMenuOrKeyboardAction in order to add to the metrics as well
+                        // instead of calling the help activity on our own. Despite this not
+                        // actually being a "menu" or "keyboard" action but still a user action.

[Theresa, 2016/11/30 19:42:38]

We probably want to introduce a new user action when the user taps "Send
Feedback" on the sad tab page so that we can track how often this feature is
used separately from how often the option is chosen from the menu.

[JJ, 2016/11/30 23:55:53]

Tell me if this is the way you interpreted it.

+                        getActivity().onMenuOrKeyboardAction(R.id.help_id, false);
+                    }
+                };
+                buttonText = getActivity().getString(R.string.sad_tab_send_feedback_label);
+            }
+
             // Make sure we are not adding the "Aw, snap" view over an existing one.
             assert mSadTabView == null;
-            mSadTabView = SadTabViewFactory.createSadTabView(
-                    mThemedApplicationContext, suggestionAction, reloadButtonAction);
 
+            mSadTabView = SadTabViewFactory.createSadTabView(
+                    mThemedApplicationContext, suggestionAction, reloadButtonAction, buttonText);

[Theresa, 2016/11/30 19:42:38]

I think this logic could be streamlined a bit by using a final boolean to decide
whether the send feedback button is showing.

final boolean showSendFeedbackButton = mSadTabSuccessiveRefreshCounter >= 1;
OnClickListener reloadButtonAction = new OnClickListener() {
    @Override
    public void onClick(View view) {
        if (showSendFeedbackButton) {
           // logic to send feedback
        } else {
           reload();
        }
    }
};

....

mSadTabView = SadTabViewFactory.createSadTab(....,
               showSendFeedbackButton ? R.string.sad_tab_send_feedback_label
                   : R.string.sad_tab_reload_button);

[JJ, 2016/11/30 23:55:53]

Alright changed it to be almost exactly that, tell me if there is something that
you feel is off!

+            mSadTabSuccessiveRefreshCounter++;
             // Show the sad tab inside ContentView.
             getContentViewCore().getContainerView().addView(
                     mSadTabView, new FrameLayout.LayoutParams(
@@ -1902,6 +1932,17 @@ public class Tab implements ViewGroup.OnHierarchyChangeListener,
     }
 
     /**
+     * This first removes any existing sad tab view and shows it again. We use this to "reload"

[Theresa, 2016/11/30 19:42:38]

nit: "Removes any existing sad tab view then shows it again. This "reloads" the
tab without going through any formal loading logic."

I don't think the last sentence is necessary.

[JJ, 2016/11/30 23:55:53]

Done.

+     * the tab without going through any formal loading logic. We use this in testing to see how
+     * the button context changes within SadTab after a reload.
+     */
+    @VisibleForTesting
+    public void reloadSadTabForTesting() {
+        removeSadTabIfPresent();
+        showSadTab();
+    }
+
+    /**
      * @return Whether or not the sad tab is showing.
      */
     public boolean isShowingSadTab() {