Removing PrefObserver usage, batch 4.

chrome/browser/extensions/api/font_settings/font_settings_api.cc

Index: chrome/browser/extensions/api/font_settings/font_settings_api.cc
diff --git a/chrome/browser/extensions/api/font_settings/font_settings_api.cc b/chrome/browser/extensions/api/font_settings/font_settings_api.cc
index a3ec6519a431379c1b2e8828a5eec13a18deede2..8d425e36c632e4dfe3c8132ea1c67e113b218634 100644
--- a/chrome/browser/extensions/api/font_settings/font_settings_api.cc
+++ b/chrome/browser/extensions/api/font_settings/font_settings_api.cc
@@ -85,13 +85,14 @@ std::string MaybeGetLocalizedFontName(const std::string& font_name) {
 }
 
 // Registers |obs| to observe per-script font prefs under the path |map_name|.
-void RegisterFontFamilyMapObserver(PrefChangeRegistrar* registrar,
-                                   const char* map_name,
-                                   PrefObserver* obs) {
+void RegisterFontFamilyMapObserver(
+    PrefChangeRegistrar* registrar,
+    const char* map_name,
+    const PrefChangeRegistrar::NamedChangeCallback& callback) {
   for (size_t i = 0; i < prefs::kWebKitScriptsForFontFamilyMapsLength; ++i) {
     const char* script = prefs::kWebKitScriptsForFontFamilyMaps[i];
     std::string pref_name = base::StringPrintf("%s.%s", map_name, script);
-    registrar->Add(pref_name.c_str(), obs);
+    registrar->Add(pref_name.c_str(), callback);
   }
 }
 
@@ -111,20 +112,23 @@ FontSettingsEventRouter::FontSettingsEventRouter(
                    kOnMinimumFontSizeChanged,
                    kPixelSizeKey);
 
+  PrefChangeRegistrar::NamedChangeCallback callback = base::Bind(
+      &FontSettingsEventRouter::OnPreferenceChanged, base::Unretained(this));
   RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitStandardFontFamilyMap, this);
+                                prefs::kWebKitStandardFontFamilyMap, callback);
   RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitSerifFontFamilyMap, this);
+                                prefs::kWebKitSerifFontFamilyMap, callback);
   RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitSansSerifFontFamilyMap, this);
+                                prefs::kWebKitSansSerifFontFamilyMap, callback);
   RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitFixedFontFamilyMap, this);
+                                prefs::kWebKitFixedFontFamilyMap, callback);
   RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitCursiveFontFamilyMap, this);
+                                prefs::kWebKitCursiveFontFamilyMap, callback);
   RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitFantasyFontFamilyMap, this);
+                                prefs::kWebKitFantasyFontFamilyMap, callback);
   RegisterFontFamilyMapObserver(&registrar_,
-                                prefs::kWebKitPictographFontFamilyMap, this);
+                                prefs::kWebKitPictographFontFamilyMap,
+                                callback);

[Mattias Nissler (ping if slow), 2012/11/13 15:09:49]

All these could bind directly to the second half of OnPreferenceChanged
(extracting that to a separate function).

[Jói, 2012/11/13 15:16:45]

They could, but we would lose the DCHECK at the end, which I think is a useful
check to ensure that you don't add stuff to observe without adding handling for
it at the same time.

 }
 
 FontSettingsEventRouter::~FontSettingsEventRouter() {}
@@ -132,14 +136,15 @@ FontSettingsEventRouter::~FontSettingsEventRouter() {}
 void FontSettingsEventRouter::AddPrefToObserve(const char* pref_name,
                                                const char* event_name,
                                                const char* key) {
-  registrar_.Add(pref_name, this);
+  registrar_.Add(pref_name,
+                 base::Bind(&FontSettingsEventRouter::OnPreferenceChanged,
+                            base::Unretained(this)));
   pref_event_map_[pref_name] = std::make_pair(event_name, key);

[Mattias Nissler (ping if slow), 2012/11/13 15:09:49]

it seems weird to keep this separate map and the lookup in the callback if we
could just bind to OnFontNamePrefChanged with |event_name| and |key| bound.
Memory-wise it should be OK I hope given that the strings are already stored in
a map, we'd just be moving them into the closures.

[Jói, 2012/11/13 15:16:45]

I think you're probably right.  Am looping in falken@ for feedback, who seems to
have written the OnFont(Name)PrefChanged methods.

[falken, 2012/11/14 03:35:31]

I think it's a good idea to bind directly to OnFontPrefChanged with |event_name|
and |key| bound. This would avoid the need for pref_event_map_.

On the other hand, I'm not sure if we want to bind directly to
OnFontNamePrefChanged for the font name prefs. There are about 1000 font name
prefs so that would create 1000 callbacks. Note that these font name strings
aren't stored in a map; they are parsed out by ParseFontNamePrefPath.

 }
 
 void FontSettingsEventRouter::OnPreferenceChanged(
-    PrefServiceBase* pref_service,
     const std::string& pref_name) {
-  bool incognito = (pref_service != profile_->GetPrefs());
+  bool incognito = (registrar_.prefs() != profile_->GetPrefs());
   // We're only observing pref changes on the regular profile.
   DCHECK(!incognito);
 
@@ -147,7 +152,7 @@ void FontSettingsEventRouter::OnPreferenceChanged(
   if (iter != pref_event_map_.end()) {
     const std::string& event_name = iter->second.first;
     const std::string& key = iter->second.second;
-    OnFontPrefChanged(pref_service, pref_name, event_name, key, incognito);
+    OnFontPrefChanged(pref_name, event_name, key, incognito);
     return;
   }
 
@@ -155,8 +160,7 @@ void FontSettingsEventRouter::OnPreferenceChanged(
   std::string script;
   if (pref_names_util::ParseFontNamePrefPath(pref_name, &generic_family,
                                              &script)) {
-    OnFontNamePrefChanged(pref_service, pref_name, generic_family, script,
-                          incognito);
+    OnFontNamePrefChanged(pref_name, generic_family, script, incognito);
     return;
   }
 
@@ -164,12 +168,11 @@ void FontSettingsEventRouter::OnPreferenceChanged(
 }
 
 void FontSettingsEventRouter::OnFontNamePrefChanged(
-    PrefServiceBase* pref_service,
     const std::string& pref_name,
     const std::string& generic_family,
     const std::string& script,
     bool incognito) {
-  const PrefServiceBase::Preference* pref = pref_service->FindPreference(
+  const PrefServiceBase::Preference* pref = registrar_.prefs()->FindPreference(
       pref_name.c_str());
   CHECK(pref);
 
@@ -197,12 +200,11 @@ void FontSettingsEventRouter::OnFontNamePrefChanged(
 }
 
 void FontSettingsEventRouter::OnFontPrefChanged(
-    PrefServiceBase* pref_service,
     const std::string& pref_name,
     const std::string& event_name,
     const std::string& key,
     bool incognito) {
-  const PrefServiceBase::Preference* pref = pref_service->FindPreference(
+  const PrefServiceBase::Preference* pref = registrar_.prefs()->FindPreference(
       pref_name.c_str());
   CHECK(pref);