diff options
author | danno@chromium.org <danno@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-09-29 12:24:28 +0000 |
---|---|---|
committer | danno@chromium.org <danno@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-09-29 12:24:28 +0000 |
commit | 2fb7dc983456e980d631501f4a120eb091d197e7 (patch) | |
tree | fa96aef59bf1900f56ae1b0457c4f9d36e0fb38e /chrome | |
parent | 0ce04e5a148136f3849e8a8b0dd351a0fc4798b5 (diff) | |
download | chromium_src-2fb7dc983456e980d631501f4a120eb091d197e7.zip chromium_src-2fb7dc983456e980d631501f4a120eb091d197e7.tar.gz chromium_src-2fb7dc983456e980d631501f4a120eb091d197e7.tar.bz2 |
Use PrefChangeRegistrar everywhere
BUG=54955
TEST=PrefChangeRegistrarTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=60169
Review URL: http://codereview.chromium.org/3304015
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60935 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
53 files changed, 368 insertions, 246 deletions
diff --git a/chrome/browser/background_mode_manager.cc b/chrome/browser/background_mode_manager.cc index 01a9a36..b0110a1 100644 --- a/chrome/browser/background_mode_manager.cc +++ b/chrome/browser/background_mode_manager.cc @@ -163,7 +163,8 @@ BackgroundModeManager::BackgroundModeManager(Profile* profile, NotificationService::AllSources()); // Listen for changes to the background mode preference. - profile_->GetPrefs()->AddPrefObserver(prefs::kBackgroundModeEnabled, this); + pref_registrar_.Init(profile_->GetPrefs()); + pref_registrar_.Add(prefs::kBackgroundModeEnabled, this); } BackgroundModeManager::~BackgroundModeManager() { @@ -172,10 +173,6 @@ BackgroundModeManager::~BackgroundModeManager() { // because in an actual running system we'd get an APP_TERMINATING // notification before being destroyed. EndBackgroundMode(); - // Manually remove our pref observer so we don't get notified for prefs - // changes (have to do it manually because we can't use the registrar for - // prefs notifications). - profile_->GetPrefs()->RemovePrefObserver(prefs::kBackgroundModeEnabled, this); } bool BackgroundModeManager::IsBackgroundModeEnabled() { diff --git a/chrome/browser/background_mode_manager.h b/chrome/browser/background_mode_manager.h index 46e8847..52f37da 100644 --- a/chrome/browser/background_mode_manager.h +++ b/chrome/browser/background_mode_manager.h @@ -8,6 +8,7 @@ #include "app/menus/simple_menu_model.h" #include "base/gtest_prod_util.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/status_icons/status_icon.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" @@ -142,6 +143,9 @@ class BackgroundModeManager // Reference to our status icon (if any) - owned by the StatusTray. StatusIcon* status_icon_; + // Ensure observed preferences are properly cleaned up. + PrefChangeRegistrar pref_registrar_; + DISALLOW_COPY_AND_ASSIGN(BackgroundModeManager); }; diff --git a/chrome/browser/cocoa/content_settings_dialog_controller.h b/chrome/browser/cocoa/content_settings_dialog_controller.h index d48b962..8fe92da 100644 --- a/chrome/browser/cocoa/content_settings_dialog_controller.h +++ b/chrome/browser/cocoa/content_settings_dialog_controller.h @@ -7,6 +7,7 @@ #import "base/cocoa_protocols_mac.h" #include "base/scoped_ptr.h" #include "chrome/common/content_settings_types.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_member.h" // Index of the "enabled" and "disabled" radio group settings in all tabs except @@ -50,6 +51,7 @@ class Profile; Profile* profile_; // weak IntegerPrefMember lastSelectedTab_; BooleanPrefMember clearSiteDataOnExit_; + PrefChangeRegistrar registrar_; scoped_ptr<ContentSettingsDialogControllerInternal::PrefObserverBridge> observer_; // Watches for pref changes. } diff --git a/chrome/browser/cocoa/content_settings_dialog_controller.mm b/chrome/browser/cocoa/content_settings_dialog_controller.mm index 9ed2078..001a3ec 100644 --- a/chrome/browser/cocoa/content_settings_dialog_controller.mm +++ b/chrome/browser/cocoa/content_settings_dialog_controller.mm @@ -140,11 +140,11 @@ class PrefObserverDisabler { // Manually observe notifications for preferences that are grouped in // the HostContentSettingsMap or GeolocationContentSettingsMap. PrefService* prefs = profile_->GetPrefs(); - prefs->AddPrefObserver(prefs::kBlockThirdPartyCookies, observer_.get()); - prefs->AddPrefObserver(prefs::kBlockNonsandboxedPlugins, observer_.get()); - prefs->AddPrefObserver(prefs::kDefaultContentSettings, observer_.get()); - prefs->AddPrefObserver(prefs::kGeolocationDefaultContentSetting, - observer_.get()); + registrar_.Init(prefs); + registrar_.Add(prefs::kBlockThirdPartyCookies, observer_.get()); + registrar_.Add(prefs::kBlockNonsandboxedPlugins, observer_.get()); + registrar_.Add(prefs::kDefaultContentSettings, observer_.get()); + registrar_.Add(prefs::kGeolocationDefaultContentSetting, observer_.get()); // We don't need to observe changes in this value. lastSelectedTab_.Init(prefs::kContentSettingsWindowLastTabIndex, @@ -153,20 +153,6 @@ class PrefObserverDisabler { return self; } -- (void)dealloc { - if (profile_) { - PrefService* prefs = profile_->GetPrefs(); - prefs->RemovePrefObserver(prefs::kBlockThirdPartyCookies, observer_.get()); - prefs->RemovePrefObserver(prefs::kBlockNonsandboxedPlugins, - observer_.get()); - prefs->RemovePrefObserver(prefs::kDefaultContentSettings, observer_.get()); - prefs->RemovePrefObserver(prefs::kGeolocationDefaultContentSetting, - observer_.get()); - } - - [super dealloc]; -} - - (void)closeExceptionsSheet { NSWindow* attachedSheet = [[self window] attachedSheet]; if (attachedSheet) { diff --git a/chrome/browser/cocoa/extensions/extension_action_context_menu.mm b/chrome/browser/cocoa/extensions/extension_action_context_menu.mm index 10ea079..81aa61e 100644 --- a/chrome/browser/cocoa/extensions/extension_action_context_menu.mm +++ b/chrome/browser/cocoa/extensions/extension_action_context_menu.mm @@ -18,6 +18,7 @@ #include "chrome/browser/extensions/extension_install_ui.h" #include "chrome/browser/extensions/extensions_service.h" #include "chrome/browser/extensions/extension_tabs_module.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/profile.h" #include "chrome/common/extensions/extension.h" @@ -71,14 +72,10 @@ class DevmodeObserver : public NotificationObserver { DevmodeObserver(ExtensionActionContextMenu* menu, PrefService* service) : menu_(menu), pref_service_(service) { - pref_service_->AddPrefObserver(prefs::kExtensionsUIDeveloperMode, - this); - } - - ~DevmodeObserver() { - pref_service_->RemovePrefObserver(prefs::kExtensionsUIDeveloperMode, - this); + registrar_.Init(pref_service_); + registrar_.Add(prefs::kExtensionsUIDeveloperMode, this); } + virtual ~DevmodeObserver() {} void Observe(NotificationType type, const NotificationSource& source, @@ -92,6 +89,7 @@ class DevmodeObserver : public NotificationObserver { private: ExtensionActionContextMenu* menu_; PrefService* pref_service_; + PrefChangeRegistrar registrar_; }; } // namespace extension_action_context_menu diff --git a/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm b/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm index 110f719..ba7e2b7 100644 --- a/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm +++ b/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm @@ -27,8 +27,7 @@ class ExtensionTestingProfile : public TestingProfile { manager_.reset(ExtensionProcessManager::Create(this)); service_ = new ExtensionsService(this, CommandLine::ForCurrentProcess(), - GetPrefs(), - GetExtensionsInstallDir(), + GetExtensionsInstallDir(), false); service_->set_extensions_enabled(true); service_->set_show_extensions_prompts(false); diff --git a/chrome/browser/cocoa/preferences_window_controller.h b/chrome/browser/cocoa/preferences_window_controller.h index e7b8383..54d4778 100644 --- a/chrome/browser/cocoa/preferences_window_controller.h +++ b/chrome/browser/cocoa/preferences_window_controller.h @@ -9,6 +9,7 @@ #include "chrome/browser/options_window.h" #include "chrome/browser/prefs/pref_member.h" #include "chrome/browser/prefs/pref_set_observer.h" +#include "chrome/browser/prefs/pref_change_registrar.h" namespace PreferencesWindowControllerInternal { class PrefObserverBridge; @@ -45,6 +46,7 @@ class ProfileSyncService; ProfileSyncService* syncService_; scoped_ptr<PreferencesWindowControllerInternal::PrefObserverBridge> observer_; // Watches for pref changes. + PrefChangeRegistrar registrar_; // Manages pref change observer registration. scoped_nsobject<WindowSizeAutosaver> sizeSaver_; NSView* currentPrefsView_; // weak ref - current prefs page view. scoped_ptr<PreferencesWindowControllerInternal::ManagedPrefsBannerState> diff --git a/chrome/browser/cocoa/preferences_window_controller.mm b/chrome/browser/cocoa/preferences_window_controller.mm index c7c2b7b..0f11f63 100644 --- a/chrome/browser/cocoa/preferences_window_controller.mm +++ b/chrome/browser/cocoa/preferences_window_controller.mm @@ -330,7 +330,6 @@ CGFloat AutoSizeUnderTheHoodContent(NSView* view, // Record the user performed a certain action and save the preferences. - (void)recordUserAction:(const UserMetricsAction&) action; - (void)registerPrefObservers; -- (void)unregisterPrefObservers; // KVC setter methods. - (void)setNewTabPageIsHomePageIndex:(NSInteger)val; @@ -761,7 +760,6 @@ class ManagedPrefsBannerState : public policy::ManagedPrefsBannerBase { syncService_->RemoveObserver(observer_.get()); } [[NSNotificationCenter defaultCenter] removeObserver:self]; - [self unregisterPrefObservers]; [animation_ setDelegate:nil]; [animation_ stopAnimation]; [super dealloc]; @@ -781,7 +779,8 @@ class ManagedPrefsBannerState : public policy::ManagedPrefsBannerBase { if (!prefs_) return; // Basics panel - prefs_->AddPrefObserver(prefs::kURLsToRestoreOnStartup, observer_.get()); + registrar_.Init(prefs_); + registrar_.Add(prefs::kURLsToRestoreOnStartup, observer_.get()); restoreOnStartup_.Init(prefs::kRestoreOnStartup, prefs_, observer_.get()); newTabPageIsHomePage_.Init(prefs::kHomePageIsNewTabPage, prefs_, observer_.get()); @@ -823,17 +822,6 @@ class ManagedPrefsBannerState : public policy::ManagedPrefsBannerBase { lastSelectedPage_.Init(prefs::kOptionsWindowLastTabIndex, local, NULL); } -// Clean up what was registered in -registerPrefObservers. We only have to -// clean up the non-PrefMember registrations. -- (void)unregisterPrefObservers { - if (!prefs_) return; - - // Basics - prefs_->RemovePrefObserver(prefs::kURLsToRestoreOnStartup, observer_.get()); - - // Nothing to do for other panels... -} - // Called when the window wants to be closed. - (BOOL)windowShouldClose:(id)sender { // Stop any animation and clear the delegate to avoid stale pointers. diff --git a/chrome/browser/dom_ui/core_options_handler.cc b/chrome/browser/dom_ui/core_options_handler.cc index 2739ebc..849e768b 100644 --- a/chrome/browser/dom_ui/core_options_handler.cc +++ b/chrome/browser/dom_ui/core_options_handler.cc @@ -88,6 +88,13 @@ void CoreOptionsHandler::Uninitialize() { } } +DOMMessageHandler* CoreOptionsHandler::Attach(DOMUI* dom_ui) { + DOMMessageHandler* result = DOMMessageHandler::Attach(dom_ui); + DCHECK(dom_ui_); + registrar_.Init(dom_ui_->GetProfile()->GetPrefs()); + return result; +} + void CoreOptionsHandler::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { @@ -138,9 +145,7 @@ Value* CoreOptionsHandler::FetchPref(const std::string& pref_name) { } void CoreOptionsHandler::ObservePref(const std::string& pref_name) { - DCHECK(dom_ui_); - PrefService* pref_service = dom_ui_->GetProfile()->GetPrefs(); - pref_service->AddPrefObserver(pref_name.c_str(), this); + registrar_.Add(pref_name.c_str(), this); } void CoreOptionsHandler::SetPref(const std::string& pref_name, @@ -185,9 +190,7 @@ void CoreOptionsHandler::ProcessUserMetric(Value::ValueType pref_type, } void CoreOptionsHandler::StopObservingPref(const std::string& path) { - DCHECK(dom_ui_); - PrefService* pref_service = dom_ui_->GetProfile()->GetPrefs(); - pref_service->RemovePrefObserver(path.c_str(), this); + registrar_.Remove(path.c_str(), this); } void CoreOptionsHandler::HandleFetchPrefs(const ListValue* args) { diff --git a/chrome/browser/dom_ui/core_options_handler.h b/chrome/browser/dom_ui/core_options_handler.h index 748ab83..fd94d10 100644 --- a/chrome/browser/dom_ui/core_options_handler.h +++ b/chrome/browser/dom_ui/core_options_handler.h @@ -11,6 +11,7 @@ #include "base/values.h" #include "chrome/browser/dom_ui/options_ui.h" +#include "chrome/browser/prefs/pref_change_registrar.h" // Core options UI handler. // Handles resource and JS calls common to all options sub-pages. @@ -22,7 +23,6 @@ class CoreOptionsHandler : public OptionsPageUIHandler { virtual void GetLocalizedValues(DictionaryValue* localized_strings); virtual void Uninitialize(); - // NotificationObserver implementation. virtual void Observe(NotificationType type, const NotificationSource& source, @@ -30,6 +30,7 @@ class CoreOptionsHandler : public OptionsPageUIHandler { // DOMMessageHandler implementation. virtual void RegisterMessages(); + virtual DOMMessageHandler* Attach(DOMUI* dom_ui); protected: // Fetches a pref value of given |pref_name|. @@ -87,6 +88,8 @@ class CoreOptionsHandler : public OptionsPageUIHandler { void NotifyPrefChanged(const std::string* pref_name); + PrefChangeRegistrar registrar_; + DISALLOW_COPY_AND_ASSIGN(CoreOptionsHandler); }; diff --git a/chrome/browser/dom_ui/ntp_resource_cache.cc b/chrome/browser/dom_ui/ntp_resource_cache.cc index 600710f..bd7ff37 100644 --- a/chrome/browser/dom_ui/ntp_resource_cache.cc +++ b/chrome/browser/dom_ui/ntp_resource_cache.cc @@ -133,15 +133,9 @@ NTPResourceCache::NTPResourceCache(Profile* profile) : profile_(profile) { NotificationService::AllSources()); // Watch for pref changes that cause us to need to invalidate the HTML cache. - PrefService* pref_service = profile_->GetPrefs(); - pref_service->AddPrefObserver(prefs::kShowBookmarkBar, this); - pref_service->AddPrefObserver(prefs::kNTPShownSections, this); -} - -NTPResourceCache::~NTPResourceCache() { - PrefService* pref_service = profile_->GetPrefs(); - pref_service->RemovePrefObserver(prefs::kShowBookmarkBar, this); - pref_service->RemovePrefObserver(prefs::kNTPShownSections, this); + pref_change_registrar_.Init(profile_->GetPrefs()); + pref_change_registrar_.Add(prefs::kShowBookmarkBar, this); + pref_change_registrar_.Add(prefs::kNTPShownSections, this); } RefCountedBytes* NTPResourceCache::GetNewTabHTML(bool is_off_the_record) { diff --git a/chrome/browser/dom_ui/ntp_resource_cache.h b/chrome/browser/dom_ui/ntp_resource_cache.h index f669325..3cdc3e5 100644 --- a/chrome/browser/dom_ui/ntp_resource_cache.h +++ b/chrome/browser/dom_ui/ntp_resource_cache.h @@ -10,6 +10,7 @@ #include "base/ref_counted.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" +#include "chrome/browser/prefs/pref_change_registrar.h" class Profile; class RefCountedBytes; @@ -19,7 +20,7 @@ class RefCountedBytes; class NTPResourceCache : public NotificationObserver { public: explicit NTPResourceCache(Profile* profile); - virtual ~NTPResourceCache(); + virtual ~NTPResourceCache() {} RefCountedBytes* GetNewTabHTML(bool is_off_the_record); RefCountedBytes* GetNewTabCSS(bool is_off_the_record); @@ -43,6 +44,7 @@ class NTPResourceCache : public NotificationObserver { scoped_refptr<RefCountedBytes> new_tab_css_; NotificationRegistrar registrar_; + PrefChangeRegistrar pref_change_registrar_; DISALLOW_COPY_AND_ASSIGN(NTPResourceCache); }; diff --git a/chrome/browser/dom_ui/shown_sections_handler.cc b/chrome/browser/dom_ui/shown_sections_handler.cc index 78ff716..e32511d 100644 --- a/chrome/browser/dom_ui/shown_sections_handler.cc +++ b/chrome/browser/dom_ui/shown_sections_handler.cc @@ -48,11 +48,8 @@ int ShownSectionsHandler::GetShownSections(PrefService* prefs) { ShownSectionsHandler::ShownSectionsHandler(PrefService* pref_service) : pref_service_(pref_service) { - pref_service_->AddPrefObserver(prefs::kNTPShownSections, this); -} - -ShownSectionsHandler::~ShownSectionsHandler() { - pref_service_->RemovePrefObserver(prefs::kNTPShownSections, this); + registrar_.Init(pref_service); + registrar_.Add(prefs::kNTPShownSections, this); } void ShownSectionsHandler::RegisterMessages() { diff --git a/chrome/browser/dom_ui/shown_sections_handler.h b/chrome/browser/dom_ui/shown_sections_handler.h index 14a3215..a281346 100644 --- a/chrome/browser/dom_ui/shown_sections_handler.h +++ b/chrome/browser/dom_ui/shown_sections_handler.h @@ -8,6 +8,7 @@ #include "chrome/browser/dom_ui/dom_ui.h" #include "chrome/common/notification_observer.h" +#include "chrome/browser/prefs/pref_change_registrar.h" class DOMUI; class Value; @@ -25,7 +26,7 @@ class ShownSectionsHandler : public DOMMessageHandler, public NotificationObserver { public: explicit ShownSectionsHandler(PrefService* pref_service); - virtual ~ShownSectionsHandler(); + virtual ~ShownSectionsHandler() {} // Helper to get the current shown sections. static int GetShownSections(PrefService* pref_service); @@ -52,6 +53,7 @@ class ShownSectionsHandler : public DOMMessageHandler, private: PrefService* pref_service_; + PrefChangeRegistrar registrar_; DISALLOW_COPY_AND_ASSIGN(ShownSectionsHandler); }; diff --git a/chrome/browser/extensions/extension_toolbar_model.cc b/chrome/browser/extensions/extension_toolbar_model.cc index 325f0c4..100c7aa 100644 --- a/chrome/browser/extensions/extension_toolbar_model.cc +++ b/chrome/browser/extensions/extension_toolbar_model.cc @@ -33,6 +33,10 @@ ExtensionToolbarModel::ExtensionToolbarModel(ExtensionsService* service) ExtensionToolbarModel::~ExtensionToolbarModel() { } +void ExtensionToolbarModel::DestroyingProfile() { + registrar_.RemoveAll(); +} + void ExtensionToolbarModel::AddObserver(Observer* observer) { observers_.AddObserver(observer); } diff --git a/chrome/browser/extensions/extension_toolbar_model.h b/chrome/browser/extensions/extension_toolbar_model.h index 028c8b5..a38fc8c 100644 --- a/chrome/browser/extensions/extension_toolbar_model.h +++ b/chrome/browser/extensions/extension_toolbar_model.h @@ -20,6 +20,10 @@ class ExtensionToolbarModel : public NotificationObserver { explicit ExtensionToolbarModel(ExtensionsService* service); ~ExtensionToolbarModel(); + // Notifies the toolbar model that the Profile that suplied its + // prefs is being destroyed. + void DestroyingProfile(); + // A class which is informed of changes to the model; represents the view of // MVC. class Observer { diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc index e2e6342..017890f 100644 --- a/chrome/browser/extensions/extensions_service.cc +++ b/chrome/browser/extensions/extensions_service.cc @@ -528,11 +528,11 @@ bool ExtensionsService::IsDownloadFromMiniGallery(const GURL& download_url) { ExtensionsService::ExtensionsService(Profile* profile, const CommandLine* command_line, - PrefService* prefs, const FilePath& install_directory, bool autoupdate_enabled) : profile_(profile), - extension_prefs_(new ExtensionPrefs(prefs, install_directory)), + extension_prefs_(new ExtensionPrefs(profile->GetPrefs(), + install_directory)), install_directory_(install_directory), extensions_enabled_(true), show_extensions_prompts_(true), @@ -547,8 +547,9 @@ ExtensionsService::ExtensionsService(Profile* profile, registrar_.Add(this, NotificationType::EXTENSION_PROCESS_TERMINATED, NotificationService::AllSources()); - prefs->AddPrefObserver(prefs::kExtensionInstallAllowList, this); - prefs->AddPrefObserver(prefs::kExtensionInstallDenyList, this); + pref_change_registrar_.Init(profile->GetPrefs()); + pref_change_registrar_.Add(prefs::kExtensionInstallAllowList, this); + pref_change_registrar_.Add(prefs::kExtensionInstallDenyList, this); // Set up the ExtensionUpdater if (autoupdate_enabled) { @@ -558,7 +559,9 @@ ExtensionsService::ExtensionsService(Profile* profile, switches::kExtensionsUpdateFrequency), &update_frequency); } - updater_ = new ExtensionUpdater(this, prefs, update_frequency); + updater_ = new ExtensionUpdater(this, + profile->GetPrefs(), + update_frequency); } backend_ = new ExtensionsServiceBackend(install_directory_, @@ -572,6 +575,7 @@ ExtensionsService::ExtensionsService(Profile* profile, } ExtensionsService::~ExtensionsService() { + DCHECK(!profile_); // Profile should have told us it's going away. UnloadAllExtensions(); if (updater_.get()) { updater_->Stop(); @@ -1186,12 +1190,9 @@ void ExtensionsService::UpdateExtensionBlacklist( } void ExtensionsService::DestroyingProfile() { - profile_->GetPrefs()->RemovePrefObserver( - prefs::kExtensionInstallAllowList, this); - profile_->GetPrefs()->RemovePrefObserver( - prefs::kExtensionInstallDenyList, this); - + pref_change_registrar_.RemoveAll(); profile_ = NULL; + toolbar_model_.DestroyingProfile(); } void ExtensionsService::CheckAdminBlacklist() { diff --git a/chrome/browser/extensions/extensions_service.h b/chrome/browser/extensions/extensions_service.h index cc9bb769..7d5ad97 100644 --- a/chrome/browser/extensions/extensions_service.h +++ b/chrome/browser/extensions/extensions_service.h @@ -28,6 +28,7 @@ #include "chrome/browser/extensions/extensions_quota_service.h" #include "chrome/browser/extensions/external_extension_provider.h" #include "chrome/browser/extensions/sandboxed_extension_unpacker.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" #include "chrome/common/extensions/extension.h" @@ -140,7 +141,6 @@ class ExtensionsService ExtensionsService(Profile* profile, const CommandLine* command_line, - PrefService* prefs, const FilePath& install_directory, bool autoupdate_enabled); @@ -486,6 +486,7 @@ class ExtensionsService OrphanedDevTools orphaned_dev_tools_; NotificationRegistrar registrar_; + PrefChangeRegistrar pref_change_registrar_; // Keeps track of menu items added by extensions. ExtensionMenuManager menu_manager_; diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc index a434b8f..7781945 100644 --- a/chrome/browser/extensions/extensions_service_unittest.cc +++ b/chrome/browser/extensions/extensions_service_unittest.cc @@ -287,23 +287,22 @@ void ExtensionsServiceTestBase::InitializeExtensionsService( ExtensionTestingProfile* profile = new ExtensionTestingProfile(); // Create a preference service that only contains user defined // preference values. - prefs_.reset(PrefService::CreateUserPrefService(pref_file)); + PrefService* prefs = PrefService::CreateUserPrefService(pref_file); + Profile::RegisterUserPrefs(prefs); + browser::RegisterUserPrefs(prefs); + profile->SetPrefService(prefs); - Profile::RegisterUserPrefs(prefs_.get()); - browser::RegisterUserPrefs(prefs_.get()); profile_.reset(profile); // TODO(scherkus): Remove this when we no longer need to have Talk // component extension state as a preference http://crbug.com/56429 DictionaryValue* dict = - prefs_->GetMutableDictionary("extensions.settings"); + profile->GetPrefs()->GetMutableDictionary("extensions.settings"); dict->Remove("ggnioahjipcehijkhpdjekioddnjoben", NULL); - service_ = new ExtensionsService(profile_.get(), - CommandLine::ForCurrentProcess(), - prefs_.get(), - extensions_install_dir, - false); + service_ = profile->CreateExtensionsService( + CommandLine::ForCurrentProcess(), + extensions_install_dir); service_->set_extensions_enabled(true); service_->set_show_extensions_prompts(false); profile->set_extensions_service(service_.get()); @@ -524,7 +523,7 @@ class ExtensionsServiceTest void ValidatePrefKeyCount(size_t count) { DictionaryValue* dict = - prefs_->GetMutableDictionary("extensions.settings"); + profile_->GetPrefs()->GetMutableDictionary("extensions.settings"); ASSERT_TRUE(dict != NULL); EXPECT_EQ(count, dict->size()); } @@ -539,7 +538,9 @@ class ExtensionsServiceTest msg += " == "; msg += expected_val ? "true" : "false"; - const DictionaryValue* dict = prefs_->GetDictionary("extensions.settings"); + PrefService* prefs = profile_->GetPrefs(); + const DictionaryValue* dict = + prefs->GetDictionary("extensions.settings"); ASSERT_TRUE(dict != NULL) << msg; DictionaryValue* pref = NULL; ASSERT_TRUE(dict->GetDictionary(extension_id, &pref)) << msg; @@ -551,7 +552,8 @@ class ExtensionsServiceTest bool IsPrefExist(const std::string& extension_id, const std::string& pref_path) { - const DictionaryValue* dict = prefs_->GetDictionary("extensions.settings"); + const DictionaryValue* dict = + profile_->GetPrefs()->GetDictionary("extensions.settings"); if (dict == NULL) return false; DictionaryValue* pref = NULL; if (!dict->GetDictionary(extension_id, &pref)) { @@ -577,7 +579,9 @@ class ExtensionsServiceTest msg += " == "; msg += base::IntToString(expected_val); - const DictionaryValue* dict = prefs_->GetDictionary("extensions.settings"); + PrefService* prefs = profile_->GetPrefs(); + const DictionaryValue* dict = + prefs->GetDictionary("extensions.settings"); ASSERT_TRUE(dict != NULL) << msg; DictionaryValue* pref = NULL; ASSERT_TRUE(dict->GetDictionary(extension_id, &pref)) << msg; @@ -597,7 +601,8 @@ class ExtensionsServiceTest msg += " == "; msg += expected_val; - const DictionaryValue* dict = prefs_->GetDictionary("extensions.settings"); + const DictionaryValue* dict = + profile_->GetPrefs()->GetDictionary("extensions.settings"); ASSERT_TRUE(dict != NULL) << msg; DictionaryValue* pref = NULL; std::string manifest_path = extension_id + ".manifest"; @@ -619,7 +624,7 @@ class ExtensionsServiceTest msg += base::IntToString(value); const DictionaryValue* dict = - prefs_->GetMutableDictionary("extensions.settings"); + profile_->GetPrefs()->GetMutableDictionary("extensions.settings"); ASSERT_TRUE(dict != NULL) << msg; DictionaryValue* pref = NULL; ASSERT_TRUE(dict->GetDictionary(extension_id, &pref)) << msg; @@ -845,7 +850,8 @@ TEST_F(ExtensionsServiceTest, CleanupOnStartup) { InitializeInstalledExtensionsService(pref_path, source_install_dir); // Simulate that one of them got partially deleted by clearing its pref. - DictionaryValue* dict = prefs_->GetMutableDictionary("extensions.settings"); + DictionaryValue* dict = + profile_->GetPrefs()->GetMutableDictionary("extensions.settings"); ASSERT_TRUE(dict != NULL); dict->Remove("behllobkkfkfnphdnhnkndlbkcpglgmj", NULL); @@ -1868,8 +1874,10 @@ TEST_F(ExtensionsServiceTest, WillNotLoadPluginExtensionsFromDirectory) { TEST_F(ExtensionsServiceTest, BlacklistedByPolicyWillNotInstall) { InitializeEmptyExtensionsService(); - ListValue* whitelist = prefs_->GetMutableList("extensions.install.allowlist"); - ListValue* blacklist = prefs_->GetMutableList("extensions.install.denylist"); + ListValue* whitelist = + profile_->GetPrefs()->GetMutableList("extensions.install.allowlist"); + ListValue* blacklist = + profile_->GetPrefs()->GetMutableList("extensions.install.denylist"); ASSERT_TRUE(whitelist != NULL && blacklist != NULL); // Blacklist everything. @@ -1906,16 +1914,18 @@ TEST_F(ExtensionsServiceTest, BlacklistedByPolicyRemovedIfRunning) { loop_.RunAllPending(); EXPECT_EQ(1u, service_->extensions()->size()); - ListValue* blacklist = prefs_->GetMutableList("extensions.install.denylist"); + PrefService* prefs = profile_->GetPrefs(); + ListValue* blacklist = + prefs->GetMutableList("extensions.install.denylist"); ASSERT_TRUE(blacklist != NULL); // Blacklist this extension. blacklist->Append(Value::CreateStringValue(good_crx)); - prefs_->ScheduleSavePersistentPrefs(); + prefs->ScheduleSavePersistentPrefs(); // Programmatically appending to the prefs doesn't seem to notify the // observers... :/ - prefs_->pref_notifier()->FireObservers("extensions.install.denylist"); + prefs->pref_notifier()->FireObservers("extensions.install.denylist"); // Extension should not be running now. loop_.RunAllPending(); @@ -2233,7 +2243,7 @@ void ExtensionsServiceTest::TestExternalProvider( // Now clear the preference and reinstall. SetPrefInteg(good_crx, "state", Extension::ENABLED); - prefs_->ScheduleSavePersistentPrefs(); + profile_->GetPrefs()->ScheduleSavePersistentPrefs(); loaded_.clear(); service_->CheckForExternalUpdates(); @@ -2461,8 +2471,8 @@ TEST(ExtensionsServiceTestSimple, Enabledness) { // By default, we are enabled. command_line.reset(new CommandLine(CommandLine::ARGUMENTS_ONLY)); - service = new ExtensionsService(profile.get(), command_line.get(), - profile->GetPrefs(), install_dir, false); + service = profile->CreateExtensionsService(command_line.get(), + install_dir); EXPECT_TRUE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); @@ -2472,8 +2482,8 @@ TEST(ExtensionsServiceTestSimple, Enabledness) { recorder.set_ready(false); profile.reset(new TestingProfile()); command_line->AppendSwitch(switches::kDisableExtensions); - service = new ExtensionsService(profile.get(), command_line.get(), - profile->GetPrefs(), install_dir, false); + service = profile->CreateExtensionsService(command_line.get(), + install_dir); EXPECT_FALSE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); @@ -2482,8 +2492,8 @@ TEST(ExtensionsServiceTestSimple, Enabledness) { recorder.set_ready(false); profile.reset(new TestingProfile()); profile->GetPrefs()->SetBoolean(prefs::kDisableExtensions, true); - service = new ExtensionsService(profile.get(), command_line.get(), - profile->GetPrefs(), install_dir, false); + service = profile->CreateExtensionsService(command_line.get(), + install_dir); EXPECT_FALSE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); @@ -2493,8 +2503,8 @@ TEST(ExtensionsServiceTestSimple, Enabledness) { profile.reset(new TestingProfile()); profile->GetPrefs()->SetBoolean(prefs::kDisableExtensions, true); command_line.reset(new CommandLine(CommandLine::ARGUMENTS_ONLY)); - service = new ExtensionsService(profile.get(), command_line.get(), - profile->GetPrefs(), install_dir, false); + service = profile->CreateExtensionsService(command_line.get(), + install_dir); EXPECT_FALSE(service->extensions_enabled()); service->Init(); loop.RunAllPending(); diff --git a/chrome/browser/extensions/extensions_service_unittest.h b/chrome/browser/extensions/extensions_service_unittest.h index 62f6a95..fe298b4b 100644 --- a/chrome/browser/extensions/extensions_service_unittest.h +++ b/chrome/browser/extensions/extensions_service_unittest.h @@ -38,7 +38,6 @@ class ExtensionsServiceTestBase : public testing::Test { protected: ScopedTempDir temp_dir_; - scoped_ptr<PrefService> prefs_; scoped_ptr<Profile> profile_; FilePath extensions_install_dir_; scoped_refptr<ExtensionsService> service_; diff --git a/chrome/browser/gtk/gtk_theme_provider.cc b/chrome/browser/gtk/gtk_theme_provider.cc index 7e5a2c2..5bcd8a3 100644 --- a/chrome/browser/gtk/gtk_theme_provider.cc +++ b/chrome/browser/gtk/gtk_theme_provider.cc @@ -258,7 +258,6 @@ GtkThemeProvider::GtkThemeProvider() } GtkThemeProvider::~GtkThemeProvider() { - profile()->GetPrefs()->RemovePrefObserver(prefs::kUsesSystemTheme, this); gtk_widget_destroy(fake_window_); gtk_widget_destroy(fake_frame_); fake_label_.Destroy(); @@ -273,7 +272,8 @@ GtkThemeProvider::~GtkThemeProvider() { } void GtkThemeProvider::Init(Profile* profile) { - profile->GetPrefs()->AddPrefObserver(prefs::kUsesSystemTheme, this); + registrar_.Init(profile->GetPrefs()); + registrar_.Add(prefs::kUsesSystemTheme, this); use_gtk_ = profile->GetPrefs()->GetBoolean(prefs::kUsesSystemTheme); BrowserThemeProvider::Init(profile); diff --git a/chrome/browser/gtk/gtk_theme_provider.h b/chrome/browser/gtk/gtk_theme_provider.h index 33b1ec2..2ecef7a 100644 --- a/chrome/browser/gtk/gtk_theme_provider.h +++ b/chrome/browser/gtk/gtk_theme_provider.h @@ -12,6 +12,7 @@ #include "app/gtk_integers.h" #include "app/gtk_signal.h" #include "base/scoped_ptr.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/gtk/owned_widget_gtk.h" #include "chrome/browser/themes/browser_theme_provider.h" #include "chrome/common/notification_observer.h" @@ -275,6 +276,8 @@ class GtkThemeProvider : public BrowserThemeProvider, PerDisplaySurfaceMap per_display_surfaces_; PerDisplaySurfaceMap per_display_unthemed_surfaces_; + PrefChangeRegistrar registrar_; + // This is a dummy widget that only exists so we have something to pass to // gtk_widget_render_icon(). static GtkWidget* icon_widget_; diff --git a/chrome/browser/gtk/options/general_page_gtk.cc b/chrome/browser/gtk/options/general_page_gtk.cc index fb4db2b..734ee8c 100644 --- a/chrome/browser/gtk/options/general_page_gtk.cc +++ b/chrome/browser/gtk/options/general_page_gtk.cc @@ -95,8 +95,9 @@ GeneralPageGtk::GeneralPageGtk(Profile* profile) InitDefaultBrowserGroup(), false); #endif - profile->GetPrefs()->AddPrefObserver(prefs::kRestoreOnStartup, this); - profile->GetPrefs()->AddPrefObserver(prefs::kURLsToRestoreOnStartup, this); + registrar_.Init(profile->GetPrefs()); + registrar_.Add(prefs::kRestoreOnStartup, this); + registrar_.Add(prefs::kURLsToRestoreOnStartup, this); new_tab_page_is_home_page_.Init(prefs::kHomePageIsNewTabPage, profile->GetPrefs(), this); @@ -108,10 +109,6 @@ GeneralPageGtk::GeneralPageGtk(Profile* profile) } GeneralPageGtk::~GeneralPageGtk() { - profile()->GetPrefs()->RemovePrefObserver(prefs::kRestoreOnStartup, this); - profile()->GetPrefs()->RemovePrefObserver( - prefs::kURLsToRestoreOnStartup, this); - if (template_url_model_) template_url_model_->RemoveObserver(this); diff --git a/chrome/browser/gtk/options/general_page_gtk.h b/chrome/browser/gtk/options/general_page_gtk.h index 3a072ac..143f447 100644 --- a/chrome/browser/gtk/options/general_page_gtk.h +++ b/chrome/browser/gtk/options/general_page_gtk.h @@ -14,6 +14,7 @@ #include "chrome/browser/gtk/gtk_tree.h" #include "chrome/browser/gtk/options/managed_prefs_banner_gtk.h" #include "chrome/browser/options_page_base.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_member.h" #include "chrome/browser/search_engines/template_url_model_observer.h" #include "chrome/browser/shell_integration.h" @@ -160,6 +161,8 @@ class GeneralPageGtk : public OptionsPageBase, // Tracks managed preference warning banner state. ManagedPrefsBannerGtk managed_prefs_banner_; + PrefChangeRegistrar registrar_; + DISALLOW_COPY_AND_ASSIGN(GeneralPageGtk); }; diff --git a/chrome/browser/host_content_settings_map.cc b/chrome/browser/host_content_settings_map.cc index cffe57e..4ac9f405 100644 --- a/chrome/browser/host_content_settings_map.cc +++ b/chrome/browser/host_content_settings_map.cc @@ -230,10 +230,11 @@ HostContentSettingsMap::HostContentSettingsMap(Profile* profile) // Read exceptions. ReadExceptions(false); - prefs->AddPrefObserver(prefs::kDefaultContentSettings, this); - prefs->AddPrefObserver(prefs::kContentSettingsPatterns, this); - prefs->AddPrefObserver(prefs::kBlockThirdPartyCookies, this); - prefs->AddPrefObserver(prefs::kBlockNonsandboxedPlugins, this); + pref_change_registrar_.Init(prefs); + pref_change_registrar_.Add(prefs::kDefaultContentSettings, this); + pref_change_registrar_.Add(prefs::kContentSettingsPatterns, this); + pref_change_registrar_.Add(prefs::kBlockThirdPartyCookies, this); + pref_change_registrar_.Add(prefs::kBlockNonsandboxedPlugins, this); notification_registrar_.Add(this, NotificationType::PROFILE_DESTROYED, Source<Profile>(profile_)); } @@ -909,11 +910,7 @@ void HostContentSettingsMap::UnregisterObservers() { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); if (!profile_) return; - PrefService* prefs = profile_->GetPrefs(); - prefs->RemovePrefObserver(prefs::kDefaultContentSettings, this); - prefs->RemovePrefObserver(prefs::kContentSettingsPatterns, this); - prefs->RemovePrefObserver(prefs::kBlockThirdPartyCookies, this); - prefs->RemovePrefObserver(prefs::kBlockNonsandboxedPlugins, this); + pref_change_registrar_.RemoveAll(); notification_registrar_.Remove(this, NotificationType::PROFILE_DESTROYED, Source<Profile>(profile_)); profile_ = NULL; diff --git a/chrome/browser/host_content_settings_map.h b/chrome/browser/host_content_settings_map.h index ff9c8f0..7afb808 100644 --- a/chrome/browser/host_content_settings_map.h +++ b/chrome/browser/host_content_settings_map.h @@ -18,6 +18,7 @@ #include "base/lock.h" #include "base/ref_counted.h" #include "chrome/browser/chrome_thread.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/content_settings.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" @@ -285,6 +286,7 @@ class HostContentSettingsMap Profile* profile_; NotificationRegistrar notification_registrar_; + PrefChangeRegistrar pref_change_registrar_; // Copies of the pref data, so that we can read it on the IO thread. ContentSettings default_content_settings_; diff --git a/chrome/browser/host_zoom_map.cc b/chrome/browser/host_zoom_map.cc index 0c1d96a..cb67a04 100644 --- a/chrome/browser/host_zoom_map.cc +++ b/chrome/browser/host_zoom_map.cc @@ -27,8 +27,10 @@ HostZoomMap::HostZoomMap(Profile* profile) // Don't observe pref changes (e.g. from sync) in Incognito; once we create // the incognito window it should have no further connection to the main // profile/prefs. - if (!profile_->IsOffTheRecord()) - profile_->GetPrefs()->AddPrefObserver(prefs::kPerHostZoomLevels, this); + if (!profile_->IsOffTheRecord()) { + pref_change_registrar_.Init(profile_->GetPrefs()); + pref_change_registrar_.Add(prefs::kPerHostZoomLevels, this); + } } void HostZoomMap::Load() { @@ -128,7 +130,7 @@ void HostZoomMap::Shutdown() { NotificationType::PROFILE_DESTROYED, Source<Profile>(profile_)); if (!profile_->IsOffTheRecord()) - profile_->GetPrefs()->RemovePrefObserver(prefs::kPerHostZoomLevels, this); + pref_change_registrar_.RemoveAll(); profile_ = NULL; } diff --git a/chrome/browser/host_zoom_map.h b/chrome/browser/host_zoom_map.h index caa60b7..3142024 100644 --- a/chrome/browser/host_zoom_map.h +++ b/chrome/browser/host_zoom_map.h @@ -15,6 +15,7 @@ #include "base/basictypes.h" #include "base/lock.h" #include "base/ref_counted.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" @@ -83,6 +84,7 @@ class HostZoomMap : public NotificationObserver, bool updating_preferences_; NotificationRegistrar registrar_; + PrefChangeRegistrar pref_change_registrar_; DISALLOW_COPY_AND_ASSIGN(HostZoomMap); }; diff --git a/chrome/browser/host_zoom_map_unittest.cc b/chrome/browser/host_zoom_map_unittest.cc index e6b1f72..5d206e1 100644 --- a/chrome/browser/host_zoom_map_unittest.cc +++ b/chrome/browser/host_zoom_map_unittest.cc @@ -70,7 +70,9 @@ TEST_F(HostZoomMapTest, Load) { TEST_F(HostZoomMapTest, SetZoomLevel) { scoped_refptr<HostZoomMap> map(new HostZoomMap(&profile_)); - prefs_->AddPrefObserver(prefs::kPerHostZoomLevels, &pref_observer_); + PrefChangeRegistrar registrar; + registrar.Init(prefs_); + registrar.Add(prefs::kPerHostZoomLevels, &pref_observer_); SetPrefObserverExpectation(); map->SetZoomLevel(url_, kZoomLevel); EXPECT_EQ(kZoomLevel, map->GetZoomLevel(url_)); @@ -84,19 +86,19 @@ TEST_F(HostZoomMapTest, SetZoomLevel) { map->SetZoomLevel(url_, 0); EXPECT_EQ(0, map->GetZoomLevel(url_)); EXPECT_FALSE(dict->HasKey(host_)); - prefs_->RemovePrefObserver(prefs::kPerHostZoomLevels, &pref_observer_); } TEST_F(HostZoomMapTest, ResetToDefaults) { scoped_refptr<HostZoomMap> map(new HostZoomMap(&profile_)); map->SetZoomLevel(url_, kZoomLevel); - prefs_->AddPrefObserver(prefs::kPerHostZoomLevels, &pref_observer_); + PrefChangeRegistrar registrar; + registrar.Init(prefs_); + registrar.Add(prefs::kPerHostZoomLevels, &pref_observer_); SetPrefObserverExpectation(); map->ResetToDefaults(); EXPECT_EQ(0, map->GetZoomLevel(url_)); EXPECT_EQ(NULL, prefs_->GetDictionary(prefs::kPerHostZoomLevels)); - prefs_->RemovePrefObserver(prefs::kPerHostZoomLevels, &pref_observer_); } TEST_F(HostZoomMapTest, ReloadOnPrefChange) { diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc index c9271d9..88f8ee0 100644 --- a/chrome/browser/net/chrome_url_request_context.cc +++ b/chrome/browser/net/chrome_url_request_context.cc @@ -526,8 +526,7 @@ ChromeURLRequestContext* FactoryForMedia::Create() { ChromeURLRequestContextGetter::ChromeURLRequestContextGetter( Profile* profile, ChromeURLRequestContextFactory* factory) - : prefs_(NULL), - factory_(factory), + : factory_(factory), url_request_context_(NULL) { DCHECK(factory); @@ -539,7 +538,7 @@ ChromeURLRequestContextGetter::ChromeURLRequestContextGetter( ChromeURLRequestContextGetter::~ChromeURLRequestContextGetter() { CheckCurrentlyOnIOThread(); - DCHECK(!prefs_) << "Probably didn't call CleanupOnUIThread"; + DCHECK(registrar_.IsEmpty()) << "Probably didn't call CleanupOnUIThread"; // Either we already transformed the factory into a URLRequestContext, or // we still have a pending factory. @@ -652,13 +651,8 @@ ChromeURLRequestContextGetter::CreateOffTheRecordForExtensions( void ChromeURLRequestContextGetter::CleanupOnUIThread() { CheckCurrentlyOnMainThread(); - - if (prefs_) { - // Unregister for pref notifications. - prefs_->RemovePrefObserver(prefs::kAcceptLanguages, this); - prefs_->RemovePrefObserver(prefs::kDefaultCharset, this); - prefs_ = NULL; - } + // Unregister for pref notifications. + registrar_.RemoveAll(); } void ChromeURLRequestContextGetter::OnNewExtensions( @@ -710,10 +704,9 @@ void ChromeURLRequestContextGetter::Observe( void ChromeURLRequestContextGetter::RegisterPrefsObserver(Profile* profile) { CheckCurrentlyOnMainThread(); - prefs_ = profile->GetPrefs(); - - prefs_->AddPrefObserver(prefs::kAcceptLanguages, this); - prefs_->AddPrefObserver(prefs::kDefaultCharset, this); + registrar_.Init(profile->GetPrefs()); + registrar_.Add(prefs::kAcceptLanguages, this); + registrar_.Add(prefs::kDefaultCharset, this); } // static diff --git a/chrome/browser/net/chrome_url_request_context.h b/chrome/browser/net/chrome_url_request_context.h index b7ff017..3141364 100644 --- a/chrome/browser/net/chrome_url_request_context.h +++ b/chrome/browser/net/chrome_url_request_context.h @@ -19,6 +19,7 @@ #include "chrome/browser/host_zoom_map.h" #include "chrome/browser/io_thread.h" #include "chrome/browser/net/chrome_cookie_policy.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/common/extensions/extension.h" #include "chrome/common/extensions/extension_icon_set.h" @@ -382,8 +383,7 @@ class ChromeURLRequestContextGetter : public URLRequestContextGetter, void GetCookieStoreAsyncHelper(base::WaitableEvent* completion, net::CookieStore** result); - // Access only from the UI thread. - PrefService* prefs_; + PrefChangeRegistrar registrar_; // Deferred logic for creating a ChromeURLRequestContext. // Access only from the IO thread. diff --git a/chrome/browser/notifications/desktop_notification_service.cc b/chrome/browser/notifications/desktop_notification_service.cc index b55c573..1b3df09 100644 --- a/chrome/browser/notifications/desktop_notification_service.cc +++ b/chrome/browser/notifications/desktop_notification_service.cc @@ -210,6 +210,7 @@ DesktopNotificationService::DesktopNotificationService(Profile* profile, NotificationUIManager* ui_manager) : profile_(profile), ui_manager_(ui_manager) { + registrar_.Init(profile_->GetPrefs()); InitPrefs(); StartObserving(); } @@ -254,21 +255,15 @@ void DesktopNotificationService::InitPrefs() { void DesktopNotificationService::StartObserving() { if (!profile_->IsOffTheRecord()) { - PrefService* prefs = profile_->GetPrefs(); - prefs->AddPrefObserver(prefs::kDesktopNotificationDefaultContentSetting, - this); - prefs->AddPrefObserver(prefs::kDesktopNotificationAllowedOrigins, this); - prefs->AddPrefObserver(prefs::kDesktopNotificationDeniedOrigins, this); + registrar_.Add(prefs::kDesktopNotificationDefaultContentSetting, this); + registrar_.Add(prefs::kDesktopNotificationAllowedOrigins, this); + registrar_.Add(prefs::kDesktopNotificationDeniedOrigins, this); } } void DesktopNotificationService::StopObserving() { if (!profile_->IsOffTheRecord()) { - PrefService* prefs = profile_->GetPrefs(); - prefs->RemovePrefObserver(prefs::kDesktopNotificationDefaultContentSetting, - this); - prefs->RemovePrefObserver(prefs::kDesktopNotificationAllowedOrigins, this); - prefs->RemovePrefObserver(prefs::kDesktopNotificationDeniedOrigins, this); + registrar_.RemoveAll(); } } diff --git a/chrome/browser/notifications/desktop_notification_service.h b/chrome/browser/notifications/desktop_notification_service.h index d1a4ccc..2282cd2 100644 --- a/chrome/browser/notifications/desktop_notification_service.h +++ b/chrome/browser/notifications/desktop_notification_service.h @@ -11,6 +11,7 @@ #include "base/basictypes.h" #include "base/string16.h" #include "chrome/browser/notifications/notification.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/content_settings.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" @@ -136,6 +137,8 @@ class DesktopNotificationService : public NotificationObserver { // UI for desktop toasts. NotificationUIManager* ui_manager_; + PrefChangeRegistrar registrar_; + DISALLOW_COPY_AND_ASSIGN(DesktopNotificationService); }; diff --git a/chrome/browser/prefs/pref_change_registrar.cc b/chrome/browser/prefs/pref_change_registrar.cc index dc8bb97..05372b1 100644 --- a/chrome/browser/prefs/pref_change_registrar.cc +++ b/chrome/browser/prefs/pref_change_registrar.cc @@ -9,16 +9,11 @@ PrefChangeRegistrar::PrefChangeRegistrar() : service_(NULL) {} PrefChangeRegistrar::~PrefChangeRegistrar() { - if (service_) { - for (std::set<ObserverRegistration>::const_iterator it = observers_.begin(); - it != observers_.end(); ++it) { - service_->RemovePrefObserver(it->first.c_str(), it->second); - } - } + RemoveAll(); } void PrefChangeRegistrar::Init(PrefService* service) { - DCHECK(!service_); + DCHECK(IsEmpty() || service_ == service); service_ = service; } @@ -51,3 +46,17 @@ void PrefChangeRegistrar::Remove(const char* path, NotificationObserver* obs) { service_->RemovePrefObserver(it->first.c_str(), it->second); observers_.erase(it); } + +void PrefChangeRegistrar::RemoveAll() { + if (service_) { + for (std::set<ObserverRegistration>::const_iterator it = observers_.begin(); + it != observers_.end(); ++it) { + service_->RemovePrefObserver(it->first.c_str(), it->second); + } + observers_.clear(); + } +} + +bool PrefChangeRegistrar::IsEmpty() const { + return observers_.empty(); +} diff --git a/chrome/browser/prefs/pref_change_registrar.h b/chrome/browser/prefs/pref_change_registrar.h index cd8f5eb..773c556 100644 --- a/chrome/browser/prefs/pref_change_registrar.h +++ b/chrome/browser/prefs/pref_change_registrar.h @@ -23,7 +23,8 @@ class PrefChangeRegistrar { PrefChangeRegistrar(); virtual ~PrefChangeRegistrar(); - // Must be called before adding or removing observers. + // Must be called before adding or removing observers. Can be called more + // than once as long as the value of |service| doesn't change. void Init(PrefService* service); // Adds an pref observer for the specified pref |path| and |obs| observer @@ -38,6 +39,12 @@ class PrefChangeRegistrar { void Remove(const char* path, NotificationObserver* obs); + // Removes all observers that have been previously added with a call to Add. + void RemoveAll(); + + // Returns true if no pref observers are registered. + bool IsEmpty() const; + private: typedef std::pair<std::string, NotificationObserver*> ObserverRegistration; diff --git a/chrome/browser/prefs/pref_change_registrar_unittest.cc b/chrome/browser/prefs/pref_change_registrar_unittest.cc index 4d7f747..8096ee6 100644 --- a/chrome/browser/prefs/pref_change_registrar_unittest.cc +++ b/chrome/browser/prefs/pref_change_registrar_unittest.cc @@ -63,6 +63,7 @@ TEST_F(PrefChangeRegistrarTest, AddAndRemove) { AddPrefObserver(Eq(std::string("test.pref.2")), observer())); registrar.Add("test.pref.1", observer()); registrar.Add("test.pref.2", observer()); + EXPECT_FALSE(registrar.IsEmpty()); // Test removing. Mock::VerifyAndClearExpectations(service()); @@ -72,6 +73,11 @@ TEST_F(PrefChangeRegistrarTest, AddAndRemove) { RemovePrefObserver(Eq(std::string("test.pref.2")), observer())); registrar.Remove("test.pref.1", observer()); registrar.Remove("test.pref.2", observer()); + EXPECT_TRUE(registrar.IsEmpty()); + + // Explicitly check the expectations now to make sure that the Removes + // worked (rather than the registrar destructor doing the work). + Mock::VerifyAndClearExpectations(service()); } TEST_F(PrefChangeRegistrarTest, AutoRemove) { @@ -83,8 +89,33 @@ TEST_F(PrefChangeRegistrarTest, AutoRemove) { AddPrefObserver(Eq(std::string("test.pref.1")), observer())); registrar.Add("test.pref.1", observer()); Mock::VerifyAndClearExpectations(service()); + EXPECT_FALSE(registrar.IsEmpty()); // Test auto-removing. EXPECT_CALL(*service(), RemovePrefObserver(Eq(std::string("test.pref.1")), observer())); } + +TEST_F(PrefChangeRegistrarTest, RemoveAll) { + PrefChangeRegistrar registrar; + registrar.Init(service()); + + EXPECT_CALL(*service(), + AddPrefObserver(Eq(std::string("test.pref.1")), observer())); + EXPECT_CALL(*service(), + AddPrefObserver(Eq(std::string("test.pref.2")), observer())); + registrar.Add("test.pref.1", observer()); + registrar.Add("test.pref.2", observer()); + Mock::VerifyAndClearExpectations(service()); + + EXPECT_CALL(*service(), + RemovePrefObserver(Eq(std::string("test.pref.1")), observer())); + EXPECT_CALL(*service(), + RemovePrefObserver(Eq(std::string("test.pref.2")), observer())); + registrar.RemoveAll(); + EXPECT_TRUE(registrar.IsEmpty()); + + // Explicitly check the expectations now to make sure that the RemoveAll + // worked (rather than the registrar destructor doing the work). + Mock::VerifyAndClearExpectations(service()); +} diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h index 450c6c1..5cedd52 100644 --- a/chrome/browser/prefs/pref_service.h +++ b/chrome/browser/prefs/pref_service.h @@ -19,9 +19,14 @@ class FilePath; class NotificationObserver; +class PrefChangeObserver; class PrefNotifier; class Profile; +namespace subtle { + class PrefMemberBase; +}; + class PrefService : public NonThreadSafe { public: // A helper class to store all the information associated with a preference. @@ -161,11 +166,6 @@ class PrefService : public NonThreadSafe { const DictionaryValue* GetDictionary(const char* path) const; const ListValue* GetList(const char* path) const; - // If the pref at the given path changes, we call the observer's Observe - // method with NOTIFY_PREF_CHANGED. - virtual void AddPrefObserver(const char* path, NotificationObserver* obs); - virtual void RemovePrefObserver(const char* path, NotificationObserver* obs); - // Removes a user pref and restores the pref to its default value. void ClearPref(const char* path); @@ -229,6 +229,22 @@ class PrefService : public NonThreadSafe { scoped_ptr<PrefNotifier> pref_notifier_; private: + // Registration of pref change observers must be done using the + // PrefChangeRegistrar, which is declared as a friend here to grant it + // access to the otherwise protected members Add/RemovePrefObserver. + // PrefMember registers for preferences changes notification directly to + // avoid the storage overhead of the registrar, so its base class must be + // declared as a friend, too. + friend class PrefChangeRegistrar; + friend class subtle::PrefMemberBase; + + // If the pref at the given path changes, we call the observer's Observe + // method with NOTIFY_PREF_CHANGED. Note that observers should not call + // these methods directly but rather use a PrefChangeRegistrar to make sure + // the observer gets cleaned up properly. + virtual void AddPrefObserver(const char* path, NotificationObserver* obs); + virtual void RemovePrefObserver(const char* path, NotificationObserver* obs); + // Add a preference to the PreferenceMap. If the pref already exists, return // false. This method takes ownership of |default_value|. void RegisterPreference(const char* path, Value* default_value); diff --git a/chrome/browser/prefs/pref_service_unittest.cc b/chrome/browser/prefs/pref_service_unittest.cc index 461f108..d62679d 100644 --- a/chrome/browser/prefs/pref_service_unittest.cc +++ b/chrome/browser/prefs/pref_service_unittest.cc @@ -8,6 +8,7 @@ #include "base/scoped_ptr.h" #include "base/values.h" #include "chrome/browser/prefs/dummy_pref_store.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_value_store.h" #include "chrome/common/chrome_paths.h" #include "chrome/common/notification_observer_mock.h" @@ -94,7 +95,10 @@ TEST(PrefServiceTest, NoObserverFire) { const std::string new_pref_value("http://www.google.com/"); TestPrefObserver obs(&prefs, pref_name, new_pref_value); - prefs.AddPrefObserver(pref_name, &obs); + + PrefChangeRegistrar registrar; + registrar.Init(&prefs); + registrar.Add(pref_name, &obs); // This should fire the checks in TestPrefObserver::Observe. prefs.SetString(pref_name, new_pref_value); @@ -116,9 +120,6 @@ TEST(PrefServiceTest, NoObserverFire) { obs.Reset(""); prefs.ClearPref(pref_name); EXPECT_FALSE(obs.observer_fired()); - - // Ok, clean up. - prefs.RemovePrefObserver(pref_name, &obs); } TEST(PrefServiceTest, HasPrefPath) { @@ -148,7 +149,9 @@ TEST(PrefServiceTest, Observers) { const std::string new_pref_value("http://www.google.com/"); TestPrefObserver obs(&prefs, pref_name, new_pref_value); - prefs.AddPrefObserver(pref_name, &obs); + PrefChangeRegistrar registrar; + registrar.Init(&prefs); + registrar.Add(pref_name, &obs); // This should fire the checks in TestPrefObserver::Observe. prefs.SetString(pref_name, new_pref_value); @@ -159,23 +162,20 @@ TEST(PrefServiceTest, Observers) { const std::string new_pref_value2("http://www.youtube.com/"); obs.Reset(new_pref_value2); TestPrefObserver obs2(&prefs, pref_name, new_pref_value2); - prefs.AddPrefObserver(pref_name, &obs2); + registrar.Add(pref_name, &obs2); // This should fire the checks in obs and obs2. prefs.SetString(pref_name, new_pref_value2); EXPECT_TRUE(obs.observer_fired()); EXPECT_TRUE(obs2.observer_fired()); // Make sure obs2 still works after removing obs. - prefs.RemovePrefObserver(pref_name, &obs); + registrar.Remove(pref_name, &obs); obs.Reset(""); obs2.Reset(new_pref_value); // This should only fire the observer in obs2. prefs.SetString(pref_name, new_pref_value); EXPECT_FALSE(obs.observer_fired()); EXPECT_TRUE(obs2.observer_fired()); - - // Ok, clean up. - prefs.RemovePrefObserver(pref_name, &obs2); } class PrefServiceSetValueTest : public testing::Test { @@ -211,7 +211,11 @@ TEST_F(PrefServiceSetValueTest, SetStringValue) { const char default_string[] = "default"; scoped_ptr<Value> default_value(Value::CreateStringValue(default_string)); prefs_.RegisterStringPref(name_, default_string); - prefs_.AddPrefObserver(name_, &observer_); + + PrefChangeRegistrar registrar; + registrar.Init(&prefs_); + registrar.Add(name_, &observer_); + // Changing the controlling store from default to user triggers notification. SetExpectPrefChanged(); prefs_.Set(name_, *default_value); @@ -225,13 +229,13 @@ TEST_F(PrefServiceSetValueTest, SetStringValue) { SetExpectPrefChanged(); prefs_.Set(name_, *new_value); EXPECT_EQ(value_, prefs_.GetString(name_)); - - prefs_.RemovePrefObserver(name_, &observer_); } TEST_F(PrefServiceSetValueTest, SetDictionaryValue) { prefs_.RegisterDictionaryPref(name_); - prefs_.AddPrefObserver(name_, &observer_); + PrefChangeRegistrar registrar; + registrar.Init(&prefs_); + registrar.Add(name_, &observer_); // Dictionary values are special: setting one to NULL is the same as clearing // the user value, allowing the NULL default to take (or keep) control. @@ -259,13 +263,13 @@ TEST_F(PrefServiceSetValueTest, SetDictionaryValue) { Mock::VerifyAndClearExpectations(&observer_); dict = prefs_.GetMutableDictionary(name_); EXPECT_EQ(0U, dict->size()); - - prefs_.RemovePrefObserver(name_, &observer_); } TEST_F(PrefServiceSetValueTest, SetListValue) { prefs_.RegisterListPref(name_); - prefs_.AddPrefObserver(name_, &observer_); + PrefChangeRegistrar registrar; + registrar.Init(&prefs_); + registrar.Add(name_, &observer_); // List values are special: setting one to NULL is the same as clearing the // user value, allowing the NULL default to take (or keep) control. @@ -293,6 +297,4 @@ TEST_F(PrefServiceSetValueTest, SetListValue) { Mock::VerifyAndClearExpectations(&observer_); list = prefs_.GetMutableList(name_); EXPECT_EQ(0U, list->GetSize()); - - prefs_.RemovePrefObserver(name_, &observer_); } diff --git a/chrome/browser/prefs/pref_set_observer.cc b/chrome/browser/prefs/pref_set_observer.cc index a4ecf6f..dd067d3 100644 --- a/chrome/browser/prefs/pref_set_observer.cc +++ b/chrome/browser/prefs/pref_set_observer.cc @@ -11,23 +11,19 @@ PrefSetObserver::PrefSetObserver(PrefService* pref_service, NotificationObserver* observer) : pref_service_(pref_service), observer_(observer) { -} - -PrefSetObserver::~PrefSetObserver() { - for (PrefSet::const_iterator i(prefs_.begin()); i != prefs_.end(); ++i) - pref_service_->RemovePrefObserver(i->c_str(), this); + registrar_.Init(pref_service); } void PrefSetObserver::AddPref(const std::string& pref) { if (!prefs_.count(pref) && pref_service_->FindPreference(pref.c_str())) { prefs_.insert(pref); - pref_service_->AddPrefObserver(pref.c_str(), this); + registrar_.Add(pref.c_str(), this); } } void PrefSetObserver::RemovePref(const std::string& pref) { if (prefs_.erase(pref)) - pref_service_->RemovePrefObserver(pref.c_str(), this); + registrar_.Remove(pref.c_str(), this); } bool PrefSetObserver::IsObserved(const std::string& pref) { diff --git a/chrome/browser/prefs/pref_set_observer.h b/chrome/browser/prefs/pref_set_observer.h index beaec01..ff350df 100644 --- a/chrome/browser/prefs/pref_set_observer.h +++ b/chrome/browser/prefs/pref_set_observer.h @@ -10,6 +10,7 @@ #include "base/basictypes.h" #include "chrome/browser/prefs/pref_service.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/notification_observer.h" // Observes the state of a set of preferences and allows to query their combined @@ -19,7 +20,7 @@ class PrefSetObserver : public NotificationObserver { // Initialize with an empty set of preferences. PrefSetObserver(PrefService* pref_service, NotificationObserver* observer); - virtual ~PrefSetObserver(); + virtual ~PrefSetObserver() {} // Add a |pref| to the set of preferences to observe. void AddPref(const std::string& pref); @@ -51,6 +52,7 @@ class PrefSetObserver : public NotificationObserver { PrefSet prefs_; PrefService* pref_service_; + PrefChangeRegistrar registrar_; NotificationObserver* observer_; DISALLOW_COPY_AND_ASSIGN(PrefSetObserver); diff --git a/chrome/browser/profile_impl.cc b/chrome/browser/profile_impl.cc index 5a61e1c..f72a6a5 100644 --- a/chrome/browser/profile_impl.cc +++ b/chrome/browser/profile_impl.cc @@ -292,9 +292,10 @@ ProfileImpl::ProfileImpl(const FilePath& path) &ProfileImpl::EnsureSessionServiceCreated); PrefService* prefs = GetPrefs(); - prefs->AddPrefObserver(prefs::kSpellCheckDictionary, this); - prefs->AddPrefObserver(prefs::kEnableSpellCheck, this); - prefs->AddPrefObserver(prefs::kEnableAutoSpellCorrect, this); + pref_change_registrar_.Init(prefs); + pref_change_registrar_.Add(prefs::kSpellCheckDictionary, this); + pref_change_registrar_.Add(prefs::kEnableSpellCheck, this); + pref_change_registrar_.Add(prefs::kEnableAutoSpellCorrect, this); #if defined(OS_MACOSX) // If the profile directory doesn't already have a cache directory and it @@ -390,7 +391,6 @@ void ProfileImpl::InitExtensions() { extensions_service_ = new ExtensionsService( this, CommandLine::ForCurrentProcess(), - GetPrefs(), GetPath().AppendASCII(ExtensionsService::kInstallDirectoryName), true); @@ -501,11 +501,8 @@ ProfileImpl::~ProfileImpl() { // The theme provider provides bitmaps to whoever wants them. theme_provider_.reset(); - // Remove pref observers. - PrefService* prefs = GetPrefs(); - prefs->RemovePrefObserver(prefs::kSpellCheckDictionary, this); - prefs->RemovePrefObserver(prefs::kEnableSpellCheck, this); - prefs->RemovePrefObserver(prefs::kEnableAutoSpellCorrect, this); + // Remove pref observers + pref_change_registrar_.RemoveAll(); // Delete the NTP resource cache so we can unregister pref observers. ntp_resource_cache_.reset(); diff --git a/chrome/browser/profile_impl.h b/chrome/browser/profile_impl.h index 9331d34..f324fbd 100644 --- a/chrome/browser/profile_impl.h +++ b/chrome/browser/profile_impl.h @@ -13,6 +13,7 @@ #include "base/scoped_ptr.h" #include "base/timer.h" #include "chrome/browser/profile.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/spellcheck_host_observer.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" @@ -144,6 +145,7 @@ class ProfileImpl : public Profile, } NotificationRegistrar registrar_; + PrefChangeRegistrar pref_change_registrar_; FilePath path_; FilePath base_cache_path_; diff --git a/chrome/browser/sync/glue/preference_change_processor.cc b/chrome/browser/sync/glue/preference_change_processor.cc index ec0eeb3..874f786 100644 --- a/chrome/browser/sync/glue/preference_change_processor.cc +++ b/chrome/browser/sync/glue/preference_change_processor.cc @@ -195,6 +195,7 @@ Value* PreferenceChangeProcessor::ReadPreference( void PreferenceChangeProcessor::StartImpl(Profile* profile) { DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI)); pref_service_ = profile->GetPrefs(); + registrar_.Init(pref_service_); StartObserving(); } @@ -210,7 +211,7 @@ void PreferenceChangeProcessor::StartObserving() { for (std::set<std::string>::const_iterator it = model_associator_->synced_preferences().begin(); it != model_associator_->synced_preferences().end(); ++it) { - pref_service_->AddPrefObserver((*it).c_str(), this); + registrar_.Add((*it).c_str(), this); } } @@ -219,7 +220,7 @@ void PreferenceChangeProcessor::StopObserving() { for (std::set<std::string>::const_iterator it = model_associator_->synced_preferences().begin(); it != model_associator_->synced_preferences().end(); ++it) { - pref_service_->RemovePrefObserver((*it).c_str(), this); + registrar_.Remove((*it).c_str(), this); } } diff --git a/chrome/browser/sync/glue/preference_change_processor.h b/chrome/browser/sync/glue/preference_change_processor.h index cb25511..1ef88da 100644 --- a/chrome/browser/sync/glue/preference_change_processor.h +++ b/chrome/browser/sync/glue/preference_change_processor.h @@ -8,6 +8,7 @@ #include <string> +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/change_processor.h" @@ -60,6 +61,8 @@ class PreferenceChangeProcessor : public ChangeProcessor, // Whether we are currently processing a preference change notification. bool processing_pref_change_; + PrefChangeRegistrar registrar_; + DISALLOW_COPY_AND_ASSIGN(PreferenceChangeProcessor); }; diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc index 086d1f1..b33f3d8 100644 --- a/chrome/browser/tab_contents/tab_contents.cc +++ b/chrome/browser/tab_contents/tab_contents.cc @@ -400,9 +400,10 @@ TabContents::TabContents(Profile* profile, // Register for notifications about all interested prefs change. PrefService* prefs = profile->GetPrefs(); + pref_change_registrar_.Init(prefs); if (prefs) { for (int i = 0; i < kPrefsToObserveLength; ++i) - prefs->AddPrefObserver(kPrefsToObserve[i], this); + pref_change_registrar_.Add(kPrefsToObserve[i], this); } // Register for notifications about URL starredness changing on any profile. @@ -442,13 +443,7 @@ TabContents::~TabContents() { // We don't want any notifications while we're running our destructor. registrar_.RemoveAll(); - - // Unregister the notifications of all observed prefs change. - PrefService* prefs = profile()->GetPrefs(); - if (prefs) { - for (int i = 0; i < kPrefsToObserveLength; ++i) - prefs->RemovePrefObserver(kPrefsToObserve[i], this); - } + pref_change_registrar_.RemoveAll(); NotifyDisconnected(); hung_renderer_dialog::HideForTabContents(this); diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h index 90ded2e..d37aab8 100644 --- a/chrome/browser/tab_contents/tab_contents.h +++ b/chrome/browser/tab_contents/tab_contents.h @@ -24,6 +24,7 @@ #include "chrome/browser/find_bar_controller.h" #include "chrome/browser/find_notification_details.h" #include "chrome/browser/jsmessage_box_client.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/password_manager/password_manager_delegate.h" #include "chrome/browser/renderer_host/render_view_host_delegate.h" #include "chrome/browser/tab_contents/constrained_window.h" @@ -1057,6 +1058,9 @@ class TabContents : public PageNavigator, // Registers and unregisters us for notifications. NotificationRegistrar registrar_; + // Registers and unregisters for pref notifications. + PrefChangeRegistrar pref_change_registrar_; + // Handles print preview and print job for this contents. scoped_ptr<printing::PrintViewManager> printing_; diff --git a/chrome/browser/translate/translate_manager.cc b/chrome/browser/translate/translate_manager.cc index 6f649fd..1c70dd4 100644 --- a/chrome/browser/translate/translate_manager.cc +++ b/chrome/browser/translate/translate_manager.cc @@ -249,7 +249,7 @@ void TranslateManager::Observe(NotificationType type, // We should know about this profile since we are listening for // notifications on it. DCHECK(count > 0); - profile->GetPrefs()->RemovePrefObserver(prefs::kAcceptLanguages, this); + pref_change_registrar_.Remove(prefs::kAcceptLanguages, this); break; } case NotificationType::PREF_CHANGED: { @@ -342,6 +342,8 @@ void TranslateManager::InitiateTranslation(TabContents* tab, if (!prefs->GetBoolean(prefs::kEnableTranslate)) return; + pref_change_registrar_.Init(prefs); + // Allow disabling of translate from the command line to assist with // automated browser testing. if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableTranslate)) @@ -534,7 +536,7 @@ bool TranslateManager::IsAcceptLanguage(TabContents* tab, notification_registrar_.Add(this, NotificationType::PROFILE_DESTROYED, Source<Profile>(tab->profile())); // Also start listening for changes in the accept languages. - tab->profile()->GetPrefs()->AddPrefObserver(prefs::kAcceptLanguages, this); + pref_change_registrar_.Add(prefs::kAcceptLanguages, this); iter = accept_languages_.find(pref_service); } diff --git a/chrome/browser/translate/translate_manager.h b/chrome/browser/translate/translate_manager.h index adec281..3ddcabf 100644 --- a/chrome/browser/translate/translate_manager.h +++ b/chrome/browser/translate/translate_manager.h @@ -14,6 +14,7 @@ #include "base/lazy_instance.h" #include "base/singleton.h" #include "base/task.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/common/net/url_fetcher.h" #include "chrome/common/notification_observer.h" #include "chrome/common/notification_registrar.h" @@ -153,6 +154,7 @@ class TranslateManager : public NotificationObserver, TabContents* tab); NotificationRegistrar notification_registrar_; + PrefChangeRegistrar pref_change_registrar_; // A map that associates a profile with its parsed "accept languages". typedef std::set<std::string> LanguageSet; diff --git a/chrome/browser/translate/translate_manager_unittest.cc b/chrome/browser/translate/translate_manager_unittest.cc index 8f2b801..f578b3f 100644 --- a/chrome/browser/translate/translate_manager_unittest.cc +++ b/chrome/browser/translate/translate_manager_unittest.cc @@ -7,6 +7,7 @@ #include "base/utf_string_conversions.h" #include "chrome/app/chrome_dll_resource.h" #include "chrome/browser/prefs/pref_service.h" +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/renderer_host/mock_render_process_host.h" #include "chrome/browser/tab_contents/navigation_controller.h" #include "chrome/browser/tab_contents/render_view_context_menu.h" @@ -851,8 +852,10 @@ TEST_F(TranslateManagerTest, NeverTranslateLanguagePref) { // Select never translate this language. PrefService* prefs = contents()->profile()->GetPrefs(); - prefs->AddPrefObserver(TranslatePrefs::kPrefTranslateLanguageBlacklist, - &pref_observer_); + PrefChangeRegistrar registrar; + registrar.Init(prefs); + registrar.Add(TranslatePrefs::kPrefTranslateLanguageBlacklist, + &pref_observer_); TranslatePrefs translate_prefs(prefs); EXPECT_FALSE(translate_prefs.IsLanguageBlacklisted("fr")); EXPECT_TRUE(translate_prefs.CanTranslate(prefs, "fr", url)); @@ -881,8 +884,6 @@ TEST_F(TranslateManagerTest, NeverTranslateLanguagePref) { // There should be a translate infobar. EXPECT_TRUE(GetTranslateInfoBar() != NULL); - prefs->RemovePrefObserver(TranslatePrefs::kPrefTranslateLanguageBlacklist, - &pref_observer_); } // Tests the "Never translate this site" pref. @@ -897,8 +898,10 @@ TEST_F(TranslateManagerTest, NeverTranslateSitePref) { // Select never translate this site. PrefService* prefs = contents()->profile()->GetPrefs(); - prefs->AddPrefObserver(TranslatePrefs::kPrefTranslateSiteBlacklist, - &pref_observer_); + PrefChangeRegistrar registrar; + registrar.Init(prefs); + registrar.Add(TranslatePrefs::kPrefTranslateSiteBlacklist, + &pref_observer_); TranslatePrefs translate_prefs(prefs); EXPECT_FALSE(translate_prefs.IsSiteBlacklisted(host)); EXPECT_TRUE(translate_prefs.CanTranslate(prefs, "fr", url)); @@ -927,16 +930,16 @@ TEST_F(TranslateManagerTest, NeverTranslateSitePref) { // There should be a translate infobar. EXPECT_TRUE(GetTranslateInfoBar() != NULL); - prefs->RemovePrefObserver(TranslatePrefs::kPrefTranslateSiteBlacklist, - &pref_observer_); } // Tests the "Always translate this language" pref. TEST_F(TranslateManagerTest, AlwaysTranslateLanguagePref) { // Select always translate French to English. PrefService* prefs = contents()->profile()->GetPrefs(); - prefs->AddPrefObserver(TranslatePrefs::kPrefTranslateWhitelists, - &pref_observer_); + PrefChangeRegistrar registrar; + registrar.Init(prefs); + registrar.Add(TranslatePrefs::kPrefTranslateWhitelists, + &pref_observer_); TranslatePrefs translate_prefs(prefs); SetPrefObserverExpectation(TranslatePrefs::kPrefTranslateWhitelists); translate_prefs.WhitelistLanguagePair("fr", "en"); @@ -985,8 +988,6 @@ TEST_F(TranslateManagerTest, AlwaysTranslateLanguagePref) { infobar = GetTranslateInfoBar(); ASSERT_TRUE(infobar != NULL); EXPECT_EQ(TranslateInfoBarDelegate::BEFORE_TRANSLATE, infobar->type()); - prefs->RemovePrefObserver(TranslatePrefs::kPrefTranslateWhitelists, - &pref_observer_); } // Context menu. diff --git a/chrome/browser/views/options/general_page_view.cc b/chrome/browser/views/options/general_page_view.cc index 830dac1..d618770 100644 --- a/chrome/browser/views/options/general_page_view.cc +++ b/chrome/browser/views/options/general_page_view.cc @@ -217,9 +217,6 @@ GeneralPageView::GeneralPageView(Profile* profile) } GeneralPageView::~GeneralPageView() { - profile()->GetPrefs()->RemovePrefObserver(prefs::kRestoreOnStartup, this); - profile()->GetPrefs()->RemovePrefObserver( - prefs::kURLsToRestoreOnStartup, this); if (startup_custom_pages_table_) startup_custom_pages_table_->SetModel(NULL); default_browser_worker_->ObserverDestroyed(); @@ -356,8 +353,9 @@ void GeneralPageView::InitControlLayout() { #endif // Register pref observers that update the controls when a pref changes. - profile()->GetPrefs()->AddPrefObserver(prefs::kRestoreOnStartup, this); - profile()->GetPrefs()->AddPrefObserver(prefs::kURLsToRestoreOnStartup, this); + registrar_.Init(profile()->GetPrefs()); + registrar_.Add(prefs::kRestoreOnStartup, this); + registrar_.Add(prefs::kURLsToRestoreOnStartup, this); new_tab_page_is_home_page_.Init(prefs::kHomePageIsNewTabPage, profile()->GetPrefs(), this); diff --git a/chrome/browser/views/options/general_page_view.h b/chrome/browser/views/options/general_page_view.h index 93a7af5..f2b126f 100644 --- a/chrome/browser/views/options/general_page_view.h +++ b/chrome/browser/views/options/general_page_view.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_VIEWS_OPTIONS_GENERAL_PAGE_VIEW_H_ #pragma once +#include "chrome/browser/prefs/pref_change_registrar.h" #include "chrome/browser/prefs/pref_member.h" #include "chrome/browser/shell_integration.h" #include "chrome/browser/views/options/options_page_view.h" @@ -156,6 +157,8 @@ class GeneralPageView : public OptionsPageView, // The helper object that performs default browser set/check tasks. scoped_refptr<ShellIntegration::DefaultBrowserWorker> default_browser_worker_; + PrefChangeRegistrar registrar_; + DISALLOW_COPY_AND_ASSIGN(GeneralPageView); }; diff --git a/chrome/test/testing_profile.cc b/chrome/test/testing_profile.cc index 6fd04fc..359f64d 100644 --- a/chrome/test/testing_profile.cc +++ b/chrome/test/testing_profile.cc @@ -16,6 +16,7 @@ #include "chrome/browser/bookmarks/bookmark_model.h" #include "chrome/browser/chrome_thread.h" #include "chrome/browser/dom_ui/ntp_resource_cache.h" +#include "chrome/browser/extensions/extensions_service.h" #include "chrome/browser/favicon_service.h" #include "chrome/browser/find_bar_state.h" #include "chrome/browser/geolocation/geolocation_content_settings_map.h" @@ -166,6 +167,7 @@ class TestExtensionURLRequestContextGetter : public URLRequestContextGetter { TestingProfile::TestingProfile() : start_time_(Time::Now()), + testing_prefs_(NULL), created_theme_provider_(false), has_history_service_(false), off_the_record_(false), @@ -207,6 +209,10 @@ TestingProfile::~TestingProfile() { if (top_sites_.get()) top_sites_->ClearProfile(); history::TopSites::DeleteTopSites(top_sites_); + if (extensions_service_.get()) { + extensions_service_->DestroyingProfile(); + extensions_service_ = NULL; + } } void TestingProfile::CreateFaviconService() { @@ -315,13 +321,26 @@ void TestingProfile::UseThemeProvider(BrowserThemeProvider* theme_provider) { theme_provider_.reset(theme_provider); } +scoped_refptr<ExtensionsService> TestingProfile::CreateExtensionsService( + const CommandLine* command_line, + const FilePath& install_directory) { + extensions_service_ = new ExtensionsService(this, + command_line, + install_directory, + false); + return extensions_service_; +} + FilePath TestingProfile::GetPath() { DCHECK(temp_dir_.IsValid()); // TODO(phajdan.jr): do it better. return temp_dir_.path(); } TestingPrefService* TestingProfile::GetTestingPrefService() { - return static_cast<TestingPrefService*>(GetPrefs()); + if (!prefs_.get()) + CreateTestingPrefService(); + DCHECK(testing_prefs_); + return testing_prefs_; } webkit_database::DatabaseTracker* TestingProfile::GetDatabaseTracker() { @@ -330,6 +349,10 @@ webkit_database::DatabaseTracker* TestingProfile::GetDatabaseTracker() { return db_tracker_; } +ExtensionsService* TestingProfile::GetExtensionsService() { + return extensions_service_.get(); +} + net::CookieMonster* TestingProfile::GetCookieMonster() { if (!GetRequestContext()) return NULL; @@ -348,11 +371,22 @@ void TestingProfile::InitThemes() { } } +void TestingProfile::SetPrefService(PrefService* prefs) { + DCHECK(!prefs_.get()); + prefs_.reset(prefs); +} + +void TestingProfile::CreateTestingPrefService() { + DCHECK(!prefs_.get()); + testing_prefs_ = new TestingPrefService(); + prefs_.reset(testing_prefs_); + Profile::RegisterUserPrefs(prefs_.get()); + browser::RegisterAllPrefs(prefs_.get(), prefs_.get()); +} + PrefService* TestingProfile::GetPrefs() { if (!prefs_.get()) { - prefs_.reset(new TestingPrefService()); - Profile::RegisterUserPrefs(prefs_.get()); - browser::RegisterAllPrefs(prefs_.get(), prefs_.get()); + CreateTestingPrefService(); } return prefs_.get(); } diff --git a/chrome/test/testing_profile.h b/chrome/test/testing_profile.h index d75b7c1..b1e466d 100644 --- a/chrome/test/testing_profile.h +++ b/chrome/test/testing_profile.h @@ -23,6 +23,7 @@ class CookieMonster; class AutocompleteClassifier; class BookmarkModel; class BrowserThemeProvider; +class CommandLine; class DesktopNotificationService; class FaviconService; class FindBarState; @@ -86,6 +87,14 @@ class TestingProfile : public Profile { // ownership of |theme_provider|. void UseThemeProvider(BrowserThemeProvider* theme_provider); + // Creates an ExtensionsService initialized with the testing profile and + // returns it. The profile keeps its own copy of a scoped_refptr to the + // ExtensionsService to make sure that is still alive to be notified when the + // profile is destroyed. + scoped_refptr<ExtensionsService> CreateExtensionsService( + const CommandLine* command_line, + const FilePath& install_directory); + TestingPrefService* GetTestingPrefService(); virtual ProfileId GetRuntimeId() { @@ -109,7 +118,7 @@ class TestingProfile : public Profile { virtual ChromeAppCacheService* GetAppCacheService() { return NULL; } virtual webkit_database::DatabaseTracker* GetDatabaseTracker(); virtual VisitedLinkMaster* GetVisitedLinkMaster() { return NULL; } - virtual ExtensionsService* GetExtensionsService() { return NULL; } + virtual ExtensionsService* GetExtensionsService(); virtual UserScriptMaster* GetUserScriptMaster() { return NULL; } virtual ExtensionDevToolsManager* GetExtensionDevToolsManager() { return NULL; @@ -148,6 +157,10 @@ class TestingProfile : public Profile { virtual PasswordStore* GetPasswordStore(ServiceAccessType access) { return NULL; } + // Initialized the profile's PrefService with an explicity specified + // PrefService. Must be called before the TestingProfile. + // The profile takes ownership of |pref|. + void SetPrefService(PrefService* prefs); virtual PrefService* GetPrefs(); virtual TemplateURLModel* GetTemplateURLModel() { return template_url_model_.get(); @@ -263,7 +276,9 @@ class TestingProfile : public Profile { protected: base::Time start_time_; - scoped_ptr<TestingPrefService> prefs_; + scoped_ptr<PrefService> prefs_; + // ref only for right type, lifecycle is managed by prefs_ + TestingPrefService* testing_prefs_; private: // Destroys favicon service if it has been created. @@ -277,6 +292,9 @@ class TestingProfile : public Profile { // from the destructor. void DestroyWebDataService(); + // Creates a TestingPrefService and associates it with the TestingProfile + void CreateTestingPrefService(); + // The favicon service. Only created if CreateFaviconService is invoked. scoped_refptr<FaviconService> favicon_service_; @@ -346,6 +364,10 @@ class TestingProfile : public Profile { FilePath last_selected_directory_; scoped_refptr<history::TopSites> top_sites_; // For history and thumbnails. + // For properly notifying the ExtensionsService when the profile + // is disposed. + scoped_refptr<ExtensionsService> extensions_service_; + // We use a temporary directory to store testing profile data. ScopedTempDir temp_dir_; }; |