CL for readability review.

chrome/browser/extensions/extension_font_settings_api.h

Index: chrome/browser/extensions/extension_font_settings_api.h
diff --git a/chrome/browser/extensions/extension_font_settings_api.h b/chrome/browser/extensions/extension_font_settings_api.h
index ba9697378da03a70c7c606f92c01ef1b74036b64..3761c2798d5aa69583535a32988b611103cb7659 100644
--- a/chrome/browser/extensions/extension_font_settings_api.h
+++ b/chrome/browser/extensions/extension_font_settings_api.h
@@ -2,30 +2,45 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+// Defines the classes to realize the Font Settings Extension API as specified
+// in the extension API JSON.
+
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_FONT_SETTINGS_API_H__
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_FONT_SETTINGS_API_H__
 #pragma once
 
 #include <map>
+#include <string>
+#include <utility>
 
 #include "chrome/browser/extensions/extension_function.h"
 #include "chrome/browser/prefs/pref_change_registrar.h"
 
+// This class observes pref changed events on a profile and dispatches the
+// corresponding extension API events to extensions.
 class ExtensionFontSettingsEventRouter : public content::NotificationObserver {
  public:
+  // Constructor for observing pref changed events on |profile|. Does not take
+  // ownership of |profile|. Init() must be called to start observing pref

[rpang, 2012/05/30 11:45:51]

Does |profile| need to stay alive? Can it be NULL?

[falken, 2012/05/31 02:38:30]

Done. Added more comments.

+  // changed events.
   explicit ExtensionFontSettingsEventRouter(Profile* profile);
   virtual ~ExtensionFontSettingsEventRouter();
 
+  // Starts observing pref changed events on the profile. Must not be called
+  // more than once.
   void Init();
 
  private:
   typedef std::pair<std::string, std::string> EventAndKeyPair;
-  // Map of pref name to a pair of event name and key (defined in the API) for
-  // dispatching a pref changed event. For example,
+  // Map of pref name to a pair of event name and key (defined in the extension
+  // API) for dispatching a pref changed event. For example,
   // "webkit.webprefs.default_font_size" to ("onDefaultFontSizedChanged",
   // "pixelSize").
   typedef std::map<std::string, EventAndKeyPair> PrefEventMap;
 
+  // Observes browser pref |pref_name|. When a change is observed, dispatches
+  // event |event_name| to extensions. A JavaScript object is passed to the
+  // extension event function with the new value of the pref in property |key|.
   void AddPrefToObserve(const char* pref_name,
                         const char* event_name,
                         const char* key);
@@ -35,19 +50,33 @@ class ExtensionFontSettingsEventRouter : public content::NotificationObserver {
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
+  // Dispatches a changed event for the font setting for |generic_family| and
+  // |script| to extensions. The new value of the setting is the value of
+  // browser pref |pref_name|. If the pref changed on the incognito profile,
+  // |incognito| must be set to true for extensions to get the appropriate
+  // event.
   void OnFontNamePrefChanged(PrefService* pref_service,
                              const std::string& pref_name,
                              const std::string& generic_family,
                              const std::string& script,
                              bool incognito);
+
+  // Dispatches the setting changed event |event_name| to extensions. The new
+  // value of the setting is the value of browser pref |pref_name|. This value
+  // is passed in the JavaScript object argument to the extension event function
+  // under the key |key|. If the pref changed on the incognito profile,
+  // |incognito| must be set to true for extensions to get the appropriate
+  // event.
   void OnFontPrefChanged(PrefService* pref_service,
                          const std::string& pref_name,
                          const std::string& event_name,
                          const std::string& key,
                          bool incognito);
 
+  // Manages pref observation registration.
   PrefChangeRegistrar registrar_;
 
+  // Maps browser pref names to extension API <event name, key> pairs.
   PrefEventMap pref_event_map_;
 
   // Weak, owns us (transitively via ExtensionService).
@@ -56,6 +85,7 @@ class ExtensionFontSettingsEventRouter : public content::NotificationObserver {
   DISALLOW_COPY_AND_ASSIGN(ExtensionFontSettingsEventRouter);
 };
 
+// clearFont API function.

[rpang, 2012/05/30 11:45:51]

clearFont -> ClearFont?

[falken, 2012/05/31 02:38:30]

No, it means the JavaScript extension API function, which is
"experimental.fontSettings.clearFont"

[rpang, 2012/05/31 02:42:42]

Is it self-evident in this code base? If not, comment with the full API function
name?

[falken, 2012/05/31 03:34:27]

I think it should be OK, but I changed to fontSettings.clearFont to make it more
obvious.

 class ClearFontFunction : public SyncExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.fontSettings.clearFont")
@@ -67,6 +97,7 @@ class ClearFontFunction : public SyncExtensionFunction {
   virtual bool RunImpl() OVERRIDE;
 };
 
+// getFont API function.
 class GetFontFunction : public SyncExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.fontSettings.getFont")
@@ -78,6 +109,7 @@ class GetFontFunction : public SyncExtensionFunction {
   virtual bool RunImpl() OVERRIDE;
 };
 
+// setFont API function.
 class SetFontFunction : public SyncExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.fontSettings.setFont")
@@ -89,6 +121,7 @@ class SetFontFunction : public SyncExtensionFunction {
   virtual bool RunImpl() OVERRIDE;
 };
 
+// getFontList API function.
 class GetFontListFunction : public AsyncExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.fontSettings.getFontList")
@@ -104,7 +137,7 @@ class GetFontListFunction : public AsyncExtensionFunction {
   bool CopyFontsToResult(base::ListValue* fonts);
 };
 
-// Base class for functions that clear a font pref.
+// Base class for extension API functions that clear a browser font pref.
 class ClearFontPrefExtensionFunction : public SyncExtensionFunction {
  protected:
   virtual ~ClearFontPrefExtensionFunction() {}
@@ -117,7 +150,7 @@ class ClearFontPrefExtensionFunction : public SyncExtensionFunction {
   virtual const char* GetPrefName() = 0;
 };
 
-// Base class for functions that get a font pref.
+// Base class for extension API functions that get a browser font pref.
 class GetFontPrefExtensionFunction : public SyncExtensionFunction {
  protected:
   virtual ~GetFontPrefExtensionFunction() {}
@@ -134,7 +167,7 @@ class GetFontPrefExtensionFunction : public SyncExtensionFunction {
   virtual const char* GetKey() = 0;
 };
 
-// Base class for functions that set a font pref.
+// Base class for extension API functions that set a browser font pref.
 class SetFontPrefExtensionFunction : public SyncExtensionFunction {
  protected:
   virtual ~SetFontPrefExtensionFunction() {}
@@ -151,6 +184,9 @@ class SetFontPrefExtensionFunction : public SyncExtensionFunction {
   virtual const char* GetKey() = 0;
 };
 
+// The following are get/set/clear API functions that act on a browser font
+// pref.
+
 class ClearDefaultFontSizeFunction : public ClearFontPrefExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION_NAME(