Combine many copy-and-pasted observers into one.

Lots of tests were listening for "permission request shown" using the
same class, and I want to add another.  First refactor to a common
location to avoid yet-more-copy-and-paste.

Bug: none
Change-Id: Iabf5ff8ad250e8b50a7f97ac3e2b15e75ef6c76f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2068093Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Kamila Hasanbega <hkamila@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#744020}

chrome/browser/extensions/api/webrtc_from_web_accessible_resource_browsertest.cc

@@ -8,46 +8,13 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/permissions/permission_request_manager.h"
+#include "components/permissions/test/permission_request_observer.h"
 #include "extensions/test/result_catcher.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
 namespace extensions {
 
-namespace {
-
-// Used to observe the creation of permission prompt without responding.
-class PermissionRequestObserver
-    : public permissions::PermissionRequestManager::Observer {
- public:
-  explicit PermissionRequestObserver(content::WebContents* web_contents)
-      : request_manager_(permissions::PermissionRequestManager::FromWebContents(
-            web_contents)),
-        request_shown_(false) {
-    request_manager_->AddObserver(this);
-  }
-  ~PermissionRequestObserver() override {
-    // Safe to remove twice if it happens.
-    request_manager_->RemoveObserver(this);
-  }
-
-  bool request_shown() const { return request_shown_; }
-
- private:
-  // PermissionRequestManager::Observer
-  void OnBubbleAdded() override {
-    request_shown_ = true;
-    request_manager_->RemoveObserver(this);
-  }
-
-  permissions::PermissionRequestManager* request_manager_;
-  bool request_shown_;
-
-  DISALLOW_COPY_AND_ASSIGN(PermissionRequestObserver);
-};
-
-}  // namespace
-
 class WebRtcFromWebAccessibleResourceTest : public ExtensionApiTest {
  public:
   WebRtcFromWebAccessibleResourceTest() {}
@@ -94,7 +61,8 @@ IN_PROC_BROWSER_TEST_F(WebRtcFromWebAccessibleResourceTest,
       permissions::PermissionRequestManager::FromWebContents(web_contents);
   request_manager->set_auto_response_for_test(
       permissions::PermissionRequestManager::ACCEPT_ALL);
-  PermissionRequestObserver permission_request_observer(web_contents);
+  permissions::PermissionRequestObserver permission_request_observer(
+      web_contents);
   extensions::ResultCatcher catcher;
   ui_test_utils::NavigateToURL(browser(), url);
 
@@ -117,7 +85,8 @@ IN_PROC_BROWSER_TEST_F(WebRtcFromWebAccessibleResourceTest,
       permissions::PermissionRequestManager::FromWebContents(web_contents);
   request_manager->set_auto_response_for_test(
       permissions::PermissionRequestManager::DENY_ALL);
-  PermissionRequestObserver permission_request_observer(web_contents);
+  permissions::PermissionRequestObserver permission_request_observer(
+      web_contents);
   extensions::ResultCatcher catcher;
   ui_test_utils::NavigateToURL(browser(), url);
 

chrome/browser/geolocation/geolocation_browsertest.cc

@@ -31,6 +31,7 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/permissions/features.h"
 #include "components/permissions/permission_request_manager.h"
+#include "components/permissions/test/permission_request_observer.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
@@ -134,44 +135,6 @@ void IFrameLoader::Observe(int type,
     base::RunLoop::QuitCurrentWhenIdleDeprecated();
 }
 
-// PermissionRequestObserver ---------------------------------------------------
-
-// Used to observe the creation of a single permission request without
-// responding.
-class PermissionRequestObserver
-    : public permissions::PermissionRequestManager::Observer {
- public:
-  explicit PermissionRequestObserver(content::WebContents* web_contents)
-      : request_manager_(permissions::PermissionRequestManager::FromWebContents(
-            web_contents)),
-        request_shown_(false),
-        message_loop_runner_(new content::MessageLoopRunner) {
-    request_manager_->AddObserver(this);
-  }
-  ~PermissionRequestObserver() override {
-    // Safe to remove twice if it happens.
-    request_manager_->RemoveObserver(this);
-  }
-
-  void Wait() { message_loop_runner_->Run(); }
-
-  bool request_shown() { return request_shown_; }
-
- private:
-  // PermissionRequestManager::Observer
-  void OnBubbleAdded() override {
-    request_shown_ = true;
-    request_manager_->RemoveObserver(this);
-    message_loop_runner_->Quit();
-  }
-
-  permissions::PermissionRequestManager* request_manager_;
-  bool request_shown_;
-  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
-
-  DISALLOW_COPY_AND_ASSIGN(PermissionRequestObserver);
-};
-
 }  // namespace
 
 
@@ -377,7 +340,7 @@ std::string GeolocationBrowserTest::WatchPositionAndRespondToPermissionRequest(
 
 void GeolocationBrowserTest::WatchPositionAndObservePermissionRequest(
     bool request_should_display) {
-  PermissionRequestObserver observer(
+  permissions::PermissionRequestObserver observer(
       current_browser_->tab_strip_model()->GetActiveWebContents());
   if (request_should_display) {
     // Control will return as soon as the API call is made, and then the

chrome/browser/media/webrtc/webrtc_browsertest_base.cc

@@ -28,6 +28,7 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/permissions/permission_request_manager.h"
+#include "components/permissions/test/permission_request_observer.h"
 #include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
@@ -102,43 +103,6 @@ bool JavascriptErrorDetectingLogHandler(int severity,
   return false;
 }
 
-// PermissionRequestObserver ---------------------------------------------------
-
-// Used to observe the creation of permission prompt without responding.
-class PermissionRequestObserver
-    : public permissions::PermissionRequestManager::Observer {
- public:
-  explicit PermissionRequestObserver(content::WebContents* web_contents)
-      : request_manager_(permissions::PermissionRequestManager::FromWebContents(
-            web_contents)),
-        request_shown_(false),
-        message_loop_runner_(new content::MessageLoopRunner) {
-    request_manager_->AddObserver(this);
-  }
-  ~PermissionRequestObserver() override {
-    // Safe to remove twice if it happens.
-    request_manager_->RemoveObserver(this);
-  }
-
-  void Wait() { message_loop_runner_->Run(); }
-
-  bool request_shown() const { return request_shown_; }
-
- private:
-  // PermissionRequestManager::Observer
-  void OnBubbleAdded() override {
-    request_shown_ = true;
-    request_manager_->RemoveObserver(this);
-    message_loop_runner_->Quit();
-  }
-
-  permissions::PermissionRequestManager* request_manager_;
-  bool request_shown_;
-  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
-
-  DISALLOW_COPY_AND_ASSIGN(PermissionRequestObserver);
-};
-
 std::vector<std::string> JsonArrayToVectorOfStrings(
     const std::string& json_array) {
   std::unique_ptr<base::Value> value =
@@ -192,9 +156,9 @@ bool WebRtcTestBase::GetUserMediaWithSpecificConstraintsAndAccept(
   permissions::PermissionRequestManager::FromWebContents(tab_contents)
       ->set_auto_response_for_test(
           permissions::PermissionRequestManager::ACCEPT_ALL);
-  PermissionRequestObserver permissionRequestObserver(tab_contents);
+  permissions::PermissionRequestObserver observer(tab_contents);
   GetUserMedia(tab_contents, constraints);
-  EXPECT_TRUE(permissionRequestObserver.request_shown());
+  EXPECT_TRUE(observer.request_shown());
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result));
   return kOkGotStream == result;
@@ -225,9 +189,9 @@ void WebRtcTestBase::GetUserMediaWithSpecificConstraintsAndDeny(
   permissions::PermissionRequestManager::FromWebContents(tab_contents)
       ->set_auto_response_for_test(
           permissions::PermissionRequestManager::DENY_ALL);
-  PermissionRequestObserver permissionRequestObserver(tab_contents);
+  permissions::PermissionRequestObserver observer(tab_contents);
   GetUserMedia(tab_contents, constraints);
-  EXPECT_TRUE(permissionRequestObserver.request_shown());
+  EXPECT_TRUE(observer.request_shown());
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result));
   EXPECT_EQ(kFailedWithNotAllowedError, result);
@@ -239,9 +203,9 @@ void WebRtcTestBase::GetUserMediaAndDismiss(
   permissions::PermissionRequestManager::FromWebContents(tab_contents)
       ->set_auto_response_for_test(
           permissions::PermissionRequestManager::DISMISS);
-  PermissionRequestObserver permissionRequestObserver(tab_contents);
+  permissions::PermissionRequestObserver observer(tab_contents);
   GetUserMedia(tab_contents, kAudioVideoCallConstraints);
-  EXPECT_TRUE(permissionRequestObserver.request_shown());
+  EXPECT_TRUE(observer.request_shown());
   // A dismiss should be treated like a deny.
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result));
@@ -262,9 +226,9 @@ void WebRtcTestBase::GetUserMediaAndExpectAutoAcceptWithoutPrompt(
   permissions::PermissionRequestManager::FromWebContents(tab_contents)
       ->set_auto_response_for_test(
           permissions::PermissionRequestManager::DENY_ALL);
-  PermissionRequestObserver permissionRequestObserver(tab_contents);
+  permissions::PermissionRequestObserver observer(tab_contents);
   GetUserMedia(tab_contents, kAudioVideoCallConstraints);
-  EXPECT_FALSE(permissionRequestObserver.request_shown());
+  EXPECT_FALSE(observer.request_shown());
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result));
   EXPECT_EQ(kOkGotStream, result);
@@ -284,9 +248,9 @@ void WebRtcTestBase::GetUserMediaAndExpectAutoDenyWithoutPrompt(
   permissions::PermissionRequestManager::FromWebContents(tab_contents)
       ->set_auto_response_for_test(
           permissions::PermissionRequestManager::ACCEPT_ALL);
-  PermissionRequestObserver permissionRequestObserver(tab_contents);
+  permissions::PermissionRequestObserver observer(tab_contents);
   GetUserMedia(tab_contents, kAudioVideoCallConstraints);
-  EXPECT_FALSE(permissionRequestObserver.request_shown());
+  EXPECT_FALSE(observer.request_shown());
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       tab_contents->GetMainFrame(), "obtainGetUserMediaResult();", &result));
   EXPECT_EQ(kFailedWithNotAllowedError, result);

chrome/browser/notifications/notification_interactive_uitest_support.cc

@@ -17,50 +17,12 @@
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/permissions/features.h"
 #include "components/permissions/permission_request_manager.h"
+#include "components/permissions/test/permission_request_observer.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/message_center_observer.h"
 
-namespace {
-
-// Used to observe the creation of permission prompt without responding.
-class PermissionRequestObserver
-    : public permissions::PermissionRequestManager::Observer {
- public:
-  explicit PermissionRequestObserver(content::WebContents* web_contents)
-      : request_manager_(permissions::PermissionRequestManager::FromWebContents(
-            web_contents)),
-        request_shown_(false),
-        message_loop_runner_(new content::MessageLoopRunner) {
-    request_manager_->AddObserver(this);
-  }
-  ~PermissionRequestObserver() override {
-    // Safe to remove twice if it happens.
-    request_manager_->RemoveObserver(this);
-  }
-
-  void Wait() { message_loop_runner_->Run(); }
-
-  bool request_shown() { return request_shown_; }
-
- private:
-  // PermissionRequestManager::Observer
-  void OnBubbleAdded() override {
-    request_shown_ = true;
-    request_manager_->RemoveObserver(this);
-    message_loop_runner_->Quit();
-  }
-
-  permissions::PermissionRequestManager* request_manager_;
-  bool request_shown_;
-  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
-
-  DISALLOW_COPY_AND_ASSIGN(PermissionRequestObserver);
-};
-
-}  // namespace
-
 class MessageCenterChangeObserver::Impl
     : public message_center::MessageCenterObserver {
  public:
@@ -236,7 +198,7 @@ bool NotificationsTest::RequestAndDismissPermission(Browser* browser) {
 bool NotificationsTest::RequestPermissionAndWait(Browser* browser) {
   content::WebContents* web_contents = GetActiveWebContents(browser);
   ui_test_utils::NavigateToURL(browser, GetTestPageURL());
-  PermissionRequestObserver observer(web_contents);
+  permissions::PermissionRequestObserver observer(web_contents);
   std::string result;
   EXPECT_TRUE(content::ExecuteScriptAndExtractString(
       web_contents, "requestPermissionAndRespond();", &result));

chrome/browser/wake_lock/wake_lock_browsertest.cc

@@ -9,7 +9,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
-#include "components/permissions/permission_request_manager.h"
+#include "components/permissions/test/permission_request_observer.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
@@ -20,38 +20,6 @@
 
 namespace {
 
-// Trimmed down version of the class found in geolocation_browsertest.cc.
-// Used to observe the creation of a single permission request without
-// responding.
-class PermissionRequestObserver
-    : public permissions::PermissionRequestManager::Observer {
- public:
-  explicit PermissionRequestObserver(content::WebContents* web_contents)
-      : request_manager_(permissions::PermissionRequestManager::FromWebContents(
-            web_contents)),
-        request_shown_(false) {
-    request_manager_->AddObserver(this);
-  }
-  ~PermissionRequestObserver() override {
-    // Safe to remove twice if it happens.
-    request_manager_->RemoveObserver(this);
-  }
-
-  bool request_shown() { return request_shown_; }
-
- private:
-  // PermissionRequestManager::Observer
-  void OnBubbleAdded() override {
-    request_shown_ = true;
-    request_manager_->RemoveObserver(this);
-  }
-
-  permissions::PermissionRequestManager* request_manager_;
-  bool request_shown_;
-
-  DISALLOW_COPY_AND_ASSIGN(PermissionRequestObserver);
-};
-
 // Handles HTTP requests to |path| with |content| as the response body.
 // |content| is expected to be JavaScript; the response mime type is always set
 // to "text/javascript".
@@ -117,7 +85,7 @@ void WakeLockBrowserTest::NavigateToAndRespondWithScript(
 // https://w3c.github.io/wake-lock/#request-static-method
 // Screen locks are never allowed from workers.
 IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestScreenLockFromWorker) {
-  PermissionRequestObserver observer(
+  permissions::PermissionRequestObserver observer(
       browser()->tab_strip_model()->GetActiveWebContents());
   const std::string kWorkerScript =
       "navigator.wakeLock.request('screen').catch(err => "
@@ -129,13 +97,13 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestScreenLockFromWorker) {
       "NotAllowedError",
       content::EvalJs(browser()->tab_strip_model()->GetActiveWebContents(),
                       "waitForMessage();"));
-  EXPECT_EQ(observer.request_shown(), false);
+  EXPECT_FALSE(observer.request_shown());
 }
 
 // Requests for a system lock should always be denied, and there should be no
 // permission prompt.
 IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestSystemLockFromWorker) {
-  PermissionRequestObserver observer(
+  permissions::PermissionRequestObserver observer(
       browser()->tab_strip_model()->GetActiveWebContents());
   const std::string kWorkerScript =
       "navigator.wakeLock.request('system').catch(err => "
@@ -147,7 +115,7 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestSystemLockFromWorker) {
       "NotAllowedError",
       content::EvalJs(browser()->tab_strip_model()->GetActiveWebContents(),
                       "waitForMessage();"));
-  EXPECT_EQ(observer.request_shown(), false);
+  EXPECT_FALSE(observer.request_shown());
 }
 
 IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestPermissionScreen) {
@@ -155,13 +123,13 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestPermissionScreen) {
   // permission prompt.
   NavigateToSimplePage();
 
-  PermissionRequestObserver observer(
+  permissions::PermissionRequestObserver observer(
       browser()->tab_strip_model()->GetActiveWebContents());
   EXPECT_EQ("granted", content::EvalJs(
                            browser()->tab_strip_model()->GetActiveWebContents(),
                            "navigator.wakeLock.request('screen').then(lock => {"
                            "    lock.release(); return 'granted'; });"));
-  EXPECT_EQ(observer.request_shown(), false);
+  EXPECT_FALSE(observer.request_shown());
 }
 
 IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest,
@@ -170,7 +138,7 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest,
   // permission prompt.
   NavigateToSimplePage();
 
-  PermissionRequestObserver observer(
+  permissions::PermissionRequestObserver observer(
       browser()->tab_strip_model()->GetActiveWebContents());
   EXPECT_EQ(
       "granted",
@@ -178,7 +146,7 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest,
                       "navigator.wakeLock.request('screen').then(lock => {"
                       "    lock.release(); return 'granted'; });",
                       content::EvalJsOptions::EXECUTE_SCRIPT_NO_USER_GESTURE));
-  EXPECT_EQ(observer.request_shown(), false);
+  EXPECT_FALSE(observer.request_shown());
 }
 
 IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestPermissionSystem) {
@@ -186,14 +154,14 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest, RequestPermissionSystem) {
   // permission prompt.
   NavigateToSimplePage();
 
-  PermissionRequestObserver observer(
+  permissions::PermissionRequestObserver observer(
       browser()->tab_strip_model()->GetActiveWebContents());
   EXPECT_EQ(
       "NotAllowedError",
       content::EvalJs(browser()->tab_strip_model()->GetActiveWebContents(),
                       "navigator.wakeLock.request('system').catch(err => {"
                       "    return err.name; });"));
-  EXPECT_EQ(observer.request_shown(), false);
+  EXPECT_FALSE(observer.request_shown());
 }
 
 IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest,
@@ -202,7 +170,7 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest,
   // permission prompt.
   NavigateToSimplePage();
 
-  PermissionRequestObserver observer(
+  permissions::PermissionRequestObserver observer(
       browser()->tab_strip_model()->GetActiveWebContents());
   EXPECT_EQ(
       "NotAllowedError",
@@ -210,5 +178,5 @@ IN_PROC_BROWSER_TEST_F(WakeLockBrowserTest,
                       "navigator.wakeLock.request('system').catch(err => {"
                       "    return err.name; });",
                       content::EvalJsOptions::EXECUTE_SCRIPT_NO_USER_GESTURE));
-  EXPECT_EQ(observer.request_shown(), false);
+  EXPECT_FALSE(observer.request_shown());
 }

components/permissions/BUILD.gn

@@ -61,6 +61,8 @@ source_set("test_support") {
     "test/mock_permission_prompt_factory.h",
     "test/mock_permission_request.cc",
     "test/mock_permission_request.h",
+    "test/permission_request_observer.cc",
+    "test/permission_request_observer.h",
     "test/test_permissions_client.cc",
     "test/test_permissions_client.h",
   ]

components/permissions/test/permission_request_observer.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 "components/permissions/test/permission_request_observer.h"
+
+namespace permissions {
+
+PermissionRequestObserver::PermissionRequestObserver(
+    content::WebContents* web_contents) {
+  observer_.Add(PermissionRequestManager::FromWebContents(web_contents));
+}
+
+PermissionRequestObserver::~PermissionRequestObserver() = default;
+
+void PermissionRequestObserver::Wait() {
+  loop_.Run();
+}
+
+void PermissionRequestObserver::OnBubbleAdded() {
+  request_shown_ = true;
+  loop_.Quit();
+}
+
+}  // namespace permissions

components/permissions/test/permission_request_observer.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 COMPONENTS_PERMISSIONS_TEST_PERMISSION_REQUEST_OBSERVER_H_
+#define COMPONENTS_PERMISSIONS_TEST_PERMISSION_REQUEST_OBSERVER_H_
+
+#include "base/run_loop.h"
+#include "base/scoped_observer.h"
+#include "components/permissions/permission_request_manager.h"
+
+namespace permissions {
+
+// Observer for use in tests that want to detect whether a permission request
+// was shown.  Can be used in a blocking or non-blocking way.
+class PermissionRequestObserver : public PermissionRequestManager::Observer {
+ public:
+  explicit PermissionRequestObserver(content::WebContents* web_contents);
+  PermissionRequestObserver(const PermissionRequestObserver&) = delete;
+  PermissionRequestObserver& operator=(const PermissionRequestObserver&) =
+      delete;
+  ~PermissionRequestObserver() override;
+
+  bool request_shown() const { return request_shown_; }
+
+  // Blocks until a request is shown.
+  void Wait();
+
+  // PermissionRequestManager::Observer:
+  void OnBubbleAdded() override;
+
+ private:
+  ScopedObserver<PermissionRequestManager, PermissionRequestManager::Observer>
+      observer_{this};
+  base::RunLoop loop_;
+  bool request_shown_ = false;
+};
+
+}  // namespace permissions
+
+#endif  // COMPONENTS_PERMISSIONS_TEST_PERMISSION_REQUEST_OBSERVER_H_