Some cleanup in App Distro's Dagger usage

Author: lfkellogg

firebase-appdistribution/firebase-appdistribution.gradle

@@ -55,11 +55,11 @@ dependencies {
     testImplementation project(':integ-testing')
     runtimeOnly project(':firebase-installations')
 
-    implementation 'javax.inject:javax.inject:1'
-    vendor ('com.google.dagger:dagger:2.43.2') {
+    implementation libs.javax.inject
+    vendor (libs.dagger.dagger) {
         exclude group: "javax.inject", module: "javax.inject"
     }
-    annotationProcessor 'com.google.dagger:dagger-compiler:2.43.2'
+    annotationProcessor libs.dagger.compiler
 
     testImplementation 'junit:junit:4.13.2'
     testImplementation "org.robolectric:robolectric:$robolectricVersion"

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

@@ -32,9 +32,11 @@
 import java.io.IOException;
 import java.util.concurrent.Executor;
 import javax.inject.Inject;
+import javax.inject.Singleton;
 import javax.net.ssl.HttpsURLConnection;
 
 /** Class that handles updateApp functionality for AABs in {@link FirebaseAppDistribution}. */
+@Singleton // Only one update should happen at a time, even across FirebaseAppDistribution instances
 class AabUpdater {
   private static final String TAG = "AabUpdater";
 

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

@@ -24,8 +24,10 @@
 import com.google.firebase.appdistribution.FirebaseAppDistributionException;
 import java.util.concurrent.Executor;
 import javax.inject.Inject;
+import javax.inject.Singleton;
 
 /** Class that handles installing APKs in {@link FirebaseAppDistribution}. */
+@Singleton // Only one update should happen at a time, even across FirebaseAppDistribution instances
 class ApkInstaller {
   private static final String TAG = "ApkInstaller";
 

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

@@ -37,9 +37,11 @@
 import java.util.concurrent.Executor;
 import java.util.jar.JarFile;
 import javax.inject.Inject;
+import javax.inject.Singleton;
 import javax.net.ssl.HttpsURLConnection;
 
 /** Class that handles updateApp functionality for APKs in {@link FirebaseAppDistribution}. */
+@Singleton // Only one update should happen at a time, even across FirebaseAppDistribution instances
 class ApkUpdater {
   private static final String TAG = "ApkUpdater";
   private static final int UPDATE_INTERVAL_MS = 250;

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

@@ -48,11 +48,14 @@
 import java.util.concurrent.Executor;
 import java.util.concurrent.atomic.AtomicBoolean;
 import javax.inject.Inject;
+import javax.inject.Singleton;
 
 /**
  * This class is the "real" implementation of the Firebase App Distribution API which should only be
  * included in pre-release builds.
  */
+// TODO(b/266704696): This currently only supports one FirebaseAppDistribution instance app-wide
+@Singleton
 class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
 
   private static final String TAG = "Impl";

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

@@ -18,11 +18,13 @@
 
 import android.app.Activity;
 import android.app.Application;
+import android.app.Application.ActivityLifecycleCallbacks;
 import android.os.Bundle;
 import android.os.Looper;
 import androidx.annotation.GuardedBy;
 import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 import com.google.android.gms.tasks.SuccessContinuation;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
@@ -35,8 +37,8 @@
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
-@Singleton
-class FirebaseAppDistributionLifecycleNotifier implements Application.ActivityLifecycleCallbacks {
+@Singleton // Only one lifecycle notifier is required across the entire app
+class FirebaseAppDistributionLifecycleNotifier {
 
   /** An {@link Executor} that runs tasks on the current thread. */
   private static final Executor DIRECT_EXECUTOR = Runnable::run;
@@ -59,8 +61,12 @@ interface NullableActivityFunction<T> {
   }
 
   @UiThread private final Executor uiThreadExecutor;
+  @VisibleForTesting final LifecycleCallbacks lifecycleCallbacks = new LifecycleCallbacks();
   private final Object lock = new Object();
 
+  @GuardedBy("lock")
+  private boolean lifecycleCallbacksRegistered = false;
+
   @GuardedBy("lock")
   private Activity currentActivity;
 
@@ -112,6 +118,21 @@ interface OnActivityDestroyedListener {
     void onDestroyed(Activity activity);
   }
 
+  /**
+   * Register for activity lifecycle callbacks for the application.
+   *
+   * <p>This must be called for this class to provide information about activity state.
+   */
+  void registerActivityLifecycleCallbacks(Application application) {
+    synchronized (lock) {
+      // Make sure we register for callbacks only once, so callbacks are not called twice
+      if (!lifecycleCallbacksRegistered) {
+        application.registerActivityLifecycleCallbacks(lifecycleCallbacks);
+        lifecycleCallbacksRegistered = true;
+      }
+    }
+  }
+
   /**
    * Apply a function to a foreground activity, when one is available, returning a {@link Task} that
    * will complete immediately after the function is applied.
@@ -311,67 +332,71 @@ void addOnActivityPausedListener(@NonNull OnActivityPausedListener listener) {
     }
   }
 
-  @Override
-  public void onActivityCreated(@NonNull Activity activity, @Nullable Bundle bundle) {
-    synchronized (lock) {
-      updateCurrentActivity(activity);
-      for (OnActivityCreatedListener listener : onActivityCreatedListeners) {
-        listener.onCreated(activity);
+  @VisibleForTesting
+  class LifecycleCallbacks implements ActivityLifecycleCallbacks {
+
+    @Override
+    public void onActivityCreated(@NonNull Activity activity, @Nullable Bundle bundle) {
+      synchronized (lock) {
+        updateCurrentActivity(activity);
+        for (OnActivityCreatedListener listener : onActivityCreatedListeners) {
+          listener.onCreated(activity);
+        }
       }
     }
-  }
 
-  @Override
-  public void onActivityStarted(@NonNull Activity activity) {
-    synchronized (lock) {
-      updateCurrentActivity(activity);
-      for (OnActivityStartedListener listener : onActivityStartedListeners) {
-        listener.onStarted(activity);
+    @Override
+    public void onActivityStarted(@NonNull Activity activity) {
+      synchronized (lock) {
+        updateCurrentActivity(activity);
+        for (OnActivityStartedListener listener : onActivityStartedListeners) {
+          listener.onStarted(activity);
+        }
       }
     }
-  }
 
-  @Override
-  public void onActivityResumed(@NonNull Activity activity) {
-    synchronized (lock) {
-      updateCurrentActivity(activity);
-      for (OnActivityResumedListener listener : onActivityResumedListeners) {
-        listener.onResumed(activity);
+    @Override
+    public void onActivityResumed(@NonNull Activity activity) {
+      synchronized (lock) {
+        updateCurrentActivity(activity);
+        for (OnActivityResumedListener listener : onActivityResumedListeners) {
+          listener.onResumed(activity);
+        }
       }
     }
-  }
 
-  @Override
-  public void onActivityPaused(@NonNull Activity activity) {
-    synchronized (lock) {
-      if (currentActivity == activity) {
-        updateCurrentActivity(null);
-      }
-      for (OnActivityPausedListener listener : onActivityPausedListeners) {
-        listener.onPaused(activity);
+    @Override
+    public void onActivityPaused(@NonNull Activity activity) {
+      synchronized (lock) {
+        if (currentActivity == activity) {
+          updateCurrentActivity(null);
+        }
+        for (OnActivityPausedListener listener : onActivityPausedListeners) {
+          listener.onPaused(activity);
+        }
       }
     }
-  }
 
-  @Override
-  public void onActivityStopped(@NonNull Activity activity) {}
+    @Override
+    public void onActivityStopped(@NonNull Activity activity) {}
 
-  @Override
-  public void onActivitySaveInstanceState(@NonNull Activity activity, @NonNull Bundle bundle) {}
+    @Override
+    public void onActivitySaveInstanceState(@NonNull Activity activity, @NonNull Bundle bundle) {}
 
-  @Override
-  public void onActivityDestroyed(@NonNull Activity activity) {
-    synchronized (lock) {
-      // If an activity is destroyed, delete all references to it, including the previous activity
-      if (currentActivity == activity) {
-        updateCurrentActivity(null);
-      }
-      if (previousActivity == activity) {
-        previousActivity = null;
-      }
+    @Override
+    public void onActivityDestroyed(@NonNull Activity activity) {
+      synchronized (lock) {
+        // If an activity is destroyed, delete all references to it, including the previous activity
+        if (currentActivity == activity) {
+          updateCurrentActivity(null);
+        }
+        if (previousActivity == activity) {
+          previousActivity = null;
+        }
 
-      for (OnActivityDestroyedListener listener : onDestroyedListeners) {
-        listener.onDestroyed(activity);
+        for (OnActivityDestroyedListener listener : onDestroyedListeners) {
+          listener.onDestroyed(activity);
+        }
       }
     }
   }

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

@@ -99,7 +99,7 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(
 
     if (applicationContext instanceof Application) {
       Application application = (Application) applicationContext;
-      application.registerActivityLifecycleCallbacks(appDistroComponent.getLifecycleNotifier());
+      appDistroComponent.getLifecycleNotifier().registerActivityLifecycleCallbacks(application);
     } else {
       LogWrapper.e(
           TAG,

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

@@ -37,8 +37,10 @@
 import java.io.IOException;
 import java.util.concurrent.Executor;
 import javax.inject.Inject;
+import javax.inject.Singleton;
 
 /** A class that takes screenshots of the host app. */
+@Singleton // All instances store images at the same URI, so there should really only be one
 class ScreenshotTaker {
 
   static final String SCREENSHOT_FILE_NAME =

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

@@ -23,8 +23,11 @@
 import com.google.firebase.annotations.concurrent.Background;
 import java.util.concurrent.Executor;
 import javax.inject.Inject;
+import javax.inject.Singleton;
 
 /** Class that handles storage for App Distribution SignIn persistence. */
+// TODO(b/266704696): This currently only supports one FirebaseAppDistribution instance app-wide
+@Singleton
 class SignInStorage {
   @VisibleForTesting
   static final String SIGNIN_PREFERENCES_NAME = "FirebaseAppDistributionSignInStorage";

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

@@ -39,8 +39,11 @@
 import java.util.List;
 import java.util.concurrent.Executor;
 import javax.inject.Inject;
+import javax.inject.Singleton;
 
 /** Class that handles signing in the tester. */
+// TODO(b/266704696): This currently only supports one FirebaseAppDistribution instance app-wide
+@Singleton
 class TesterSignInManager {
   private static final String TAG = "TesterSignInManager";