Implement new dangerous download reporting dialog for UNCOMMON_DOWNLOAD, in Views

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

+// 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/ui/views/download/download_feedback_dialog_view.h"
+
+#include "chrome/browser/prefs/pref_service_syncable.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/views/constrained_window_views.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/views/controls/message_box_view.h"
+#include "ui/views/widget/widget.h"
+
+// static
+void DownloadFeedbackDialogView::Show(
+    gfx::NativeWindow parent_window,
+    Profile* profile,
+    const base::Callback<void(bool)>& callback) {
+  // Enforce the constraint that this dialog should only be shown if it hasn't

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

Nit: DCHECKs don't really "enforce" anything, so I'd drop the "Enforce the
constraint that".

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

Done.

+  // been shown before.
+  DCHECK(profile->GetPrefs()->
+      GetInteger(prefs::kSafeBrowsingDownloadReportingEnabled) == kNeverShown);
+  DownloadFeedbackDialogView* window = new DownloadFeedbackDialogView(

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

Nit: Breaking after '=' instead of '(' seems a little nicer

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

Done.

+      profile, callback);
+  CreateBrowserModalDialogViews(window, parent_window)->Show();
+}
+
+DownloadFeedbackDialogView::DownloadFeedbackDialogView(
+    Profile* profile,
+    const base::Callback<void(bool)>& callback)
+    : profile_(profile),
+      callback_(callback),
+      explanation_box_view_(NULL) {
+  title_text_ = l10n_util::GetStringUTF16(IDS_FEEDBACK_SERVICE_DIALOG_TITLE);

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

Nit: All these strings can be set in the initializer list.

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

Done.

+  base::string16 explanation_text = l10n_util::GetStringUTF16(

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

Nit: You can just inline this into the next statement; doing this will allow you
to set |explanation_box_view_| in the initializer list.

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

Done.

+      IDS_FEEDBACK_SERVICE_DIALOG_EXPLANATION);
+  explanation_box_view_ = new views::MessageBoxView(
+      views::MessageBoxView::InitParams(explanation_text));
+  ok_button_text_ =
+      l10n_util::GetStringUTF16(IDS_FEEDBACK_SERVICE_DIALOG_OK_BUTTON_LABEL);
+  cancel_button_text_ = l10n_util::GetStringUTF16(
+      IDS_FEEDBACK_SERVICE_DIALOG_CANCEL_BUTTON_LABEL);
+}
+
+DownloadFeedbackDialogView::~DownloadFeedbackDialogView() {}
+
+int DownloadFeedbackDialogView::GetDefaultDialogButton() const {
+  return ui::DIALOG_BUTTON_CANCEL;
+}
+
+base::string16 DownloadFeedbackDialogView::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  return (button == ui::DIALOG_BUTTON_OK) ?
+      ok_button_text_ : cancel_button_text_;
+}
+
+bool DownloadFeedbackDialogView::Cancel() {
+  profile_->GetPrefs()->
+      SetInteger(prefs::kSafeBrowsingDownloadReportingEnabled, kUserDisabled);
+  callback_.Run(false);
+  return true;
+}
+
+bool DownloadFeedbackDialogView::Accept() {
+  profile_->GetPrefs()->
+      SetInteger(prefs::kSafeBrowsingDownloadReportingEnabled, kUserEnabled);
+  callback_.Run(true);
+  return true;
+}
+
+ui::ModalType DownloadFeedbackDialogView::GetModalType() const {
+  return ui::MODAL_TYPE_SYSTEM;

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

Why does this need to be system-modal?

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

I was copying how similar dialogs seem to work. I admit to not entirely
understanding the types of modality. What is your recommendation?

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

In general, we want to be as non-modal as possible.  There are several
possibilities:
(1) If the dialog is totally optional, we could make it dismiss on focus loss,
in which case it could be entirely non-modal.  If we did this, we'd presumably
want to show the dialog again in the future since the user didn't actually
choose an answer.
(2) If the dialog needs to be modal to the current page, we could just do that
without making it system-modal.  In this case, we'd still need to worry about
what should happen if a user gets the dialog in one tab, then while that dialog
is still open also has a dangerous download happen in a second tab.  Perhaps we
should show the dialog on both tabs but dismiss both when the user answers
either.
(3) If we make the dialog system-modal we avoid these sorts of complications,
but we block the user from doing anything else, which isn't very user-friendly. 
It can feel frustrating to have Chrome demanding an answer to this question
before I can do even unrelated tasks.

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

This dialog doesn't pertain to any specific tab. It is related to the window
that has the download shelf on it. So, I think that would mean it would have to
be modal to that window (but not necessarily the entire browser?). Is that #3
(system-modal), or some other modality?

[Peter Kasting, 2014/02/05 02:10:54]

I think that's MODAL_TYPE_WINDOW?  sky might know for sure.

[felt, 2014/02/05 18:41:32]

sky agrees it should be MODAL_TYPE_WINDOW, so done.

+}
+
+base::string16 DownloadFeedbackDialogView::GetWindowTitle() const {
+  return title_text_;
+}
+
+void DownloadFeedbackDialogView::DeleteDelegate() {
+  delete this;
+}
+
+views::Widget* DownloadFeedbackDialogView::GetWidget() {
+  return explanation_box_view_->GetWidget();
+}
+
+const views::Widget* DownloadFeedbackDialogView::GetWidget() const {
+  return explanation_box_view_->GetWidget();
+}
+
+views::View* DownloadFeedbackDialogView::GetContentsView() {
+  return explanation_box_view_;
+}