Skip to content

Commit

Permalink
Change EXTENSION_PROCESS_CREATED observers to use EXTENSION_HOST_CREA…
Browse files Browse the repository at this point in the history
…TED.

Change EXTENSION_HOST_CREATED to be fired on RenderViewReady.
Remove the EXTENSION_PROCESS_CREATED notification.

This is related to fixing bug 71629, in particular, making the correct things appear in the task manager when the extension host reuses a process. This affects the new reload path (but only for unpacked extensions), but it's also visible when a process isn't created for a popup in lots-of-tabs situations.

The extension settings page (the other observer) already listens to enough notifications that it updates properly (too many of them, but that's another story -- see bug).

BUG=102950
TEST=TaskManagerBrowserTest.* and also: Open Task Manager. Open an extension popup, extension notification, and extension page in a tab; check that they all appear. Kill an extension with a background page and reload it from the notification bubble; check that it reappears.

Review URL: http://codereview.chromium.org/8480007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@109163 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
yoz@chromium.org committed Nov 9, 2011
1 parent 552a01e commit 8f51a1c
Show file tree
Hide file tree
Showing 8 changed files with 29 additions and 51 deletions.
5 changes: 0 additions & 5 deletions chrome/browser/extensions/extension_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -466,11 +466,6 @@ void ExtensionBrowserTest::Observe(
MessageLoopForUI::current()->Quit();
break;

case chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED:
VLOG(1) << "Got EXTENSION_PROCESS_CREATED notification.";
MessageLoopForUI::current()->Quit();
break;

case chrome::NOTIFICATION_EXTENSION_PROCESS_TERMINATED:
VLOG(1) << "Got EXTENSION_PROCESS_TERMINATED notification.";
MessageLoopForUI::current()->Quit();
Expand Down
17 changes: 7 additions & 10 deletions chrome/browser/extensions/extension_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -148,10 +148,6 @@ ExtensionHost::ExtensionHost(const Extension* extension,
if (enable_dom_automation_)
render_view_host_->AllowBindings(content::BINDINGS_POLICY_DOM_AUTOMATION);

// Listen for when the render process' handle is available so we can add it
// to the task manager then.
registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED,
content::Source<RenderProcessHost>(render_process_host()));
// Listen for when an extension is unloaded from the same profile, as it may
// be the same extension that this points to.
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
Expand Down Expand Up @@ -297,12 +293,6 @@ void ExtensionHost::Observe(int type,
IsBackgroundPageReady(extension_));
NavigateToURL(url_);
break;
case content::NOTIFICATION_RENDERER_PROCESS_CREATED:
content::NotificationService::current()->Notify(
chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED,
content::Source<Profile>(profile_),
content::Details<ExtensionHost>(this));
break;
case chrome::NOTIFICATION_EXTENSION_UNLOADED:
// The extension object will be deleted after this notification has been
// sent. NULL it out so that dirty pointer issues don't arise in cases
Expand Down Expand Up @@ -809,3 +799,10 @@ void ExtensionHost::RenderViewCreated(RenderViewHost* render_view_host) {
kPreferredSizeWidth | kPreferredSizeHeightThisIsSlow);
}
}

void ExtensionHost::RenderViewReady(RenderViewHost* render_view_host) {
content::NotificationService::current()->Notify(
chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
content::Source<Profile>(profile_),
content::Details<ExtensionHost>(this));
}
1 change: 1 addition & 0 deletions chrome/browser/extensions/extension_host.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ class ExtensionHost : public RenderViewHostDelegate,
virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
virtual const GURL& GetURL() const OVERRIDE;
virtual void RenderViewCreated(RenderViewHost* render_view_host) OVERRIDE;
virtual void RenderViewReady(RenderViewHost* render_view_host) OVERRIDE;
virtual content::ViewType GetRenderViewType() const OVERRIDE;
virtual void RenderViewGone(RenderViewHost* render_view_host,
base::TerminationStatus status,
Expand Down
4 changes: 0 additions & 4 deletions chrome/browser/extensions/extension_process_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -534,10 +534,6 @@ void ExtensionProcessManager::OnExtensionHostCreated(ExtensionHost* host,
all_hosts_.insert(host);
if (is_background)
background_hosts_.insert(host);
content::NotificationService::current()->Notify(
chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
content::Source<ExtensionProcessManager>(this),
content::Details<ExtensionHost>(host));
}

void ExtensionProcessManager::CloseBackgroundHost(ExtensionHost* host) {
Expand Down
1 change: 0 additions & 1 deletion chrome/browser/extensions/extension_process_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ class ExtensionProcessManager : public content::NotificationObserver {
typedef std::set<RenderViewHost*> RenderViewHostSet;
RenderViewHostSet all_extension_views_;

private:
// Close the given |host| iff it's a background page.
void CloseBackgroundHost(ExtensionHost* host);

Expand Down
36 changes: 15 additions & 21 deletions chrome/browser/task_manager/task_manager_resource_providers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1161,33 +1161,31 @@ void TaskManagerExtensionProcessResourceProvider::StartUpdating() {
DCHECK(!updating_);
updating_ = true;

// Add all the existing ExtensionHosts.
// Add all the existing ExtensionHosts from all Profiles, including those from
// incognito split mode.
ProfileManager* profile_manager = g_browser_process->profile_manager();
std::vector<Profile*> profiles(profile_manager->GetLoadedProfiles());
size_t num_default_profiles = profiles.size();
for (size_t i = 0; i < num_default_profiles; ++i) {
if (profiles[i]->HasOffTheRecordProfile()) {
profiles.push_back(profiles[i]->GetOffTheRecordProfile());
}
}
for (size_t i = 0; i < profiles.size(); ++i) {
ExtensionProcessManager* process_manager =
profiles[i]->GetExtensionProcessManager();
if (process_manager) {
ExtensionProcessManager::const_iterator jt;
for (jt = process_manager->begin(); jt != process_manager->end(); ++jt)
AddToTaskManager(*jt);
}

// If we have an incognito profile active, include the split-mode incognito
// extensions.
if (BrowserList::IsOffTheRecordSessionActiveForProfile(profiles[i])) {
ExtensionProcessManager* process_manager =
profiles[i]->GetOffTheRecordProfile()->GetExtensionProcessManager();
if (process_manager) {
ExtensionProcessManager::const_iterator jt;
for (jt = process_manager->begin(); jt != process_manager->end(); ++jt)
AddToTaskManager(*jt);
for (jt = process_manager->begin(); jt != process_manager->end(); ++jt) {
// Don't add dead extension processes.
if ((*jt)->IsRenderViewLive())
AddToTaskManager(*jt);
}
}
}

// Register for notifications about extension process changes.
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED,
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
content::NotificationService::AllBrowserContextsAndSources());
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_PROCESS_TERMINATED,
content::NotificationService::AllBrowserContextsAndSources());
Expand All @@ -1201,7 +1199,7 @@ void TaskManagerExtensionProcessResourceProvider::StopUpdating() {

// Unregister for notifications about extension process changes.
registrar_.Remove(
this, chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED,
this, chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
content::NotificationService::AllBrowserContextsAndSources());
registrar_.Remove(
this, chrome::NOTIFICATION_EXTENSION_PROCESS_TERMINATED,
Expand All @@ -1222,7 +1220,7 @@ void TaskManagerExtensionProcessResourceProvider::Observe(
const content::NotificationSource& source,
const content::NotificationDetails& details) {
switch (type) {
case chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED:
case chrome::NOTIFICATION_EXTENSION_HOST_CREATED:
AddToTaskManager(content::Details<ExtensionHost>(details).ptr());
break;
case chrome::NOTIFICATION_EXTENSION_PROCESS_TERMINATED:
Expand All @@ -1237,10 +1235,6 @@ void TaskManagerExtensionProcessResourceProvider::Observe(

void TaskManagerExtensionProcessResourceProvider::AddToTaskManager(
ExtensionHost* extension_host) {
// Don't add dead extension processes.
if (!extension_host->IsRenderViewLive())
return;

TaskManagerExtensionProcessResource* resource =
new TaskManagerExtensionProcessResource(extension_host);
DCHECK(resources_.find(extension_host) == resources_.end());
Expand Down
10 changes: 5 additions & 5 deletions chrome/browser/ui/webui/options/extension_settings_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -210,14 +210,14 @@ void ExtensionSettingsHandler::MaybeRegisterForNotifications() {
// Register for notifications that we need to reload the page.
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
content::Source<Profile>(profile));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED,
content::Source<Profile>(profile));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
content::Source<Profile>(profile));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UPDATE_DISABLED,
content::Source<Profile>(profile));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_WARNING_CHANGED,
content::Source<Profile>(profile));
registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_CREATED,
content::NotificationService::AllBrowserContextsAndSources());
registrar_.Add(this,
content::NOTIFICATION_RENDER_VIEW_HOST_CREATED,
content::NotificationService::AllBrowserContextsAndSources());
Expand Down Expand Up @@ -597,9 +597,9 @@ void ExtensionSettingsHandler::Observe(
switch (type) {
// We listen for notifications that will result in the page being
// repopulated with data twice for the same event in certain cases.
// For instance, EXTENSION_LOADED & EXTENSION_PROCESS_CREATED because
// For instance, EXTENSION_LOADED & EXTENSION_HOST_CREATED because
// we don't know about the views for an extension at EXTENSION_LOADED, but
// if we only listen to EXTENSION_PROCESS_CREATED, we'll miss extensions
// if we only listen to EXTENSION_HOST_CREATED, we'll miss extensions
// that don't have a process at startup.
//
// Doing it this way gets everything but causes the page to be rendered
Expand All @@ -620,13 +620,13 @@ void ExtensionSettingsHandler::Observe(
content::Details<BackgroundContents>(details)->render_view_host();
// Fall through.
case chrome::NOTIFICATION_BACKGROUND_CONTENTS_NAVIGATED:
case chrome::NOTIFICATION_EXTENSION_HOST_CREATED:
source_profile = content::Source<Profile>(source).ptr();
if (!profile->IsSameProfile(source_profile))
return;
MaybeUpdateAfterNotification();
break;
case chrome::NOTIFICATION_EXTENSION_LOADED:
case chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED:
case chrome::NOTIFICATION_EXTENSION_UNLOADED:
case chrome::NOTIFICATION_EXTENSION_UPDATE_DISABLED:
case chrome::NOTIFICATION_EXTENSION_WARNING_CHANGED:
Expand Down
6 changes: 1 addition & 5 deletions chrome/common/chrome_notification_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ enum NotificationType {
NOTIFICATION_EXTENSION_UNLOADED,

// Sent after a new ExtensionHost is created. The details are
// an ExtensionHost* and the source is an ExtensionProcessManager*.
// an ExtensionHost* and the source is a Profile*.
NOTIFICATION_EXTENSION_HOST_CREATED,

// Sent before an ExtensionHost is destroyed. The details are
Expand All @@ -433,10 +433,6 @@ enum NotificationType {
// Profile*.
NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE,

// Sent after an extension render process is created and fully functional.
// The details are an ExtensionHost*.
NOTIFICATION_EXTENSION_PROCESS_CREATED,

// Sent when extension render process ends (whether it crashes or closes).
// The details are an ExtensionHost* and the source is a Profile*. Not sent
// during browser shutdown.
Expand Down

0 comments on commit 8f51a1c

Please sign in to comment.