From 8651185e36048032ea848f0df7bd3f1890b0fd95 Mon Sep 17 00:00:00 2001 From: Beth Thibodeau Date: Tue, 8 Jun 2021 19:35:05 -0400 Subject: [PATCH] Handle media load events while testing for resume An app can send additional notifications, resulting in onMediaDataLoaded calls, while we are waiting for the callback from its MediaBrowserService to return. Previously this would result in disconnecting and then starting a new attempt to connect to the MBS. Instead, 1. notify MediaDataManager immediately when we initiate the test connection, so that if we get additional onMediaDataLoaded calls while testing, it will not try again 2. only disconnect the mediaBrowser in onMediaDataLoaded when the key changes, since this is only relevant when resuming via the MBS. Otherwise we might prematurely disconnect while waiting for the callback result. Bug: 189702977 Test: manual - checked repro with test app Test: atest MediaResumeListenerTest Change-Id: I55cf2be6e62ef72c777899575d9caab78eda31be --- .../systemui/media/MediaResumeListener.kt | 10 ++++-- .../systemui/media/MediaResumeListenerTest.kt | 34 +++++++++++++++++-- 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt b/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt index 9aeb63d724e38..0da84fbac6000 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt @@ -163,8 +163,10 @@ class MediaResumeListener @Inject constructor( ) { if (useMediaResumption) { // If this had been started from a resume state, disconnect now that it's live - mediaBrowser?.disconnect() - mediaBrowser = null + if (!key.equals(oldKey)) { + mediaBrowser?.disconnect() + mediaBrowser = null + } // If we don't have a resume action, check if we haven't already if (data.resumeAction == null && !data.hasCheckedForResume && data.isLocalSession) { // TODO also check for a media button receiver intended for restarting (b/154127084) @@ -194,6 +196,9 @@ class MediaResumeListener @Inject constructor( */ private fun tryUpdateResumptionList(key: String, componentName: ComponentName) { Log.d(TAG, "Testing if we can connect to $componentName") + // Set null action to prevent additional attempts to connect + mediaDataManager.setResumeAction(key, null) + mediaBrowser?.disconnect() mediaBrowser = mediaBrowserFactory.create( object : ResumeMediaBrowser.Callback() { override fun onConnected() { @@ -202,7 +207,6 @@ class MediaResumeListener @Inject constructor( override fun onError() { Log.e(TAG, "Cannot resume with $componentName") - mediaDataManager.setResumeAction(key, null) mediaBrowser = null } diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt index 4e1627ff03433..150f4545bd43d 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt @@ -44,6 +44,7 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentCaptor import org.mockito.ArgumentMatchers.anyInt +import org.mockito.ArgumentMatchers.isNotNull import org.mockito.Captor import org.mockito.Mock import org.mockito.Mockito @@ -182,6 +183,31 @@ class MediaResumeListenerTest : SysuiTestCase() { verify(mediaDataManager).setResumeAction(KEY, null) } + @Test + fun testOnLoad_checksForResume_badService() { + // Set up MBS that will allow connection but not return valid media + val pm = mock(PackageManager::class.java) + whenever(mockContext.packageManager).thenReturn(pm) + val resolveInfo = ResolveInfo() + val serviceInfo = ServiceInfo() + serviceInfo.packageName = PACKAGE_NAME + resolveInfo.serviceInfo = serviceInfo + resolveInfo.serviceInfo.name = CLASS_NAME + val resumeInfo = listOf(resolveInfo) + whenever(pm.queryIntentServices(any(), anyInt())).thenReturn(resumeInfo) + + whenever(resumeBrowser.testConnection()).thenAnswer { + callbackCaptor.value.onError() + } + + // When media data is loaded that has not been checked yet, and does not have a MBS + resumeListener.onMediaDataLoaded(KEY, null, data) + executor.runAllReady() + + // Then we report back to the manager + verify(mediaDataManager).setResumeAction(eq(KEY), eq(null)) + } + @Test fun testOnLoad_remotePlayback_doesNotCheck() { // When media data is loaded that has not been checked yet, and is not local @@ -217,10 +243,11 @@ class MediaResumeListenerTest : SysuiTestCase() { // Then we test whether the service is valid executor.runAllReady() + verify(mediaDataManager).setResumeAction(eq(KEY), eq(null)) verify(resumeBrowser).testConnection() - // And since it is, we report back to the manager - verify(mediaDataManager).setResumeAction(eq(KEY), any()) + // And since it is, we send info to the manager + verify(mediaDataManager).setResumeAction(eq(KEY), isNotNull()) // But we do not tell it to add new controls verify(mediaDataManager, never()) @@ -291,8 +318,9 @@ class MediaResumeListenerTest : SysuiTestCase() { // Then we test whether the service is valid and set the resume action executor.runAllReady() + verify(mediaDataManager).setResumeAction(eq(KEY), eq(null)) verify(resumeBrowser).testConnection() - verify(mediaDataManager).setResumeAction(eq(KEY), capture(actionCaptor)) + verify(mediaDataManager, times(2)).setResumeAction(eq(KEY), capture(actionCaptor)) // When the resume action is run actionCaptor.value.run()