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. User should only see this dialog once!

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

Nit: User -> The user

I would probably avoid the "!" at the end too.

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

Done.

[Peter Kasting, 2014/02/05 19:47:08]

Tiny nit: It still just says "User".

[felt, 2014/02/05 19:57:26]

Done.

+class DownloadFeedbackDialogView : public views::DialogDelegate {
+ public:
+  // Used to set prefs::kSafeBrowsingDownloadReportingEnabled.

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

Nit: "Possible values for the kSafeBrowsingDownloadReportingEnabled pref."?

Also, maybe the pref should be called "kSafeBrowsingDownloadReportingStatus"?

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

Done.
I don't mind renaming the pref, but I'd rather rename the pref in another CL.

[Peter Kasting, 2014/02/05 19:47:08]

Not really... there's now just an added "pref" at the end.
Fine with me.

[felt, 2014/02/05 19:57:26]

Done.

+  enum Response {

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

Nit: Since not all of these are responses, what about calling this
DownloadReportingStatus, or DownloadReportingPrefValues?

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

Done.

+    kNeverShown,

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

Nit: kDialogNotYetShown?

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

Done.

+    kUserDisabled,  // Set by Show() as a default.

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

Nit: kDownloadReportingEnabled?  (And similarly with disabled)

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

Done.

+    kUserEnabled,   // Set by Accept().
+    kMaxValue
+  };
+
+  static void Show(gfx::NativeWindow parent_window,
+                   Profile* profile,
+                   const base::Callback<void(bool)>& callback);
+
+ private:
+  DownloadFeedbackDialogView(Profile* profile,
+                             const base::Callback<void(bool)>& callback);
+  virtual ~DownloadFeedbackDialogView();
+
+  // views::DialogDelegate:
+  virtual int GetDefaultDialogButton() const OVERRIDE;
+  virtual base::string16 GetDialogButtonLabel(
+      ui::DialogButton button) const OVERRIDE;
+  virtual bool Cancel() OVERRIDE;
+  virtual bool Accept() OVERRIDE;
+
+  // views::WidgetDelegate:

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

Nit: You don't directly subclass WidgetDelegate.  Put these overrides with the
others, in the appropriate order.  (e.g. if DialogDelegate is a subclass of
WidgetDelegate and does not override WidgetDelegate::Foo(), you should list
Foo() before any of the DialogDelegate overrides, in the order in which it
appears in WidgetDelegate.  If DialogDelegate does override it, place it with
other DialogDelegate functions relative to where it is listed in the
DialogDelegate header.)

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

Done.

+  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;
+
+  Profile* profile_;
+  const base::Callback<void(bool)> 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_