Commit e819edb8 authored by Sam Maier's avatar Sam Maier Committed by Commit Bot

Android: making ShareHelper usage of AsyncTask explicit

Currently, AsyncTask.execute() defaults to the SERIAL_EXECUTOR. This
exector is good for preventing concurrency errors since it guarantees
serial execution, but bad for performance since the entire app shares
this single queue.

We are using SERIAL_EXECUTOR in clearSharedImages and shareImage since
they don't appear to be thread safe, and they could potentially be
called multiple times.

We are using the UI thread in getShareableIconAndName since we
immediately call get() on it anyway. This negates any benefit for having
an AsyncTask.

Bug: 869907, 729737
Change-Id: I9ea44efba2155bb2b643f6d4d8f2b38c8365f418
Reviewed-on: https://chromium-review.googlesource.com/1161218
Commit-Queue: Sam Maier <smaier@chromium.org>
Reviewed-by: default avatarTheresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581308}
parent 61985bcc
...@@ -42,6 +42,7 @@ import org.chromium.base.Callback; ...@@ -42,6 +42,7 @@ import org.chromium.base.Callback;
import org.chromium.base.ContextUtils; import org.chromium.base.ContextUtils;
import org.chromium.base.Log; import org.chromium.base.Log;
import org.chromium.base.StreamUtil; import org.chromium.base.StreamUtil;
import org.chromium.base.StrictModeContext;
import org.chromium.base.VisibleForTesting; import org.chromium.base.VisibleForTesting;
import org.chromium.base.metrics.CachedMetrics; import org.chromium.base.metrics.CachedMetrics;
import org.chromium.chrome.R; import org.chromium.chrome.R;
...@@ -53,9 +54,6 @@ import java.io.FileOutputStream; ...@@ -53,9 +54,6 @@ import java.io.FileOutputStream;
import java.io.IOException; import java.io.IOException;
import java.util.Collections; import java.util.Collections;
import java.util.List; import java.util.List;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
/** /**
* A helper class that helps to start an intent to share titles and URLs. * A helper class that helps to start an intent to share titles and URLs.
...@@ -261,20 +259,15 @@ public class ShareHelper { ...@@ -261,20 +259,15 @@ public class ShareHelper {
* Clears all shared image files. * Clears all shared image files.
*/ */
public static void clearSharedImages() { public static void clearSharedImages() {
new AsyncTask<Void>() { AsyncTask.SERIAL_EXECUTOR.execute(() -> {
@Override try {
protected Void doInBackground() { File imagePath =
try { UiUtils.getDirectoryForImageCapture(ContextUtils.getApplicationContext());
File imagePath = UiUtils.getDirectoryForImageCapture( deleteShareImageFiles(new File(imagePath, SHARE_IMAGES_DIRECTORY_NAME));
ContextUtils.getApplicationContext()); } catch (IOException ie) {
deleteShareImageFiles(new File(imagePath, SHARE_IMAGES_DIRECTORY_NAME)); // Ignore exception.
} catch (IOException ie) {
// Ignore exception.
}
return null;
} }
} });
.execute();
} }
/** /**
...@@ -365,7 +358,7 @@ public class ShareHelper { ...@@ -365,7 +358,7 @@ public class ShareHelper {
} }
} }
} }
.execute(); .executeOnExecutor(AsyncTask.SERIAL_EXECUTOR);
} }
private static class ExternallyVisibleUriCallback implements Callback<String> { private static class ExternallyVisibleUriCallback implements Callback<String> {
...@@ -547,39 +540,17 @@ public class ShareHelper { ...@@ -547,39 +540,17 @@ public class ShareHelper {
} }
if (isComponentValid) { if (isComponentValid) {
boolean retrieved = false; boolean retrieved = false;
final PackageManager pm = ContextUtils.getApplicationContext().getPackageManager();
try { try {
final PackageManager pm = ContextUtils.getApplicationContext().getPackageManager(); // TODO(dtrainor): Make asynchronous and have a callback to update the menu.
AsyncTask<Pair<Drawable, CharSequence>> task = // https://crbug.com/729737
new AsyncTask<Pair<Drawable, CharSequence>>() { try (StrictModeContext unused = StrictModeContext.allowDiskReads()) {
@Override directShareIcon = pm.getActivityIcon(component);
protected Pair<Drawable, CharSequence> doInBackground() { ApplicationInfo ai = pm.getApplicationInfo(component.getPackageName(), 0);
Drawable directShareIcon = null; directShareTitle = pm.getApplicationLabel(ai);
CharSequence directShareTitle = null; }
try {
directShareIcon = pm.getActivityIcon(component);
ApplicationInfo ai =
pm.getApplicationInfo(component.getPackageName(), 0);
directShareTitle = pm.getApplicationLabel(ai);
} catch (NameNotFoundException exception) {
// Use the default null values.
}
return new Pair<Drawable, CharSequence>(
directShareIcon, directShareTitle);
}
};
task.execute();
// TODO(ltian): Return nothing for the AsyncTask and have a callback to update the
// the menu.
Pair<Drawable, CharSequence> result =
task.get(COMPONENT_INFO_READ_TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
directShareIcon = result.first;
directShareTitle = result.second;
retrieved = true; retrieved = true;
} catch (InterruptedException ie) { } catch (NameNotFoundException exception) {
// Use the default null values.
} catch (ExecutionException ee) {
// Use the default null values.
} catch (TimeoutException te) {
// Use the default null values. // Use the default null values.
} }
CachedMetrics.BooleanHistogramSample isLastSharedAppInfoRetrieved = CachedMetrics.BooleanHistogramSample isLastSharedAppInfoRetrieved =
......
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