Incorporate some feedback

Author: lfkellogg

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

@@ -32,6 +32,7 @@
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.android.gms.tasks.Tasks;
 import com.google.firebase.FirebaseApp;
+import com.google.firebase.annotations.concurrent.Lightweight;
 import com.google.firebase.appdistribution.AppDistributionRelease;
 import com.google.firebase.appdistribution.BinaryType;
 import com.google.firebase.appdistribution.FirebaseAppDistribution;
@@ -40,7 +41,6 @@
 import com.google.firebase.appdistribution.UpdateProgress;
 import com.google.firebase.appdistribution.UpdateStatus;
 import com.google.firebase.appdistribution.UpdateTask;
-import com.google.firebase.concurrent.FirebaseExecutors;
 import java.util.concurrent.Executor;
 
 /**
@@ -61,7 +61,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
   private final SequentialReference<AppDistributionReleaseInternal> cachedNewRelease;
   private TaskCache<UpdateTask> updateIfNewReleaseAvailableTaskCache = new TaskCache<>();
   private TaskCache<Task<AppDistributionRelease>> checkForNewReleaseTaskCache = new TaskCache<>();
-  private Executor lightweightExecutor;
+  @Lightweight private Executor lightweightExecutor;
   private AlertDialog updateConfirmationDialog;
   private AlertDialog signInConfirmationDialog;
   @Nullable private Activity dialogHostActivity = null;
@@ -81,15 +81,15 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
       @NonNull AabUpdater aabUpdater,
       @NonNull SignInStorage signInStorage,
       @NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
-      @NonNull Executor lightweightExecutor) {
+      @NonNull @Lightweight Executor lightweightExecutor) {
     this.firebaseApp = firebaseApp;
     this.testerSignInManager = testerSignInManager;
     this.newReleaseFetcher = newReleaseFetcher;
     this.apkUpdater = apkUpdater;
     this.aabUpdater = aabUpdater;
     this.signInStorage = signInStorage;
     this.lifecycleNotifier = lifecycleNotifier;
-    this.cachedNewRelease = SequentialReference.withBaseExecutor(lightweightExecutor);
+    this.cachedNewRelease = new SequentialReference<>(lightweightExecutor);
     this.lightweightExecutor = lightweightExecutor;
     lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
     lifecycleNotifier.addOnActivityPausedListener(this::onActivityPaused);
@@ -220,8 +220,7 @@ public Task<Void> signInTester() {
   public void signOutTester() {
     cachedNewRelease
         .set(null)
-        .addOnSuccessListener(
-            FirebaseExecutors.directExecutor(), unused -> signInStorage.setSignInStatus(false));
+        .addOnSuccessListener(lightweightExecutor, unused -> signInStorage.setSignInStatus(false));
   }
 
   @Override
@@ -240,7 +239,7 @@ public synchronized Task<AppDistributionRelease> checkForNewRelease() {
                 .checkForNewRelease()
                 .onSuccessTask(lightweightExecutor, release -> cachedNewRelease.set(release))
                 .onSuccessTask(
-                    FirebaseExecutors.directExecutor(),
+                    lightweightExecutor,
                     release ->
                         Tasks.forResult(ReleaseUtils.convertToAppDistributionRelease(release)))
                 .addOnFailureListener(

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

@@ -14,8 +14,6 @@
 
 package com.google.firebase.appdistribution.impl;
 
-import androidx.annotation.VisibleForTesting;
-import com.google.android.gms.tasks.OnSuccessListener;
 import com.google.android.gms.tasks.Task;
 import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.firebase.concurrent.FirebaseExecutors;
@@ -30,21 +28,12 @@ class SequentialReference<T> {
   private final Executor sequentialExecutor;
   private T value;
 
-  /** Get a {@link SequentialReference} that controls access uses the given sequential executor. */
-  static SequentialReference withSequentialExecutor(Executor sequentialExecutor) {
-    return new SequentialReference(sequentialExecutor);
-  }
-
   /**
-   * Get a {@link SequentialReference} that controls access using its own sequential executor backed
-   * by the given base executor.
+   * Constructor for a {@link SequentialReference} that controls access using its own sequential
+   * executor backed by the given base executor.
    */
-  static SequentialReference withBaseExecutor(Executor baseExecutor) {
-    return new SequentialReference(FirebaseExecutors.newSequentialExecutor(baseExecutor));
-  }
-
-  private SequentialReference(Executor sequentialExecutor) {
-    this.sequentialExecutor = sequentialExecutor;
+  SequentialReference(Executor baseExecutor) {
+    this.sequentialExecutor = FirebaseExecutors.newSequentialExecutor(baseExecutor);
   }
 
   /**
@@ -65,18 +54,12 @@ Task<T> set(T newValue) {
   /**
    * Gets a {@link Task} that will complete with the value.
    *
-   * <p>Unless a direct executer is passed to {@link Task#addOnSuccessListener(Executor,
-   * OnSuccessListener)} or similar methods, be aware that the value may have changed by the time it
-   * is used.
+   * <p>In some cases the value may have changed by the time any callbacks attached to the returned
+   * {@link Task} are called.
    */
   Task<T> get() {
     TaskCompletionSource<T> taskCompletionSource = new TaskCompletionSource<>();
     sequentialExecutor.execute(() -> taskCompletionSource.setResult(value));
     return taskCompletionSource.getTask();
   }
-
-  @VisibleForTesting
-  T getSnapshot() {
-    return value;
-  }
 }

firebase-appdistribution/src/test/java/com/google/firebase/appdistribution/impl/FirebaseAppDistributionServiceImplTest.java

@@ -221,7 +221,9 @@ public void checkForNewRelease_whenCheckForNewReleaseSucceeds_returnsRelease()
     awaitTask(task);
 
     assertEquals(TEST_RELEASE_NEWER_AAB, task.getResult());
-    assertEquals(internalRelease, firebaseAppDistribution.getCachedNewRelease().getSnapshot());
+    AppDistributionReleaseInternal cachedNewRelease =
+        awaitTask(firebaseAppDistribution.getCachedNewRelease().get());
+    assertEquals(internalRelease, cachedNewRelease);
   }
 
   @Test