Skip to content

Commit

Permalink
Migrate ChromeExtensionTestNotificationObserver off of Notification S…
Browse files Browse the repository at this point in the history 
…ervice

Note these intentional changes:
1. Since NotificationSet is essentially polling for a change in the
result of the predicate passed to it instead of waiting for a
specific event, we don't bother having callers specify which notifications to check the predicate on.
2. Observing render process termination appears to be redundant with
OnExtensionFrameUnregistered for the purposes of this class, so we
no longer explicitly observe the former.
3. It does not appear necessary to observe all sources, so we now
only observe any existing extension related WebContents at the
creation of the observer and then any WebContents created afterwards.

Bug: 1174764, 357627
Change-Id: I64ac3d2810fc49bb5ef2ebc18cad8c9f8fd5f404
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4654944
Auto-Submit: Kevin McNee <mcnee@chromium.org>
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1184319}
  • Loading branch information
@kjmcnee
kjmcnee authored and Chromium LUCI CQ committed Aug 16, 2023
1 parent a09ca9c commit df0c85e
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 65 deletions.
13 changes: 5 additions & 8 deletions chrome/browser/extensions/chrome_extension_test_notification_observer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,7 @@ bool ChromeExtensionTestNotificationObserver::WaitForExtensionViewsToLoad() {
base::RunLoop().RunUntilIdle();

ProcessManager* manager = ProcessManager::Get(GetBrowserContext());
NotificationSet notification_set;
notification_set.AddWebContentsDestroyed(manager);
notification_set.Add(content::NOTIFICATION_LOAD_STOP);
notification_set.AddExtensionFrameUnregistration(manager);
NotificationSet notification_set(manager);
WaitForCondition(
base::BindRepeating(&HaveAllExtensionRenderFrameHostsFinishedLoading,
manager),
Expand All @@ -98,8 +95,8 @@ bool ChromeExtensionTestNotificationObserver::WaitForExtensionViewsToLoad() {

bool ChromeExtensionTestNotificationObserver::WaitForExtensionIdle(
const std::string& extension_id) {
NotificationSet notification_set;
notification_set.Add(content::NOTIFICATION_RENDERER_PROCESS_TERMINATED);
ProcessManager* manager = ProcessManager::Get(GetBrowserContext());
NotificationSet notification_set(manager);
WaitForCondition(base::BindRepeating(&util::IsExtensionIdle, extension_id,
GetBrowserContext()),
&notification_set);
Expand All @@ -108,8 +105,8 @@ bool ChromeExtensionTestNotificationObserver::WaitForExtensionIdle(

bool ChromeExtensionTestNotificationObserver::WaitForExtensionNotIdle(
const std::string& extension_id) {
NotificationSet notification_set;
notification_set.Add(content::NOTIFICATION_LOAD_STOP);
ProcessManager* manager = ProcessManager::Get(GetBrowserContext());
NotificationSet notification_set(manager);
WaitForCondition(base::BindRepeating(
[](const std::string& extension_id,
content::BrowserContext* context) -> bool {
Expand Down
65 changes: 28 additions & 37 deletions extensions/test/extension_test_notification_observer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,26 +6,19 @@
#include "base/memory/raw_ptr.h"

#include <memory>
#include <set>

#include "base/containers/contains.h"
#include "base/functional/bind.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/notification_details.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_observer.h"
#include "content/public/test/test_utils.h"
#include "extensions/browser/notification_types.h"
#include "extensions/common/extension.h"

namespace extensions {

// A callback that returns true if the condition has been met and takes no
// arguments.
using ConditionCallback = base::RepeatingCallback<bool(void)>;

////////////////////////////////////////////////////////////////////////////////
// NotificationSet::ForwardingWebContentsObserver

Expand All @@ -39,6 +32,8 @@ class ExtensionTestNotificationObserver::NotificationSet::

private:
// content::WebContentsObserver
void DidStopLoading() override { owner_->DidStopLoading(web_contents()); }

void WebContentsDestroyed() override {
// Do not add code after this line, deletes `this`.
owner_->WebContentsDestroyed(web_contents());
Expand All @@ -50,47 +45,43 @@ class ExtensionTestNotificationObserver::NotificationSet::
////////////////////////////////////////////////////////////////////////////////
// ExtensionTestNotificationObserver::NotificationSet

ExtensionTestNotificationObserver::NotificationSet::NotificationSet() = default;
ExtensionTestNotificationObserver::NotificationSet::~NotificationSet() =
default;
ExtensionTestNotificationObserver::NotificationSet::NotificationSet(
ProcessManager* manager) {
process_manager_observation_.Observe(manager);

void ExtensionTestNotificationObserver::NotificationSet::Add(
int type,
const content::NotificationSource& source) {
notification_registrar_.Add(this, type, source);
std::set<content::WebContents*> initial_extension_contents;
for (content::RenderFrameHost* rfh : manager->GetAllFrames()) {
initial_extension_contents.insert(
content::WebContents::FromRenderFrameHost(rfh));
}
for (content::WebContents* web_contents : initial_extension_contents) {
StartObservingWebContents(web_contents);
}
}

void ExtensionTestNotificationObserver::NotificationSet::Add(int type) {
Add(type, content::NotificationService::AllSources());
}
ExtensionTestNotificationObserver::NotificationSet::~NotificationSet() =
default;

void ExtensionTestNotificationObserver::NotificationSet::
AddExtensionFrameUnregistration(ProcessManager* manager) {
process_manager_observation_.Observe(manager);
OnExtensionFrameUnregistered(const std::string& extension_id,
content::RenderFrameHost* render_frame_host) {
closure_list_.Notify();
}

void ExtensionTestNotificationObserver::NotificationSet::Observe(
int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) {
closure_list_.Notify();
void ExtensionTestNotificationObserver::NotificationSet::OnWebContentsCreated(
content::WebContents* web_contents) {
StartObservingWebContents(web_contents);
}

void ExtensionTestNotificationObserver::NotificationSet::
AddWebContentsDestroyed(extensions::ProcessManager* manager) {
for (content::RenderFrameHost* render_frame_host : manager->GetAllFrames()) {
content::WebContents* contents =
content::WebContents::FromRenderFrameHost(render_frame_host);
if (!base::Contains(web_contents_observers_, contents)) {
web_contents_observers_[contents] =
std::make_unique<ForwardingWebContentsObserver>(contents, this);
}
}
StartObservingWebContents(content::WebContents* web_contents) {
CHECK(!base::Contains(web_contents_observers_, web_contents));
web_contents_observers_[web_contents] =
std::make_unique<ForwardingWebContentsObserver>(web_contents, this);
}

void ExtensionTestNotificationObserver::NotificationSet::
OnExtensionFrameUnregistered(const std::string& extension_id,
content::RenderFrameHost* render_frame_host) {
void ExtensionTestNotificationObserver::NotificationSet::DidStopLoading(
content::WebContents* web_contents) {
closure_list_.Notify();
}

Expand Down
40 changes: 20 additions & 20 deletions extensions/test/extension_test_notification_observer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,12 @@
#include "base/functional/callback.h"
#include "base/memory/raw_ptr.h"
#include "base/scoped_observation.h"
#include "content/public/browser/notification_observer.h"
#include "content/public/browser/notification_registrar.h"
#include "content/public/test/browser_test_utils.h"
#include "extensions/browser/process_manager.h"
#include "extensions/browser/process_manager_observer.h"

namespace content {
class BrowserContext;
class NotificationDetails;
class WebContents;
}

Expand All @@ -39,56 +37,58 @@ class ExtensionTestNotificationObserver {
~ExtensionTestNotificationObserver();

protected:
class NotificationSet : public content::NotificationObserver,
public extensions::ProcessManagerObserver {
class NotificationSet : public extensions::ProcessManagerObserver {
public:
NotificationSet();
explicit NotificationSet(ProcessManager* manager);

NotificationSet(const NotificationSet&) = delete;
NotificationSet& operator=(const NotificationSet&) = delete;

~NotificationSet() override;

void Add(int type, const content::NotificationSource& source);
void Add(int type);
void AddExtensionFrameUnregistration(extensions::ProcessManager* manager);
void AddWebContentsDestroyed(extensions::ProcessManager* manager);

// Notified any time an Add()ed notification is received.
// Notified any time a notification is received.
// The details of the notification are dropped.
base::RepeatingClosureList& closure_list() { return closure_list_; }

private:
class ForwardingWebContentsObserver;

// content::NotificationObserver:
void Observe(int type,
const content::NotificationSource& source,
const content::NotificationDetails& details) override;

// extensions::ProcessManagerObserver:
void OnExtensionFrameUnregistered(
const std::string& extension_id,
content::RenderFrameHost* render_frame_host) override;

void OnWebContentsCreated(content::WebContents* web_contents);
void StartObservingWebContents(content::WebContents* web_contents);

void DidStopLoading(content::WebContents* web_contents);
void WebContentsDestroyed(content::WebContents* web_contents);

content::NotificationRegistrar notification_registrar_;
base::RepeatingClosureList closure_list_;

base::ScopedObservation<extensions::ProcessManager,
extensions::ProcessManagerObserver>
process_manager_observation_{this};

base::CallbackListSubscription web_contents_creation_subscription_ =
content::RegisterWebContentsCreationCallback(
base::BindRepeating(&NotificationSet::OnWebContentsCreated,
base::Unretained(this)));

std::map<content::WebContents*,
std::unique_ptr<ForwardingWebContentsObserver>>
web_contents_observers_;
};

// A callback that returns true if the condition has been met and takes no
// arguments.
using ConditionCallback = base::RepeatingCallback<bool(void)>;

// Wait for |condition_| to be met. |notification_set| is the set of
// notifications to wait for and to check |condition| when observing. This
// can be NULL if we are instead waiting for a different observer method, like
// OnPageActionsUpdated().
void WaitForCondition(const base::RepeatingCallback<bool(void)>& condition,
void WaitForCondition(const ConditionCallback& condition,
NotificationSet* notification_set);

// Quits the message loop if |condition_| is met.
Expand All @@ -99,7 +99,7 @@ class ExtensionTestNotificationObserver {
private:
// The condition for which we are waiting. This should be checked in any
// observing methods that could trigger it.
base::RepeatingCallback<bool(void)> condition_;
ConditionCallback condition_;

// The closure to quit the currently-running message loop.
base::OnceClosure quit_closure_;
Expand Down

0 comments on commit df0c85e

Please sign in to comment.