[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}

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

@@ -108,8 +108,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,11 @@ public final class GeolocationTest {
                 () -> { return getCountFromJS(variableName) == count; });
     }
 
+    private void waitForDialog() {
+        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"
@@ -153,14 +152,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

@@ -176,15 +176,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

@@ -143,9 +143,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);
+    }
 }