CL for readability review.

chrome/browser/extensions/extension_font_settings_api.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

[rpang, 2012/05/29 17:10:17]

File comments?

http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co...

[falken, 2012/05/30 06:52:16]

Done.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 
 class ExtensionFontSettingsEventRouter : public content::NotificationObserver {

[rpang, 2012/05/29 17:10:17]

Class comments?

[falken, 2012/05/30 06:52:16]

Done. What do you recommend for the group of similar classes at the end of the
file? It seems a little redundant to put an comment on each. I put a single
comment "The following are get/set/clear API functions that act on a brower
pref."

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

This is fine.

  public:
   explicit ExtensionFontSettingsEventRouter(Profile* profile);

[rpang, 2012/05/29 17:10:17]

Comment on function, esp. the parameters. What does 'profile' represent? Does it
take ownership of 'profile'? Can 'profile' be NULL?

[falken, 2012/05/30 06:52:16]

Done.

   virtual ~ExtensionFontSettingsEventRouter();
 
   void Init();
 
  private:
   typedef std::pair<std::string, std::string> EventAndKeyPair;

[rpang, 2012/05/29 17:10:17]

I'm not familiar with chrome style, but STL in google3 code  uses "map",
"string" instead of "std::map", "std::string".

[falken, 2012/05/30 06:52:16]

Chrome code seems to consistently use std::. This post explains more:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/qSrl_UAqAkQ/Cak1f...

   // Map of pref name to a pair of event name and key (defined in the API) for
   // dispatching a pref changed event. For example,
   // "webkit.webprefs.default_font_size" to ("onDefaultFontSizedChanged",
   // "pixelSize").
   typedef std::map<std::string, EventAndKeyPair> PrefEventMap;
 
   void AddPrefToObserve(const char* pref_name,
                         const char* event_name,
                         const char* key);
 
   // content::NotificationObserver implementation.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
   void OnFontNamePrefChanged(PrefService* pref_service,
                              const std::string& pref_name,
                              const std::string& generic_family,
                              const std::string& script,
                              bool incognito);
   void OnFontPrefChanged(PrefService* pref_service,
                          const std::string& pref_name,
                          const std::string& event_name,
                          const std::string& key,
                          bool incognito);
 
   PrefChangeRegistrar registrar_;

[rpang, 2012/05/29 17:10:17]

Comment on member variables.

[falken, 2012/05/30 06:52:16]

Done.

 
   PrefEventMap pref_event_map_;
 
   // Weak, owns us (transitively via ExtensionService).
   Profile* profile_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionFontSettingsEventRouter);
 };
 
 class ClearFontFunction : public SyncExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.fontSettings.clearFont")
 
  protected:
   virtual ~ClearFontFunction() {}

[rpang, 2012/05/29 17:10:17]

Comment on why the destructor is protected.

[falken, 2012/05/30 06:52:16]

Hm, I found most other extension APIs do this, but I don't know for sure why.
mpcomplete?

[Matt Perry, 2012/05/30 18:39:05]

This is from http://codereview.chromium.org/10071035 - Chrome coding policy is
now to make destructors non-public for all RefCounted types. Since it's a global
policy, I don't think we need to comment on each instance of it.

 
   // ExtensionFunction:
   virtual bool RunImpl() OVERRIDE;
 };
 
 class GetFontFunction : public SyncExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION_NAME("experimental.fontSettings.getFont")
 
  protected:
@@ skipping 223 matching lines @@
 
  protected:
   virtual ~SetDefaultCharacterSetFunction() {}
 
   // SetFontPrefExtensionFunction:
   virtual const char* GetPrefName() OVERRIDE;
   virtual const char* GetKey() OVERRIDE;
 };
 
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_FONT_SETTINGS_API_H__