Bring the feedback button to the Mac sad tab

chrome/browser/ui/sad_tab.cc

Index: chrome/browser/ui/sad_tab.cc
diff --git a/chrome/browser/ui/sad_tab.cc b/chrome/browser/ui/sad_tab.cc
index 03c5c4dc8d254bff248da5ab2a29104ca374703f..df8f379f47307332fb75a5c7db84d2748bdf2145 100644
--- a/chrome/browser/ui/sad_tab.cc
+++ b/chrome/browser/ui/sad_tab.cc
@@ -2,20 +2,193 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "build/build_config.h"
+#include "base/metrics/histogram.h"

[Ilya Sherman, 2016/08/22 20:52:13]

nit: histogram_macros

[Sidney San Martín, 2016/08/25 00:55:11]

Done.

+#include "chrome/browser/net/referrer.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/sad_tab.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/feedback/feedback_util.h"
+#include "components/strings/grit/components_strings.h"
+#include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/web_contents.h"
+#include "grit/components_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/memory/oom_memory_details.h"
+#endif
+
+namespace {
+
+// They use the same counting approach and bucket sizes as the tab discard
+// events in memory::OomPriorityManager so they can be directly compared.
+
+// TODO(jamescook): Maybe track time between sad tabs?
+
+// This macro uses a a static counter to add to the next numbered bucket each
+// time it's hit, to keep track of how many sad tabs of a given kind users see
+// per session. For details, see Tabs.SadTab.CrashCreated in histograms.xml.
+#define UMA_SAD_TAB_COUNTER(NAME)                            \
+  {                                                          \
+    static int count = 0;                                    \
+    UMA_HISTOGRAM_COUNTS_1000("Tabs.SadTab." NAME, ++count); \

[Ilya Sherman, 2016/08/22 20:52:13]

nit: Please move the increment statement to a separate line, so that readers
don't need to think about pre- vs post-increment semantics.

+  }
+
+enum class SadTabEvent {

[Ilya Sherman, 2016/08/22 20:52:12]

Please document that this enum is used to back an UMA histogram, and should
therefore be treated as append-only.

[Sidney San Martín, 2016/08/25 00:55:11]

Done — borrowed an existing comment.

+  DISPLAYED,
+  BUTTON_CLICKED,
+  HELP_LINK_CLICKED,
+  MAX_SAD_TAB_EVENT
+};
+
+void RecordEvent(bool feedback, SadTabEvent event) {
+  if (feedback) {

[Ilya Sherman, 2016/08/22 20:52:13]

What determines whether the feedback style or reload style of sad tab is shown? 
If it's a field trial (aka Finch experiment), then I'd recommend having a single
histogram rather than two separate ones.

[Sidney San Martín, 2016/08/25 00:55:11]

The first sad tab you see in a session will be reload style, subsequent ones are
feedback style. The rule might change, but the idea is that we ask for feedback
when you get multiple sad tabs in a short period of time.

+    UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleFeedback", event,
+                              SadTabEvent::MAX_SAD_TAB_EVENT);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleReload", event,
+                              SadTabEvent::MAX_SAD_TAB_EVENT);
+  }
+}
+
+static const int kCrashesBeforeFeedbackIsDisplayed = 1;
+static const char kCategoryTagCrash[] = "Crash";
+
+bool ShouldWantFeedback() {
+  static int total_crashes = 0;
+  return ++total_crashes > kCrashesBeforeFeedbackIsDisplayed;
+}
+
+}  // namespace
 
 namespace chrome {
 
 // static
 bool SadTab::ShouldShow(base::TerminationStatus status) {
-  return (status == base::TERMINATION_STATUS_ABNORMAL_TERMINATION ||
-          status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED ||
+  switch (status) {
+    case base::TERMINATION_STATUS_ABNORMAL_TERMINATION:
+    case base::TERMINATION_STATUS_PROCESS_WAS_KILLED:
+#if defined(OS_CHROMEOS)
+    case base::TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM:
+#endif
+    case base::TERMINATION_STATUS_PROCESS_CRASHED:
+    case base::TERMINATION_STATUS_OOM:
+      return true;
+    default:
+      return false;
+  }
+}
+
+SadTab::SadTab(content::WebContents* web_contents, SadTabKind kind)
+    : web_contents_(web_contents),
+      kind_(kind),
+      want_feedback_(ShouldWantFeedback()) {
+  switch (kind) {
+    case chrome::SAD_TAB_KIND_CRASHED:
+      UMA_SAD_TAB_COUNTER("CrashCreated");
+      break;
+    case chrome::SAD_TAB_KIND_OOM:
+      UMA_SAD_TAB_COUNTER("OomCreated");
+      break;
 #if defined(OS_CHROMEOS)
-          status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM ||
+    case chrome::SAD_TAB_KIND_KILLED_BY_OOM:
+      UMA_SAD_TAB_COUNTER("KillCreated.OOM");
+      {
+        const std::string spec = web_contents->GetURL().GetOrigin().spec();
+        memory::OomMemoryDetails::Log(
+            "Tab OOM-Killed Memory details: " + spec + ", ", base::Closure());
+      }
+// Fall through
 #endif
-          status == base::TERMINATION_STATUS_PROCESS_CRASHED ||
-          status == base::TERMINATION_STATUS_OOM);
+    case chrome::SAD_TAB_KIND_KILLED:
+      UMA_SAD_TAB_COUNTER("KillCreated");
+      break;
+  }
+}
+
+int SadTab::GetTitle() {
+  return IDS_SAD_TAB_TITLE;
+}
+
+int SadTab::GetMessage() {
+  switch (kind_) {
+#if defined(OS_CHROMEOS)
+    case chrome::SAD_TAB_KIND_KILLED_BY_OOM:
+      return IDS_KILLED_TAB_BY_OOM_MESSAGE;
+#endif
+    case chrome::SAD_TAB_KIND_OOM:
+      return IDS_SAD_TAB_OOM_MESSAGE;
+    default:
+      return IDS_SAD_TAB_MESSAGE;
+  }
+}
+
+int SadTab::GetButtonTitle() {
+  return want_feedback_ ? IDS_CRASHED_TAB_FEEDBACK_LINK
+                        : IDS_SAD_TAB_RELOAD_LABEL;
+}
+
+int SadTab::GetHelpLinkTitle() {
+  return IDS_SAD_TAB_LEARN_MORE_LINK;
+}
+
+const char* SadTab::GetHelpLinkURL() {
+  return want_feedback_ ? chrome::kCrashReasonFeedbackDisplayedURL
+                        : chrome::kCrashReasonURL;
+}
+
+void SadTab::RecordFirstPaint() {
+#if DCHECK_IS_ON()
+  DLOG_ASSERT(!recorded_paint_);
+  recorded_paint_ = true;
+#endif
+
+  switch (kind_) {
+    case chrome::SAD_TAB_KIND_CRASHED:
+      UMA_SAD_TAB_COUNTER("CrashDisplayed");
+      break;
+    case chrome::SAD_TAB_KIND_OOM:
+      UMA_SAD_TAB_COUNTER("OomDisplayed");
+      break;
+#if defined(OS_CHROMEOS)
+    case chrome::SAD_TAB_KIND_KILLED_BY_OOM:
+      UMA_SAD_TAB_COUNTER("KillDisplayed.OOM");
+// Fallthrough
+#endif
+    case chrome::SAD_TAB_KIND_KILLED:
+      UMA_SAD_TAB_COUNTER("KillDisplayed");
+      break;
+  }
+
+  RecordEvent(want_feedback_, SadTabEvent::DISPLAYED);
+}
+
+void SadTab::PerformAction(SadTab::Action action) {
+  DLOG_ASSERT(recorded_paint_);
+  switch (action) {
+    case Action::BUTTON:
+      RecordEvent(want_feedback_, SadTabEvent::BUTTON_CLICKED);
+      if (want_feedback_) {
+        chrome::ShowFeedbackPage(

[Sidney San Martín, 2016/08/22 05:54:51]

ShowFeedbackPage looks like a no-op on Chromium builds. What should I do about
that — only enable the feedback state for Chrome? It looks like there's no Help
menu in Chromium builds, either, I'll look at how that's done.

+            chrome::FindBrowserWithWebContents(web_contents_),
+            l10n_util::GetStringUTF8(kind_ == chrome::SAD_TAB_KIND_CRASHED
+                                         ? IDS_CRASHED_TAB_FEEDBACK_MESSAGE
+                                         : IDS_KILLED_TAB_FEEDBACK_MESSAGE),
+            std::string(kCategoryTagCrash));
+      } else {
+        web_contents_->GetController().Reload(true);
+      }
+      break;
+    case Action::HELP_LINK:
+      RecordEvent(want_feedback_, SadTabEvent::HELP_LINK_CLICKED);
+      content::OpenURLParams params(GURL(GetHelpLinkURL()), content::Referrer(),
+                                    CURRENT_TAB, ui::PAGE_TRANSITION_LINK,
+                                    false);
+      web_contents_->OpenURL(params);
+      break;
+  }
 }
 
 }  // namespace chrome