From ec07e815f344ae58ca6b2cd8e5f526b69b0f75df Mon Sep 17 00:00:00 2001 From: Lee Kellogg Date: Wed, 7 Dec 2022 10:09:54 -0500 Subject: [PATCH 1/2] Migrate executors in fad/in-app-feedback --- .../impl/FirebaseAppDistributionImpl.java | 18 +++++++++-------- .../FirebaseAppDistributionRegistrar.java | 17 +++++++++------- .../appdistribution/impl/ScreenshotTaker.java | 20 +++++-------------- .../appdistribution/impl/SignInStorage.java | 3 +++ 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java index 26761a4facd..027c04b2bc4 100644 --- a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java +++ b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionImpl.java @@ -68,7 +68,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution { private final SequentialReference cachedNewRelease; private final ReleaseIdentifier releaseIdentifier; private final ScreenshotTaker screenshotTaker; - private final Executor taskExecutor; + private final Executor blockingExecutor; private final FirebaseAppDistributionNotificationsManager notificationsManager; private TaskCache updateIfNewReleaseAvailableTaskCache = new TaskCache<>(); private TaskCache> 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); diff --git a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java index 0f03c5fdff1..076501558d5 100644 --- a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java +++ b/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> getComponents() { + Qualified backgroundExecutor = Qualified.qualified(Background.class, Executor.class); Qualified blockingExecutor = Qualified.qualified(Blocking.class, Executor.class); Qualified lightweightExecutor = Qualified.qualified(Lightweight.class, Executor.class); @@ -57,9 +58,10 @@ 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() @@ -67,6 +69,7 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar { 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 backgroundExecutorType, Qualified blockingExecutorType, Qualified 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; diff --git a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java index 150b2824f9b..c9ad4dd71d1 100644 --- a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java +++ b/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 takeScreenshot() { /** Deletes any previously taken screenshot. */ Task deleteScreenshot() { return TaskUtils.runAsyncInTask( - taskExecutor, + backgroundExecutor, () -> { firebaseApp.getApplicationContext().deleteFile(SCREENSHOT_FILE_NAME); return null; @@ -159,7 +149,7 @@ private Task 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 diff --git a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/SignInStorage.java b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/SignInStorage.java index 4cae3bdc107..5bd9a6b7b3a 100644 --- a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/SignInStorage.java +++ b/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)); } From 8e75ef4e441f8a3fc20eb00b355ce51f2ba2dcc6 Mon Sep 17 00:00:00 2001 From: Lee Kellogg Date: Tue, 20 Dec 2022 17:07:42 -0500 Subject: [PATCH 2/2] Add an import and format --- .../impl/FirebaseAppDistributionRegistrar.java | 6 ++++-- .../firebase/appdistribution/impl/ScreenshotTaker.java | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java index 076501558d5..bb49e9e7420 100644 --- a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java +++ b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionRegistrar.java @@ -14,7 +14,6 @@ package com.google.firebase.appdistribution.impl; -import android.annotation.SuppressLint; import android.app.Application; import android.content.Context; import androidx.annotation.Keep; @@ -61,7 +60,10 @@ public class FirebaseAppDistributionRegistrar implements ComponentRegistrar { .add(Dependency.required(backgroundExecutor)) .add(Dependency.required(blockingExecutor)) .add(Dependency.required(lightweightExecutor)) - .factory(c -> buildFirebaseAppDistribution(c, backgroundExecutor, 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() diff --git a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java index c9ad4dd71d1..9436d5ca3fa 100644 --- a/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java +++ b/firebase-appdistribution/src/main/java/com/google/firebase/appdistribution/impl/ScreenshotTaker.java @@ -14,6 +14,7 @@ package com.google.firebase.appdistribution.impl; +import android.annotation.SuppressLint; import android.annotation.TargetApi; import android.app.Activity; import android.content.Context;