SRT Bubble

chrome/browser/safe_browsing/srt_global_error.cc

Index: chrome/browser/safe_browsing/srt_global_error.cc
diff --git a/chrome/browser/safe_browsing/srt_global_error.cc b/chrome/browser/safe_browsing/srt_global_error.cc
new file mode 100644
index 0000000000000000000000000000000000000000..32fc411101d0fd260b7d5b7e334ef313c7b45869
--- /dev/null
+++ b/chrome/browser/safe_browsing/srt_global_error.cc
@@ -0,0 +1,122 @@
+// Copyright 2014 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 "chrome/browser/safe_browsing/srt_global_error.h"
+
+#include "base/callback.h"
+#include "base/metrics/histogram.h"
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/global_error/global_error_service.h"
+#include "grit/chromium_strings.h"
+#include "grit/generated_resources.h"
+#include "grit/google_chrome_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace {
+// The download link of the Software Removal Tool.
+const char kSRTDownloadURL[] = "https://www.google.com/chrome/srt/";

[robertshield, 2014/09/24 13:02:12]

should some of this file be #if defined(GOOGLE_CHROME_BUILD)? Not sure we should
have google.com URLs in Chromium.

[MAD, 2014/09/24 20:40:35]

Hum, but then, which value do we put here? Actually, it's the whole bubble that
should only show up in official builds... I'll put a TODO for now...

+
+// Enum values for the SRTPrompt histogram. Don't change order, always add
+// to the end, and adjust SRT_PROMPT_MAX.
+enum SRTPromptHistogramValue {
+  SRT_PROMPT_SHOWN = 0,
+  SRT_PROMPT_ACCEPTED = 1,
+  SRT_PROMPT_DENIED = 2,
+  SRT_PROMPT_MAX = 3,

[gab, 2014/09/24 16:22:37]

Do not explicitly label SRT_PROMPT_MAX (let it be bumped up naturally as things
are inserted before it)

[MAD, 2014/09/24 20:40:35]

Done.

+};
+
+void RecordSRTPromptHistogram(SRTPromptHistogramValue value) {
+  UMA_HISTOGRAM_ENUMERATION("SRTPrompt", value, SRT_PROMPT_MAX);
+}
+
+}  // namespace
+
+// SRTGlobalError ------------------------------------------------------------
+
+SRTGlobalError::SRTGlobalError(GlobalErrorService* global_error_service)
+    : global_error_service_(global_error_service) {
+  DCHECK(global_error_service_);
+}
+
+SRTGlobalError::~SRTGlobalError() {
+}
+
+// static
+bool SRTGlobalError::IsSupportedOnPlatform() {

[gab, 2014/09/24 16:22:37]

Are there any callers of this method? Shouldn't this entire file be compiled out
on non-win (or at least stubbed-out, e.g.,
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
)?

[MAD, 2014/09/24 20:40:36]

Good point, this was a leftover from the profile reset thingy... 

+// The Software Removal Tool only runs on windows.
+#if defined(OS_WIN)
+  return true;
+#else
+  return false;
+#endif
+}
+
+bool SRTGlobalError::HasMenuItem() {
+  return true;
+}
+
+int SRTGlobalError::MenuItemCommandID() {
+  return IDC_SHOW_SRT_BUBBLE;
+}
+
+base::string16 SRTGlobalError::MenuItemLabel() {
+  return l10n_util::GetStringUTF16(IDS_SRT_MENU_ITEM);
+}
+
+void SRTGlobalError::ExecuteMenuItem(Browser* browser) {
+  ShowBubbleView(browser);

[engedy, 2014/09/24 13:37:15]

Showing more than one instance of the bubble is unusual for a GlobalError.
However, by looking at the code, I could not find anything that could go
terribly wrong here. Just wanted to make sure that you are aware of this and
double-checked.

[MAD, 2014/09/24 20:40:36]

Since the bubble is closed when it loses focus, this shouldn't happen. Added a
DCHECK.

+}
+
+void SRTGlobalError::ShowBubbleView(Browser* browser) {
+  RecordSRTPromptHistogram(SRT_PROMPT_SHOWN);
+  GlobalErrorWithStandardBubble::ShowBubbleView(browser);
+}
+
+base::string16 SRTGlobalError::GetBubbleViewTitle() {
+  return l10n_util::GetStringUTF16(IDS_SRT_BUBBLE_TITLE);
+}
+
+std::vector<base::string16> SRTGlobalError::GetBubbleViewMessages() {
+  std::vector<base::string16> messages;
+  messages.push_back(l10n_util::GetStringUTF16(IDS_SRT_BUBBLE_TEXT));
+  return messages;
+}
+
+base::string16 SRTGlobalError::GetBubbleViewAcceptButtonLabel() {
+  return l10n_util::GetStringUTF16(IDS_SRT_BUBBLE_DOWNLOAD_BUTTON_TEXT);
+}
+
+base::string16 SRTGlobalError::GetBubbleViewCancelButtonLabel() {
+  return l10n_util::GetStringUTF16(IDS_NO_THANKS);
+}
+
+void SRTGlobalError::OnBubbleViewDidClose(Browser* browser) {
+}
+
+void SRTGlobalError::BubbleViewAcceptButtonPressed(Browser* browser) {
+  RecordSRTPromptHistogram(SRT_PROMPT_ACCEPTED);
+  browser->OpenURL(content::OpenURLParams(GURL(kSRTDownloadURL),
+                                          content::Referrer(),
+                                          NEW_FOREGROUND_TAB,
+                                          ui::PAGE_TRANSITION_LINK,
+                                          false));
+  DismissGlobalError();
+}
+
+void SRTGlobalError::BubbleViewCancelButtonPressed(Browser* browser) {
+  RecordSRTPromptHistogram(SRT_PROMPT_DENIED);
+  DismissGlobalError();
+}
+
+void SRTGlobalError::DismissGlobalError() {
+  global_error_service_->RemoveGlobalError(this);
+
+  // Even though OnBubbleViewDidClose will be called asynchronously when the
+  // bubble actually gets closed by RemoveGlobalError, we can safely suicide

[robertshield, 2014/09/24 13:02:12]

totally ignorable grammar nit: s/suicide/commit suicide/

[MAD, 2014/09/24 20:40:35]

Done.

+  // here since the bubble holds on a weak pointer of the global error. This
+  // way, if the destruction of the bubble UI ever gets changed to be
+  // synchronous, we'll still be OK.

[gab, 2014/09/24 16:22:36]

This comment should not be necessary. All your code ever does is "create the
error and give to the service" (the service's API then claims to grab ownership
of the error); and here it calls RemoveGlobalError() which's API claims it's
giving ownership back to you.

How the service handles that in between is none of this code's business, if the
service claims it gives ownership back to us, it's up to it to make sure it
didn't give a strong dependency to anybody; thus you can just delete this
pointer without having to justify (justifying it here only binds you to
implementation details of the service which this code should have nothing to do
with).

[MAD, 2014/09/24 20:40:35]

Actually, this comment was added to explain the state of the code to another
code reviews, but since the code now changed to not rely on the
global_error_service_ being NULL or not, the comment isn't necessary anymore...
Thanks! :-)
On 2014/09/24 16:22:36, gab wrote:

+  delete this;
+}