From b828621a6f3f547ed2b88fb2bb9c9d7f232d663f Mon Sep 17 00:00:00 2001 From: Xing Liu Date: Wed, 30 May 2018 21:54:19 +0000 Subject: [PATCH] Download location: Fix issue for location dialog prompt pref. When users choose not to prompt the download location dialog. We still prompt the dialog. This is caused by two reasons: 1. The pref is persisted when user click the download button. Instead, the pref should also be persisted when users click on the checkbox. 2. When exisiting file results in file name conflict, we didn't check the pref. TBR=xingliu@chromium.org (cherry picked from commit f865da82f22a9aed0f4cc1325182f28b766acaf1) Bug: 845311 Change-Id: I8a3bf53cbb9edcd5905003b5739400243977e55a Reviewed-on: https://chromium-review.googlesource.com/1074378 Commit-Queue: Xing Liu Reviewed-by: Min Qin Cr-Original-Commit-Position: refs/heads/master@{#562558} Reviewed-on: https://chromium-review.googlesource.com/1080037 Reviewed-by: Xing Liu Cr-Commit-Position: refs/branch-heads/3440@{#51} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} --- .../browser/download/DownloadLocationDialog.java | 13 ++++++++++++- .../download/chrome_download_manager_delegate.cc | 14 +++++++++++--- .../chrome_download_manager_delegate_unittest.cc | 5 +++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadLocationDialog.java b/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadLocationDialog.java index 12211f2a441e1..258565d41c763 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadLocationDialog.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadLocationDialog.java @@ -10,6 +10,8 @@ import android.view.LayoutInflater; import android.view.View; import android.widget.CheckBox; +import android.widget.CompoundButton; +import android.widget.CompoundButton.OnCheckedChangeListener; import android.widget.Spinner; import android.widget.TextView; @@ -26,7 +28,7 @@ /** * Dialog that is displayed to ask user where they want to download the file. */ -public class DownloadLocationDialog extends ModalDialogView { +public class DownloadLocationDialog extends ModalDialogView implements OnCheckedChangeListener { private DownloadDirectoryAdapter mDirectoryAdapter; private AlertDialogEditText mFileName; @@ -109,6 +111,15 @@ private DownloadLocationDialog(Controller controller, Context context, boolean isInitial = PrefServiceBridge.getInstance().getPromptForDownloadAndroid() == DownloadPromptStatus.SHOW_INITIAL; mDontShowAgain.setChecked(isInitial); + mDontShowAgain.setOnCheckedChangeListener(this); + } + + // CompoundButton.OnCheckedChangeListener implementation. + @Override + public void onCheckedChanged(CompoundButton buttonView, boolean isChecked) { + int newStatus = + isChecked ? DownloadPromptStatus.DONT_SHOW : DownloadPromptStatus.SHOW_PREFERENCE; + PrefServiceBridge.getInstance().setPromptForDownloadAndroid(newStatus); } // Helper methods available to DownloadLocationDialogBridge. diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc index d1d588fc50d1e..9415c5706a330 100644 --- a/chrome/browser/download/chrome_download_manager_delegate.cc +++ b/chrome/browser/download/chrome_download_manager_delegate.cc @@ -957,9 +957,17 @@ void ChromeDownloadManagerDelegate::GenerateUniqueFileNameDone( // with the filename automatically set to be the unique filename. DCHECK_CURRENTLY_ON(content::BrowserThread::UI); if (result == PathValidationResult::SUCCESS) { - ChooseDownloadLocation( - native_window, DownloadLocationDialogType::NAME_CONFLICT, target_path, - base::BindOnce(&OnDownloadLocationDetermined, callback)); + if (download_prefs_ && download_prefs_->PromptForDownload()) { + ChooseDownloadLocation( + native_window, DownloadLocationDialogType::NAME_CONFLICT, target_path, + base::BindOnce(&OnDownloadLocationDetermined, callback)); + return; + } + + // If user chose not to show download location dialog, uses current unique + // target path. + callback.Run(DownloadConfirmationResult::CONTINUE_WITHOUT_CONFIRMATION, + target_path); } else { // If the name generation failed, fail the download. callback.Run(DownloadConfirmationResult::FAILED, base::FilePath()); diff --git a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc index 96d5d43e93002..34d077bc9ef50 100644 --- a/chrome/browser/download/chrome_download_manager_delegate_unittest.cc +++ b/chrome/browser/download/chrome_download_manager_delegate_unittest.cc @@ -53,6 +53,7 @@ #endif #if defined(OS_ANDROID) +#include "chrome/browser/download/download_prompt_status.h" #include "chrome/browser/infobars/infobar_service.h" #include "components/infobars/core/infobar.h" #include "components/infobars/core/infobar_delegate.h" @@ -1195,6 +1196,10 @@ TEST_F(ChromeDownloadManagerDelegateTest, scoped_list.InitAndEnableFeature(features::kDownloadsLocationChange); EXPECT_TRUE(base::FeatureList::IsEnabled(features::kDownloadsLocationChange)); + profile()->GetTestingPrefService()->SetInteger( + prefs::kPromptForDownloadAndroid, + static_cast(DownloadPromptStatus::SHOW_PREFERENCE)); + enum class WebContents { AVAILABLE, NONE }; enum class ExpectPath { FULL, EMPTY }; struct {