Skip to content

Commit d37ac1e

Browse files
lfkellogglfkellogg
authored
Use explicit executors in more App Distro code (#4535)
1 parent f40c24a commit d37ac1e

File tree

7 files changed

+88
-80
lines changed

7 files changed

+88
-80
lines changed

‎firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FeedbackSender.java

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,38 +14,42 @@
1414

1515
package com.google.firebase.appdistribution.impl;
1616

17-
import android.annotation.SuppressLint;
1817
import android.net.Uri;
1918
import androidx.annotation.Nullable;
2019
import com.google.android.gms.tasks.Task;
2120
import com.google.android.gms.tasks.Tasks;
2221
import com.google.firebase.FirebaseApp;
22+
import com.google.firebase.annotations.concurrent.Blocking;
23+
import com.google.firebase.annotations.concurrent.Lightweight;
2324
import java.util.concurrent.Executor;
2425

2526
/** Sends tester feedback to the Tester API. */
2627
class FeedbackSender {
2728
private final FirebaseAppDistributionTesterApiClient testerApiClient;
28-
private final Executor blockingExecutor;
29+
@Blocking private final Executor blockingExecutor;
30+
@Lightweight private final Executor lightweightExecutor;
2931

3032
FeedbackSender(
31-
FirebaseAppDistributionTesterApiClient testerApiClient, Executor blockingExecutor) {
33+
FirebaseAppDistributionTesterApiClient testerApiClient,
34+
@Blocking Executor blockingExecutor,
35+
@Lightweight Executor lightweightExecutor) {
3236
this.testerApiClient = testerApiClient;
3337
this.blockingExecutor = blockingExecutor;
38+
this.lightweightExecutor = lightweightExecutor;
3439
}
3540

3641
/** Get an instance of FeedbackSender. */
3742
static FeedbackSender getInstance() {
3843
return FirebaseApp.getInstance().get(FeedbackSender.class);
3944
}
4045

41-
// TODO(b/261014422): Use an explicit executor in continuations.
42-
@SuppressLint("TaskMainThread")
4346
/** Send feedback text and optionally a screenshot to the Tester API for the given release. */
4447
Task<Void> sendFeedback(String releaseName, String feedbackText, @Nullable Uri screenshotUri) {
4548
return testerApiClient
4649
.createFeedback(releaseName, feedbackText)
47-
.onSuccessTask(feedbackName -> attachScreenshot(feedbackName, screenshotUri))
48-
.onSuccessTask(testerApiClient::commitFeedback);
50+
.onSuccessTask(
51+
lightweightExecutor, feedbackName -> attachScreenshot(feedbackName, screenshotUri))
52+
.onSuccessTask(lightweightExecutor, testerApiClient::commitFeedback);
4953
}
5054

5155
// TODO(kbolay): Remove this hack to make the executor available in FeedbackAction and use a more

‎firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
2222
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskResult;
2323

24-
import android.annotation.SuppressLint;
2524
import android.app.Activity;
2625
import android.app.AlertDialog;
2726
import android.content.Context;
@@ -36,8 +35,8 @@
3635
import com.google.android.gms.tasks.TaskCompletionSource;
3736
import com.google.android.gms.tasks.Tasks;
3837
import com.google.firebase.FirebaseApp;
39-
import com.google.firebase.annotations.concurrent.Blocking;
4038
import com.google.firebase.annotations.concurrent.Lightweight;
39+
import com.google.firebase.annotations.concurrent.UiThread;
4140
import com.google.firebase.appdistribution.AppDistributionRelease;
4241
import com.google.firebase.appdistribution.BinaryType;
4342
import com.google.firebase.appdistribution.FirebaseAppDistribution;
@@ -69,7 +68,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
6968
private final ReleaseIdentifier releaseIdentifier;
7069
private final ScreenshotTaker screenshotTaker;
7170
@Lightweight private final Executor lightweightExecutor;
72-
@Blocking private final Executor blockingExecutor;
71+
@UiThread private final Executor uiThreadExecutor;
7372
private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
7473
private final TaskCache<AppDistributionRelease> checkForNewReleaseTaskCache;
7574
private final UpdateTaskCache updateIfNewReleaseAvailableTaskCache;
@@ -96,7 +95,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
9695
@NonNull ReleaseIdentifier releaseIdentifier,
9796
@NonNull ScreenshotTaker screenshotTaker,
9897
@NonNull @Lightweight Executor lightweightExecutor,
99-
@NonNull @Blocking Executor blockingExecutor) {
98+
@NonNull @UiThread Executor uiThreadExecutor) {
10099
this.firebaseApp = firebaseApp;
101100
this.testerSignInManager = testerSignInManager;
102101
this.newReleaseFetcher = newReleaseFetcher;
@@ -107,7 +106,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
107106
this.lifecycleNotifier = lifecycleNotifier;
108107
this.screenshotTaker = screenshotTaker;
109108
this.lightweightExecutor = lightweightExecutor;
110-
this.blockingExecutor = blockingExecutor;
109+
this.uiThreadExecutor = uiThreadExecutor;
111110
this.cachedNewRelease = new SequentialReference<>(lightweightExecutor);
112111
this.checkForNewReleaseTaskCache = new TaskCache<>(lightweightExecutor);
113112
this.updateIfNewReleaseAvailableTaskCache = new UpdateTaskCache(lightweightExecutor);
@@ -350,13 +349,13 @@ public void startFeedback(@NonNull CharSequence infoText) {
350349
screenshotTaker
351350
.takeScreenshot()
352351
.addOnFailureListener(
353-
blockingExecutor,
352+
lightweightExecutor,
354353
e -> {
355354
LogWrapper.w(TAG, "Failed to take screenshot for feedback", e);
356355
doStartFeedback(infoText, null);
357356
})
358357
.addOnSuccessListener(
359-
blockingExecutor, screenshotUri -> doStartFeedback(infoText, screenshotUri));
358+
lightweightExecutor, screenshotUri -> doStartFeedback(infoText, screenshotUri));
360359
}
361360

362361
@Override
@@ -390,24 +389,22 @@ public void cancelFeedbackNotification() {
390389
notificationsManager.cancelFeedbackNotification();
391390
}
392391

393-
// TODO(b/261014422): Use an explicit executor in continuations.
394-
@SuppressLint("TaskMainThread")
395392
private void doStartFeedback(CharSequence infoText, @Nullable Uri screenshotUri) {
396393
testerSignInManager
397394
.signInTester()
398395
.addOnFailureListener(
399-
blockingExecutor,
396+
lightweightExecutor,
400397
e -> {
401398
feedbackInProgress.set(false);
402399
LogWrapper.e(TAG, "Failed to sign in tester. Could not collect feedback.", e);
403400
})
404401
.onSuccessTask(
405-
blockingExecutor,
402+
lightweightExecutor,
406403
unused ->
407404
releaseIdentifier
408405
.identifyRelease()
409406
.addOnFailureListener(
410-
blockingExecutor,
407+
uiThreadExecutor,
411408
e -> {
412409
feedbackInProgress.set(false);
413410
LogWrapper.e(TAG, "Failed to identify release", e);
@@ -418,12 +415,12 @@ private void doStartFeedback(CharSequence infoText, @Nullable Uri screenshotUri)
418415
.show();
419416
})
420417
.onSuccessTask(
421-
blockingExecutor,
418+
lightweightExecutor,
422419
releaseName ->
423420
// in development-mode the releaseName might be null
424421
launchFeedbackActivity(releaseName, infoText, screenshotUri)
425422
.addOnFailureListener(
426-
blockingExecutor,
423+
uiThreadExecutor,
427424
e -> {
428425
feedbackInProgress.set(false);
429426
LogWrapper.e(TAG, "Failed to launch feedback flow", e);

‎firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,14 +79,19 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
7979
.add(Dependency.required(FirebaseApp.class))
8080
.add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
8181
.add(Dependency.required(blockingExecutor))
82-
.factory(c -> buildFeedbackSender(c, c.get(blockingExecutor)))
82+
.add(Dependency.required(lightweightExecutor))
83+
.factory(c -> buildFeedbackSender(c, blockingExecutor, lightweightExecutor))
8384
.build(),
8485
LibraryVersionComponent.create(LIBRARY_NAME, BuildConfig.VERSION_NAME));
8586
}
8687

8788
private FeedbackSender buildFeedbackSender(
88-
ComponentContainer container, Executor blockingExecutor) {
89+
ComponentContainer container,
90+
Qualified<Executor> blockingExecutorType,
91+
Qualified<Executor> lightweightExecutorType) {
8992
FirebaseApp firebaseApp = container.get(FirebaseApp.class);
93+
Executor blockingExecutor = container.get(blockingExecutorType);
94+
Executor lightweightExecutor = container.get(lightweightExecutorType);
9095
Provider<FirebaseInstallationsApi> firebaseInstallationsApiProvider =
9196
container.getProvider(FirebaseInstallationsApi.class);
9297
FirebaseAppDistributionTesterApiClient testerApiClient =
@@ -95,7 +100,7 @@ private FeedbackSender buildFeedbackSender(
95100
firebaseInstallationsApiProvider,
96101
new TesterApiHttpClient(firebaseApp),
97102
blockingExecutor);
98-
return new FeedbackSender(testerApiClient, blockingExecutor);
103+
return new FeedbackSender(testerApiClient, blockingExecutor, lightweightExecutor);
99104
}
100105

101106
private FirebaseAppDistribution buildFirebaseAppDistribution(
@@ -150,7 +155,7 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
150155
releaseIdentifier,
151156
new ScreenshotTaker(firebaseApp, lifecycleNotifier, backgroundExecutor),
152157
lightweightExecutor,
153-
blockingExecutor);
158+
uiThreadExecutor);
154159

155160
if (context instanceof Application) {
156161
Application firebaseApplication = (Application) context;

‎firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java

Lines changed: 29 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414

1515
package com.google.firebase.appdistribution.impl;
1616

17-
import android.annotation.SuppressLint;
1817
import android.annotation.TargetApi;
1918
import android.app.Activity;
2019
import android.content.Context;
@@ -31,6 +30,7 @@
3130
import com.google.android.gms.tasks.TaskCompletionSource;
3231
import com.google.android.gms.tasks.Tasks;
3332
import com.google.firebase.FirebaseApp;
33+
import com.google.firebase.annotations.concurrent.Background;
3434
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
3535
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
3636
import java.io.FileNotFoundException;
@@ -46,12 +46,12 @@ class ScreenshotTaker {
4646

4747
private final FirebaseApp firebaseApp;
4848
private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
49-
private final Executor backgroundExecutor;
49+
@Background private final Executor backgroundExecutor;
5050

5151
ScreenshotTaker(
5252
FirebaseApp firebaseApp,
5353
FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
54-
Executor backgroundExecutor) {
54+
@Background Executor backgroundExecutor) {
5555
this.firebaseApp = firebaseApp;
5656
this.lifecycleNotifier = lifecycleNotifier;
5757
this.backgroundExecutor = backgroundExecutor;
@@ -63,22 +63,9 @@ class ScreenshotTaker {
6363
* @return a {@link Task} that will complete with the file URI in app-level storage where the
6464
* screenshot was written, or null if no activity could be found to screenshot.
6565
*/
66-
// TODO(b/261014422): Use an explicit executor in continuations.
67-
@SuppressLint("TaskMainThread")
6866
Task<Uri> takeScreenshot() {
69-
return deleteScreenshot()
70-
.onSuccessTask(unused -> captureScreenshot())
71-
.onSuccessTask(this::writeToFile);
72-
}
73-
74-
/** Deletes any previously taken screenshot. */
75-
Task<Void> deleteScreenshot() {
76-
return TaskUtils.runAsyncInTask(
77-
backgroundExecutor,
78-
() -> {
79-
firebaseApp.getApplicationContext().deleteFile(SCREENSHOT_FILE_NAME);
80-
return null;
81-
});
67+
return captureScreenshot()
68+
.onSuccessTask(backgroundExecutor, bitmap -> Tasks.forResult(writeToFile(bitmap)));
8269
}
8370

8471
@VisibleForTesting
@@ -98,7 +85,7 @@ Task<Bitmap> captureScreenshot() {
9885
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
9986
return captureScreenshotOreo(activity);
10087
} else {
101-
return captureScreenshotLegacy(activity);
88+
return Tasks.forResult(captureScreenshotLegacy(activity));
10289
}
10390
} catch (Exception | OutOfMemoryError e) {
10491
throw new FirebaseAppDistributionException(
@@ -112,11 +99,11 @@ private static Bitmap getBitmapForScreenshot(Activity activity) {
11299
return Bitmap.createBitmap(view.getWidth(), view.getHeight(), Bitmap.Config.RGB_565);
113100
}
114101

115-
private static Task<Bitmap> captureScreenshotLegacy(Activity activity) {
102+
private static Bitmap captureScreenshotLegacy(Activity activity) {
116103
Bitmap bitmap = getBitmapForScreenshot(activity);
117104
Canvas canvas = new Canvas(bitmap);
118105
activity.getWindow().getDecorView().getRootView().draw(canvas);
119-
return Tasks.forResult(bitmap);
106+
return bitmap;
120107
}
121108

122109
@TargetApi(Build.VERSION_CODES.O)
@@ -145,23 +132,29 @@ private static Task<Bitmap> captureScreenshotOreo(Activity activity) {
145132
return taskCompletionSource.getTask();
146133
}
147134

148-
private Task<Uri> writeToFile(@Nullable Bitmap bitmap) {
135+
@Nullable
136+
private Uri writeToFile(@Nullable Bitmap bitmap) throws FirebaseAppDistributionException {
149137
if (bitmap == null) {
150-
return Tasks.forResult(null);
138+
return null;
151139
}
152-
return TaskUtils.runAsyncInTask(
153-
backgroundExecutor,
154-
() -> {
155-
try (FileOutputStream outputStream = openFileOutputStream()) {
156-
// PNG is a lossless format, the compression factor (100) is ignored
157-
bitmap.compress(Bitmap.CompressFormat.PNG, /* quality= */ 100, outputStream);
158-
} catch (IOException e) {
159-
throw new FirebaseAppDistributionException(
160-
"Failed to write screenshot to storage", Status.UNKNOWN, e);
161-
}
162-
return Uri.fromFile(
163-
firebaseApp.getApplicationContext().getFileStreamPath(SCREENSHOT_FILE_NAME));
164-
});
140+
141+
// First delete the previous file if it's still there
142+
deleteScreenshot();
143+
144+
try (FileOutputStream outputStream = openFileOutputStream()) {
145+
// PNG is a lossless format, the compression factor (100) is ignored
146+
bitmap.compress(Bitmap.CompressFormat.PNG, /* quality= */ 100, outputStream);
147+
} catch (IOException e) {
148+
throw new FirebaseAppDistributionException(
149+
"Failed to write screenshot to storage", Status.UNKNOWN, e);
150+
}
151+
return Uri.fromFile(
152+
firebaseApp.getApplicationContext().getFileStreamPath(SCREENSHOT_FILE_NAME));
153+
}
154+
155+
@VisibleForTesting
156+
void deleteScreenshot() {
157+
firebaseApp.getApplicationContext().deleteFile(SCREENSHOT_FILE_NAME);
165158
}
166159

167160
private FileOutputStream openFileOutputStream() throws FileNotFoundException {

‎firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/impl/FeedbackSenderTest.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,12 @@
2323
import androidx.test.core.app.ApplicationProvider;
2424
import com.google.android.gms.tasks.Task;
2525
import com.google.android.gms.tasks.Tasks;
26+
import com.google.firebase.annotations.concurrent.Blocking;
27+
import com.google.firebase.annotations.concurrent.Lightweight;
2628
import com.google.firebase.appdistribution.FirebaseAppDistributionException;
2729
import com.google.firebase.appdistribution.FirebaseAppDistributionException.Status;
28-
import java.util.concurrent.Executors;
30+
import com.google.firebase.concurrent.TestOnlyExecutors;
31+
import java.util.concurrent.Executor;
2932
import org.junit.Before;
3033
import org.junit.Test;
3134
import org.junit.runner.RunWith;
@@ -41,14 +44,17 @@ public class FeedbackSenderTest {
4144
private static final Uri TEST_SCREENSHOT_URI =
4245
Uri.fromFile(ApplicationProvider.getApplicationContext().getFileStreamPath("test.png"));
4346

47+
@Blocking private final Executor blockingExecutor = TestOnlyExecutors.blocking();
48+
@Lightweight private final Executor lightweightExecutor = TestOnlyExecutors.lite();
49+
4450
@Mock private FirebaseAppDistributionTesterApiClient mockTesterApiClient;
4551

4652
private FeedbackSender feedbackSender;
4753

4854
@Before
4955
public void setup() {
5056
MockitoAnnotations.initMocks(this);
51-
feedbackSender = new FeedbackSender(mockTesterApiClient, Executors.newSingleThreadExecutor());
57+
feedbackSender = new FeedbackSender(mockTesterApiClient, blockingExecutor, lightweightExecutor);
5258
}
5359

5460
@Test

0 commit comments

Comments
 (0)