Use TestExtensionRegistryObserver in two test files.

These tests extend ExtensionRegistryObserver to wait for extension
load/unload etc. TestExtensionRegistryObserver already provides
this functionality.

Bug: 808136
Test: None
Change-Id: I195f206163f54b7547079b10c48e677a86eaf6db
Reviewed-on: https://chromium-review.googlesource.com/897468Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533967}

chrome/browser/autocomplete/keyword_extensions_delegate_impl_unittest.cc

@@ -9,7 +9,6 @@
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
-#include "base/run_loop.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ -21,50 +20,14 @@
 #include "components/omnibox/browser/mock_autocomplete_provider_client.h"
 #include "components/search_engines/template_url_service.h"
 #include "extensions/browser/extension_registry.h"
-#include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/extension_util.h"
+#include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/extension.h"
 
 namespace extensions {
 
 namespace {
 
-class ScopedExtensionLoadObserver : public ExtensionRegistryObserver {
- public:
-  ScopedExtensionLoadObserver(ExtensionRegistry* registry,
-                              const base::Closure& quit_closure);
-  ~ScopedExtensionLoadObserver() override;
-
- private:
-  void OnExtensionInstalled(content::BrowserContext* browser_context,
-                            const Extension* extension,
-                            bool is_update) override;
-
-  ExtensionRegistry* registry_;
-  base::Closure quit_closure_;
-
-  DISALLOW_COPY_AND_ASSIGN(ScopedExtensionLoadObserver);
-};
-
-ScopedExtensionLoadObserver::ScopedExtensionLoadObserver(
-    ExtensionRegistry* registry,
-    const base::Closure& quit_closure)
-    : registry_(registry),
-      quit_closure_(quit_closure) {
-  registry_->AddObserver(this);
-}
-
-ScopedExtensionLoadObserver::~ScopedExtensionLoadObserver() {
-  registry_->RemoveObserver(this);
-}
-
-void ScopedExtensionLoadObserver::OnExtensionInstalled(
-    content::BrowserContext* browser_context,
-    const Extension* extension,
-    bool is_update) {
-  quit_closure_.Run();
-}
-
 class KeywordExtensionsDelegateImplTest : public ExtensionServiceTestBase {
  public:
   KeywordExtensionsDelegateImplTest() {}
@@ -98,15 +61,11 @@ void KeywordExtensionsDelegateImplTest::RunTest(bool incognito) {
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &path));
     path = path.AppendASCII("extensions").AppendASCII("good_unpacked");
 
-    base::RunLoop run_loop;
-    ScopedExtensionLoadObserver load_observer(registry(),
-                                              run_loop.QuitClosure());
-
+    TestExtensionRegistryObserver load_observer(registry());
     scoped_refptr<UnpackedInstaller> installer(
         UnpackedInstaller::Create(service()));
     installer->Load(path);
-
-    run_loop.Run();
+    EXPECT_TRUE(load_observer.WaitForExtensionInstalled());
   }
 
   ASSERT_EQ(1U, registry()->enabled_extensions().size());
@@ -123,13 +82,9 @@ void KeywordExtensionsDelegateImplTest::RunTest(bool incognito) {
 
   // Enable the extension in incognito mode, which requires a reload.
   {
-    base::RunLoop run_loop;
-    ScopedExtensionLoadObserver load_observer(registry(),
-                                              run_loop.QuitClosure());
-
+    TestExtensionRegistryObserver observer(registry());
     util::SetIsIncognitoEnabled(extension->id(), profile(), true);
-
-    run_loop.Run();
+    EXPECT_TRUE(observer.WaitForExtensionInstalled());
   }
 
   ASSERT_EQ(1U, registry()->enabled_extensions().size());

chrome/browser/extensions/content_verifier_browsertest.cc

@@ -10,8 +10,6 @@
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/macros.h"
-#include "base/run_loop.h"
-#include "base/scoped_observer.h"
 #include "base/strings/string_split.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -32,10 +30,10 @@
 #include "extensions/browser/crx_file_info.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
-#include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/external_install_info.h"
 #include "extensions/browser/external_provider_interface.h"
 #include "extensions/browser/management_policy.h"
+#include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/browser/updater/extension_downloader.h"
 #include "extensions/browser/updater/extension_downloader_test_delegate.h"
 #include "extensions/browser/updater/manifest_fetch_data.h"
@@ -45,77 +43,6 @@ namespace extensions {
 
 namespace {
 
-// Helper for observing extension registry events.
-class RegistryObserver : public ExtensionRegistryObserver {
- public:
-  explicit RegistryObserver(ExtensionRegistry* registry) : observer_(this) {
-    observer_.Add(registry);
-  }
-  ~RegistryObserver() override {}
-
-  // Waits until we've seen an unload for extension with |id|, returning true
-  // if we saw one or false otherwise (typically because of test timeout).
-  bool WaitForUnload(const ExtensionId& id) {
-    if (base::ContainsKey(unloaded_, id))
-      return true;
-
-    base::RunLoop run_loop;
-    awaited_unload_id_ = id;
-    quit_closure_ = run_loop.QuitClosure();
-    run_loop.Run();
-    return base::ContainsKey(unloaded_, id);
-  }
-
-  // Same as WaitForUnload, but for an install.
-  bool WaitForInstall(const ExtensionId& id) {
-    if (base::ContainsKey(installed_, id))
-      return true;
-
-    base::RunLoop run_loop;
-    awaited_install_id_ = id;
-    quit_closure_ = run_loop.QuitClosure();
-    run_loop.Run();
-    return base::ContainsKey(installed_, id);
-  }
-
-  // ExtensionRegistryObserver
-  void OnExtensionUnloaded(content::BrowserContext* browser_context,
-                           const Extension* extension,
-                           UnloadedExtensionReason reason) override {
-    unloaded_.insert(extension->id());
-    if (awaited_unload_id_ == extension->id()) {
-      awaited_unload_id_.clear();
-      base::ResetAndReturn(&quit_closure_).Run();
-    }
-  }
-
-  void OnExtensionInstalled(content::BrowserContext* browser_context,
-                            const Extension* extension,
-                            bool is_update) override {
-    installed_.insert(extension->id());
-    if (awaited_install_id_ == extension->id()) {
-      awaited_install_id_.clear();
-      base::ResetAndReturn(&quit_closure_).Run();
-    }
-  }
-
- private:
-  // The id we're waiting for a load/install of respectively.
-  ExtensionId awaited_unload_id_;
-  ExtensionId awaited_install_id_;
-
-  // The quit closure for stopping a running RunLoop, if we're waiting.
-  base::Closure quit_closure_;
-
-  // The extension id's we've seen unloaded and installed, respectively.
-  std::set<ExtensionId> unloaded_;
-  std::set<ExtensionId> installed_;
-
-  ScopedObserver<ExtensionRegistry, RegistryObserver> observer_;
-
-  DISALLOW_COPY_AND_ASSIGN(RegistryObserver);
-};
-
 class JobObserver : public ContentVerifyJob::TestObserver {
  public:
   JobObserver();
@@ -529,50 +456,54 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierTest, PolicyCorrupted) {
   ExtensionService* service = system->extension_service();
 
   // The id of our test extension.
-  std::string id("npnbmohejbjohgpjnmjagbafnjhkmgko");
+  ExtensionId kExtensionId("npnbmohejbjohgpjnmjagbafnjhkmgko");
 
   // Setup fake policy and update check objects.
-  ForceInstallProvider policy(id);
+  ForceInstallProvider policy(kExtensionId);
   DownloaderTestDelegate downloader;
   system->management_policy()->RegisterProvider(&policy);
   ExtensionDownloader::set_test_delegate(&downloader);
   service->AddProviderForTesting(
-      std::make_unique<TestExternalProvider>(service, id));
+      std::make_unique<TestExternalProvider>(service, kExtensionId));
 
   base::FilePath crx_path =
       test_data_dir_.AppendASCII("content_verifier/v1.crx");
   const Extension* extension =
       InstallExtension(crx_path, 1, Manifest::EXTERNAL_POLICY_DOWNLOAD);
-  EXPECT_NE(extension, nullptr);
+  ASSERT_TRUE(extension);
 
-  downloader.AddResponse(id, extension->VersionString(), crx_path);
+  downloader.AddResponse(kExtensionId, extension->VersionString(), crx_path);
+  EXPECT_EQ(kExtensionId, extension->id());
 
-  RegistryObserver registry_observer(ExtensionRegistry::Get(profile()));
+  TestExtensionRegistryObserver registry_observer(
+      ExtensionRegistry::Get(profile()), kExtensionId);
   ContentVerifier* verifier = system->content_verifier();
-  verifier->VerifyFailed(extension->id(), ContentVerifyJob::HASH_MISMATCH);
+  verifier->VerifyFailed(kExtensionId, ContentVerifyJob::HASH_MISMATCH);
 
   // Make sure the extension first got disabled due to corruption.
-  EXPECT_TRUE(registry_observer.WaitForUnload(id));
+  EXPECT_TRUE(registry_observer.WaitForExtensionUnloaded());
   ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
-  int reasons = prefs->GetDisableReasons(id);
+  int reasons = prefs->GetDisableReasons(kExtensionId);
   EXPECT_TRUE(reasons & disable_reason::DISABLE_CORRUPTED);
 
   // Make sure the extension then got re-installed, and that after reinstall it
   // is no longer disabled due to corruption.
-  EXPECT_TRUE(registry_observer.WaitForInstall(id));
-  reasons = prefs->GetDisableReasons(id);
+  EXPECT_TRUE(registry_observer.WaitForExtensionInstalled());
+
+  reasons = prefs->GetDisableReasons(kExtensionId);
   EXPECT_FALSE(reasons & disable_reason::DISABLE_CORRUPTED);
 
   // Make sure that the update check request properly included a parameter
   // indicating that this was a corrupt policy reinstall.
   bool found = false;
   for (const auto& request : downloader.requests()) {
-    if (request->Includes(id)) {
+    if (request->Includes(kExtensionId)) {
       std::string query = request->full_url().query();
       for (const auto& part : base::SplitString(
                query, "&", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL)) {
         if (base::StartsWith(part, "x=", base::CompareCase::SENSITIVE) &&
-            part.find(std::string("id%3D") + id) != std::string::npos) {
+            part.find(std::string("id%3D") + kExtensionId) !=
+                std::string::npos) {
           found = true;
           EXPECT_NE(std::string::npos, part.find("installsource%3Dreinstall"));
         }
@@ -585,10 +516,6 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierTest, PolicyCorrupted) {
 // Tests that verification failure during navigating to an extension resource
 // correctly disables the extension.
 IN_PROC_BROWSER_TEST_F(ContentVerifierTest, VerificationFailureOnNavigate) {
-  // |unload_observer| needs to destroy before the ExtensionRegistry gets
-  // deleted, which happens before TearDownOnMainThread is called.
-  // TODO(lazyboy): Use TestExtensionRegistryObserver.
-  RegistryObserver unload_observer(ExtensionRegistry::Get(profile()));
   const Extension* extension = InstallExtensionFromWebstore(
       test_data_dir_.AppendASCII("content_verifier/dot_slash_paths.crx"), 1);
   ASSERT_TRUE(extension);
@@ -603,13 +530,15 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierTest, VerificationFailureOnNavigate) {
   }
 
   GURL page_url = extension->GetResourceURL("page.html");
+  TestExtensionRegistryObserver unload_observer(
+      ExtensionRegistry::Get(profile()), kExtensionId);
   // Wait for 0 navigations to complete because with PlzNavigate it's racy
   // when the didstop IPC arrives relative to the tab being closed. The
   // wait call below is what the tests care about.
   ui_test_utils::NavigateToURLWithDispositionBlockUntilNavigationsComplete(
       browser(), page_url, 0, WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_NONE);
-  EXPECT_TRUE(unload_observer.WaitForUnload(kExtensionId));
+  EXPECT_TRUE(unload_observer.WaitForExtensionUnloaded());
   ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
   int reasons = prefs->GetDisableReasons(kExtensionId);
   EXPECT_TRUE(reasons & disable_reason::DISABLE_CORRUPTED);
@@ -657,20 +586,19 @@ class ContentVerifierPolicyTest : public ContentVerifierTest {
 IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest,
                        PRE_PolicyCorruptedOnStartup) {
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile());
-  RegistryObserver registry_observer(registry);
+  TestExtensionRegistryObserver registry_observer(registry, id_);
 
   // Wait for the extension to be installed by policy we set up in
   // SetUpInProcessBrowserTestFixture.
-  if (!registry->GetInstalledExtension(id_)) {
-    EXPECT_TRUE(registry_observer.WaitForInstall(id_));
-  }
+  if (!registry->GetInstalledExtension(id_))
+    EXPECT_TRUE(registry_observer.WaitForExtensionInstalled());
 
   // Simulate corruption of the extension so that we can test what happens
   // at startup in the non-PRE test.
   ExtensionSystem* system = ExtensionSystem::Get(profile());
   ContentVerifier* verifier = system->content_verifier();
   verifier->VerifyFailed(id_, ContentVerifyJob::HASH_MISMATCH);
-  EXPECT_TRUE(registry_observer.WaitForUnload(id_));
+  EXPECT_TRUE(registry_observer.WaitForExtensionUnloaded());
   ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
   int reasons = prefs->GetDisableReasons(id_);
   EXPECT_TRUE(reasons & disable_reason::DISABLE_CORRUPTED);
@@ -687,8 +615,8 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest, PolicyCorruptedOnStartup) {
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile());
   int disable_reasons = prefs->GetDisableReasons(id_);
   if (disable_reasons & disable_reason::DISABLE_CORRUPTED) {
-    RegistryObserver registry_observer(registry);
-    EXPECT_TRUE(registry_observer.WaitForInstall(id_));
+    TestExtensionRegistryObserver registry_observer(registry, id_);
+    EXPECT_TRUE(registry_observer.WaitForExtensionInstalled());
     disable_reasons = prefs->GetDisableReasons(id_);
   }
   EXPECT_FALSE(disable_reasons & disable_reason::DISABLE_CORRUPTED);
@@ -746,8 +674,8 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest, Backoff) {
   // Wait for the extension to be installed by the policy we set up in
   // SetUpInProcessBrowserTestFixture.
   if (!registry->GetInstalledExtension(id_)) {
-    RegistryObserver registry_observer(registry);
-    EXPECT_TRUE(registry_observer.WaitForInstall(id_));
+    TestExtensionRegistryObserver registry_observer(registry, id_);
+    EXPECT_TRUE(registry_observer.WaitForExtensionInstalled());
   }
 
   // Setup to intercept reinstall action, so we can see what the delay would
@@ -757,13 +685,13 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest, Backoff) {
   // Do 4 iterations of disabling followed by reinstall.
   const size_t iterations = 4;
   for (size_t i = 0; i < iterations; i++) {
-    RegistryObserver registry_observer(registry);
+    TestExtensionRegistryObserver registry_observer(registry, id_);
     verifier->VerifyFailed(id_, ContentVerifyJob::HASH_MISMATCH);
-    EXPECT_TRUE(registry_observer.WaitForUnload(id_));
+    EXPECT_TRUE(registry_observer.WaitForExtensionUnloaded());
     // Resolve the request to |delay_tracker|, so the reinstallation can
     // proceed.
     delay_tracker.Proceed();
-    EXPECT_TRUE(registry_observer.WaitForInstall(id_));
+    EXPECT_TRUE(registry_observer.WaitForExtensionInstalled());
   }
   const std::vector<base::TimeDelta>& calls = delay_tracker.calls();
 
@@ -794,15 +722,15 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest, FailedUpdateRetries) {
   // Wait for the extension to be installed by the policy we set up in
   // SetUpInProcessBrowserTestFixture.
   if (!registry->GetInstalledExtension(id_)) {
-    RegistryObserver registry_observer(registry);
-    EXPECT_TRUE(registry_observer.WaitForInstall(id_));
+    TestExtensionRegistryObserver registry_observer(registry, id_);
+    EXPECT_TRUE(registry_observer.WaitForExtensionInstalled());
   }
 
   DelayTracker delay_tracker;
   service->set_external_updates_disabled_for_test(true);
-  RegistryObserver registry_observer(registry);
+  TestExtensionRegistryObserver registry_observer(registry, id_);
   verifier->VerifyFailed(id_, ContentVerifyJob::HASH_MISMATCH);
-  EXPECT_TRUE(registry_observer.WaitForUnload(id_));
+  EXPECT_TRUE(registry_observer.WaitForExtensionUnloaded());
 
   const std::vector<base::TimeDelta>& calls = delay_tracker.calls();
   ASSERT_EQ(1u, calls.size());
@@ -816,7 +744,7 @@ IN_PROC_BROWSER_TEST_F(ContentVerifierPolicyTest, FailedUpdateRetries) {
   service->set_external_updates_disabled_for_test(false);
   delay_tracker.Proceed();
 
-  EXPECT_TRUE(registry_observer.WaitForInstall(id_));
+  EXPECT_TRUE(registry_observer.WaitForExtensionInstalled());
 }
 
 }  // namespace extensions

extensions/browser/test_extension_registry_observer.cc

@@ -4,6 +4,8 @@
 
 #include "extensions/browser/test_extension_registry_observer.h"
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "extensions/browser/extension_registry.h"
@@ -43,11 +45,12 @@ TestExtensionRegistryObserver::TestExtensionRegistryObserver(
 TestExtensionRegistryObserver::TestExtensionRegistryObserver(
     ExtensionRegistry* registry,
     const std::string& extension_id)
-    : will_be_installed_waiter_(new Waiter()),
-      uninstalled_waiter_(new Waiter()),
-      loaded_waiter_(new Waiter()),
-      ready_waiter_(new Waiter()),
-      unloaded_waiter_(new Waiter()),
+    : will_be_installed_waiter_(std::make_unique<Waiter>()),
+      installed_waiter_(std::make_unique<Waiter>()),
+      uninstalled_waiter_(std::make_unique<Waiter>()),
+      loaded_waiter_(std::make_unique<Waiter>()),
+      ready_waiter_(std::make_unique<Waiter>()),
+      unloaded_waiter_(std::make_unique<Waiter>()),
       extension_registry_observer_(this),
       extension_id_(extension_id) {
   extension_registry_observer_.Add(registry);
@@ -65,6 +68,10 @@ TestExtensionRegistryObserver::WaitForExtensionWillBeInstalled() {
   return Wait(&will_be_installed_waiter_);
 }
 
+const Extension* TestExtensionRegistryObserver::WaitForExtensionInstalled() {
+  return Wait(&installed_waiter_);
+}
+
 const Extension* TestExtensionRegistryObserver::WaitForExtensionLoaded() {
   return Wait(&loaded_waiter_);
 }
@@ -86,6 +93,14 @@ void TestExtensionRegistryObserver::OnExtensionWillBeInstalled(
     will_be_installed_waiter_->OnObserved(extension);
 }
 
+void TestExtensionRegistryObserver::OnExtensionInstalled(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    bool is_update) {
+  if (extension_id_.empty() || extension->id() == extension_id_)
+    installed_waiter_->OnObserved(extension);
+}
+
 void TestExtensionRegistryObserver::OnExtensionUninstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
@@ -123,7 +138,7 @@ const Extension* TestExtensionRegistryObserver::Wait(
   // Reset the waiter for future uses.
   // We could have a Waiter::Reset method, but it would reset every field in the
   // class, so let's just reset the pointer.
-  waiter->reset(new Waiter());
+  *waiter = std::make_unique<Waiter>();
   return extension;
 }
 

extensions/browser/test_extension_registry_observer.h

@@ -28,6 +28,7 @@ class TestExtensionRegistryObserver : public ExtensionRegistryObserver {
 
   // Waits for the notification, and returns the extension that caused it.
   const Extension* WaitForExtensionWillBeInstalled();
+  const Extension* WaitForExtensionInstalled();
   const Extension* WaitForExtensionUninstalled();
   const Extension* WaitForExtensionLoaded();
   const Extension* WaitForExtensionReady();
@@ -41,6 +42,9 @@ class TestExtensionRegistryObserver : public ExtensionRegistryObserver {
                                   const Extension* extension,
                                   bool is_update,
                                   const std::string& old_name) override;
+  void OnExtensionInstalled(content::BrowserContext* browser_context,
+                            const Extension* extension,
+                            bool is_update) override;
   void OnExtensionUninstalled(content::BrowserContext* browser_context,
                               const Extension* extension,
                               extensions::UninstallReason reason) override;
@@ -55,6 +59,7 @@ class TestExtensionRegistryObserver : public ExtensionRegistryObserver {
   const Extension* Wait(std::unique_ptr<Waiter>* waiter);
 
   std::unique_ptr<Waiter> will_be_installed_waiter_;
+  std::unique_ptr<Waiter> installed_waiter_;
   std::unique_ptr<Waiter> uninstalled_waiter_;
   std::unique_ptr<Waiter> loaded_waiter_;
   std::unique_ptr<Waiter> ready_waiter_;