Commit 8d5afbb1 authored by Xing Liu's avatar Xing Liu Committed by Commit Bot

Download location: fix ANR issue.

Currently we access disks on Android main thread to pull download
directories, which can cause Android to skip over 100 frames during
rendering, or even totally bust the rendering and show a black screen.
When try to build download location UI, it may stuck a few seconds(ANR).

This CL moves all disk access operation to background thread, so UI
will be fast and smooth.

Bug: 844107
Change-Id: Icba6db01a4a638cdc62b2e1d207e73981af15dca
Reviewed-on: https://chromium-review.googlesource.com/1107041Reviewed-by: default avatarDavid Trainor <dtrainor@chromium.org>
Reviewed-by: default avatarMin Qin <qinmin@chromium.org>
Commit-Queue: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569129}
parent e1c8719a
......@@ -4,12 +4,18 @@
package org.chromium.chrome.browser.download;
import android.os.AsyncTask;
import android.os.Build;
import android.os.Environment;
import android.support.annotation.IntDef;
import org.chromium.base.ContextUtils;
import org.chromium.base.metrics.RecordHistogram;
import java.io.File;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.util.ArrayList;
/**
* Denotes a given option for directory selection; includes name, location, and space.
......@@ -26,10 +32,63 @@ public class DirectoryOption {
public static final int ERROR_OPTION = 2;
public static final int OPTION_COUNT = 3;
/**
* Asynchronous task to retrieve all download directories on a background thread.
*/
public static class AllDirectoriesTask
extends AsyncTask<Void, Void, ArrayList<DirectoryOption>> {
@Override
protected ArrayList<DirectoryOption> doInBackground(Void... params) {
ArrayList<DirectoryOption> dirs = new ArrayList<>();
// Retrieve default directory.
File defaultDirectory =
Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS);
// If no default directory, return an error option.
if (defaultDirectory == null) {
dirs.add(new DirectoryOption(null, 0, 0, DirectoryOption.ERROR_OPTION));
return dirs;
}
DirectoryOption defaultOption =
toDirectoryOption(defaultDirectory, DirectoryOption.DEFAULT_OPTION);
dirs.add(defaultOption);
// Retrieve additional directories, i.e. the external SD card directory.
String primaryStorageDir = Environment.getExternalStorageDirectory().getAbsolutePath();
File[] files;
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
files = ContextUtils.getApplicationContext().getExternalFilesDirs(
Environment.DIRECTORY_DOWNLOADS);
} else {
files = new File[] {Environment.getExternalStorageDirectory()};
}
if (files.length <= 1) return dirs;
for (int i = 0; i < files.length; ++i) {
if (files[i] == null) continue;
// Skip primary storage directory.
if (files[i].getAbsolutePath().contains(primaryStorageDir)) continue;
dirs.add(toDirectoryOption(files[i], DirectoryOption.ADDITIONAL_OPTION));
}
return dirs;
}
private DirectoryOption toDirectoryOption(
File dir, @DownloadLocationDirectoryType int type) {
if (dir == null) return null;
return new DirectoryOption(
dir.getAbsolutePath(), dir.getUsableSpace(), dir.getTotalSpace(), type);
}
}
/**
* Name of the current download directory.
*/
public final String name;
public String name;
/**
* The absolute path of the download location.
......@@ -49,11 +108,16 @@ public class DirectoryOption {
/**
* The type of the directory option.
*/
public final int type;
public final @DownloadLocationDirectoryType int type;
public DirectoryOption(String name, String location, long availableSpace, long totalSpace,
@DownloadLocationDirectoryType int type) {
this(location, availableSpace, totalSpace, type);
this.name = name;
}
public DirectoryOption(String location, long availableSpace, long totalSpace,
@DownloadLocationDirectoryType int type) {
this.location = location;
this.availableSpace = availableSpace;
this.totalSpace = totalSpace;
......
......@@ -28,12 +28,14 @@ import javax.annotation.Nullable;
/**
* Dialog that is displayed to ask user where they want to download the file.
*/
public class DownloadLocationDialog extends ModalDialogView implements OnCheckedChangeListener {
public class DownloadLocationDialog extends ModalDialogView
implements OnCheckedChangeListener, DownloadDirectoryAdapter.Delegate {
private DownloadDirectoryAdapter mDirectoryAdapter;
private AlertDialogEditText mFileName;
private Spinner mFileLocation;
private CheckBox mDontShowAgain;
private @DownloadLocationDialogType int mDialogType;
/**
* Create a {@link DownloadLocationDialog} with the given properties.
......@@ -96,21 +98,12 @@ public class DownloadLocationDialog extends ModalDialogView implements OnChecked
@DownloadLocationDialogType int dialogType, File suggestedPath, Params params) {
super(controller, params);
mDirectoryAdapter = new DownloadDirectoryAdapter(context);
mDirectoryAdapter = new DownloadDirectoryAdapter(context, this);
mFileName = (AlertDialogEditText) params.customView.findViewById(R.id.file_name);
mFileName.setText(suggestedPath.getName());
mFileLocation = (Spinner) params.customView.findViewById(R.id.file_location);
mFileLocation.setAdapter(mDirectoryAdapter);
int selectedItemId = mDirectoryAdapter.getSelectedItemId();
if (selectedItemId == NO_SELECTED_ITEM_ID
|| dialogType == DownloadLocationDialogType.LOCATION_FULL
|| dialogType == DownloadLocationDialogType.LOCATION_NOT_FOUND) {
selectedItemId = mDirectoryAdapter.useFirstValidSelectableItemId();
}
mFileLocation.setSelection(selectedItemId);
// Automatically check "don't show again" the first time the user is seeing the dialog.
mDontShowAgain = (CheckBox) params.customView.findViewById(R.id.show_again_checkbox);
......@@ -118,6 +111,8 @@ public class DownloadLocationDialog extends ModalDialogView implements OnChecked
== DownloadPromptStatus.SHOW_INITIAL;
mDontShowAgain.setChecked(isInitial);
mDontShowAgain.setOnCheckedChangeListener(this);
mDialogType = dialogType;
}
// CompoundButton.OnCheckedChangeListener implementation.
......@@ -155,4 +150,22 @@ public class DownloadLocationDialog extends ModalDialogView implements OnChecked
boolean getDontShowAgain() {
return mDontShowAgain != null && mDontShowAgain.isChecked();
}
// DownloadDirectoryAdapter.Delegate implementation.
@Override
public void onDirectoryOptionsReady() {
int selectedItemId = mDirectoryAdapter.getSelectedItemId();
if (selectedItemId == NO_SELECTED_ITEM_ID
|| mDialogType == DownloadLocationDialogType.LOCATION_FULL
|| mDialogType == DownloadLocationDialogType.LOCATION_NOT_FOUND) {
selectedItemId = mDirectoryAdapter.useFirstValidSelectableItemId();
}
mFileLocation.setAdapter(mDirectoryAdapter);
mFileLocation.setSelection(selectedItemId);
}
@Override
public void onDirectorySelectionChanged() {}
}
......@@ -25,7 +25,6 @@ import org.chromium.base.Callback;
import org.chromium.base.ContextUtils;
import org.chromium.base.Log;
import org.chromium.base.ObserverList;
import org.chromium.base.StrictModeContext;
import org.chromium.base.ThreadUtils;
import org.chromium.base.VisibleForTesting;
import org.chromium.base.annotations.CalledByNative;
......@@ -33,6 +32,7 @@ import org.chromium.base.library_loader.LibraryProcessType;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ChromeFeatureList;
import org.chromium.chrome.browser.download.DirectoryOption.AllDirectoriesTask;
import org.chromium.chrome.browser.download.DownloadMetrics.DownloadOpenSource;
import org.chromium.chrome.browser.download.ui.BackendProvider;
import org.chromium.chrome.browser.externalnav.ExternalNavigationDelegateImpl;
......@@ -1541,26 +1541,29 @@ public class DownloadManagerService
// Only show the missing directory snackbar once.
if (!prefServiceBridge.getBoolean(Pref.SHOW_MISSING_SD_CARD_ERROR_ANDROID)) return;
String[] downloadDirs = DownloadUtils.getAllDownloadDirectories();
if (downloadDirs.length > 1) return;
String externalStorageDir = null;
try (StrictModeContext unused = StrictModeContext.allowDiskWrites()) {
externalStorageDir = Environment.getExternalStorageDirectory().getAbsolutePath();
}
for (DownloadItem item : list) {
boolean missingOnSDCard = isFilePathOnMissingExternalDrive(
item.getDownloadInfo().getFilePath(), externalStorageDir, downloadDirs);
if (!isUnresumableOrCancelled(item) && missingOnSDCard) {
mHandler.post(() -> {
// TODO(shaktisahu): Show it on infobar in the right way.
mDownloadSnackbarController.onDownloadDirectoryNotFound();
});
prefServiceBridge.setBoolean(Pref.SHOW_MISSING_SD_CARD_ERROR_ANDROID, false);
break;
AllDirectoriesTask task = new AllDirectoriesTask() {
@Override
protected void onPostExecute(ArrayList<DirectoryOption> dirs) {
if (dirs.size() > 1) return;
String externalStorageDir =
Environment.getExternalStorageDirectory().getAbsolutePath();
for (DownloadItem item : list) {
boolean missingOnSDCard = isFilePathOnMissingExternalDrive(
item.getDownloadInfo().getFilePath(), externalStorageDir, dirs);
if (!isUnresumableOrCancelled(item) && missingOnSDCard) {
mHandler.post(() -> {
// TODO(shaktisahu): Show it on infobar in the right way.
mDownloadSnackbarController.onDownloadDirectoryNotFound();
});
prefServiceBridge.setBoolean(
Pref.SHOW_MISSING_SD_CARD_ERROR_ANDROID, false);
break;
}
}
}
}
};
task.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
}
/**
......@@ -1583,20 +1586,20 @@ public class DownloadManagerService
* @param filePath The file path to check.
* @param externalStorageDir The absolute path of external storage directory for primary
* storage.
* @param downloadDirs All available download directories including primary storage and
* @param directoryOptions All available download directories including primary storage and
* secondary storage.
*
* @return Whether this file path is in a directory that is no longer available.
*/
private boolean isFilePathOnMissingExternalDrive(
String filePath, String externalStorageDir, String[] downloadDirs) {
private boolean isFilePathOnMissingExternalDrive(String filePath, String externalStorageDir,
ArrayList<DirectoryOption> directoryOptions) {
if (filePath.contains(externalStorageDir)) {
return false;
}
for (String dir : downloadDirs) {
if (TextUtils.isEmpty(dir)) continue;
if (filePath.contains(dir)) return false;
for (DirectoryOption directory : directoryOptions) {
if (TextUtils.isEmpty(directory.location)) continue;
if (filePath.contains(directory.location)) return false;
}
return true;
......
......@@ -23,7 +23,6 @@ import org.chromium.base.Callback;
import org.chromium.base.ContextUtils;
import org.chromium.base.FileUtils;
import org.chromium.base.Log;
import org.chromium.base.PathUtils;
import org.chromium.base.StrictModeContext;
import org.chromium.base.VisibleForTesting;
import org.chromium.base.library_loader.LibraryProcessType;
......@@ -1149,18 +1148,6 @@ public class DownloadUtils {
return cal.getTime();
}
/**
* Gets all of the directories available for downloads, including internal & external storage.
*
* If the external directories are not available for querying (on older versions of Android),
* return an array with just the internal directory.
*
* @return The absolute paths of download directories.
*/
public static String[] getAllDownloadDirectories() {
return PathUtils.getAllPrivateDownloadsDirectories();
}
/**
* Returns if the path is in the download directory on primary storage.
* @param path The directory to check.
......
......@@ -33,6 +33,8 @@ import org.chromium.chrome.R;
import org.chromium.chrome.browser.BasicNativePage;
import org.chromium.chrome.browser.ChromeApplication;
import org.chromium.chrome.browser.ChromeFeatureList;
import org.chromium.chrome.browser.download.DirectoryOption;
import org.chromium.chrome.browser.download.DirectoryOption.AllDirectoriesTask;
import org.chromium.chrome.browser.download.DownloadManagerService;
import org.chromium.chrome.browser.download.DownloadUtils;
import org.chromium.chrome.browser.download.items.OfflineContentAggregatorFactory;
......@@ -561,29 +563,35 @@ public class DownloadManagerUi
private void maybeShowDownloadSettingsTextBubble(final Tracker tracker) {
// If the user doesn't have an SD card don't show the IPH.
String[] externalDirs = DownloadUtils.getAllDownloadDirectories();
if (externalDirs.length < 2) return;
AllDirectoriesTask task = new AllDirectoriesTask() {
@Override
protected void onPostExecute(ArrayList<DirectoryOption> dirs) {
if (dirs.size() < 2) return;
// Check to see if the help UI should be triggered.
if (!tracker.shouldTriggerHelpUI(FeatureConstants.DOWNLOAD_SETTINGS_FEATURE)) return;
// Check to see if the help UI should be triggered.
if (!tracker.shouldTriggerHelpUI(FeatureConstants.DOWNLOAD_SETTINGS_FEATURE))
return;
// Build and show text bubble.
View anchorView = mToolbar.findViewById(R.id.settings_menu_id);
// Build and show text bubble.
View anchorView = mToolbar.findViewById(R.id.settings_menu_id);
// Show the setting text bubble after the root view is attached to window.
if (mToolbar.isAttachedToWindow()) {
showDownloadSettingsInProductHelp(tracker, anchorView);
} else {
mToolbar.addOnAttachStateChangeListener(new View.OnAttachStateChangeListener() {
@Override
public void onViewAttachedToWindow(View v) {
// Show the setting text bubble after the root view is attached to window.
if (mToolbar.isAttachedToWindow()) {
showDownloadSettingsInProductHelp(tracker, anchorView);
mToolbar.removeOnAttachStateChangeListener(this);
} else {
mToolbar.addOnAttachStateChangeListener(new View.OnAttachStateChangeListener() {
@Override
public void onViewAttachedToWindow(View v) {
showDownloadSettingsInProductHelp(tracker, anchorView);
mToolbar.removeOnAttachStateChangeListener(this);
}
@Override
public void onViewDetachedFromWindow(View v) {}
});
}
@Override
public void onViewDetachedFromWindow(View v) {}
});
}
}
};
task.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
}
private void showDownloadSettingsInProductHelp(final Tracker tracker, View anchorView) {
......
......@@ -5,11 +5,10 @@
package org.chromium.chrome.browser.preferences.download;
import android.content.Context;
import android.os.Environment;
import android.os.AsyncTask;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.v4.widget.TextViewCompat;
import android.text.TextUtils;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
......@@ -19,11 +18,11 @@ import android.widget.TextView;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.download.DirectoryOption;
import org.chromium.chrome.browser.download.DirectoryOption.AllDirectoriesTask;
import org.chromium.chrome.browser.download.DownloadUtils;
import org.chromium.chrome.browser.preferences.PrefServiceBridge;
import org.chromium.chrome.browser.widget.TintedImageView;
import java.io.File;
import java.util.ArrayList;
import java.util.List;
......@@ -32,6 +31,21 @@ import java.util.List;
* download location.
*/
public class DownloadDirectoryAdapter extends ArrayAdapter<Object> {
/**
* Delegate to handle directory options results and observe data changes.
*/
public interface Delegate {
/**
* Called after getting all download directories.
*/
void onDirectoryOptionsReady();
/**
* Called after the user selected another download directory option.
*/
void onDirectorySelectionChanged();
}
public static int NO_SELECTED_ITEM_ID = -1;
public static int SELECTED_ITEM_NOT_INITIALIZED = -2;
......@@ -39,15 +53,18 @@ public class DownloadDirectoryAdapter extends ArrayAdapter<Object> {
private Context mContext;
private LayoutInflater mLayoutInflater;
protected Delegate mDelegate;
private List<DirectoryOption> mCanonicalOptions = new ArrayList<>();
private List<DirectoryOption> mAdditionalOptions = new ArrayList<>();
private List<DirectoryOption> mErrorOptions = new ArrayList<>();
boolean mIsDirectoryOptionsReady;
public DownloadDirectoryAdapter(@NonNull Context context) {
public DownloadDirectoryAdapter(@NonNull Context context, Delegate delegate) {
super(context, android.R.layout.simple_spinner_item);
mContext = context;
mDelegate = delegate;
mLayoutInflater = LayoutInflater.from(context);
refreshData();
......@@ -153,6 +170,8 @@ public class DownloadDirectoryAdapter extends ArrayAdapter<Object> {
* NO_SELECTED_ITEM_ID if no item matches the default path.
*/
public int getSelectedItemId() {
assert mIsDirectoryOptionsReady : "Must be called after directory options query is done";
if (mSelectedPosition == SELECTED_ITEM_NOT_INITIALIZED) {
mSelectedPosition = initSelectedIdFromPref();
}
......@@ -199,48 +218,55 @@ public class DownloadDirectoryAdapter extends ArrayAdapter<Object> {
return mErrorOptions.isEmpty();
}
private void refreshData() {
setCanonicalDirectoryOptions();
setAdditionalDirectoryOptions();
adjustErrorDirectoryOption();
boolean isDirectoryOptionsReady() {
return mIsDirectoryOptionsReady;
}
private void setCanonicalDirectoryOptions() {
private void refreshData() {
mCanonicalOptions.clear();
File directoryLocation =
Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_DOWNLOADS);
mCanonicalOptions.add(new DirectoryOption(mContext.getString(R.string.menu_downloads),
directoryLocation.getAbsolutePath(), directoryLocation.getUsableSpace(),
directoryLocation.getTotalSpace(), DirectoryOption.DEFAULT_OPTION));
}
private void setAdditionalDirectoryOptions() {
mAdditionalOptions.clear();
// If there are no more additional directories, it is only the primary storage available.
String[] externalDirs = DownloadUtils.getAllDownloadDirectories();
if (externalDirs.length <= 1) return;
int numOtherAdditionalDirectories = 0;
for (String dir : externalDirs) {
if (TextUtils.isEmpty(dir)) continue;
// Skip the directory that is in primary storage.
if (dir.contains(Environment.getExternalStorageDirectory().getAbsolutePath())) continue;
// Add index (ie. SD Card 2) if there is more than one secondary storage option.
String directoryName = (numOtherAdditionalDirectories > 0)
? mContext.getString(
org.chromium.chrome.R.string.downloads_location_sd_card_number,
numOtherAdditionalDirectories + 1)
: mContext.getString(org.chromium.chrome.R.string.downloads_location_sd_card);
File file = new File(dir);
mAdditionalOptions.add(new DirectoryOption(directoryName, file.getAbsolutePath(),
file.getUsableSpace(), file.getTotalSpace(),
DirectoryOption.ADDITIONAL_OPTION));
numOtherAdditionalDirectories++;
}
mErrorOptions.clear();
// Retrieve all download directories.
AllDirectoriesTask task = new AllDirectoriesTask() {
@Override
protected void onPostExecute(ArrayList<DirectoryOption> dirs) {
int numOtherAdditionalDirectories = 0;
for (DirectoryOption directory : dirs) {
switch (directory.type) {
case DirectoryOption.DEFAULT_OPTION:
directory.name = mContext.getString(R.string.menu_downloads);
mCanonicalOptions.add(directory);
break;
case DirectoryOption.ADDITIONAL_OPTION:
String directoryName = (numOtherAdditionalDirectories > 0)
? mContext.getString(org.chromium.chrome.R.string
.downloads_location_sd_card_number,
numOtherAdditionalDirectories + 1)
: mContext.getString(org.chromium.chrome.R.string
.downloads_location_sd_card);
directory.name = directoryName;
mAdditionalOptions.add(directory);
numOtherAdditionalDirectories++;
break;
case DirectoryOption.ERROR_OPTION:
directory.name = mContext.getString(
R.string.download_location_no_available_locations);
mErrorOptions.add(directory);
break;
default:
break;
}
}
// After all directory retrieved, update the UI.
// notifyDataSetChanged();
mIsDirectoryOptionsReady = true;
if (mDelegate != null) mDelegate.onDirectoryOptionsReady();
}
};
task.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
}
private void adjustErrorDirectoryOption() {
......
......@@ -20,9 +20,11 @@ import org.chromium.chrome.browser.download.DirectoryOption;
/**
* The preference used to save the download directory in download settings page.
*/
public class DownloadLocationPreference extends DialogPreference {
public class DownloadLocationPreference
extends DialogPreference implements DownloadDirectoryAdapter.Delegate {
/**
* Provides data for the list of available download directories options.
* Provides data for the list of available download directories options. Uses an asynchronous
* operation to query the directory options.
*/
private DownloadLocationPreferenceAdapter mAdapter;
......@@ -36,13 +38,15 @@ public class DownloadLocationPreference extends DialogPreference {
*/
public DownloadLocationPreference(Context context, AttributeSet attrs) {
super(context, attrs);
mAdapter = new DownloadLocationPreferenceAdapter(context, this);
mAdapter = new DownloadLocationPreferenceAdapter(getContext(), this);
}
/**
* Updates the summary that shows the download location directory.
*/
public void updateSummary() {
if (!mAdapter.isDirectoryOptionsReady()) return;
DirectoryOption directoryOption =
(DirectoryOption) mAdapter.getItem(mAdapter.getSelectedItemId());
final SpannableStringBuilder summaryBuilder = new SpannableStringBuilder();
......@@ -60,7 +64,28 @@ public class DownloadLocationPreference extends DialogPreference {
View view = LayoutInflater.from(getContext())
.inflate(R.layout.download_location_preference, null);
mListView = (ListView) (view.findViewById(R.id.location_preference_list_view));
mListView.setAdapter(mAdapter);
// Hook to the adapter if |onDirectoryOptionsReady| is called before |onCreateDialogView|.
if (mAdapter.isDirectoryOptionsReady()) {
mListView.setAdapter(mAdapter);
}
return view;
}
@Override
public void onDirectoryOptionsReady() {
if (mAdapter.getSelectedItemId() == DownloadDirectoryAdapter.NO_SELECTED_ITEM_ID) {
mAdapter.useFirstValidSelectableItemId();
}
// Hook to the adapter if |onCreateDialogView| is called before |onDirectoryOptionsReady|.
if (mListView != null) mListView.setAdapter(mAdapter);
updateSummary();
}
@Override
public void onDirectorySelectionChanged() {
updateSummary();
}
}
......@@ -26,19 +26,11 @@ import org.chromium.chrome.browser.preferences.PrefServiceBridge;
*/
public class DownloadLocationPreferenceAdapter
extends DownloadDirectoryAdapter implements OnClickListener {
private DownloadLocationPreference mPreference;
/**
* Constructor of DownloadLocationPreferenceAdapter.
*/
public DownloadLocationPreferenceAdapter(
Context context, DownloadLocationPreference preference) {
super(context);
mPreference = preference;
if (getSelectedItemId() == NO_SELECTED_ITEM_ID) {
useFirstValidSelectableItemId();
}
public DownloadLocationPreferenceAdapter(Context context, Delegate delegate) {
super(context, delegate);
}
@Override
......@@ -114,7 +106,7 @@ public class DownloadLocationPreferenceAdapter
mSelectedPosition = selectedId;
// Update the preference after selected position is updated.
mPreference.updateSummary();
if (mDelegate != null) mDelegate.onDirectorySelectionChanged();
option.recordDirectoryOptionType();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment