Removing dependency for activities #3298

manny-jimenez · 2022-01-10T23:06:35Z

No description provided.

google-oss-bot · 2022-01-10T23:15:04Z

Coverage Report ¹

Affected Products

`firebase-app-distribution`

Overall coverage changed from ? (9cd2475) to 74.70% (9745d50) by ?.

33 individual files with coverage change

Filename	Base (`9cd2475`)	Merge (9745d50)	Diff
AabUpdater.java	?	98.59%	?
ApkInstaller.java	?	93.88%	?
ApkUpdater.java	?	72.80%	?
AppDistributionRelease.java	?	100.00%	?
AppDistributionReleaseInternal.java	?	100.00%	?
AppIconSource.java	?	85.71%	?
AutoValue_AppDistributionRelease.java	?	65.45%	?
AutoValue_AppDistributionReleaseInternal.java	?	67.86%	?
AutoValue_UpdateProgress.java	?	65.96%	?
BinaryType.java	?	100.00%	?
Constants.java	?	0.00%	?
FirebaseAppDistribution.java	?	91.48%	?
FirebaseAppDistributionException.java	?	87.10%	?
FirebaseAppDistributionFileProvider.java	?	0.00%	?
FirebaseAppDistributionLifecycleNotifier.java	?	46.67%	?
FirebaseAppDistributionNotificationsManager.java	?	92.68%	?
FirebaseAppDistributionRegistrar.java	?	90.91%	?
FirebaseAppDistributionTesterApiClient.java	?	91.21%	?
HttpsUrlConnectionFactory.java	?	50.00%	?
InstallActivity.java	?	4.23%	?
LogWrapper.java	?	64.71%	?
NewReleaseFetcher.java	?	87.95%	?
OnProgressListener.java	?	0.00%	?
ReleaseIdentificationUtils.java	?	15.52%	?
ReleaseUtils.java	?	76.92%	?
SignInResultActivity.java	?	0.00%	?
SignInStorage.java	?	57.14%	?
TaskUtils.java	?	93.75%	?
TesterSignInManager.java	?	87.91%	?
UpdateProgress.java	?	100.00%	?
UpdateStatus.java	?	100.00%	?
UpdateTask.java	?	100.00%	?
UpdateTaskImpl.java	?	82.81%	?

Test Logs

Merge (9745d50): https://android-ci.firebaseopensource.com/view/gcs/android-ci/pr-logs/pull/firebase_firebase-android-sdk/3298/check-coverage-changed/1480995924070109188

Notes

Commit (9745d50) is created by Prow via merging PR base commit (9cd2475) and head commit (0e12c55).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/Bpa6BK4yDU.html

google-oss-bot · 2022-01-10T23:19:10Z

Size Report ¹

Affected Products

firebase-app-distribution
Type Base (9cd2475) Merge (9745d50) Diff
aar 120 kB 120 kB -81 B (-0.1%)
apk (aggressive) 739 kB 739 kB -24 B (-0.0%)
apk (release) 1.55 MB 1.55 MB -140 B (-0.0%)

Test Logs

Notes

Commit (9745d50) is created by Prow via merging PR base commit (9cd2475) and head commit (0e12c55).

https://storage.googleapis.com/firebase-sdk-metric-reports/Y89oHhNpbP.html

lfkellogg · 2022-01-11T14:33:44Z

...se-app-distribution/src/main/java/com/google/firebase/appdistribution/NewReleaseFetcher.java

-    this.firebaseAppDistributionTesterApiClient = firebaseAppDistributionTesterApiClient;
-    this.firebaseInstallationsApiProvider = firebaseInstallationsApiProvider;
-    this.taskExecutor = Executors.newSingleThreadExecutor();
+    this(


lfkellogg · 2022-01-11T14:36:45Z

firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/ApkInstaller.java

@@ -93,7 +97,7 @@ void handleAppResume(Activity activity) {
        cachedInstallApkPath = path;
      } else {
        // only start the install activity if current Activity is in the foreground
-        startInstallActivity(path, currentActivity);
+        startInstallActivity(path);


The if case on line 95 can be removed right?

Yes it can!

Can we now start the Install Activity regardless of whether or not the customer's app is in the foreground?

Good question! Currently working on trying to see if there is a set up to make that work. In its current set up if the user backgrounds the app it seems to hang.

It seems like this is not possible due to Install Activity relying on an activity to show unknown sources UI.

That is surprising. I'd think that once the InstallActivity starts, then it's in the foreground and we can show the dialog. That's what happened when you were calling UpdateApp from a background thread, which worked.

What error/stacktrace are you seeing in the logs?

I get a windowLeaked error. I can repro and show the error message. I believe that was because the app itself wasn't in the background, but once you background the app it wants to display the unknown sources UI but since the app is in the background you get this leaked error.

Why does the app get backgrounded? Maybe we could look at it together over VC.

We discussed this in person. Previously, backgrounding the app after starting the basic config would fail the task correctly. With this change it instead hangs the task.

I'm fine punting on this for now.

To clarify it hangs if we don't depend on the currentActivity at all with checking what to do with InstallActivity.

rachaprince

LGTM so far!

manny-jimenez · 2022-01-11T17:12:20Z

/run binary-size

lfkellogg · 2022-01-11T18:36:46Z

firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/ApkInstaller.java

@@ -111,6 +111,7 @@ private void startInstallActivity(String path, Activity currentActivity) {
    }
    Intent intent = new Intent(currentActivity, InstallActivity.class);
    intent.putExtra("INSTALL_PATH", path);
+    intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);


Why add this flag?

Good catch! Leftover from trying to use application context. When using application Context it needs to be a new task but since that was taken out this should also be taken out.

Oh interesting. Why does it need to a be a new task when using application context?

I ran into the issue but this post sums up the issue and the reason for the fix. https://stackoverflow.com/questions/3918517/calling-startactivity-from-outside-of-an-activity-context

Hmm, ok the fact that you saw that error is making me less confident in my suggestion to use intents + activities. It feels weird to have to start a new task every time.

We do this in the AAB updater

firebase-android-sdk/firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/AabUpdater.java

Line 160 in 712e5b8

updateIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);

With the application context we aren't guaranteed an activity so the flag makes sense to me.

I think it makes more sense in the AAB updater since we are launching an activity in a different app, Play.

In the case I was proposing, where we would launch a new intent/activity in order to simply show a dialog in the app, we wouldn't want an additional task. It should ideally just continue in the same task.

This error is especially interesting, since we should be starting the install activity as a new task anyway according to the manifest I think https://github.com/firebase/firebase-android-sdk/blob/master/firebase-app-distribution/src/main/AndroidManifest.xml#L34-L35

From the docs:
singleTask
The system creates a new task and instantiates the activity at the root of the new task.

lfkellogg · 2022-01-11T19:11:01Z

firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/AabUpdater.java

@@ -48,18 +50,22 @@
  private AppDistributionReleaseInternal aabReleaseInProgress;

  private final Object updateAabLock = new Object();
+  private final Context applicationContext;


Nit: for brevity, can we just call this context everywhere instead of applicationContext?

I did see we used context elsewhere. Context being so vague is the reason I added application to it, but I have no issue making it context.

lfkellogg · 2022-01-11T19:13:30Z

firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/AabUpdater.java


-  AabUpdater() {
+  AabUpdater(@NonNull FirebaseApp firebaseApp) {


This constructor should just take a Context object since that's all you need.

Good thought! I had done this elsewhere before should do it here as well.

lfkellogg · 2022-01-11T19:17:07Z

firebase-app-distribution/src/test/java/com/google/firebase/appdistribution/AabUpdaterTest.java

@@ -84,7 +98,8 @@ public void setup() throws IOException, FirebaseAppDistributionException {

    aabUpdater =
        Mockito.spy(
-            new AabUpdater(mockLifecycleNotifier, mockHttpsUrlConnectionFactory, testExecutor));
+            new AabUpdater(
+                firebaseApp, mockLifecycleNotifier, mockHttpsUrlConnectionFactory, testExecutor));


If you take my suggestion and require a Context here instead of a FirebaseApp, then you can just pass in ApplicationProvider.getApplicationContext().

lfkellogg

Some tiny nits. Looks great!

lfkellogg · 2022-01-11T19:50:51Z

firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/ApkUpdater.java

+            new BufferedOutputStream(
+                context.openFileOutput(fileName, fileCreationMode));


This can go back to being a one-liner.

lfkellogg · 2022-01-11T19:51:06Z

firebase-app-distribution/src/main/java/com/google/firebase/appdistribution/ApkUpdater.java

+          context
+              .getApplicationInfo()
+              .loadLabel(context.getPackageManager())
+              .toString();


This can also be a one-liner

lfkellogg · 2022-01-11T19:51:49Z

...-distribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java

@@ -148,7 +148,6 @@ public UpdateTask updateIfNewReleaseAvailable() {
        return cachedUpdateIfNewReleaseTask;
      }
    }
-


Nit: can you put this empty line back?

lfkellogg · 2022-01-11T20:06:03Z

firebase-app-distribution/src/test/java/com/google/firebase/appdistribution/AabUpdaterTest.java

+                .setApplicationId(TEST_APP_ID_1)
+                .setProjectId(TEST_PROJECT_ID)
+                .setApiKey(TEST_API_KEY)
+                .build());


In case you missed my other comment, you don't need to a create a FirebaseApp here. You can just inject ApplicationProvider.getApplicationContext() into the constructor, below.

I did miss this comment. Will fix now.

Removing dependancy for activities

f26de2e

google-oss-bot added the size/L label Jan 10, 2022

Fixing some tests

afa49e7

lfkellogg reviewed Jan 11, 2022

View reviewed changes

rachaprince reviewed Jan 11, 2022

View reviewed changes

Fixing tests and reverting APK changes

cf274df

google-oss-bot added size/M and removed size/L labels Jan 11, 2022

Removing comments

535b182

lfkellogg reviewed Jan 11, 2022

View reviewed changes

Removing left over line.

dd9215f

lfkellogg reviewed Jan 11, 2022

View reviewed changes

Responding to feedback

8b433d1

rachaprince approved these changes Jan 11, 2022

View reviewed changes

Changing context name

010a7f9

lfkellogg approved these changes Jan 11, 2022

View reviewed changes

Lint fixes

00faad7

lfkellogg reviewed Jan 11, 2022

View reviewed changes

Cleaning up test

0e12c55

lfkellogg approved these changes Jan 12, 2022

View reviewed changes

manny-jimenez merged commit 55ffdd5 into master Jan 12, 2022

manny-jimenez deleted the fad_remove_activity_reference branch January 12, 2022 15:22

firebase locked and limited conversation to collaborators Feb 12, 2022


		AabUpdater() {
		AabUpdater(@NonNull FirebaseApp firebaseApp) {

		new BufferedOutputStream(
		context.openFileOutput(fileName, fileCreationMode));

Removing dependency for activities #3298

Removing dependency for activities #3298

Uh oh!

Conversation

manny-jimenez commented Jan 10, 2022

Uh oh!

google-oss-bot commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-app-distribution

Test Logs

Notes

Uh oh!

google-oss-bot commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-app-distribution

Test Logs

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rachaprince left a comment

Choose a reason for hiding this comment

Uh oh!

manny-jimenez commented Jan 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfkellogg Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lfkellogg left a comment

google-oss-bot commented Jan 10, 2022 •

edited

Loading

Coverage Report ¹

`firebase-app-distribution`

google-oss-bot commented Jan 10, 2022 •

edited

Loading

Size Report ¹

`firebase-app-distribution`

lfkellogg Jan 11, 2022 •

edited

Loading