Filters duplicated missing permissions messages.

This CL resolves the duplicated missing permissions messages in permission layer, instead of client/chrome layer (a band-aid fix added in https://chromium-review.googlesource.com/c/chromium/src/+/3640708)

Bug:1351125

Change-Id: I5d8bfe73d5ebb1c9a225fa9f13f42ba83aa33eef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3990958
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Thomas Nguyen <tungnh@google.com>
Cr-Commit-Position: refs/heads/main@{#1067593}

chrome/android/javatests/src/org/chromium/chrome/browser/permissions/PermissionUpdateMessageTest.java

@@ -24,18 +24,22 @@
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.profiles.Profile;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
-import org.chromium.chrome.test.ChromeTabbedActivityTestRule;
 import org.chromium.chrome.test.util.ChromeTabUtils;
 import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
 import org.chromium.chrome.test.util.browser.LocationSettingsTestUtil;
-import org.chromium.components.browser_ui.site_settings.PermissionInfo;
+import org.chromium.components.browser_ui.site_settings.WebsitePreferenceBridgeJni;
 import org.chromium.components.content_settings.ContentSettingValues;
 import org.chromium.components.content_settings.ContentSettingsType;
+import org.chromium.components.messages.MessageDispatcher;
+import org.chromium.components.messages.MessageDispatcherProvider;
+import org.chromium.components.messages.MessageIdentifier;
+import org.chromium.components.messages.MessageStateHandler;
 import org.chromium.components.messages.MessagesTestHelper;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.ui.base.WindowAndroid;
+import org.chromium.ui.modelutil.PropertyModel;
 import org.chromium.ui.permissions.AndroidPermissionDelegate;
 import org.chromium.ui.permissions.PermissionCallback;
 
@@ -44,6 +48,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeoutException;
 
 /**
@@ -53,83 +58,19 @@
 @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
 @EnableFeatures({ChromeFeatureList.MESSAGES_FOR_ANDROID_PERMISSION_UPDATE})
 public class PermissionUpdateMessageTest {
-    @Rule
-    public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
-
     private static final String GEOLOCATION_PAGE =
             "/chrome/test/data/geolocation/geolocation_on_load.html";
-
+    private static final String MEDIASTREAM_PAGE = "/content/test/data/media/getusermedia.html";
+    private static final String DOWNLOAD_PAGE = "/chrome/test/data/android/download/get.html";
     private EmbeddedTestServer mTestServer;
 
-    @Before
-    public void setUp() throws Exception {
-        mActivityTestRule.startMainActivityOnBlankPage();
-        mTestServer = EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
-    }
-
-    @After
-    public void tearDown() {
-        mTestServer.stopAndDestroyServer();
-    }
-
-    // Ensure destroying the permission update message UI does not crash when handling geolocation
-    // permissions.
-    @Test
-    @MediumTest
-    public void testMessageShutsDownCleanlyForGeolocation()
-            throws IllegalArgumentException, TimeoutException {
-        ChromeTabUtils.newTabFromMenu(
-                InstrumentationRegistry.getInstrumentation(), mActivityTestRule.getActivity());
-
-        final String locationUrl = mTestServer.getURL(GEOLOCATION_PAGE);
-        final PermissionInfo geolocationSettings = TestThreadUtils.runOnUiThreadBlockingNoException(
-                ()
-                        -> new PermissionInfo(
-                                ContentSettingsType.GEOLOCATION, locationUrl, null, false));
-
-        WindowAndroid windowAndroid = mActivityTestRule.getActivity().getWindowAndroid();
-        windowAndroid.setAndroidPermissionDelegate(new TestAndroidPermissionDelegate(
-                null, Arrays.asList(Manifest.permission.ACCESS_FINE_LOCATION), null));
-        LocationSettingsTestUtil.setSystemLocationSettingEnabled(true);
-
-        try {
-            TestThreadUtils.runOnUiThreadBlocking(
-                    ()
-                            -> geolocationSettings.setContentSetting(
-                                    Profile.getLastUsedRegularProfile(),
-                                    ContentSettingValues.ALLOW));
-
-            mActivityTestRule.loadUrl(mTestServer.getURL(GEOLOCATION_PAGE));
-
-            CriteriaHelper.pollUiThread(() -> {
-                Criteria.checkThat("Message is not enqueued.",
-                        MessagesTestHelper.getMessageCount(windowAndroid), Matchers.is(1));
-            });
-
-            final WebContents webContents = TestThreadUtils.runOnUiThreadBlockingNoException(
-                    () -> mActivityTestRule.getActivity().getActivityTab().getWebContents());
-            Assert.assertFalse(webContents.isDestroyed());
-
-            ChromeTabUtils.closeCurrentTab(
-                    InstrumentationRegistry.getInstrumentation(), mActivityTestRule.getActivity());
-            CriteriaHelper.pollUiThread(() -> webContents.isDestroyed());
-
-            CriteriaHelper.pollUiThread(() -> {
-                Criteria.checkThat(mActivityTestRule.getActivity()
-                                           .getTabModelSelector()
-                                           .getModel(false)
-                                           .getCount(),
-                        Matchers.is(1));
-            });
-        } finally {
-            TestThreadUtils.runOnUiThreadBlocking(
-                    ()
-                            -> geolocationSettings.setContentSetting(
-                                    Profile.getLastUsedRegularProfile(),
-                                    ContentSettingValues.DEFAULT));
-        }
-    }
+    @Rule
+    public PermissionTestRule mActivityTestRule = new PermissionTestRule();
 
+    /**
+     * Utility delegate to provide the permissions to be requested for triggering a
+     * permission update message.
+     */
     private static class TestAndroidPermissionDelegate implements AndroidPermissionDelegate {
         private final Set<String> mHasPermissions;
         private final Set<String> mRequestablePermissions;
@@ -171,4 +112,175 @@ public boolean handlePermissionResult(
             return false;
         }
     }
+
+    @Before
+    public void setUp() throws Exception {
+        mActivityTestRule.startMainActivityOnBlankPage();
+        mTestServer = EmbeddedTestServer.createAndStartServer(InstrumentationRegistry.getContext());
+    }
+
+    @After
+    public void tearDown() {
+        mTestServer.stopAndDestroyServer();
+    }
+
+    /**
+     * Determines if there is exact number of message presented in the given View hierarchy.
+     * @param windowAndroid The WindowAndroid to get the messages from.
+     * @param count Number of messages should be presented.
+     */
+    private void expectMessagesCount(WindowAndroid windowAndroid, final int count) {
+        CriteriaHelper.pollUiThread(() -> {
+            Criteria.checkThat("Message is not enqueued.",
+                    MessagesTestHelper.getMessageCount(windowAndroid), Matchers.is(count));
+        });
+    }
+
+    /**
+     * Returns the {@link PropertyModel} of an enqueued permission update message.
+     *
+     * @param windowAndroid The WindowAndroid to get the messages from.
+     * @return The {@link PropertyModel} of an enqueued permission update message, null if the
+     *         message is not present.
+     * @throws ExecutionException
+     */
+    public static PropertyModel getPermissionUpdateMessage(WindowAndroid windowAndroid)
+            throws ExecutionException {
+        MessageDispatcher messageDispatcher = TestThreadUtils.runOnUiThreadBlocking(
+                () -> MessageDispatcherProvider.from(windowAndroid));
+        List<MessageStateHandler> messages = MessagesTestHelper.getEnqueuedMessages(
+                messageDispatcher, MessageIdentifier.PERMISSION_UPDATE);
+        return messages == null || messages.isEmpty()
+                ? null
+                : MessagesTestHelper.getCurrentMessage(messages.get(0));
+    }
+
+    /**
+     * Sets native ContentSetting value for the given type and origin.
+     * @param type defines ContentSetting type to call native permission setting.
+     * @param origin defines origin to call native permission setting.
+     * @param value expected value for the above ContentSetting type.
+     */
+    public void setNativeContentSetting(
+            @ContentSettingsType int type, final String origin, @ContentSettingValues int value) {
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            WebsitePreferenceBridgeJni.get().setPermissionSettingForOrigin(
+                    Profile.getLastUsedRegularProfile(), type, origin, origin, value);
+        });
+    }
+
+    /**
+     * Run a test related to the permission update message, based on the specified parameters.
+     * @param testPage The String of the test page to load in order to run the text.
+     *
+     * @param androidPermission specify Android permission type will be required for the test.
+     * @param javascriptToExecute Some javascript to execute after the page loads (empty or null to
+     *         skip).
+     * @param contentSettingsType specify content setting type will be notified of missing
+     *         permission.
+     * @param switchContent Whether to swap a web_content by switching to another tab back and
+     *         forth.
+     * @throws IllegalArgumentException,TimeoutException,ExecutionException
+     */
+    private void runTest(final String testPage, final String androidPermission,
+            final String javascriptToExecute, final int contentSettingsType,
+            final boolean switchContent)
+            throws IllegalArgumentException, TimeoutException, ExecutionException {
+        ChromeTabUtils.newTabFromMenu(
+                InstrumentationRegistry.getInstrumentation(), mActivityTestRule.getActivity());
+
+        WindowAndroid windowAndroid = mActivityTestRule.getActivity().getWindowAndroid();
+        windowAndroid.setAndroidPermissionDelegate(
+                new TestAndroidPermissionDelegate(null, Arrays.asList(androidPermission), null));
+        final String url = mTestServer.getURL(testPage);
+        try {
+            setNativeContentSetting(contentSettingsType, url, ContentSettingValues.ALLOW);
+            mActivityTestRule.loadUrl(mTestServer.getURL(testPage));
+
+            if (javascriptToExecute != null && !javascriptToExecute.isEmpty()) {
+                mActivityTestRule.runJavaScriptCodeInCurrentTabWithGesture(javascriptToExecute);
+            }
+
+            expectMessagesCount(windowAndroid, 1);
+            final WebContents webContents = TestThreadUtils.runOnUiThreadBlockingNoException(
+                    () -> mActivityTestRule.getActivity().getActivityTab().getWebContents());
+            Assert.assertFalse(webContents.isDestroyed());
+
+            // TODO(tungnh): At the moment, strings are defined in native i18_n, not from android
+            // context. We should find a way to add more UI verifications, such as description and
+            // title here.
+            PropertyModel message = getPermissionUpdateMessage(windowAndroid);
+            Assert.assertNotNull("Permission update message should be presented.", message);
+
+            if (switchContent) {
+                // Switch to a new tab and switch back
+                ChromeTabUtils.fullyLoadUrlInNewTab(InstrumentationRegistry.getInstrumentation(),
+                        mActivityTestRule.getActivity(), "about:blank", /*  incognito */ false);
+                ChromeTabUtils.switchTabInCurrentTabModel(mActivityTestRule.getActivity(), 1);
+                expectMessagesCount(windowAndroid, 1);
+            }
+
+            // Ensure destroying the permission update message UI does not crash
+            // when handling permissions.
+            ChromeTabUtils.closeCurrentTab(
+                    InstrumentationRegistry.getInstrumentation(), mActivityTestRule.getActivity());
+            CriteriaHelper.pollUiThread(() -> webContents.isDestroyed());
+
+            final int countTabs = switchContent ? 2 : 1;
+            CriteriaHelper.pollUiThread(() -> {
+                Criteria.checkThat(mActivityTestRule.getActivity()
+                                           .getTabModelSelector()
+                                           .getModel(false)
+                                           .getCount(),
+                        Matchers.is(countTabs));
+            });
+        } finally {
+            setNativeContentSetting(contentSettingsType, url, ContentSettingValues.DEFAULT);
+        }
+    }
+
+    // Ensure the correct permission update message UI, and destroying the UI
+    // does not crash when handling geolocation permissions.
+    @Test
+    @MediumTest
+    public void testMessageForGeolocation()
+            throws IllegalArgumentException, TimeoutException, ExecutionException {
+        LocationSettingsTestUtil.setSystemLocationSettingEnabled(true);
+        runTest(GEOLOCATION_PAGE, Manifest.permission.ACCESS_FINE_LOCATION,
+                null /* javascriptToExecute */, ContentSettingsType.GEOLOCATION,
+                false /* switchContent */);
+    }
+
+    // Ensure the correct permission update message UI, and destroying the UI does not crash when
+    // handling camera permissions.
+    @Test
+    @MediumTest
+    public void testMessageForMediaStreamCamera()
+            throws IllegalArgumentException, TimeoutException, ExecutionException {
+        runTest(MEDIASTREAM_PAGE, Manifest.permission.CAMERA,
+                "getUserMediaAndStop({video: true, audio: false});",
+                ContentSettingsType.MEDIASTREAM_CAMERA, false /* switchContent */);
+    }
+
+    // Ensure the correct permission update message UI, and destroying the UI does not crash when
+    // handling microphone permissions.
+    @Test
+    @MediumTest
+    public void testMessageForMediaStreamMicrophone()
+            throws IllegalArgumentException, TimeoutException, ExecutionException {
+        runTest(MEDIASTREAM_PAGE, Manifest.permission.RECORD_AUDIO,
+                "getUserMediaAndStop({video: false, audio: true});",
+                ContentSettingsType.MEDIASTREAM_MIC, false /* switchContent */);
+    }
+
+    // Make sure switching android web content will not trigger multiple prompts.
+    @Test
+    @MediumTest
+    public void testswitchContentShouldNotReprompt()
+            throws IllegalArgumentException, TimeoutException, ExecutionException {
+        LocationSettingsTestUtil.setSystemLocationSettingEnabled(true);
+        runTest(GEOLOCATION_PAGE, Manifest.permission.ACCESS_FINE_LOCATION,
+                null /* javascriptToExecute */, ContentSettingsType.GEOLOCATION,
+                true /* switchContent */);
+    }
 }

chrome/browser/media/webrtc/media_stream_device_permission_context.cc

@@ -17,6 +17,7 @@
 #if BUILDFLAG(IS_ANDROID)
 #include "base/stl_util.h"
 #include "components/permissions/android/android_permission_util.h"
+#include "components/permissions/android/permissions_reprompt_controller_android.h"
 #include "components/permissions/permission_request_id.h"
 #include "components/permissions/permissions_client.h"
 #include "content/public/browser/web_contents.h"
@@ -187,12 +188,17 @@ void MediaStreamDevicePermissionContext::NotifyPermissionSet(
 
     case permissions::PermissionRepromptState::kShow:
       // Otherwise, prompt the user that we need additional permissions.
-      permissions::PermissionsClient::Get()->RepromptForAndroidPermissions(
-          web_contents, permission_type,
-          base::BindOnce(
-              &MediaStreamDevicePermissionContext::OnAndroidPermissionDecided,
-              weak_ptr_factory_.GetWeakPtr(), id, requesting_origin,
-              embedding_origin, std::move(callback)));
+      permissions::PermissionsRepromptControllerAndroid::CreateForWebContents(
+          web_contents);
+      permissions::PermissionsRepromptControllerAndroid::FromWebContents(
+          web_contents)
+          ->RepromptPermissionRequest(
+              permission_type, content_settings_type_,
+              base::BindOnce(&MediaStreamDevicePermissionContext::
+                                 OnAndroidPermissionDecided,
+                             weak_ptr_factory_.GetWeakPtr(), id,
+                             requesting_origin, embedding_origin,
+                             std::move(callback)));
       return;
   }
 }

chrome/browser/permissions/chrome_permissions_client.cc

@@ -494,14 +494,20 @@ ChromePermissionsClient::MaybeCreateMessageUI(
 void ChromePermissionsClient::RepromptForAndroidPermissions(
     content::WebContents* web_contents,
     const std::vector<ContentSettingsType>& content_settings_types,
+    const std::vector<ContentSettingsType>& filtered_content_settings_types,
+    const std::vector<std::string>& required_permissions,
+    const std::vector<std::string>& optional_permissions,
     PermissionsUpdatedCallback callback) {
   if (messages::IsPermissionUpdateMessagesUiEnabled()) {
     PermissionUpdateMessageController::CreateForWebContents(web_contents);
     PermissionUpdateMessageController::FromWebContents(web_contents)
-        ->ShowMessage(content_settings_types, std::move(callback));
+        ->ShowMessage(content_settings_types, filtered_content_settings_types,
+                      required_permissions, optional_permissions,
+                      std::move(callback));
   } else {
     PermissionUpdateInfoBarDelegate::Create(
-        web_contents, content_settings_types, std::move(callback));
+        web_contents, content_settings_types, filtered_content_settings_types,
+        required_permissions, optional_permissions, std::move(callback));
   }
 }
 

chrome/browser/permissions/chrome_permissions_client.h

@@ -95,6 +95,9 @@ class ChromePermissionsClient : public permissions::PermissionsClient {
   void RepromptForAndroidPermissions(
       content::WebContents* web_contents,
       const std::vector<ContentSettingsType>& content_settings_types,
+      const std::vector<ContentSettingsType>& filtered_content_settings_types,
+      const std::vector<std::string>& required_permissions,
+      const std::vector<std::string>& optional_permissions,
       PermissionsUpdatedCallback callback) override;
   int MapToJavaDrawableId(int resource_id) override;
 #else