Implement new dangerous download reporting dialog for UNCOMMON_DOWNLOAD, in Views

chrome/browser/ui/views/download/download_item_view.cc

Index: chrome/browser/ui/views/download/download_item_view.cc
diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc
index 65f6495f0b3f12fd4d7b9f051b7fa4f7844a8ecb..cc38fb24ee7534e12e68835c0b9bf8f273b419fc 100644
--- a/chrome/browser/ui/views/download/download_item_view.cc
+++ b/chrome/browser/ui/views/download/download_item_view.cc
@@ -22,10 +22,12 @@
 #include "chrome/browser/download/download_item_model.h"
 #include "chrome/browser/download/download_stats.h"
 #include "chrome/browser/download/drag_download_item.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/safe_browsing/download_feedback_service.h"
 #include "chrome/browser/safe_browsing/download_protection_service.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/themes/theme_properties.h"
+#include "chrome/browser/ui/views/download/download_feedback_dialog_view.h"
 #include "chrome/browser/ui/views/download/download_shelf_context_menu_view.h"
 #include "chrome/browser/ui/views/download/download_shelf_view.h"
 #include "content/public/browser/download_danger_type.h"
@@ -550,9 +552,34 @@ void DownloadItemView::ButtonPressed(views::Button* sender,
     shelf_->RemoveDownloadView(this);
     return;
   }
-  if (model_.ShouldAllowDownloadFeedback() && BeginDownloadFeedback())
-    return;
   UMA_HISTOGRAM_LONG_TIMES("clickjacking.discard_download", warning_duration);
+  if (model_.ShouldAllowDownloadFeedback()) {
+    const int pref_value = shelf_->browser()->profile()->GetPrefs()->GetInteger(
+        prefs::kSafeBrowsingDownloadReportingEnabled);
+    switch (static_cast<DownloadFeedbackDialogView::Response>(pref_value)) {
+      case DownloadFeedbackDialogView::kNeverShown: {

[Peter Kasting, 2014/02/04 21:56:33]

Nit: {} not necessary on any of these cases.

[felt, 2014/02/04 23:37:22]

Done, but doesn't the style guide say either with or without {} is fine?

[Peter Kasting, 2014/02/05 00:07:36]

Yes, either way is acceptable by the style guide.  That's why I said "not
necessary" rather than "banned, do not use".

+        DownloadFeedbackDialogView::Show(
+            shelf_->GetParentWindowView()->GetNativeWindow(),
+            shelf_->browser()->profile(),
+            base::Bind(&DownloadItemView::BeginDownloadFeedbackWrapper,
+                       weak_ptr_factory_.GetWeakPtr()));
+        break;
+      }
+      case DownloadFeedbackDialogView::kUserEnabled: {
+        BeginDownloadFeedbackWrapper(true);
+        break;
+      }
+      case DownloadFeedbackDialogView::kUserDisabled: {
+        BeginDownloadFeedbackWrapper(false);
+        break;
+      }
+      case DownloadFeedbackDialogView::kMaxValue:

[Peter Kasting, 2014/02/04 21:56:33]

Having this value buys you nothing over just having the "default:" case below. 
Eliminate this line and eliminate the value from the original enum.

[felt, 2014/02/04 23:37:22]

Switching to just "default", but I need kMaxValue so that it can be histogrammed
in a subsequent CL.

[Peter Kasting, 2014/02/05 00:07:36]

In that case, I would eliminate "default" and only have the kMaxValue case. 
Then the compiler should hopefully complain if someone introduces another value
without updating this switch statement.

[felt, 2014/02/05 02:08:53]

Done.

+      default: {
+        NOTREACHED();
+      }
+    }
+    return;
+  }
   download()->Remove();
 }
 
@@ -890,6 +917,13 @@ void DownloadItemView::OpenDownload() {
   UpdateAccessibleName();
 }
 
+void DownloadItemView::BeginDownloadFeedbackWrapper(bool user_enabled) {

[Peter Kasting, 2014/02/04 21:56:33]

Nit: If you make the argument to this be the enum type instead of a bool, then
not only is the meaning of the argument slightly clearer, but you can slightly
simplify the switch statement above by doing:

  case DownloadFeedbackDialogView::kUserEnabled:
  case DownloadFeedbackDialogView::kUserDisabled:
    BeginDownloadFeedbackWrapper(pref_value);
    break;

[felt, 2014/02/05 02:08:53]

Done.

+  if (user_enabled && BeginDownloadFeedback())
+    return;
+  download()->Remove();
+  // WARNING: 'this' is deleted at this point. Don't access 'this'.

[Peter Kasting, 2014/02/04 21:56:33]

Nit: Shorter:

  if (!user_enabled || !BeginDownloadFeedback())
    download()->Remove();
  // WARNING: |this| is now deleted!

(This is also slightly clearer that BeginDownloadFeedback() will have deleted
|this| if it returns true.)

[felt, 2014/02/04 23:37:22]

Done.

+}
+
 bool DownloadItemView::BeginDownloadFeedback() {
 #if defined(FULL_SAFE_BROWSING)
   SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service();
@@ -899,11 +933,6 @@ bool DownloadItemView::BeginDownloadFeedback() {
       sb_service->download_protection_service();
   if (!download_protection_service)
     return false;
-  base::TimeDelta warning_duration = base::TimeDelta();
-  if (!time_download_warning_shown_.is_null())
-    warning_duration = base::Time::Now() - time_download_warning_shown_;
-  UMA_HISTOGRAM_LONG_TIMES("clickjacking.report_and_discard_download",
-                           warning_duration);
   download_protection_service->feedback_service()->BeginFeedbackForDownload(
       download());
   // WARNING: we are deleted at this point.  Don't access 'this'.
@@ -1141,8 +1170,6 @@ void DownloadItemView::ShowWarningDialog() {
   }
   int discard_button_message = model_.IsMalicious() ?
       IDS_DISMISS_DOWNLOAD : IDS_DISCARD_DOWNLOAD;
-  if (!model_.IsMalicious() && model_.ShouldAllowDownloadFeedback())
-    discard_button_message = IDS_REPORT_AND_DISCARD_DOWNLOAD;
   discard_button_ = new views::LabelButton(
       this, l10n_util::GetStringUTF16(discard_button_message));
   discard_button_->SetStyle(views::Button::STYLE_BUTTON);