Reland "[WebLayer] Switch geolocation tests to use permission prompt"

This is a reland of 6966bd53

I wasn't able to repro the flake locally, but added an extra assertion
to the test to possibly get more info if this flakes again.

Original change's description:
> [WebLayer] Switch geolocation tests to use permission prompt
>
> These tests were previously using the fake permission manager which
> always grants permissions. Now we can use the real permission manager,
> and the fake permission manager can be removed.
>
> Fixes a strict mode violation in permission dialogs (similar to fixes
> from https://crrev.com/c/2108603).
>
> Bug: 1025625, 1025609
> Change-Id: I8b756c61ee213151e53d68a375ffa4d3f8fd7643
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2111061
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#752494}

TBR=jam@chromium.org

Bug: 1025625, 1025609, 1064500
Change-Id: Idb4ff08bca8ec60321b1524a9b51ec47a07b8910
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2129888Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#755010}

components/permissions/android/java/src/org/chromium/components/permissions/PermissionDialogModel.java

@@ -12,6 +12,7 @@ import android.widget.TextView;
 
 import androidx.core.widget.TextViewCompat;
 
+import org.chromium.base.StrictModeContext;
 import org.chromium.components.browser_ui.modaldialog.R;
 import org.chromium.ui.modaldialog.ModalDialogProperties;
 import org.chromium.ui.modelutil.PropertyModel;
@@ -24,8 +25,7 @@ class PermissionDialogModel {
             ModalDialogProperties.Controller controller, PermissionDialogDelegate delegate) {
         Context context = delegate.getWindow().getContext().get();
         assert context != null;
-        LayoutInflater inflater = LayoutInflater.from(context);
-        View customView = inflater.inflate(R.layout.permission_dialog, null);
+        View customView = loadDialogView(context);
 
         String messageText = delegate.getMessageText();
         assert !TextUtils.isEmpty(messageText);
@@ -44,4 +44,12 @@ class PermissionDialogModel {
                 .with(ModalDialogProperties.FILTER_TOUCH_FOR_SECURITY, true)
                 .build();
     }
+
+    private static View loadDialogView(Context context) {
+        // LayoutInflater may access the disk.
+        try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
+            LayoutInflater inflater = LayoutInflater.from(context);
+            return inflater.inflate(R.layout.permission_dialog, null);
+        }
+    }
 }

weblayer/BUILD.gn

@@ -121,8 +121,6 @@ source_set("weblayer_lib_base") {
     "browser/download_impl.h",
     "browser/download_manager_delegate_impl.cc",
     "browser/download_manager_delegate_impl.h",
-    "browser/fake_permission_controller_delegate.cc",
-    "browser/fake_permission_controller_delegate.h",
     "browser/feature_list_creator.cc",
     "browser/feature_list_creator.h",
     "browser/file_select_helper.cc",

weblayer/browser/android/javatests/src/org/chromium/weblayer/test/GeolocationTest.java

@@ -14,7 +14,6 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.net.test.util.TestWebServer;
@@ -27,7 +26,6 @@ import org.chromium.weblayer.shell.InstrumentationActivity;
  * Tests that Geolocation Web API works as expected.
  */
 @RunWith(WebLayerJUnit4ClassRunner.class)
-@CommandLineFlags.Add("weblayer-fake-permissions")
 public final class GeolocationTest {
     @Rule
     public InstrumentationActivityTestRule mActivityTestRule =
@@ -94,6 +92,8 @@ public final class GeolocationTest {
     @MediumTest
     public void testGeolocation_getPosition() throws Throwable {
         mActivityTestRule.executeScriptSync("initiate_getCurrentPosition();", false);
+        waitForDialog();
+        mTestWebLayer.clickPermissionDialogButton(true);
         waitForCountEqual("positionCount", 1);
         mActivityTestRule.executeScriptSync("initiate_getCurrentPosition();", false);
         waitForCountEqual("positionCount", 2);
@@ -107,6 +107,8 @@ public final class GeolocationTest {
     @MediumTest
     public void testGeolocation_watchPosition() throws Throwable {
         mActivityTestRule.executeScriptSync("initiate_watchPosition();", false);
+        waitForDialog();
+        mTestWebLayer.clickPermissionDialogButton(true);
         waitForCountGreaterThan("positionCount", 1);
         ensureGeolocationIsRunning(true);
         Assert.assertEquals(0, getCountFromJS("errorCount"));
@@ -119,6 +121,8 @@ public final class GeolocationTest {
     @MediumTest
     public void testGeolocation_destroyTabStopsGeolocation() throws Throwable {
         mActivityTestRule.executeScriptSync("initiate_watchPosition();", false);
+        waitForDialog();
+        mTestWebLayer.clickPermissionDialogButton(true);
         ensureGeolocationIsRunning(true);
         TestThreadUtils.runOnUiThreadBlocking(() -> {
             Browser browser = mActivity.getBrowser();
@@ -143,6 +147,16 @@ public final class GeolocationTest {
         Assert.assertEquals(0, getCountFromJS("positionCount"));
     }
 
+    @Test
+    @MediumTest
+    public void testGeolocation_denyFromPrompt() throws Throwable {
+        mActivityTestRule.executeScriptSync("initiate_watchPosition();", false);
+        waitForDialog();
+        mTestWebLayer.clickPermissionDialogButton(false);
+        waitForCountEqual("errorCount", 1);
+        Assert.assertEquals(0, getCountFromJS("positionCount"));
+    }
+
     // helper methods
 
     private void waitForCountEqual(String variableName, int count) {
@@ -150,6 +164,24 @@ public final class GeolocationTest {
                 () -> { return getCountFromJS(variableName) == count; });
     }
 
+    private void waitForDialog() throws Exception {
+        // Make sure the current permission state is "prompt" before waiting for the dialog.
+        mActivityTestRule.executeScriptSync("var queryResult;"
+                        + "navigator.permissions.query({name: 'geolocation'}).then("
+                        + "function(result) { queryResult = result.state; })",
+                false);
+        CriteriaHelper.pollInstrumentationThread(() -> {
+            return !mActivityTestRule.executeScriptSync("queryResult || ''", false)
+                            .getString(Tab.SCRIPT_RESULT_KEY)
+                            .equals("");
+        });
+        Assert.assertEquals("prompt",
+                mActivityTestRule.executeScriptSync("queryResult", false)
+                        .getString(Tab.SCRIPT_RESULT_KEY));
+        CriteriaHelper.pollInstrumentationThread(
+                () -> { return mTestWebLayer.isPermissionDialogShown(); });
+    }
+
     private void waitForCountGreaterThan(String variableName, int count) {
         CriteriaHelper.pollInstrumentationThread(
                 () -> { return getCountFromJS(variableName) > count; });

weblayer/browser/browser_context_impl.cc

@@ -23,7 +23,6 @@
 #include "content/public/browser/device_service.h"
 #include "content/public/browser/download_request_utils.h"
 #include "content/public/browser/resource_context.h"
-#include "weblayer/browser/fake_permission_controller_delegate.h"
 #include "weblayer/browser/permissions/permission_manager_factory.h"
 #include "weblayer/browser/stateful_ssl_host_state_delegate_factory.h"
 #include "weblayer/public/common/switches.h"
@@ -154,14 +153,6 @@ content::SSLHostStateDelegate* BrowserContextImpl::GetSSLHostStateDelegate() {
 
 content::PermissionControllerDelegate*
 BrowserContextImpl::GetPermissionControllerDelegate() {
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kWebLayerFakePermissions)) {
-    if (!permission_controller_delegate_) {
-      permission_controller_delegate_ =
-          std::make_unique<FakePermissionControllerDelegate>();
-    }
-    return permission_controller_delegate_.get();
-  }
   return PermissionManagerFactory::GetForBrowserContext(this);
 }
 

weblayer/browser/browser_context_impl.h

@@ -86,8 +86,6 @@ class BrowserContextImpl : public content::BrowserContext {
       resource_context_;
   DownloadManagerDelegateImpl download_delegate_;
   std::unique_ptr<PrefService> user_pref_service_;
-  std::unique_ptr<content::PermissionControllerDelegate>
-      permission_controller_delegate_;
   std::unique_ptr<WebLayerVariationsClient> weblayer_variations_client_;
 };
 }  // namespace weblayer

weblayer/browser/content_browser_client_impl.cc

@@ -207,15 +207,6 @@ ContentBrowserClientImpl::CreateBrowserMainParts(
   return browser_main_parts;
 }
 
-void ContentBrowserClientImpl::AppendExtraCommandLineSwitches(
-    base::CommandLine* command_line,
-    int child_process_id) {
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kWebLayerFakePermissions)) {
-    command_line->AppendSwitch(switches::kWebLayerFakePermissions);
-  }
-}
-
 std::string ContentBrowserClientImpl::GetApplicationLocale() {
   return i18n::GetApplicationLocale();
 }

weblayer/browser/content_browser_client_impl.h

@@ -29,8 +29,6 @@ class ContentBrowserClientImpl : public content::ContentBrowserClient {
   // ContentBrowserClient overrides.
   std::unique_ptr<content::BrowserMainParts> CreateBrowserMainParts(
       const content::MainFunctionParams& parameters) override;
-  void AppendExtraCommandLineSwitches(base::CommandLine* command_line,
-                                      int child_process_id) override;
   std::string GetApplicationLocale() override;
   std::string GetAcceptLangs(content::BrowserContext* context) override;
   content::WebContentsViewDelegate* GetWebContentsViewDelegate(

weblayer/browser/fake_permission_controller_delegate.cc

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "weblayer/browser/fake_permission_controller_delegate.h"
-
-#include "base/callback.h"
-#include "content/public/browser/permission_controller.h"
-#include "content/public/browser/permission_type.h"
-#include "content/public/browser/web_contents.h"
-
-namespace weblayer {
-
-FakePermissionControllerDelegate::FakePermissionControllerDelegate() = default;
-
-FakePermissionControllerDelegate::~FakePermissionControllerDelegate() = default;
-
-int FakePermissionControllerDelegate::RequestPermission(
-    content::PermissionType permission,
-    content::RenderFrameHost* render_frame_host,
-    const GURL& requesting_origin,
-    bool user_gesture,
-    base::OnceCallback<void(blink::mojom::PermissionStatus)> callback) {
-  std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED);
-  return content::PermissionController::kNoPendingOperation;
-}
-
-int FakePermissionControllerDelegate::RequestPermissions(
-    const std::vector<content::PermissionType>& permissions,
-    content::RenderFrameHost* render_frame_host,
-    const GURL& requesting_origin,
-    bool user_gesture,
-    base::OnceCallback<void(const std::vector<blink::mojom::PermissionStatus>&)>
-        callback) {
-  std::vector<blink::mojom::PermissionStatus> result(
-      permissions.size(), blink::mojom::PermissionStatus::GRANTED);
-  std::move(callback).Run(result);
-  return content::PermissionController::kNoPendingOperation;
-}
-
-void FakePermissionControllerDelegate::ResetPermission(
-    content::PermissionType permission,
-    const GURL& requesting_origin,
-    const GURL& embedding_origin) {}
-
-blink::mojom::PermissionStatus
-FakePermissionControllerDelegate::GetPermissionStatus(
-    content::PermissionType permission,
-    const GURL& requesting_origin,
-    const GURL& embedding_origin) {
-  return blink::mojom::PermissionStatus::GRANTED;
-}
-
-blink::mojom::PermissionStatus
-FakePermissionControllerDelegate::GetPermissionStatusForFrame(
-    content::PermissionType permission,
-    content::RenderFrameHost* render_frame_host,
-    const GURL& requesting_origin) {
-  return GetPermissionStatus(
-      permission, requesting_origin,
-      content::WebContents::FromRenderFrameHost(render_frame_host)
-          ->GetLastCommittedURL()
-          .GetOrigin());
-}
-
-int FakePermissionControllerDelegate::SubscribePermissionStatusChange(
-    content::PermissionType permission,
-    content::RenderFrameHost* render_frame_host,
-    const GURL& requesting_origin,
-    base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback) {
-  return content::PermissionController::kNoPendingOperation;
-}
-
-void FakePermissionControllerDelegate::UnsubscribePermissionStatusChange(
-    int subscription_id) {}
-
-}  // namespace weblayer

weblayer/browser/fake_permission_controller_delegate.h

-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef WEBLAYER_BROWSER_FAKE_PERMISSION_CONTROLLER_DELEGATE_H_
-#define WEBLAYER_BROWSER_FAKE_PERMISSION_CONTROLLER_DELEGATE_H_
-
-#include "base/callback_forward.h"
-#include "base/macros.h"
-#include "content/public/browser/permission_controller_delegate.h"
-
-namespace weblayer {
-
-// Temporary permission controller delegate which grants all permissions. Once
-// permissions have been implemented, this will be removed. This is only used if
-// the --weblayer-fake-permissions switch is passed on the command line.
-class FakePermissionControllerDelegate
-    : public content::PermissionControllerDelegate {
- public:
-  FakePermissionControllerDelegate();
-  ~FakePermissionControllerDelegate() override;
-
-  // PermissionControllerDelegate implementation.
-  int RequestPermission(content::PermissionType permission,
-                        content::RenderFrameHost* render_frame_host,
-                        const GURL& requesting_origin,
-                        bool user_gesture,
-                        base::OnceCallback<void(blink::mojom::PermissionStatus)>
-                            callback) override;
-  int RequestPermissions(
-      const std::vector<content::PermissionType>& permission,
-      content::RenderFrameHost* render_frame_host,
-      const GURL& requesting_origin,
-      bool user_gesture,
-      base::OnceCallback<
-          void(const std::vector<blink::mojom::PermissionStatus>&)> callback)
-      override;
-  void ResetPermission(content::PermissionType permission,
-                       const GURL& requesting_origin,
-                       const GURL& embedding_origin) override;
-  blink::mojom::PermissionStatus GetPermissionStatus(
-      content::PermissionType permission,
-      const GURL& requesting_origin,
-      const GURL& embedding_origin) override;
-  blink::mojom::PermissionStatus GetPermissionStatusForFrame(
-      content::PermissionType permission,
-      content::RenderFrameHost* render_frame_host,
-      const GURL& requesting_origin) override;
-  int SubscribePermissionStatusChange(
-      content::PermissionType permission,
-      content::RenderFrameHost* render_frame_host,
-      const GURL& requesting_origin,
-      base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback)
-      override;
-  void UnsubscribePermissionStatusChange(int subscription_id) override;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(FakePermissionControllerDelegate);
-};
-
-}  // namespace weblayer
-
-#endif  // WEBLAYER_BROWSER_FAKE_PERMISSION_CONTROLLER_DELEGATE_H_

weblayer/browser/java/BUILD.gn

@@ -157,9 +157,12 @@ android_library("test_java") {
   sources = [ "org/chromium/weblayer_private/test/TestWebLayerImpl.java" ]
   deps = [
     ":weblayer_test_resources",
+    "//components/permissions/android:java",
+    "//content/public/test/android:content_java_test_support",
     "//net/android:net_java",
     "//services/device/public/java:geolocation_java",
     "//services/device/public/java:geolocation_java_test_support",
+    "//ui/android:ui_full_java",
   ]
   srcjar_deps = [ ":test_aidl" ]
 }

weblayer/browser/java/org/chromium/weblayer_private/test/TestWebLayerImpl.java

@@ -7,11 +7,16 @@ package org.chromium.weblayer_private.test;
 import android.os.IBinder;
 
 import org.chromium.base.annotations.UsedByReflection;
+import org.chromium.components.permissions.PermissionDialogController;
+import org.chromium.content_public.browser.test.util.TestThreadUtils;
 import org.chromium.device.geolocation.LocationProviderOverrider;
 import org.chromium.device.geolocation.MockLocationProvider;
 import org.chromium.net.NetworkChangeNotifier;
+import org.chromium.ui.modaldialog.ModalDialogProperties;
 import org.chromium.weblayer_private.test_interfaces.ITestWebLayer;
 
+import java.util.concurrent.ExecutionException;
+
 /**
  * Root implementation class for TestWebLayer.
  */
@@ -46,4 +51,24 @@ public final class TestWebLayerImpl extends ITestWebLayer.Stub {
     public boolean isMockLocationProviderRunning() {
         return mMockLocationProvider.isRunning();
     }
-}
+
\ No newline at end of file
+    @Override
+    public boolean isPermissionDialogShown() {
+        try {
+            return TestThreadUtils.runOnUiThreadBlocking(() -> {
+                return PermissionDialogController.getInstance().isDialogShownForTest();
+            });
+        } catch (ExecutionException e) {
+            return false;
+        }
+    }
+
+    @Override
+    public void clickPermissionDialogButton(boolean allow) {
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            PermissionDialogController.getInstance().clickButtonForTest(allow
+                            ? ModalDialogProperties.ButtonType.POSITIVE
+                            : ModalDialogProperties.ButtonType.NEGATIVE);
+        });
+    }
+}

weblayer/browser/java/org/chromium/weblayer_private/test_interfaces/ITestWebLayer.aidl

@@ -10,4 +10,10 @@ interface ITestWebLayer {
   // set mock location provider
   void setMockLocationProvider(in boolean enable) = 2;
   boolean isMockLocationProviderRunning() = 3;
+
+  // Whether or not a permission dialog is currently showing.
+  boolean isPermissionDialogShown() = 4;
+
+  // Clicks a button on the permission dialog.
+  void clickPermissionDialogButton(in boolean allow) = 5;
 }

weblayer/public/common/switches.cc

@@ -9,8 +9,4 @@ namespace switches {
 // Makes WebLayer Shell use the given path for its data directory.
 const char kWebLayerUserDataDir[] = "weblayer-user-data-dir";
 
-// Makes WebLayer use a fake permission controller delegate which grants all
-// permissions.
-const char kWebLayerFakePermissions[] = "weblayer-fake-permissions";
-
 }  // namespace switches

weblayer/public/common/switches.h

@@ -8,7 +8,6 @@
 namespace switches {
 
 extern const char kWebLayerUserDataDir[];
-extern const char kWebLayerFakePermissions[];
 
 }  // namespace switches
 

weblayer/public/javatestutil/org/chromium/weblayer/TestWebLayer.java

@@ -63,4 +63,12 @@ public final class TestWebLayer {
     public boolean isMockLocationProviderRunning() throws RemoteException {
         return mITestWebLayer.isMockLocationProviderRunning();
     }
+
+    public boolean isPermissionDialogShown() throws RemoteException {
+        return mITestWebLayer.isPermissionDialogShown();
+    }
+
+    public void clickPermissionDialogButton(boolean allow) throws RemoteException {
+        mITestWebLayer.clickPermissionDialogButton(allow);
+    }
 }