Skip to content

Commit

Permalink
Download location: Fix issue for location dialog prompt pref.
Browse files Browse the repository at this point in the history
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 f865da8)

Bug: 845311
Change-Id: I8a3bf53cbb9edcd5905003b5739400243977e55a
Reviewed-on: https://chromium-review.googlesource.com/1074378
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: Min Qin <qinmin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#562558}
Reviewed-on: https://chromium-review.googlesource.com/1080037
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3440@{#51}
Cr-Branched-From: 010ddcf-refs/heads/master@{#561733}
  • Loading branch information
Xing Liu committed May 30, 2018
1 parent b4de322 commit b828621
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand Down
14 changes: 11 additions & 3 deletions chrome/browser/download/chrome_download_manager_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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<int>(DownloadPromptStatus::SHOW_PREFERENCE));

enum class WebContents { AVAILABLE, NONE };
enum class ExpectPath { FULL, EMPTY };
struct {
Expand Down

0 comments on commit b828621

Please sign in to comment.