CL for readability review.

[Font Settings API]: CL for readability review.


BUG=
TEST=
TBR=mpcomplete


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139746

[rpang, 2012-05-29 17:10:16]

Some initial comments on the .h file.

In general, I've made a specific comment only once even if I've seen the issue
repeated throughout the code. Please be sure to make any changes everywhere they
should be made rather than just the specific places I mention.

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
File comments?

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

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:16: class
ExtensionFontSettingsEventRouter : public content::NotificationObserver {
Class comments?

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:18: explicit
ExtensionFontSettingsEventRouter(Profile* profile);
Comment on function, esp. the parameters. What does 'profile' represent? Does it
take ownership of 'profile'? Can 'profile' be NULL?

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:24: typedef
std::pair<std::string, std::string> EventAndKeyPair;
I'm not familiar with chrome style, but STL in google3 code  uses "map",
"string" instead of "std::map", "std::string".

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:51: PrefChangeRegistrar
registrar_;
Comment on member variables.

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:66: virtual
~ClearFontFunction() {}
Comment on why the destructor is protected.

[falken, 2012-05-30 06:52:15]

Thanks for the review!

mpcomplete: Please see question inline at the end.

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2012/05/29 17:10:17, rpang wrote:
> File comments?
> 
>
http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co...

Done.

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:16: class
ExtensionFontSettingsEventRouter : public content::NotificationObserver {
On 2012/05/29 17:10:17, rpang wrote:
> Class comments?

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."

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:18: explicit
ExtensionFontSettingsEventRouter(Profile* profile);
On 2012/05/29 17:10:17, rpang wrote:
> Comment on function, esp. the parameters. What does 'profile' represent? Does
it
> take ownership of 'profile'? Can 'profile' be NULL?

Done.

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:24: typedef
std::pair<std::string, std::string> EventAndKeyPair;
On 2012/05/29 17:10:17, rpang wrote:
> I'm not familiar with chrome style, but STL in google3 code  uses "map",
> "string" instead of "std::map", "std::string".

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...

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:51: PrefChangeRegistrar
registrar_;
On 2012/05/29 17:10:17, rpang wrote:
> Comment on member variables.

Done.

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:66: virtual
~ClearFontFunction() {}
On 2012/05/29 17:10:17, rpang wrote:
> Comment on why the destructor is protected.

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

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

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:16: class
ExtensionFontSettingsEventRouter : public content::NotificationObserver {
On 2012/05/30 06:52:16, falken wrote:
> On 2012/05/29 17:10:17, rpang wrote:
> > Class comments?
> 
> 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."

This is fine.

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:24: // ownership of
|profile|. Init() must be called to start observing pref
Does |profile| need to stay alive? Can it be NULL?

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:88: // clearFont API
function.
clearFont -> ClearFont?

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

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:66: virtual
~ClearFontFunction() {}
On 2012/05/30 06:52:16, falken wrote:
> On 2012/05/29 17:10:17, rpang wrote:
> > Comment on why the destructor is protected.
> 
> Hm, I found most other extension APIs do this, but I don't know for sure why.
> mpcomplete?

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.

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

Matt, thanks for the RefCounted info.

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:24: // ownership of
|profile|. Init() must be called to start observing pref
On 2012/05/30 11:45:51, rpang wrote:
> Does |profile| need to stay alive? Can it be NULL?

Done. Added more comments.

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:88: // clearFont API
function.
On 2012/05/30 11:45:51, rpang wrote:
> clearFont -> ClearFont?

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

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

lgtm

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:88: // clearFont API
function.
On 2012/05/31 02:38:30, falken wrote:
> On 2012/05/30 11:45:51, rpang wrote:
> > clearFont -> ClearFont?
> 
> No, it means the JavaScript extension API function, which is
> "experimental.fontSettings.clearFont"

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

[falken, 2012-05-31 03:34:26]

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
File chrome/browser/extensions/extension_font_settings_api.h (right):

https://chromiumcodereview.appspot.com/10447013/diff/5/chrome/browser/extensi...
chrome/browser/extensions/extension_font_settings_api.h:88: // clearFont API
function.
On 2012/05/31 02:42:42, rpang wrote:
> On 2012/05/31 02:38:30, falken wrote:
> > On 2012/05/30 11:45:51, rpang wrote:
> > > clearFont -> ClearFont?
> > 
> > No, it means the JavaScript extension API function, which is
> > "experimental.fontSettings.clearFont"
> 
> Is it self-evident in this code base? If not, comment with the full API
function
> name?

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

[falken, 2012-05-31 03:45:04]

I'll go ahead and submit this CL now. Does the .cc file look OK too?

[commit-bot: I haz the power, 2012-05-31 03:45:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/10447013/12001

[rpang, 2012-05-31 03:59:54]

LGTM

Yes, the .cc file looks fine.

- Ruoming


On Wed, May 30, 2012 at 11:45 PM, <falken@chromium.org> wrote:

> I'll go ahead and submit this CL now. Does the .cc file look OK too?
>
>
https://chromiumcodereview.**appspot.com/10447013/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-05-31 06:49:54]

Change committed as 139746