diff options
author | jamescook@chromium.org <jamescook@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-17 17:21:45 +0000 |
---|---|---|
committer | jamescook@chromium.org <jamescook@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-17 17:21:45 +0000 |
commit | 46a19f626f5794f298cd1e4d8bdcbfcb07fd0b4e (patch) | |
tree | 5a04abc6c4c823a4ddf1ca0d76fcc1ea47ff009a | |
parent | dddea5bb5ab70ac91118754eb09ccb30dcb178a2 (diff) | |
download | chromium_src-46a19f626f5794f298cd1e4d8bdcbfcb07fd0b4e.zip chromium_src-46a19f626f5794f298cd1e4d8bdcbfcb07fd0b4e.tar.gz chromium_src-46a19f626f5794f298cd1e4d8bdcbfcb07fd0b4e.tar.bz2 |
Revert 283678 "Refactor code that defers extension background pa..."
This broke Chrome OS valgrind bots, for example:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28valgrind%29%284%29/builds/27033
> Refactor code that defers extension background page loading
>
> src/extensions depends on chrome::NOTIFICATION_PROFILE_CREATED to support deferred loading of extension background pages when the profile isn't ready yet. This is a layering violation.
>
> * Remove Chrome concepts like "browser window ready" and "profile created" from ProcessManager. Introduce ProcessManagerDelegate with a general concept of deferring background page loading.
> * Consolidate all the tricky Chrome-specific background page loading rules into ChromeProcessManagerDelegate. This keeps all the rules in one place. Annotate each block of special case code with the bug that inspired it.
> * Extend unit test coverage for ProcessManager.
>
> This will make it easier to eliminate chrome::NOTIFICATION_PROFILE_DESTROYED in ProcessManager in a later CL.
>
> BUG=392658
> TEST=unit_tests ProcessManagerTest, browser_tests ProcessManagerBrowserTest, manual
>
> Review URL: https://codereview.chromium.org/381283002
TBR=jamescook@chromium.org
Review URL: https://codereview.chromium.org/399153002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283801 0039d316-1c4b-4281-b951-d872f2087c98
18 files changed, 174 insertions, 443 deletions
diff --git a/apps/shell/browser/shell_extensions_browser_client.cc b/apps/shell/browser/shell_extensions_browser_client.cc index 54b0df8..76f4a83 100644 --- a/apps/shell/browser/shell_extensions_browser_client.cc +++ b/apps/shell/browser/shell_extensions_browser_client.cc @@ -183,9 +183,14 @@ void ShellExtensionsBrowserClient::GetEarlyExtensionPrefsObservers( content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const {} -ProcessManagerDelegate* -ShellExtensionsBrowserClient::GetProcessManagerDelegate() const { - return NULL; +bool ShellExtensionsBrowserClient::DeferLoadingBackgroundHosts( + BrowserContext* context) const { + return false; +} + +bool ShellExtensionsBrowserClient::IsBackgroundPageAllowed( + BrowserContext* context) const { + return true; } scoped_ptr<ExtensionHostDelegate> diff --git a/apps/shell/browser/shell_extensions_browser_client.h b/apps/shell/browser/shell_extensions_browser_client.h index b6a20f7..2013c6d 100644 --- a/apps/shell/browser/shell_extensions_browser_client.h +++ b/apps/shell/browser/shell_extensions_browser_client.h @@ -59,7 +59,10 @@ class ShellExtensionsBrowserClient : public ExtensionsBrowserClient { virtual void GetEarlyExtensionPrefsObservers( content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const OVERRIDE; - virtual ProcessManagerDelegate* GetProcessManagerDelegate() const OVERRIDE; + virtual bool DeferLoadingBackgroundHosts(content::BrowserContext* context) + const OVERRIDE; + virtual bool IsBackgroundPageAllowed(content::BrowserContext* context) + const OVERRIDE; virtual scoped_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() OVERRIDE; virtual bool DidVersionUpdate(content::BrowserContext* context) OVERRIDE; diff --git a/chrome/browser/extensions/chrome_extensions_browser_client.cc b/chrome/browser/extensions/chrome_extensions_browser_client.cc index db2ae3b..c66d023 100644 --- a/chrome/browser/extensions/chrome_extensions_browser_client.cc +++ b/chrome/browser/extensions/chrome_extensions_browser_client.cc @@ -21,6 +21,7 @@ #include "chrome/browser/external_protocol/external_protocol_handler.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/ui/browser_finder.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_version_info.h" @@ -41,14 +42,12 @@ #include "chrome/browser/extensions/activity_log/activity_log.h" #include "chrome/browser/extensions/api/chrome_extensions_api_client.h" #include "chrome/browser/extensions/api/content_settings/content_settings_service.h" -#include "chrome/browser/extensions/chrome_process_manager_delegate.h" #endif namespace extensions { ChromeExtensionsBrowserClient::ChromeExtensionsBrowserClient() { #if defined(ENABLE_EXTENSIONS) - process_manager_delegate_.reset(new ChromeProcessManagerDelegate); api_client_.reset(new ChromeExtensionsAPIClient); #endif // Only set if it hasn't already been set (e.g. by a test). @@ -159,15 +158,35 @@ void ChromeExtensionsBrowserClient::GetEarlyExtensionPrefsObservers( #endif } -ProcessManagerDelegate* -ChromeExtensionsBrowserClient::GetProcessManagerDelegate() const { -#if defined(ENABLE_EXTENSIONS) - return process_manager_delegate_.get(); +bool ChromeExtensionsBrowserClient::DeferLoadingBackgroundHosts( + content::BrowserContext* context) const { + Profile* profile = static_cast<Profile*>(context); + + // The profile may not be valid yet if it is still being initialized. + // In that case, defer loading, since it depends on an initialized profile. + // http://crbug.com/222473 + if (!g_browser_process->profile_manager()->IsValidProfile(profile)) + return true; + +#if defined(OS_ANDROID) + return false; #else - return NULL; + // There are no browser windows open and the browser process was + // started to show the app launcher. + return chrome::GetTotalBrowserCountForProfile(profile) == 0 && + CommandLine::ForCurrentProcess()->HasSwitch(switches::kShowAppList); #endif } +bool ChromeExtensionsBrowserClient::IsBackgroundPageAllowed( + content::BrowserContext* context) const { + // Returns true if current session is Guest mode session and current + // browser context is *not* off-the-record. Such context is artificial and + // background page shouldn't be created in it. + return !static_cast<Profile*>(context)->IsGuestSession() || + context->IsOffTheRecord(); +} + scoped_ptr<ExtensionHostDelegate> ChromeExtensionsBrowserClient::CreateExtensionHostDelegate() { return scoped_ptr<ExtensionHostDelegate>(new ChromeExtensionHostDelegate); diff --git a/chrome/browser/extensions/chrome_extensions_browser_client.h b/chrome/browser/extensions/chrome_extensions_browser_client.h index 5957838..3c4a2c6 100644 --- a/chrome/browser/extensions/chrome_extensions_browser_client.h +++ b/chrome/browser/extensions/chrome_extensions_browser_client.h @@ -26,7 +26,6 @@ namespace extensions { class ChromeComponentExtensionResourceManager; class ChromeExtensionsAPIClient; -class ChromeProcessManagerDelegate; class ContentSettingsPrefsObserver; // Implementation of extensions::BrowserClient for Chrome, which includes @@ -77,7 +76,10 @@ class ChromeExtensionsBrowserClient : public ExtensionsBrowserClient { virtual void GetEarlyExtensionPrefsObservers( content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const OVERRIDE; - virtual ProcessManagerDelegate* GetProcessManagerDelegate() const OVERRIDE; + virtual bool DeferLoadingBackgroundHosts( + content::BrowserContext* context) const OVERRIDE; + virtual bool IsBackgroundPageAllowed( + content::BrowserContext* context) const OVERRIDE; virtual scoped_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() OVERRIDE; virtual bool DidVersionUpdate(content::BrowserContext* context) OVERRIDE; @@ -100,9 +102,6 @@ class ChromeExtensionsBrowserClient : public ExtensionsBrowserClient { ChromeNotificationObserver notification_observer_; #if defined(ENABLE_EXTENSIONS) - // Support for ProcessManager. - scoped_ptr<ChromeProcessManagerDelegate> process_manager_delegate_; - // Client for API implementations. scoped_ptr<ChromeExtensionsAPIClient> api_client_; #endif diff --git a/chrome/browser/extensions/chrome_notification_observer.cc b/chrome/browser/extensions/chrome_notification_observer.cc index c8e6016..5d966c8 100644 --- a/chrome/browser/extensions/chrome_notification_observer.cc +++ b/chrome/browser/extensions/chrome_notification_observer.cc @@ -4,22 +4,59 @@ #include "chrome/browser/extensions/chrome_notification_observer.h" +#include "base/logging.h" +#include "chrome/browser/chrome_notification_types.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/ui/browser.h" #include "chrome/common/extensions/features/feature_channel.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_types.h" #include "content/public/browser/render_process_host.h" +#include "extensions/browser/extension_system.h" +#include "extensions/browser/process_manager.h" #include "extensions/common/extension_messages.h" namespace extensions { ChromeNotificationObserver::ChromeNotificationObserver() { registrar_.Add(this, + chrome::NOTIFICATION_BROWSER_WINDOW_READY, + content::NotificationService::AllSources()); + registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED, content::NotificationService::AllBrowserContextsAndSources()); } ChromeNotificationObserver::~ChromeNotificationObserver() {} +void ChromeNotificationObserver::OnBrowserWindowReady(Browser* browser) { + Profile* profile = browser->profile(); + DCHECK(profile); + + // Inform the process manager for this profile that the window is ready. + // We continue to observe the notification in case browser windows open for + // a related incognito profile or other regular profiles. + extensions::ProcessManager* manager = + ExtensionSystem::Get(profile)->process_manager(); + if (!manager) // Tests may not have a process manager. + return; + DCHECK_EQ(profile, manager->GetBrowserContext()); + manager->OnBrowserWindowReady(); + + // For incognito profiles also inform the original profile's process manager + // that the window is ready. This will usually be a no-op because the + // original profile's process manager should have been informed when the + // non-incognito window opened. + if (profile->IsOffTheRecord()) { + Profile* original_profile = profile->GetOriginalProfile(); + extensions::ProcessManager* original_manager = + ExtensionSystem::Get(original_profile)->process_manager(); + DCHECK(original_manager); + DCHECK_EQ(original_profile, original_manager->GetBrowserContext()); + original_manager->OnBrowserWindowReady(); + } +} + void ChromeNotificationObserver::OnRendererProcessCreated( content::RenderProcessHost* process) { // Extensions need to know the channel for API restrictions. Send the channel @@ -31,6 +68,12 @@ void ChromeNotificationObserver::Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) { switch (type) { + case chrome::NOTIFICATION_BROWSER_WINDOW_READY: { + Browser* browser = content::Source<Browser>(source).ptr(); + OnBrowserWindowReady(browser); + break; + } + case content::NOTIFICATION_RENDERER_PROCESS_CREATED: { content::RenderProcessHost* process = content::Source<content::RenderProcessHost>(source).ptr(); diff --git a/chrome/browser/extensions/chrome_notification_observer.h b/chrome/browser/extensions/chrome_notification_observer.h index 8045b15..a31dc87 100644 --- a/chrome/browser/extensions/chrome_notification_observer.h +++ b/chrome/browser/extensions/chrome_notification_observer.h @@ -25,7 +25,8 @@ class ChromeNotificationObserver : public content::NotificationObserver { ChromeNotificationObserver(); virtual ~ChromeNotificationObserver(); - // Notification handlers: + // IPC message handlers: + void OnBrowserWindowReady(Browser* browser); void OnRendererProcessCreated(content::RenderProcessHost* process); // content::NotificationObserver overrides: diff --git a/chrome/browser/extensions/chrome_process_manager_delegate.cc b/chrome/browser/extensions/chrome_process_manager_delegate.cc deleted file mode 100644 index 12e0c16..0000000 --- a/chrome/browser/extensions/chrome_process_manager_delegate.cc +++ /dev/null @@ -1,143 +0,0 @@ -// Copyright 2014 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 "chrome/browser/extensions/chrome_process_manager_delegate.h" - -#include "base/command_line.h" -#include "base/logging.h" -#include "chrome/browser/browser_process.h" -#include "chrome/browser/chrome_notification_types.h" -#include "chrome/browser/profiles/profile.h" -#include "chrome/browser/profiles/profile_manager.h" -#include "chrome/browser/ui/browser.h" -#include "content/public/browser/notification_service.h" -#include "extensions/browser/extension_system.h" -#include "extensions/browser/process_manager.h" -#include "extensions/common/one_shot_event.h" - -#if !defined(OS_ANDROID) -#include "chrome/browser/ui/browser_finder.h" -#include "chrome/common/chrome_switches.h" -#endif - -namespace extensions { - -ChromeProcessManagerDelegate::ChromeProcessManagerDelegate() { - registrar_.Add(this, - chrome::NOTIFICATION_BROWSER_WINDOW_READY, - content::NotificationService::AllSources()); - registrar_.Add(this, - chrome::NOTIFICATION_PROFILE_CREATED, - content::NotificationService::AllSources()); - // TODO(jamescook): Move observation of NOTIFICATION_PROFILE_DESTROYED here. - // http://crbug.com/392658 -} - -ChromeProcessManagerDelegate::~ChromeProcessManagerDelegate() { -} - -bool ChromeProcessManagerDelegate::IsBackgroundPageAllowed( - content::BrowserContext* context) const { - Profile* profile = static_cast<Profile*>(context); - - // Disallow if the current session is a Guest mode session but the current - // browser context is *not* off-the-record. Such context is artificial and - // background page shouldn't be created in it. - // http://crbug.com/329498 - return !(profile->IsGuestSession() && !profile->IsOffTheRecord()); -} - -bool ChromeProcessManagerDelegate::DeferCreatingStartupBackgroundHosts( - content::BrowserContext* context) const { - Profile* profile = static_cast<Profile*>(context); - - // The profile may not be valid yet if it is still being initialized. - // In that case, defer loading, since it depends on an initialized profile. - // Background hosts will be loaded later via NOTIFICATION_PROFILE_CREATED. - // http://crbug.com/222473 - if (!g_browser_process->profile_manager()->IsValidProfile(profile)) - return true; - -#if defined(OS_ANDROID) - return false; -#else - // There are no browser windows open and the browser process was - // started to show the app launcher. Background hosts will be loaded later - // via NOTIFICATION_BROWSER_WINDOW_READY. http://crbug.com/178260 - return chrome::GetTotalBrowserCountForProfile(profile) == 0 && - CommandLine::ForCurrentProcess()->HasSwitch(switches::kShowAppList); -#endif -} - -void ChromeProcessManagerDelegate::Observe( - int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) { - switch (type) { - case chrome::NOTIFICATION_BROWSER_WINDOW_READY: { - Browser* browser = content::Source<Browser>(source).ptr(); - OnBrowserWindowReady(browser); - break; - } - case chrome::NOTIFICATION_PROFILE_CREATED: { - Profile* profile = content::Source<Profile>(source).ptr(); - OnProfileCreated(profile); - break; - } - - default: - NOTREACHED(); - } -} - -void ChromeProcessManagerDelegate::OnBrowserWindowReady(Browser* browser) { - Profile* profile = browser->profile(); - DCHECK(profile); - - // If the extension system isn't ready yet the background hosts will be - // created automatically when it is. - ExtensionSystem* system = ExtensionSystem::Get(profile); - if (!system->ready().is_signaled()) - return; - - // Inform the process manager for this profile that the window is ready. - // We continue to observe the notification in case browser windows open for - // a related incognito profile or other regular profiles. - ProcessManager* manager = system->process_manager(); - if (!manager) // Tests may not have a process manager. - return; - DCHECK_EQ(profile, manager->GetBrowserContext()); - manager->MaybeCreateStartupBackgroundHosts(); - - // For incognito profiles also inform the original profile's process manager - // that the window is ready. This will usually be a no-op because the - // original profile's process manager should have been informed when the - // non-incognito window opened. - if (profile->IsOffTheRecord()) { - Profile* original_profile = profile->GetOriginalProfile(); - ProcessManager* original_manager = - ExtensionSystem::Get(original_profile)->process_manager(); - DCHECK(original_manager); - DCHECK_EQ(original_profile, original_manager->GetBrowserContext()); - original_manager->MaybeCreateStartupBackgroundHosts(); - } -} - -void ChromeProcessManagerDelegate::OnProfileCreated(Profile* profile) { - // Incognito profiles are handled by their original profile. - if (profile->IsOffTheRecord()) - return; - - // The profile can be created before the extension system is ready. - ProcessManager* manager = ExtensionSystem::Get(profile)->process_manager(); - if (!manager) - return; - - // The profile might have been initialized asynchronously (in parallel with - // extension system startup). Now that initialization is complete the - // ProcessManager can load deferred background pages. - manager->MaybeCreateStartupBackgroundHosts(); -} - -} // namespace extensions diff --git a/chrome/browser/extensions/chrome_process_manager_delegate.h b/chrome/browser/extensions/chrome_process_manager_delegate.h deleted file mode 100644 index 9bcc58a..0000000 --- a/chrome/browser/extensions/chrome_process_manager_delegate.h +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright 2014 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 CHROME_BROWSER_EXTENSIONS_CHROME_PROCESS_MANAGER_DELEGATE_H_ -#define CHROME_BROWSER_EXTENSIONS_CHROME_PROCESS_MANAGER_DELEGATE_H_ - -#include "base/compiler_specific.h" -#include "base/macros.h" -#include "content/public/browser/notification_observer.h" -#include "content/public/browser/notification_registrar.h" -#include "extensions/browser/process_manager_delegate.h" - -class Browser; -class Profile; - -namespace extensions { - -// Support for ProcessManager. Controls cases where Chrome wishes to disallow -// extension background pages or defer their creation. -class ChromeProcessManagerDelegate : public ProcessManagerDelegate, - public content::NotificationObserver { - public: - ChromeProcessManagerDelegate(); - virtual ~ChromeProcessManagerDelegate(); - - // ProcessManagerDelegate implementation: - virtual bool IsBackgroundPageAllowed( - content::BrowserContext* context) const OVERRIDE; - virtual bool DeferCreatingStartupBackgroundHosts( - content::BrowserContext* context) const OVERRIDE; - - // content::NotificationObserver implementation: - virtual void Observe(int type, - const content::NotificationSource& source, - const content::NotificationDetails& details) OVERRIDE; - - private: - // Notification handlers. - void OnBrowserWindowReady(Browser* browser); - void OnProfileCreated(Profile* profile); - - content::NotificationRegistrar registrar_; - - DISALLOW_COPY_AND_ASSIGN(ChromeProcessManagerDelegate); -}; - -} // namespace extensions - -#endif // CHROME_BROWSER_EXTENSIONS_CHROME_PROCESS_MANAGER_DELEGATE_H_ diff --git a/chrome/chrome_browser_extensions.gypi b/chrome/chrome_browser_extensions.gypi index 0e955f7..194dab0e 100644 --- a/chrome/chrome_browser_extensions.gypi +++ b/chrome/chrome_browser_extensions.gypi @@ -45,8 +45,6 @@ 'browser/extensions/chrome_extensions_browser_client.h', 'browser/extensions/chrome_notification_observer.cc', 'browser/extensions/chrome_notification_observer.h', - 'browser/extensions/chrome_process_manager_delegate.cc', - 'browser/extensions/chrome_process_manager_delegate.h', 'browser/extensions/component_loader.cc', 'browser/extensions/component_loader.h', 'browser/extensions/context_menu_matcher.cc', diff --git a/extensions/DEPS b/extensions/DEPS index 4077a55..0505b02 100644 --- a/extensions/DEPS +++ b/extensions/DEPS @@ -1,5 +1,4 @@ include_rules = [ - # Do not add Chrome dependencies. Much work went into removing them. "+components/url_matcher", "+content/public/common", "+content/public/test", @@ -14,7 +13,7 @@ include_rules = [ # NOTE: Please do not add includes without talking to the app shell team; # see OWNERS for this directory. # - # TODO(jamescook): Remove this. http://crbug.com/392622 + # TODO(jamescook): Remove these. http://crbug.com/162530 "!chrome/browser/chrome_notification_types.h", ] diff --git a/extensions/browser/extensions_browser_client.h b/extensions/browser/extensions_browser_client.h index cb810b6..293b2af 100644 --- a/extensions/browser/extensions_browser_client.h +++ b/extensions/browser/extensions_browser_client.h @@ -41,7 +41,6 @@ class ExtensionPrefsObserver; class ExtensionSystem; class ExtensionSystemProvider; class InfoMap; -class ProcessManagerDelegate; class RuntimeAPIDelegate; // Interface to allow the extensions module to make browser-process-specific @@ -134,9 +133,12 @@ class ExtensionsBrowserClient { content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const = 0; - // Returns the ProcessManagerDelegate shared across all BrowserContexts. May - // return NULL in tests or for simple embedders. - virtual ProcessManagerDelegate* GetProcessManagerDelegate() const = 0; + // Returns true if loading background pages should be deferred. + virtual bool DeferLoadingBackgroundHosts( + content::BrowserContext* context) const = 0; + + virtual bool IsBackgroundPageAllowed( + content::BrowserContext* context) const = 0; // Creates a new ExtensionHostDelegate instance. virtual scoped_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() = 0; diff --git a/extensions/browser/process_manager.cc b/extensions/browser/process_manager.cc index 12afd7c..e6beddb 100644 --- a/extensions/browser/process_manager.cc +++ b/extensions/browser/process_manager.cc @@ -33,7 +33,6 @@ #include "extensions/browser/extension_registry.h" #include "extensions/browser/extension_system.h" #include "extensions/browser/extensions_browser_client.h" -#include "extensions/browser/process_manager_delegate.h" #include "extensions/browser/process_manager_observer.h" #include "extensions/browser/view_type_utils.h" #include "extensions/common/constants.h" @@ -246,6 +245,8 @@ ProcessManager::ProcessManager(BrowserContext* context, content::NotificationService::AllSources()); registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_CONNECTED, content::NotificationService::AllSources()); + registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED, + content::Source<BrowserContext>(original_context)); registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED, content::Source<BrowserContext>(context)); if (context->IsOffTheRecord()) { @@ -306,14 +307,11 @@ bool ProcessManager::CreateBackgroundHost(const Extension* extension, const GURL& url) { // Hosted apps are taken care of from BackgroundContentsService. Ignore them // here. - if (extension->is_hosted_app()) - return false; - - // Don't create hosts if the embedder doesn't allow it. - ProcessManagerDelegate* delegate = - ExtensionsBrowserClient::Get()->GetProcessManagerDelegate(); - if (delegate && !delegate->IsBackgroundPageAllowed(GetBrowserContext())) + if (extension->is_hosted_app() || + !ExtensionsBrowserClient::Get()-> + IsBackgroundPageAllowed(GetBrowserContext())) { return false; + } // Don't create multiple background hosts for an extension. if (GetBackgroundHostForExtension(extension->id())) @@ -622,6 +620,16 @@ void ProcessManager::CancelSuspend(const Extension* extension) { } } +void ProcessManager::OnBrowserWindowReady() { + // If the extension system isn't ready yet the background hosts will be + // created via NOTIFICATION_EXTENSIONS_READY below. + ExtensionSystem* system = ExtensionSystem::Get(GetBrowserContext()); + if (!system->ready().is_signaled()) + return; + + CreateBackgroundHostsForProfileStartup(); +} + content::BrowserContext* ProcessManager::GetBrowserContext() const { return site_instance_->GetBrowserContext(); } @@ -640,10 +648,15 @@ void ProcessManager::Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) { switch (type) { - case chrome::NOTIFICATION_EXTENSIONS_READY: { - // TODO(jamescook): Convert this to use ExtensionSystem::ready() instead - // of a notification. - MaybeCreateStartupBackgroundHosts(); + case chrome::NOTIFICATION_EXTENSIONS_READY: + case chrome::NOTIFICATION_PROFILE_CREATED: { + // Don't load background hosts now if the loading should be deferred. + // Instead they will be loaded when a browser window for this profile + // (or an incognito profile from this profile) is ready. + if (DeferLoadingBackgroundHosts()) + break; + + CreateBackgroundHostsForProfileStartup(); break; } @@ -771,24 +784,24 @@ void ProcessManager::OnDevToolsStateChanged( } } -void ProcessManager::MaybeCreateStartupBackgroundHosts() { - if (startup_background_hosts_created_) - return; - - // The embedder might disallow background pages entirely. - ProcessManagerDelegate* delegate = - ExtensionsBrowserClient::Get()->GetProcessManagerDelegate(); - if (delegate && !delegate->IsBackgroundPageAllowed(GetBrowserContext())) +void ProcessManager::CreateBackgroundHostsForProfileStartup() { + if (startup_background_hosts_created_ || + !ExtensionsBrowserClient::Get()-> + IsBackgroundPageAllowed(GetBrowserContext())) { return; + } - // The embedder might want to defer background page loading. For example, - // Chrome defers background page loading when it is launched to show the app - // list, then triggers a load later when a browser window opens. - if (delegate && - delegate->DeferCreatingStartupBackgroundHosts(GetBrowserContext())) - return; + const ExtensionSet& enabled_extensions = + ExtensionRegistry::Get(GetBrowserContext())->enabled_extensions(); + for (ExtensionSet::const_iterator extension = enabled_extensions.begin(); + extension != enabled_extensions.end(); + ++extension) { + CreateBackgroundHostForExtensionLoad(this, extension->get()); - CreateStartupBackgroundHosts(); + FOR_EACH_OBSERVER(ProcessManagerObserver, + observer_list_, + OnBackgroundHostStartup(*extension)); + } startup_background_hosts_created_ = true; // Background pages should only be loaded once. To prevent any further loads @@ -797,6 +810,14 @@ void ProcessManager::MaybeCreateStartupBackgroundHosts() { ExtensionsBrowserClient::Get()->GetOriginalContext(GetBrowserContext()); if (registrar_.IsRegistered( this, + chrome::NOTIFICATION_PROFILE_CREATED, + content::Source<BrowserContext>(original_context))) { + registrar_.Remove(this, + chrome::NOTIFICATION_PROFILE_CREATED, + content::Source<BrowserContext>(original_context)); + } + if (registrar_.IsRegistered( + this, chrome::NOTIFICATION_EXTENSIONS_READY, content::Source<BrowserContext>(original_context))) { registrar_.Remove(this, @@ -805,21 +826,6 @@ void ProcessManager::MaybeCreateStartupBackgroundHosts() { } } -void ProcessManager::CreateStartupBackgroundHosts() { - DCHECK(!startup_background_hosts_created_); - const ExtensionSet& enabled_extensions = - ExtensionRegistry::Get(GetBrowserContext())->enabled_extensions(); - for (ExtensionSet::const_iterator extension = enabled_extensions.begin(); - extension != enabled_extensions.end(); - ++extension) { - CreateBackgroundHostForExtensionLoad(this, extension->get()); - - FOR_EACH_OBSERVER(ProcessManagerObserver, - observer_list_, - OnBackgroundHostStartup(*extension)); - } -} - void ProcessManager::OnBackgroundHostCreated(ExtensionHost* host) { DCHECK_EQ(GetBrowserContext(), host->browser_context()); background_hosts_.insert(host); @@ -884,6 +890,12 @@ void ProcessManager::ClearBackgroundPageData(const std::string& extension_id) { } } +bool ProcessManager::DeferLoadingBackgroundHosts() const { + // The extensions embedder may have special rules about background hosts. + return ExtensionsBrowserClient::Get()->DeferLoadingBackgroundHosts( + GetBrowserContext()); +} + // // IncognitoProcessManager // @@ -902,6 +914,8 @@ IncognitoProcessManager::IncognitoProcessManager( // in the NOTIFICATION_BROWSER_WINDOW_READY notification handler. registrar_.Remove(this, chrome::NOTIFICATION_EXTENSIONS_READY, content::Source<BrowserContext>(original_context)); + registrar_.Remove(this, chrome::NOTIFICATION_PROFILE_CREATED, + content::Source<BrowserContext>(original_context)); } bool IncognitoProcessManager::CreateBackgroundHost(const Extension* extension, diff --git a/extensions/browser/process_manager.h b/extensions/browser/process_manager.h index 80736e4..e580bbd 100644 --- a/extensions/browser/process_manager.h +++ b/extensions/browser/process_manager.h @@ -33,7 +33,6 @@ namespace extensions { class Extension; class ExtensionHost; -class ProcessManagerDelegate; class ProcessManagerObserver; // Manages dynamic state of running Chromium extensions. There is one instance @@ -123,9 +122,8 @@ class ProcessManager : public content::NotificationObserver { // onSuspendCanceled() event to it. void CancelSuspend(const Extension* extension); - // Creates background hosts if the embedder is ready and they are not already - // loaded. - void MaybeCreateStartupBackgroundHosts(); + // Ensures background hosts are loaded for a new browser window. + void OnBrowserWindowReady(); // Gets the BrowserContext associated with site_instance_ and all other // related SiteInstances. @@ -146,10 +144,6 @@ class ProcessManager : public content::NotificationObserver { content::BrowserContext* original_context, ProcessManager* original_manager); - bool startup_background_hosts_created_for_test() const { - return startup_background_hosts_created_; - } - protected: // If |context| is incognito pass the master context as |original_context|. // Otherwise pass the same context for both. @@ -164,6 +158,10 @@ class ProcessManager : public content::NotificationObserver { const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; + // Load all background pages once the profile data is ready and the pages + // should be loaded. + void CreateBackgroundHostsForProfileStartup(); + content::NotificationRegistrar registrar_; // The set of ExtensionHosts running viewless background extensions. @@ -184,10 +182,6 @@ class ProcessManager : public content::NotificationObserver { typedef std::map<content::RenderViewHost*, extensions::ViewType> ExtensionRenderViews; - // Load all background pages once the profile data is ready and the pages - // should be loaded. - void CreateStartupBackgroundHosts(); - // Called just after |host| is created so it can be registered in our lists. void OnBackgroundHostCreated(ExtensionHost* host); @@ -222,6 +216,9 @@ class ProcessManager : public content::NotificationObserver { // Clears background page data for this extension. void ClearBackgroundPageData(const std::string& extension_id); + // Returns true if loading background pages should be deferred. + bool DeferLoadingBackgroundHosts() const; + void OnDevToolsStateChanged(content::DevToolsAgentHost*, bool attached); // Contains all active extension-related RenderViewHost instances for all diff --git a/extensions/browser/process_manager_delegate.h b/extensions/browser/process_manager_delegate.h deleted file mode 100644 index 7cc48c0..0000000 --- a/extensions/browser/process_manager_delegate.h +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright 2014 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 EXTENSIONS_BROWSER_PROCESS_MANAGER_DELEGATE_H_ -#define EXTENSIONS_BROWSER_PROCESS_MANAGER_DELEGATE_H_ - -namespace content { -class BrowserContext; -}; - -namespace extensions { - -// Customization of ProcessManager for the extension system embedder. -class ProcessManagerDelegate { - public: - virtual ~ProcessManagerDelegate() {} - - // Returns true if the embedder allows background pages for the given - // |context|. - virtual bool IsBackgroundPageAllowed( - content::BrowserContext* context) const = 0; - - // Returns true if the embedder wishes to defer starting up the renderers for - // extension background pages. If the embedder returns true it must call - // ProcessManager::MaybeCreateStartupBackgroundHosts() when it is ready. See - // ChromeProcessManagerDelegate for examples of how this is useful. - virtual bool DeferCreatingStartupBackgroundHosts( - content::BrowserContext* context) const = 0; -}; - -} // namespace extensions - -#endif // EXTENSIONS_BROWSER_PROCESS_MANAGER_DELEGATE_H_ diff --git a/extensions/browser/process_manager_unittest.cc b/extensions/browser/process_manager_unittest.cc index 0d1f86b..b72b112 100644 --- a/extensions/browser/process_manager_unittest.cc +++ b/extensions/browser/process_manager_unittest.cc @@ -5,12 +5,10 @@ #include "extensions/browser/process_manager.h" #include "chrome/browser/chrome_notification_types.h" -#include "components/keyed_service/content/browser_context_dependency_manager.h" #include "content/public/browser/content_browser_client.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/site_instance.h" #include "content/public/test/test_browser_context.h" -#include "extensions/browser/process_manager_delegate.h" #include "extensions/browser/test_extensions_browser_client.h" #include "testing/gtest/include/gtest/gtest.h" @@ -35,35 +33,12 @@ class TestBrowserContextIncognito : public TestBrowserContext { DISALLOW_COPY_AND_ASSIGN(TestBrowserContextIncognito); }; -// A trivial ProcessManagerDelegate. -class TestProcessManagerDelegate : public ProcessManagerDelegate { - public: - TestProcessManagerDelegate() - : is_background_page_allowed_(true), - defer_creating_startup_background_hosts_(false) {} - virtual ~TestProcessManagerDelegate() {} - - // ProcessManagerDelegate implementation. - virtual bool IsBackgroundPageAllowed(BrowserContext* context) const OVERRIDE { - return is_background_page_allowed_; - } - virtual bool DeferCreatingStartupBackgroundHosts( - BrowserContext* context) const OVERRIDE { - return defer_creating_startup_background_hosts_; - } - - bool is_background_page_allowed_; - bool defer_creating_startup_background_hosts_; -}; - } // namespace class ProcessManagerTest : public testing::Test { public: ProcessManagerTest() : extensions_browser_client_(&original_context_) { extensions_browser_client_.SetIncognitoContext(&incognito_context_); - extensions_browser_client_.set_process_manager_delegate( - &process_manager_delegate_); ExtensionsBrowserClient::Set(&extensions_browser_client_); } @@ -74,23 +49,6 @@ class ProcessManagerTest : public testing::Test { BrowserContext* original_context() { return &original_context_; } BrowserContext* incognito_context() { return &incognito_context_; } - // testing::Test implementation. - virtual void SetUp() OVERRIDE { - // Needed for ExtensionRegistry. - BrowserContextDependencyManager::GetInstance() - ->CreateBrowserContextServicesForTest(&original_context_); - BrowserContextDependencyManager::GetInstance() - ->CreateBrowserContextServicesForTest(&incognito_context_); - } - - virtual void TearDown() OVERRIDE { - // Needed to clean up ExtensionRegistry. - BrowserContextDependencyManager::GetInstance() - ->DestroyBrowserContextServices(&incognito_context_); - BrowserContextDependencyManager::GetInstance() - ->DestroyBrowserContextServices(&original_context_); - } - // Returns true if the notification |type| is registered for |manager| with // source |context|. Pass NULL for |context| for all sources. static bool IsRegistered(ProcessManager* manager, @@ -100,10 +58,9 @@ class ProcessManagerTest : public testing::Test { manager, type, content::Source<BrowserContext>(context)); } - protected: + private: TestBrowserContext original_context_; TestBrowserContextIncognito incognito_context_; - TestProcessManagerDelegate process_manager_delegate_; TestExtensionsBrowserClient extensions_browser_client_; DISALLOW_COPY_AND_ASSIGN(ProcessManagerTest); @@ -168,83 +125,6 @@ TEST_F(ProcessManagerTest, ExtensionNotificationRegistration) { incognito_context())); } -// Test that startup background hosts are created when the extension system -// becomes ready. -// -// NOTE: This test and those that follow do not try to create ExtensionsHosts -// because ExtensionHost is tightly coupled to WebContents and can't be -// constructed in unit tests. -TEST_F(ProcessManagerTest, CreateBackgroundHostsOnExtensionsReady) { - scoped_ptr<ProcessManager> manager( - ProcessManager::Create(original_context())); - ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); - - // Simulate the extension system becoming ready. - content::NotificationService::current()->Notify( - chrome::NOTIFICATION_EXTENSIONS_READY, - content::Source<BrowserContext>(original_context()), - content::NotificationService::NoDetails()); - EXPECT_TRUE(manager->startup_background_hosts_created_for_test()); -} - -// Test that startup background hosts can be created explicitly before the -// extension system is ready (this is the normal pattern in Chrome). -TEST_F(ProcessManagerTest, CreateBackgroundHostsExplicitly) { - scoped_ptr<ProcessManager> manager( - ProcessManager::Create(original_context())); - ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); - - // Embedder explicitly asks for hosts to be created. Chrome does this on - // normal startup. - manager->MaybeCreateStartupBackgroundHosts(); - EXPECT_TRUE(manager->startup_background_hosts_created_for_test()); -} - -// Test that the embedder can defer background host creation. Chrome does this -// when the profile is created asynchronously, which may take a while. -TEST_F(ProcessManagerTest, CreateBackgroundHostsDeferred) { - scoped_ptr<ProcessManager> manager( - ProcessManager::Create(original_context())); - ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); - - // Don't create background hosts if the delegate says to defer them. - process_manager_delegate_.defer_creating_startup_background_hosts_ = true; - manager->MaybeCreateStartupBackgroundHosts(); - EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); - - // The extension system becoming ready still doesn't create the hosts. - content::NotificationService::current()->Notify( - chrome::NOTIFICATION_EXTENSIONS_READY, - content::Source<BrowserContext>(original_context()), - content::NotificationService::NoDetails()); - EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); - - // Once the embedder is ready the background hosts can be created. - process_manager_delegate_.defer_creating_startup_background_hosts_ = false; - manager->MaybeCreateStartupBackgroundHosts(); - EXPECT_TRUE(manager->startup_background_hosts_created_for_test()); -} - -// Test that the embedder can disallow background host creation. -// Chrome OS does this in guest mode. -TEST_F(ProcessManagerTest, IsBackgroundHostAllowed) { - scoped_ptr<ProcessManager> manager( - ProcessManager::Create(original_context())); - ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); - - // Don't create background hosts if the delegate disallows them. - process_manager_delegate_.is_background_page_allowed_ = false; - manager->MaybeCreateStartupBackgroundHosts(); - EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); - - // The extension system becoming ready still doesn't create the hosts. - content::NotificationService::current()->Notify( - chrome::NOTIFICATION_EXTENSIONS_READY, - content::Source<BrowserContext>(original_context()), - content::NotificationService::NoDetails()); - EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); -} - // Test that extensions get grouped in the right SiteInstance (and therefore // process) based on their URLs. TEST_F(ProcessManagerTest, ProcessGrouping) { diff --git a/extensions/browser/test_extensions_browser_client.cc b/extensions/browser/test_extensions_browser_client.cc index a01b128..dcd467f 100644 --- a/extensions/browser/test_extensions_browser_client.cc +++ b/extensions/browser/test_extensions_browser_client.cc @@ -15,9 +15,7 @@ namespace extensions { TestExtensionsBrowserClient::TestExtensionsBrowserClient( BrowserContext* main_context) - : main_context_(main_context), - incognito_context_(NULL), - process_manager_delegate_(NULL) { + : main_context_(main_context), incognito_context_(NULL) { DCHECK(main_context_); DCHECK(!main_context_->IsOffTheRecord()); } @@ -118,9 +116,14 @@ void TestExtensionsBrowserClient::GetEarlyExtensionPrefsObservers( content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const {} -ProcessManagerDelegate* TestExtensionsBrowserClient::GetProcessManagerDelegate() - const { - return process_manager_delegate_; +bool TestExtensionsBrowserClient::DeferLoadingBackgroundHosts( + BrowserContext* context) const { + return false; +} + +bool TestExtensionsBrowserClient::IsBackgroundPageAllowed( + BrowserContext* context) const { + return true; } scoped_ptr<ExtensionHostDelegate> diff --git a/extensions/browser/test_extensions_browser_client.h b/extensions/browser/test_extensions_browser_client.h index 1cfffee..b793259 100644 --- a/extensions/browser/test_extensions_browser_client.h +++ b/extensions/browser/test_extensions_browser_client.h @@ -15,14 +15,10 @@ namespace extensions { // this class should call ExtensionsBrowserClient::Set() with its instance. class TestExtensionsBrowserClient : public ExtensionsBrowserClient { public: - // |main_context| is required and must not be an incognito context. + // |context| is required and must not be an incognito context. explicit TestExtensionsBrowserClient(content::BrowserContext* main_context); virtual ~TestExtensionsBrowserClient(); - void set_process_manager_delegate(ProcessManagerDelegate* delegate) { - process_manager_delegate_ = delegate; - } - // Associates an incognito context with |main_context_|. void SetIncognitoContext(content::BrowserContext* incognito_context); @@ -63,7 +59,10 @@ class TestExtensionsBrowserClient : public ExtensionsBrowserClient { virtual void GetEarlyExtensionPrefsObservers( content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const OVERRIDE; - virtual ProcessManagerDelegate* GetProcessManagerDelegate() const OVERRIDE; + virtual bool DeferLoadingBackgroundHosts( + content::BrowserContext* context) const OVERRIDE; + virtual bool IsBackgroundPageAllowed(content::BrowserContext* context) const + OVERRIDE; virtual scoped_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() OVERRIDE; virtual bool DidVersionUpdate(content::BrowserContext* context) OVERRIDE; @@ -83,9 +82,6 @@ class TestExtensionsBrowserClient : public ExtensionsBrowserClient { content::BrowserContext* main_context_; // Not owned. content::BrowserContext* incognito_context_; // Not owned, defaults to NULL. - // Not owned, defaults to NULL. - ProcessManagerDelegate* process_manager_delegate_; - DISALLOW_COPY_AND_ASSIGN(TestExtensionsBrowserClient); }; diff --git a/extensions/extensions.gyp b/extensions/extensions.gyp index 3501961..4349183 100644 --- a/extensions/extensions.gyp +++ b/extensions/extensions.gyp @@ -435,7 +435,6 @@ 'browser/pref_names.h', 'browser/process_manager.cc', 'browser/process_manager.h', - 'browser/process_manager_delegate.h', 'browser/process_manager_observer.h', 'browser/process_map.cc', 'browser/process_map.h', |