Implement new dangerous download reporting dialog for UNCOMMON_DOWNLOAD, in Views

chrome/browser/ui/views/download/download_feedback_dialog_view.h

+// 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.
+
+#ifndef CHROME_BROWSER_UI_VIEWS_DOWNLOAD_DOWNLOAD_FEEDBACK_DIALOG_VIEW_H_
+#define CHROME_BROWSER_UI_VIEWS_DOWNLOAD_DOWNLOAD_FEEDBACK_DIALOG_VIEW_H_
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/pref_names.h"
+#include "ui/views/window/dialog_delegate.h"
+
+namespace views {
+class MessageBoxView;
+}
+
+class Profile;
+
+// Asks the user whether s/he wants to participate in the Safe Browsing
+// download feedback program. Shown only for downloads marked DANGEROUS_HOST
+// or UNCOMMON_DOWNLOAD. The user should only see this dialog once.
+class DownloadFeedbackDialogView : public views::DialogDelegate {
+ public:
+  // Possible values for prefs::kSafeBrowsingDownloadReportingEnabled pref.
+  enum DownloadReportingStatus {
+    kDialogNotYetShown,
+    kDownloadReportingDisabled,  // Set by Cancel().
+    kDownloadReportingEnabled,   // Set by Accept().
+    kDialogDecisionPending,      // When a dialog is open in another window.

[mattm, 2014/02/06 23:27:26]

A little odd since the pref is never actually set to this value. Could just note
that in the comment, but it seems like it would be cleaner to modify the way
PossiblySubmitDownloadToFeedbackService handles this to avoid it.

Actually I wonder if it would be cleaner to move all that logic into
download_feedback_dialog_view, and replace the Show method with MaybeShow ?
WDYT?

[felt, 2014/02/06 23:31:57]

There are a few ways to do this:
1. Have the different enum states to differentiate
2. Add a boolean to PossiblySubmitDownloadToFeedbackService
3. Refactor the logic into the dialog's Show method

They all seem equivalent to me. Is it busywork to change it, or is #3 really
that preferable?

[mattm, 2014/02/06 23:44:36]

#3 does seem preferable to me from a code readability/maintainability
perspective, as DownloadItemView is already a pretty big class, moving the
complexity into DownloadFeedbackDialogView seems worthwhile.


It also seems like a more logical place to have it.(Eg, DownloadItemView
shouldn't have to care how the FeedbackView implements this, like if we decide
to make it show the dialog in any window that this happens instead of only the
first, if the logic was in the FeedbackView then DownloadItemView wouldn't even
have to change.)

+    kMaxValue
+  };
+
+  static void Show(
+      gfx::NativeWindow parent_window,
+      Profile* profile,
+      const base::Callback<void(DownloadReportingStatus)>& callback);
+
+ private:
+  DownloadFeedbackDialogView(
+      Profile* profile,
+      const base::Callback<void(DownloadReportingStatus)>& callback);
+  virtual ~DownloadFeedbackDialogView();
+
+  // views::DialogDelegate:
+  virtual ui::ModalType GetModalType() const OVERRIDE;
+  virtual base::string16 GetWindowTitle() const OVERRIDE;
+  virtual void DeleteDelegate() OVERRIDE;
+  virtual views::Widget* GetWidget() OVERRIDE;
+  virtual const views::Widget* GetWidget() const OVERRIDE;
+  virtual views::View* GetContentsView() OVERRIDE;
+  virtual int GetDefaultDialogButton() const OVERRIDE;
+  virtual base::string16 GetDialogButtonLabel(
+      ui::DialogButton button) const OVERRIDE;
+  virtual bool Cancel() OVERRIDE;
+  virtual bool Accept() OVERRIDE;
+
+  Profile* profile_;
+  const base::Callback<void(DownloadReportingStatus)> callback_;
+  views::MessageBoxView* explanation_box_view_;
+  base::string16 title_text_;
+  base::string16 ok_button_text_;
+  base::string16 cancel_button_text_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadFeedbackDialogView);
+};
+
+#endif  // CHROME_BROWSER_UI_VIEWS_DOWNLOAD_DOWNLOAD_FEEDBACK_DIALOG_VIEW_H_