Skip to content

Commit

Permalink
[Extensions] Remove LazyBackgroundObserver
Browse files Browse the repository at this point in the history
LazyBackgroundObserver is a test class that provides utilities to wait
for an extension's background page to open (WaitUntilLoaded()),
close (WaitUntilClosed()), or both open and then close (Wait()).

The functionality of this class can be entirely replaced by the
ExtensionHostTestHelper:

Before:
LazyBackgroundObserver lazy_background_observer(profile())
...
lazy_background_observer.Wait();

After:
ExtensionHostTestHelper host_helper(profile());
host_helper.RestrictToType(
    mojom::ViewType::kExtensionBackgroundPage);
...
host_helper.WaitForDocumentElementAvailable();
host_helper.WaitForExtensionHostDestroyed();

This is slightly more verbose, however:
- This verbosity is actually rather useful in the test. It was not clear
  from many callsites that they were actually waiting for a full open +
  close cycle, and some did not need to wait for both events.
- This reduces the number of ExtensionHost / background waiting
  utilities we have, which is helpful (and ExtensionHostTestHelper is
  far more generally useful than LazyBackgroundObserver).

Also ensure that ExtensionHostTestHelper checks the BrowserContext of
the ExtensionHost when waiting for an event (since it might be the
on- or off-the-record pair of the one the host helper was constructed
with).

Bug: 991464
Change-Id: I7cb35846441c7ad977d6cf3111c397ff84575bb4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3182756
Reviewed-by: Noel Gordon <noel@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926343}
  • Loading branch information
rdcronin authored and Chromium LUCI CQ committed Sep 29, 2021
1 parent aae5484 commit c2fafd3
Show file tree
Hide file tree
Showing 10 changed files with 135 additions and 102 deletions.
12 changes: 7 additions & 5 deletions chrome/browser/ash/file_manager/external_filesystem_apitest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include "chrome/browser/ash/profiles/profile_helper.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/extensions/extension_apitest.h"
#include "chrome/browser/extensions/lazy_background_page_test_util.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/ash/cast_config_controller_media_router.h"
Expand All @@ -37,7 +36,8 @@
#include "content/public/test/browser_test.h"
#include "content/public/test/test_navigation_observer.h"
#include "content/public/test/test_utils.h"
#include "extensions/browser/notification_types.h"
#include "extensions/browser/extension_host_test_helper.h"
#include "extensions/common/mojom/view_type.mojom.h"
#include "extensions/test/result_catcher.h"
#include "google_apis/common/test_util.h"
#include "storage/browser/file_system/external_mount_points.h"
Expand Down Expand Up @@ -339,7 +339,9 @@ class FileSystemExtensionApiTestBase : public extensions::ExtensionApiTest {
extensions::ProcessManager::SetEventPageIdleTimeForTesting(1);
}

LazyBackgroundObserver page_complete(profile());
extensions::ExtensionHostTestHelper host_helper(profile());
host_helper.RestrictToType(
extensions::mojom::ViewType::kExtensionBackgroundPage);
const Extension* file_handler =
LoadExtension(test_data_dir_.AppendASCII(filehandler_path));
if (!file_handler) {
Expand All @@ -348,9 +350,9 @@ class FileSystemExtensionApiTestBase : public extensions::ExtensionApiTest {
}

if (flags & FLAGS_LAZY_FILE_HANDLER) {
page_complete.WaitUntilClosed();
host_helper.WaitForHostDestroyed();
} else {
page_complete.WaitUntilLoaded();
host_helper.WaitForDocumentElementAvailable();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@
#include "chrome/browser/extensions/api/system_indicator/system_indicator_manager.h"
#include "chrome/browser/extensions/api/system_indicator/system_indicator_manager_factory.h"
#include "chrome/browser/extensions/extension_apitest.h"
#include "chrome/browser/extensions/lazy_background_page_test_util.h"
#include "chrome/browser/ui/browser.h"
#include "components/version_info/channel.h"
#include "content/public/test/browser_test.h"
#include "extensions/browser/extension_action.h"
#include "extensions/browser/extension_action_manager.h"
#include "extensions/browser/extension_host_test_helper.h"
#include "extensions/browser/process_manager.h"
#include "extensions/common/extension.h"
#include "extensions/common/features/feature_channel.h"
#include "extensions/common/manifest_test.h"
#include "extensions/common/mojom/view_type.mojom.h"
#include "extensions/test/result_catcher.h"

namespace extensions {
Expand All @@ -39,11 +40,15 @@ class SystemIndicatorApiTest : public ExtensionApiTest {
}

const Extension* LoadExtensionAndWait(const std::string& test_name) {
LazyBackgroundObserver page_complete(profile());
ExtensionHostTestHelper host_helper(profile());
host_helper.RestrictToType(mojom::ViewType::kExtensionBackgroundPage);
base::FilePath extdir = test_data_dir_.AppendASCII(test_name);
const Extension* extension = LoadExtension(extdir);
if (extension)
page_complete.Wait();
if (extension) {
// Wait for the background page to cycle.
host_helper.WaitForDocumentElementAvailable();
host_helper.WaitForHostDestroyed();
}
return extension;
}

Expand Down
17 changes: 12 additions & 5 deletions chrome/browser/extensions/extension_context_menu_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include "build/chromeos_buildflags.h"
#include "chrome/browser/extensions/browsertest_util.h"
#include "chrome/browser/extensions/extension_browsertest.h"
#include "chrome/browser/extensions/lazy_background_page_test_util.h"
#include "chrome/browser/extensions/menu_manager_test_observer.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/renderer_context_menu/render_view_context_menu.h"
Expand All @@ -32,12 +31,14 @@
#include "content/public/test/browser_test.h"
#include "content/public/test/browser_test_utils.h"
#include "extensions/browser/extension_api_frame_id_map.h"
#include "extensions/browser/extension_host_test_helper.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/process_manager.h"
#include "extensions/browser/state_store.h"
#include "extensions/browser/test_management_policy.h"
#include "extensions/common/extension_set.h"
#include "extensions/common/features/feature_channel.h"
#include "extensions/common/mojom/view_type.mojom.h"
#include "extensions/test/extension_test_message_listener.h"
#include "extensions/test/result_catcher.h"
#include "net/dns/mock_host_resolver.h"
Expand Down Expand Up @@ -927,11 +928,15 @@ IN_PROC_BROWSER_TEST_P(ExtensionContextMenuLazyTest, EventPage) {
if (GetParam() == ContextType::kServiceWorker)
return;
GURL about_blank("about:blank");
LazyBackgroundObserver page_complete(profile());
extensions::ExtensionHostTestHelper host_helper(profile());
host_helper.RestrictToType(
extensions::mojom::ViewType::kExtensionBackgroundPage);
const extensions::Extension* extension =
LoadContextMenuExtension("event_page");
ASSERT_TRUE(extension);
page_complete.Wait();
// Wait for the background page to cycle.
host_helper.WaitForDocumentElementAvailable();
host_helper.WaitForHostDestroyed();

// Test that menu items appear while the page is unloaded.
ASSERT_TRUE(MenuHasItemWithLabel(
Expand All @@ -940,7 +945,9 @@ IN_PROC_BROWSER_TEST_P(ExtensionContextMenuLazyTest, EventPage) {
about_blank, GURL(), GURL(), std::string("Checkbox 1")));

// Test that checked menu items retain their checkedness.
LazyBackgroundObserver checkbox_checked(profile());
extensions::ExtensionHostTestHelper checkbox_checked(profile());
host_helper.RestrictToType(
extensions::mojom::ViewType::kExtensionBackgroundPage);
std::unique_ptr<TestRenderViewContextMenu> menu(
TestRenderViewContextMenu::Create(GetWebContents(), about_blank, GURL(),
GURL()));
Expand All @@ -954,7 +961,7 @@ IN_PROC_BROWSER_TEST_P(ExtensionContextMenuLazyTest, EventPage) {
// Executing the checkbox also fires the onClicked event.
ExtensionTestMessageListener listener("onClicked fired for checkbox1", false);
menu->ExecuteCommand(command_id, 0);
checkbox_checked.WaitUntilClosed();
checkbox_checked.WaitForHostDestroyed();

EXPECT_TRUE(menu->IsCommandIdChecked(command_id));
ASSERT_TRUE(listener.WaitUntilSatisfied());
Expand Down
Loading

0 comments on commit c2fafd3

Please sign in to comment.