Move SharedPreferences usage to background thread #4480
Conversation
Generated by 🚫 Danger |
lfkellogg
force-pushed
the
lk/shared-prefs-to-background
branch
from
337977f to
0a76632
Compare
lfkellogg
force-pushed
the
lk/shared-prefs-to-background
branch
from
0a76632 to
a4b694f
Compare
| return lifecycleNotifier | ||
| .applyToForegroundActivityTask(this::showSignInConfirmationDialog) | ||
| .onSuccessTask(lightweightExecutor, unused -> signInTester()); | ||
| }) |
There was a problem hiding this comment.
maybe extract a method named something like signInIfNeeded() or ensureSignedIn() which encapsulates this
There was a problem hiding this comment.
actually, this seems like TesterSignInManager.signInTester() - could that be called from here?
There was a problem hiding this comment.
TesterSignInManager.signInTester() opens the actual sign in screen in the browser.
Agreed that it would be nice to extract this though, if only to make this method more readable. I called it signInWithConfirmationIfNeeded().
| return aabUpdater.updateAab(release); | ||
| } else { | ||
| return apkUpdater.updateApk(release, showDownloadInNotificationManager); | ||
| "Tester is not signed in", AUTHENTICATION_FAILURE)); |
There was a problem hiding this comment.
Can this use assertTesterIsSignedIn()?
There was a problem hiding this comment.
Oh actually yes! It's a little tricky because I have to use TaskUtils.onSuccessUpdateTask() but it works.
| } | ||
|
|
||
| boolean getSignInStatus() { | ||
| return signInSharedPreferences.get().getBoolean(SIGNIN_TAG, false); | ||
| private SharedPreferences getSharedPreferencesBlocking() { |
There was a problem hiding this comment.
add comment explaining that this might involve I/O
| } | ||
| } | ||
| }); | ||
| executor.awaitTermination(100, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
Isn't 100 ms too short? The thread sleeps 100 ms for every iteration.
There was a problem hiding this comment.
Ah, true. Adjusted to 50ms sleeps and a 500ms timeout.
Coverage Report 1Affected Products
Test Logs |
Size Report 1Affected Products
Test Logs |
Startup Time Report 1Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS. Startup time comparison between the CI merge commit (6c18c4c) and the base commit (8a08968) are not available. No macrobenchmark data found for the base commit (8a08968). Analysis for the CI merge commit (6c18c4c) can be found at: |
* Migrate executors in fad/in-app-feedback * Add an import and format * Move SharedPreferences usage to background thread * Address feedback * Fix onSuccessUpdateTask
Followup from #4350
Creating a SharedPreferences object can touch storage, so we should do so only on a background thread.