Bring the feedback button to the Mac sad tab

chrome/browser/ui/sad_tab.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // 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 "chrome/browser/ui/sad_tab.h"
 
+#include "base/metrics/histogram_macros.h"
+#include "chrome/browser/net/referrer.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/chrome_pages.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 "ui/base/l10n/l10n_util.h"
+
+#if defined(OS_CHROMEOS)
+#include "chrome/browser/memory/oom_memory_details.h"
+#endif
+
+namespace {
+
+// These stats should use the same counting approach and bucket size as tab
+// discard events in memory::OomPriorityManager so they can be directly
+// compared.
+// TODO(jamescook): Maybe track time between sad tabs?

[James Cook, 2016/09/08 20:15:50]

Feel free to nuke this -- it's not going to happen. :-)

[Sidney San Martín, 2016/09/09 01:36:00]

Done :).

+
+// This macro uses a static counter to track how many times it's hit in a
+// session. See Tabs.SadTab.CrashCreated in histograms.xml for details.
+#define UMA_SAD_TAB_COUNTER(NAME)                          \

[James Cook, 2016/09/08 20:15:50]

I wouldn't do this with a macro and string concatenation. It's important to me
to be able to grep the codebase for UMA statistic names like
Tabs.SadTab.KillDisplayed.OOM. Sure, the old code is a bit repetitive, but it's
not that much longer. And in general we discourage macros:

https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros

[Sidney San Martín, 2016/09/09 01:36:00]

The macro deals with creating a block with a static counter. Is there a nice
middle ground between it and repeating this pattern 8x? For now, I got rid of
the concatenation.

[James Cook, 2016/09/09 04:37:08]

This is fine. However, please change NAME to |histogram_name| or similar. Most
of our code seems to use lower-case for macro parameters.

[Sidney San Martín, 2016/09/09 15:39:44]

Done.

+  {                                                        \
+    static int count = 0;                                  \
+    ++count;                                               \
+    UMA_HISTOGRAM_COUNTS_1000("Tabs.SadTab." NAME, count); \
+  }
+
+// This enum backs an UMA histogram, so it should be treated as append-only.
+enum class SadTabEvent {
+  DISPLAYED,
+  BUTTON_CLICKED,
+  HELP_LINK_CLICKED,
+  MAX_SAD_TAB_EVENT
+};
+
+void RecordEvent(bool feedback, SadTabEvent event) {
+  if (feedback) {
+    UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleFeedback", event,

[James Cook, 2016/09/08 20:15:50]

I would call these either CreatedWithFeedback or DisplayedWithFeedback to match
the existing histograms. (It's not clear from this name whether it's created or
shown.)

[Sidney San Martín, 2016/09/09 01:36:00]

These histograms are for other events (BUTTON_CLICKED, HELP_LINK_CLICKED, and,
in the future, TAB_CLOSED, NAVIGATED_AWAY…). I'm open to other names or ways of
organizing this info, though.

[James Cook, 2016/09/09 04:37:08]

Hrm, I missed that distinction when reading this code.
Tabs.SadTab.Feedback.Event? Tabs.SadTab.Feedback.Action?

[Sidney San Martín, 2016/09/09 15:39:44]

Done. I like the idea of splitting it up.

+                              SadTabEvent::MAX_SAD_TAB_EVENT);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION("Tabs.SadTab.StyleReload", event,
+                              SadTabEvent::MAX_SAD_TAB_EVENT);
+  }
+}
+
+static const int kCrashesBeforeFeedbackIsDisplayed = 1;

[James Cook, 2016/09/08 20:15:50]

nit: static not needed in anonymous namespace

[Sidney San Martín, 2016/09/09 01:36:00]

Good catch, removed.

+static const char kCategoryTagCrash[] = "Crash";
+
+bool ShouldWantFeedback() {
+  static int total_crashes = 0;
+  return ++total_crashes > kCrashesBeforeFeedbackIsDisplayed;

[James Cook, 2016/09/08 20:15:50]

Aside: I had no idea we had this feature. It seems odd that a single crash in
any tab results in the feedback button being shown for all subsequent crashes --
I would expect it if a particular tab crashed repeatedly. But if that's what we
do on other platforms, I'm OK with it.

[Sidney San Martín, 2016/09/09 01:36:00]

It's what we do on other platforms, but I bet it was supposed to be a
placeholder for better behavior. Think it's worth making a bug to improve it?

+}
+
+}  // namespace
+
 namespace chrome {
 
 // static
 bool SadTab::ShouldShow(base::TerminationStatus status) {
-  return (status == base::TERMINATION_STATUS_ABNORMAL_TERMINATION ||
-          status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED ||
-#if defined(OS_CHROMEOS)
-          status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM ||
-#endif
-          status == base::TERMINATION_STATUS_PROCESS_CRASHED ||
-          status == base::TERMINATION_STATUS_OOM);
+  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;
+    case base::TERMINATION_STATUS_NORMAL_TERMINATION:
+    case base::TERMINATION_STATUS_STILL_RUNNING:
+#if defined(OS_ANDROID)
+    case base::TERMINATION_STATUS_OOM_PROTECTED:
+#endif
+    case base::TERMINATION_STATUS_LAUNCH_FAILED:
+    case base::TERMINATION_STATUS_MAX_ENUM:
+      return false;

[Nico, 2016/09/08 17:12:59]

Nice change!

+  }
+  NOTREACHED();
+  return false;
+}
+
+SadTab::SadTab(content::WebContents* web_contents, SadTabKind kind)

[James Cook, 2016/09/08 20:15:50]

nit: make function order in .cc file match declaration order in .h

[Sidney San Martín, 2016/09/09 01:36:00]

Done. I wonder how hard it would be to make `git cl format` fix things like
this…

[James Cook, 2016/09/09 04:37:08]

Yeah, it's kind of a judgement call for me. If you're touching most of the lines
anyway, I think it's worth doing. If it's longstanding code that just happens to
be out of order, I don't think it's worth the churn in git blame.

+    : 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)
+    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

[James Cook, 2016/09/08 20:15:50]

nit: Indent (unless this is the output of clang-format, in which case leave it
alone)

[Sidney San Martín, 2016/09/09 01:36:00]

It is from clang-format, sadly.

[Nico, 2016/09/09 01:48:42]

(I think if you switch the #endif and the comment line, clang-format might
indent it.)

[Sidney San Martín, 2016/09/09 15:39:44]

Done. I originally avoided doing this… but yeah, it looks better :).

+#endif
+    case chrome::SAD_TAB_KIND_KILLED:

[James Cook, 2016/09/08 20:15:50]

I would keep the LOG(WARNING) about kills. Kills are very interesting --
generally they mean that the renderer sent a malformed IPC to the browser.
Historically these have been due to subtle bugs and/or only occur in the field.
Having something in the logs is really helpful.

[Sidney San Martín, 2016/09/09 01:36:00]

I didn't know that! Thanks, done.

+      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;
+    case chrome::SAD_TAB_KIND_CRASHED:
+    case chrome::SAD_TAB_KIND_KILLED:
+      return IDS_SAD_TAB_MESSAGE;
+  }
+  NOTREACHED();
+  return 0;
+}
+
+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()
+  DCHECK(!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

[James Cook, 2016/09/08 20:15:50]

nit: ditto

+#endif
+    case chrome::SAD_TAB_KIND_KILLED:
+      UMA_SAD_TAB_COUNTER("KillDisplayed");
+      break;
+  }
+
+  RecordEvent(want_feedback_, SadTabEvent::DISPLAYED);
+}
+
+void SadTab::PerformAction(SadTab::Action action) {
+  DCHECK(recorded_paint_);
+  switch (action) {
+    case Action::BUTTON:
+      RecordEvent(want_feedback_, SadTabEvent::BUTTON_CLICKED);
+      if (want_feedback_) {
+        chrome::ShowFeedbackPage(
+            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