From ac569864d83871c0182461e6431366d1785ef26f Mon Sep 17 00:00:00 2001 From: "felt@chromium.org" Date: Fri, 7 Feb 2014 21:42:47 +0000 Subject: [PATCH] Implement new dangerous download reporting dialog for UNCOMMON_DOWNLOAD, in Views Adds a new dialog, DownloadFeedbackDialogView, to ask people whether they want to participate in the upload program. Currently only hooked up to UNCOMMON_DOWNLOAD. Still TODO: - need to add dialog to DANGEROUS_HOST as well - need to add new histogram for dialog once DANGEROUS_HOST is in place -------------- How to test [Win & CrOS only]: 1. Visit http://testsafebrowsing.appspot.com/, click on #5, then click discard, then click any choice in the dialog 2. If you repeat #1, you shouldn't see the dialog again unless you delete the preferences file in the User Data directory 3. If you do #1 in two separate windows, only one of them should trigger the dialog BUG=312533 TBR=asanka@chromium.org Review URL: https://codereview.chromium.org/147593002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@249796 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/app/generated_resources.grd | 14 +++ .../download/download_shelf_context_menu.cc | 2 - .../download/download_feedback_dialog_view.cc | 132 +++++++++++++++++++++ .../views/download/download_feedback_dialog_view.h | 68 +++++++++++ .../ui/views/download/download_item_view.cc | 50 ++++++-- .../browser/ui/views/download/download_item_view.h | 14 ++- .../ui/views/download/download_shelf_view.h | 3 + chrome/chrome_browser_ui.gypi | 2 + 8 files changed, 269 insertions(+), 16 deletions(-) create mode 100644 chrome/browser/ui/views/download/download_feedback_dialog_view.cc create mode 100644 chrome/browser/ui/views/download/download_feedback_dialog_view.h diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 66b83923de26..9f7ed74c5133 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -2152,6 +2152,20 @@ Even if you have downloaded files from this website before, the website might ha No + + + Help make Chrome better + + + You can help make Chrome safer and easier to use by sending suspicious downloaded files to Google. + + + Yes, I want to help + + + No, thanks + + Add shortcut? diff --git a/chrome/browser/download/download_shelf_context_menu.cc b/chrome/browser/download/download_shelf_context_menu.cc index bd98ff221d29..2d772f47ea01 100644 --- a/chrome/browser/download/download_shelf_context_menu.cc +++ b/chrome/browser/download/download_shelf_context_menu.cc @@ -350,8 +350,6 @@ ui::SimpleMenuModel* DownloadShelfContextMenu::GetMaybeMaliciousMenuModel() { maybe_malicious_download_menu_model_.reset(new ui::SimpleMenuModel(this)); maybe_malicious_download_menu_model_->AddItemWithStringId( - DISCARD, IDS_DOWNLOAD_MENU_DISCARD); - maybe_malicious_download_menu_model_->AddItemWithStringId( KEEP, IDS_DOWNLOAD_MENU_KEEP); maybe_malicious_download_menu_model_->AddSeparator(ui::NORMAL_SEPARATOR); maybe_malicious_download_menu_model_->AddItemWithStringId( diff --git a/chrome/browser/ui/views/download/download_feedback_dialog_view.cc b/chrome/browser/ui/views/download/download_feedback_dialog_view.cc new file mode 100644 index 000000000000..00774ad9b4c2 --- /dev/null +++ b/chrome/browser/ui/views/download/download_feedback_dialog_view.cc @@ -0,0 +1,132 @@ +// 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 "base/prefs/pref_service.h" +#include "base/supports_user_data.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" + +namespace { + +const void* kDialogStatusKey = &kDialogStatusKey; + +class DialogStatusData : public base::SupportsUserData::Data { + public: + DialogStatusData() : currently_shown_(false) {} + virtual ~DialogStatusData() {} + bool currently_shown() const { return currently_shown_; } + void set_currently_shown(bool shown) { currently_shown_ = shown; } + private: + bool currently_shown_; +}; + +} // namespace + +// static +void DownloadFeedbackDialogView::Show( + gfx::NativeWindow parent_window, + Profile* profile, + const base::Callback& callback) { + // This dialog should only be shown if it hasn't been shown before. + DCHECK(profile->GetPrefs()->GetInteger( + prefs::kSafeBrowsingDownloadReportingEnabled) == kDialogNotYetShown); + + // Only one dialog should be shown at a time, so check to see if another one + // is open. If another one is open, treat this parallel call as if reporting + // is disabled (to be conservative). + DialogStatusData* data = + static_cast(profile->GetUserData(kDialogStatusKey)); + if (data == NULL) { + data = new DialogStatusData(); + profile->SetUserData(kDialogStatusKey, data); + } + if (data->currently_shown() == false) { + data->set_currently_shown(true); + DownloadFeedbackDialogView* window = + new DownloadFeedbackDialogView(profile, callback); + CreateBrowserModalDialogViews(window, parent_window)->Show(); + } else { + callback.Run(kDownloadReportingDisabled); + } +} + +void DownloadFeedbackDialogView::ReleaseDialogStatusHold() { + DialogStatusData* data = + static_cast(profile_->GetUserData(kDialogStatusKey)); + DCHECK(data); + data->set_currently_shown(false); +} + +DownloadFeedbackDialogView::DownloadFeedbackDialogView( + Profile* profile, + const base::Callback& callback) + : profile_(profile), + callback_(callback), + explanation_box_view_(new views::MessageBoxView( + views::MessageBoxView::InitParams(l10n_util::GetStringUTF16( + IDS_FEEDBACK_SERVICE_DIALOG_EXPLANATION)))), + title_text_(l10n_util::GetStringUTF16(IDS_FEEDBACK_SERVICE_DIALOG_TITLE)), + 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, kDownloadReportingDisabled); + ReleaseDialogStatusHold(); + callback_.Run(kDownloadReportingDisabled); + return true; +} + +bool DownloadFeedbackDialogView::Accept() { + profile_->GetPrefs()->SetInteger( + prefs::kSafeBrowsingDownloadReportingEnabled, kDownloadReportingEnabled); + ReleaseDialogStatusHold(); + callback_.Run(kDownloadReportingEnabled); + return true; +} + +ui::ModalType DownloadFeedbackDialogView::GetModalType() const { + return ui::MODAL_TYPE_WINDOW; +} + +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_; +} diff --git a/chrome/browser/ui/views/download/download_feedback_dialog_view.h b/chrome/browser/ui/views/download/download_feedback_dialog_view.h new file mode 100644 index 000000000000..6037512ee20e --- /dev/null +++ b/chrome/browser/ui/views/download/download_feedback_dialog_view.h @@ -0,0 +1,68 @@ +// 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/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(). + kMaxValue + }; + + static void Show( + gfx::NativeWindow parent_window, + Profile* profile, + const base::Callback& callback); + + private: + DownloadFeedbackDialogView( + Profile* profile, + const base::Callback& callback); + virtual ~DownloadFeedbackDialogView(); + + void ReleaseDialogStatusHold(); + + // 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 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_ diff --git a/chrome/browser/ui/views/download/download_item_view.cc b/chrome/browser/ui/views/download/download_item_view.cc index 65f6495f0b3f..7a3d9714d357 100644 --- a/chrome/browser/ui/views/download/download_item_view.cc +++ b/chrome/browser/ui/views/download/download_item_view.cc @@ -13,6 +13,7 @@ #include "base/i18n/break_iterator.h" #include "base/i18n/rtl.h" #include "base/metrics/histogram.h" +#include "base/prefs/pref_service.h" #include "base/strings/string_util.h" #include "base/strings/stringprintf.h" #include "base/strings/sys_string_conversions.h" @@ -22,12 +23,15 @@ #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/profiles/profile.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 "chrome/browser/ui/views/frame/browser_view.h" #include "content/public/browser/download_danger_type.h" #include "grit/generated_resources.h" #include "grit/theme_resources.h" @@ -550,9 +554,33 @@ 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() && + !shelf_->browser()->profile()->IsOffTheRecord()) { + DownloadFeedbackDialogView::DownloadReportingStatus pref_value = + static_cast( + shelf_->browser()->profile()->GetPrefs()->GetInteger( + prefs::kSafeBrowsingDownloadReportingEnabled)); + switch (pref_value) { + case DownloadFeedbackDialogView::kDialogNotYetShown: + DownloadFeedbackDialogView::Show( + shelf_->get_parent()->GetNativeWindow(), + shelf_->browser()->profile(), + base::Bind( + &DownloadItemView::PossiblySubmitDownloadToFeedbackService, + weak_ptr_factory_.GetWeakPtr())); + break; + + case DownloadFeedbackDialogView::kDownloadReportingEnabled: + case DownloadFeedbackDialogView::kDownloadReportingDisabled: + PossiblySubmitDownloadToFeedbackService(pref_value); + break; + + case DownloadFeedbackDialogView::kMaxValue: + NOTREACHED(); + } + return; + } download()->Remove(); } @@ -890,7 +918,7 @@ void DownloadItemView::OpenDownload() { UpdateAccessibleName(); } -bool DownloadItemView::BeginDownloadFeedback() { +bool DownloadItemView::SubmitDownloadToFeedbackService() { #if defined(FULL_SAFE_BROWSING) SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service(); if (!sb_service) @@ -899,11 +927,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'. @@ -914,6 +937,15 @@ bool DownloadItemView::BeginDownloadFeedback() { #endif } +void DownloadItemView::PossiblySubmitDownloadToFeedbackService( + DownloadFeedbackDialogView::DownloadReportingStatus status) { + if (status != DownloadFeedbackDialogView::kDownloadReportingEnabled || + !SubmitDownloadToFeedbackService()) { + download()->Remove(); + } + // WARNING: 'this' is deleted at this point. Don't access 'this'. +} + void DownloadItemView::LoadIcon() { IconManager* im = g_browser_process->icon_manager(); last_download_item_path_ = download()->GetTargetFilePath(); @@ -1141,8 +1173,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); diff --git a/chrome/browser/ui/views/download/download_item_view.h b/chrome/browser/ui/views/download/download_item_view.h index 4b4a9fda65a5..01cd11d11636 100644 --- a/chrome/browser/ui/views/download/download_item_view.h +++ b/chrome/browser/ui/views/download/download_item_view.h @@ -27,6 +27,7 @@ #include "base/timer/timer.h" #include "chrome/browser/download/download_item_model.h" #include "chrome/browser/icon_manager.h" +#include "chrome/browser/ui/views/download/download_feedback_dialog_view.h" #include "content/public/browser/download_item.h" #include "content/public/browser/download_manager.h" #include "ui/gfx/animation/animation_delegate.h" @@ -146,10 +147,15 @@ class DownloadItemView : public views::ButtonListener, void OpenDownload(); - // Submit the downloaded file to the safebrowsing download feedback service. - // If true is returned, the DownloadItem and |this| have been deleted. If - // false is returned, nothing has changed. - bool BeginDownloadFeedback(); + // Submits the downloaded file to the safebrowsing download feedback service. + // Returns whether submission was successful. On successful submission, + // |this| and the DownloadItem will have been deleted. + bool SubmitDownloadToFeedbackService(); + + // If the user has enabled uploading, calls SubmitDownloadToFeedbackService. + // Otherwise, it simply removes the DownloadItem without uploading. + void PossiblySubmitDownloadToFeedbackService( + DownloadFeedbackDialogView::DownloadReportingStatus status); void LoadIcon(); void LoadIconIfItemPathChanged(); diff --git a/chrome/browser/ui/views/download/download_shelf_view.h b/chrome/browser/ui/views/download/download_shelf_view.h index f5362679f1e8..e7ce06cbebda 100644 --- a/chrome/browser/ui/views/download/download_shelf_view.h +++ b/chrome/browser/ui/views/download/download_shelf_view.h @@ -56,6 +56,9 @@ class DownloadShelfView : public views::AccessiblePaneView, // i.e. the |browser_|. content::PageNavigator* GetNavigator(); + // Returns the parent_. + BrowserView* get_parent() { return parent_; } + // Implementation of View. virtual gfx::Size GetPreferredSize() OVERRIDE; virtual void Layout() OVERRIDE; diff --git a/chrome/chrome_browser_ui.gypi b/chrome/chrome_browser_ui.gypi index abc8c4e954e3..c34340c6c7c1 100644 --- a/chrome/chrome_browser_ui.gypi +++ b/chrome/chrome_browser_ui.gypi @@ -1809,6 +1809,8 @@ 'browser/ui/views/detachable_toolbar_view.cc', 'browser/ui/views/detachable_toolbar_view.h', 'browser/ui/views/download/download_danger_prompt_views.cc', + 'browser/ui/views/download/download_feedback_dialog_view.cc', + 'browser/ui/views/download/download_feedback_dialog_view.h', 'browser/ui/views/download/download_in_progress_dialog_view.cc', 'browser/ui/views/download/download_in_progress_dialog_view.h', 'browser/ui/views/download/download_item_view.cc', -- 2.11.4.GIT