Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(569)

Unified Diff: chrome/browser/ui/sad_tab.cc

Issue 2261793002: Bring the feedback button to the Mac sad tab (Closed) Base URL: https://chromium.googlesource.com/chromium/src@master
Patch Set: Ditch a pointless |explicit|, add a comment on SadTabView ownership. Created 4 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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..0a1c0ecc9f230e74ef0c296b0eaf38765af55a5f 100644
--- a/chrome/browser/ui/sad_tab.cc
+++ b/chrome/browser/ui/sad_tab.cc
@@ -2,20 +2,205 @@
// 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 concate
Sidney San Martín 2016/09/09 01:36:00 The macro deals with creating a block with a stati
James Cook 2016/09/09 04:37:08 This is fine. However, please change NAME to |hist
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 D
Sidney San Martín 2016/09/09 01:36:00 These histograms are for other events (BUTTON_CLIC
James Cook 2016/09/09 04:37:08 Hrm, I missed that distinction when reading this c
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
Sidney San Martín 2016/09/09 01:36:00 It's what we do on other platforms, but I bet it w
+}
+
+} // 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;
+ 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 declara
Sidney San Martín 2016/09/09 01:36:00 Done. I wonder how hard it would be to make `git c
James Cook 2016/09/09 04:37:08 Yeah, it's kind of a judgement call for me. If you
+ : 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-fo
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
Sidney San Martín 2016/09/09 15:39:44 Done. I originally avoided doing this… but yeah, i
+#endif
+ case chrome::SAD_TAB_KIND_KILLED:
James Cook 2016/09/08 20:15:50 I would keep the LOG(WARNING) about kills. Kills a
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)
- status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED_BY_OOM ||
+ 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
- status == base::TERMINATION_STATUS_PROCESS_CRASHED ||
- status == base::TERMINATION_STATUS_OOM);
+ 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

Powered by Google App Engine
This is Rietveld 408576698