Migrate executors in fad/in-app-feedback

Author: lfkellogg

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

@@ -68,7 +68,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
   private final ReleaseIdentifier releaseIdentifier;
   private final ScreenshotTaker screenshotTaker;
-  private final Executor taskExecutor;
+  private final Executor blockingExecutor;
   private final FirebaseAppDistributionNotificationsManager notificationsManager;
   private TaskCache<UpdateTask> updateIfNewReleaseAvailableTaskCache = new TaskCache<>();
   private TaskCache<Task<AppDistributionRelease>> checkForNewReleaseTaskCache = new TaskCache<>();
@@ -94,7 +94,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
       @NonNull ReleaseIdentifier releaseIdentifier,
       @NonNull ScreenshotTaker screenshotTaker,
       @NonNull @Lightweight Executor lightweightExecutor,
-      @NonNull Executor taskExecutor) {
+      @NonNull Executor blockingExecutor) {
     this.firebaseApp = firebaseApp;
     this.testerSignInManager = testerSignInManager;
     this.newReleaseFetcher = newReleaseFetcher;
@@ -106,7 +106,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
     this.cachedNewRelease = new SequentialReference<>(lightweightExecutor);
     this.lightweightExecutor = lightweightExecutor;
     this.screenshotTaker = screenshotTaker;
-    this.taskExecutor = taskExecutor;
+    this.blockingExecutor = blockingExecutor;
     this.notificationsManager =
         new FirebaseAppDistributionNotificationsManager(firebaseApp.getApplicationContext());
     lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
@@ -332,13 +332,13 @@ public void startFeedback(@NonNull CharSequence infoText) {
     screenshotTaker
         .takeScreenshot()
         .addOnFailureListener(
-            taskExecutor,
+            blockingExecutor,
             e -> {
               LogWrapper.w(TAG, "Failed to take screenshot for feedback", e);
               doStartFeedback(infoText, null);
             })
         .addOnSuccessListener(
-            taskExecutor, screenshotUri -> doStartFeedback(infoText, screenshotUri));
+            blockingExecutor, screenshotUri -> doStartFeedback(infoText, screenshotUri));
   }
 
   @Override
@@ -378,17 +378,18 @@ private void doStartFeedback(CharSequence infoText, @Nullable Uri screenshotUri)
     testerSignInManager
         .signInTester()
         .addOnFailureListener(
-            taskExecutor,
+            blockingExecutor,
             e -> {
               feedbackInProgress.set(false);
               LogWrapper.e(TAG, "Failed to sign in tester. Could not collect feedback.", e);
             })
         .onSuccessTask(
-            taskExecutor,
+            blockingExecutor,
             unused ->
                 releaseIdentifier
                     .identifyRelease()
                     .addOnFailureListener(
+                        blockingExecutor,
                         e -> {
                           feedbackInProgress.set(false);
                           LogWrapper.e(TAG, "Failed to identify release", e);
@@ -399,11 +400,12 @@ private void doStartFeedback(CharSequence infoText, @Nullable Uri screenshotUri)
                               .show();
                         })
                     .onSuccessTask(
-                        taskExecutor,
+                        blockingExecutor,
                         releaseName ->
                             // in development-mode the releaseName might be null
                             launchFeedbackActivity(releaseName, infoText, screenshotUri)
                                 .addOnFailureListener(
+                                    blockingExecutor,
                                     e -> {
                                       feedbackInProgress.set(false);
                                       LogWrapper.e(TAG, "Failed to launch feedback flow", e);

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

@@ -20,6 +20,7 @@
 import androidx.annotation.Keep;
 import androidx.annotation.NonNull;
 import com.google.firebase.FirebaseApp;
+import com.google.firebase.annotations.concurrent.Background;
 import com.google.firebase.annotations.concurrent.Blocking;
 import com.google.firebase.annotations.concurrent.Lightweight;
 import com.google.firebase.appdistribution.FirebaseAppDistribution;
@@ -34,10 +35,9 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.Executor;
-import java.util.concurrent.Executors;
 
 /**
- * Registers FirebaseAppDistribution.
+ * Registers FirebaseAppDistribution and related components.
  *
  * @hide
  */
@@ -49,6 +49,7 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
 
   @Override
   public @NonNull List<Component<?>> getComponents() {
+    Qualified<Executor> backgroundExecutor = Qualified.qualified(Background.class, Executor.class);
     Qualified<Executor> blockingExecutor = Qualified.qualified(Blocking.class, Executor.class);
     Qualified<Executor> lightweightExecutor =
         Qualified.qualified(Lightweight.class, Executor.class);
@@ -57,16 +58,18 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar {
             .name(LIBRARY_NAME)
             .add(Dependency.required(FirebaseApp.class))
             .add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
+            .add(Dependency.required(backgroundExecutor))
             .add(Dependency.required(blockingExecutor))
             .add(Dependency.required(lightweightExecutor))
-            .factory(c -> buildFirebaseAppDistribution(c, blockingExecutor, lightweightExecutor))
+            .factory(c -> buildFirebaseAppDistribution(c, backgroundExecutor, blockingExecutor, lightweightExecutor))
             // construct FirebaseAppDistribution instance on startup so we can register for
             // activity lifecycle callbacks before the API is called
             .alwaysEager()
             .build(),
         Component.builder(FeedbackSender.class)
             .add(Dependency.required(FirebaseApp.class))
             .add(Dependency.requiredProvider(FirebaseInstallationsApi.class))
+            .add(Dependency.required(backgroundExecutor))
             .add(Dependency.required(blockingExecutor))
             .factory(c -> buildFeedbackSender(c, c.get(blockingExecutor)))
             .build(),
@@ -87,13 +90,13 @@ private FeedbackSender buildFeedbackSender(
     return new FeedbackSender(testerApiClient, blockingExecutor);
   }
 
-  // TODO(b/258264924): Migrate to go/firebase-android-executors
-  @SuppressLint("ThreadPoolCreation")
   private FirebaseAppDistribution buildFirebaseAppDistribution(
       ComponentContainer container,
+      Qualified<Executor> backgroundExecutorType,
       Qualified<Executor> blockingExecutorType,
       Qualified<Executor> lightweightExecutorType) {
     FirebaseApp firebaseApp = container.get(FirebaseApp.class);
+    Executor backgroundExecutor = container.get(backgroundExecutorType);
     Executor blockingExecutor = container.get(blockingExecutorType);
     Executor lightweightExecutor = container.get(lightweightExecutorType);
     Context context = firebaseApp.getApplicationContext();
@@ -120,9 +123,9 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
             signInStorage,
             lifecycleNotifier,
             releaseIdentifier,
-            new ScreenshotTaker(firebaseApp, lifecycleNotifier),
+            new ScreenshotTaker(firebaseApp, lifecycleNotifier, backgroundExecutor),
             lightweightExecutor,
-            Executors.newSingleThreadExecutor());
+            blockingExecutor);
 
     if (context instanceof Application) {
       Application firebaseApplication = (Application) context;

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

@@ -14,7 +14,6 @@
 
 package com.google.firebase.appdistribution.impl;
 
-import android.annotation.SuppressLint;
 import android.annotation.TargetApi;
 import android.app.Activity;
 import android.content.Context;
@@ -37,7 +36,6 @@
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.util.concurrent.Executor;
-import java.util.concurrent.Executors;
 
 /** A class that takes screenshots of the host app. */
 class ScreenshotTaker {
@@ -47,23 +45,15 @@ class ScreenshotTaker {
 
   private final FirebaseApp firebaseApp;
   private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
-  private final Executor taskExecutor;
+  private final Executor backgroundExecutor;
 
-  // TODO(b/258264924): Migrate to go/firebase-android-executors
-  @SuppressLint("ThreadPoolCreation")
-  ScreenshotTaker(
-      FirebaseApp firebaseApp, FirebaseAppDistributionLifecycleNotifier lifecycleNotifier) {
-    this(firebaseApp, lifecycleNotifier, Executors.newSingleThreadExecutor());
-  }
-
-  @VisibleForTesting
   ScreenshotTaker(
       FirebaseApp firebaseApp,
       FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
-      Executor taskExecutor) {
+      Executor backgroundExecutor) {
     this.firebaseApp = firebaseApp;
     this.lifecycleNotifier = lifecycleNotifier;
-    this.taskExecutor = taskExecutor;
+    this.backgroundExecutor = backgroundExecutor;
   }
 
   /**
@@ -83,7 +73,7 @@ Task<Uri> takeScreenshot() {
   /** Deletes any previously taken screenshot. */
   Task<Void> deleteScreenshot() {
     return TaskUtils.runAsyncInTask(
-        taskExecutor,
+        backgroundExecutor,
         () -> {
           firebaseApp.getApplicationContext().deleteFile(SCREENSHOT_FILE_NAME);
           return null;
@@ -159,7 +149,7 @@ private Task<Uri> writeToFile(@Nullable Bitmap bitmap) {
       return Tasks.forResult(null);
     }
     return TaskUtils.runAsyncInTask(
-        taskExecutor,
+        backgroundExecutor,
         () -> {
           try (FileOutputStream outputStream = openFileOutputStream()) {
             // PNG is a lossless format, the compression factor (100) is ignored

firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/SignInStorage.java

@@ -30,6 +30,9 @@ class SignInStorage {
     this.signInSharedPreferences =
         new Lazy(
             () ->
+                // TODO(lkellogg): This constructs a SharedPreferences object, which touches disk
+                //   and therefore should be run on a @Background executor. This could ideally be
+                //   done by a new shared component.
                 applicationContext.getSharedPreferences(
                     SIGNIN_PREFERENCES_NAME, Context.MODE_PRIVATE));
   }