Removing PrefObserver usage, batch 4.

Removing PrefObserver usage, batch 4.

TBR=estade@chromium.org,aa@chromium.org
BUG=155525
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167704

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

Thanks for your work on this, I like the way how code becomes simpler in many
instances!

Regarding font_settings_api.cc, you might want to loop in the original author in
case I misunderstand the code.

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:131: callback);
All these could bind directly to the second half of OnPreferenceChanged
(extracting that to a separate function).

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:142:
pref_event_map_[pref_name] = std::make_pair(event_name, key);
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]

falken: Please see discussion around font_settings_api.cc; for context, we are
switching PrefChangeRegistrar and PrefMember to use a base::Callback for
notifications that preferences have changed.

Cheers,
Jói

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:131: callback);
On 2012/11/13 15:09:49, Mattias Nissler wrote:
> All these could bind directly to the second half of OnPreferenceChanged
> (extracting that to a separate function).

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.

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:142:
pref_event_map_[pref_name] = std::make_pair(event_name, key);
On 2012/11/13 15:09:49, Mattias Nissler wrote:
> 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.

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:30]

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/font_settings/font_settings_api.cc (right):

http://codereview.chromium.org/11377131/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/font_settings/font_settings_api.cc:142:
pref_event_map_[pref_name] = std::make_pair(event_name, key);
On 2012/11/13 15:16:45, Jói wrote:
> On 2012/11/13 15:09:49, Mattias Nissler wrote:
> > 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.
> 
> I think you're probably right.  Am looping in falken@ for feedback, who seems
to
> have written the OnFont(Name)PrefChanged methods.

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.

[Jói, 2012-11-14 13:03:27]

PTAL, I hope I did this right.

Note that I completely removed the |incognito| parameters, as they were always
set to false (there was a DCHECK enforcing this).

Cheers,
Jói

[Mattias Nissler (ping if slow), 2012-11-14 13:35:44]

What you did seems right to me, so LGTM from my side. However, I defer to falken
to check the font pref logic.

[Jói, 2012-11-14 13:40:18]

Thanks, I plan to wait for feedback from falken@ before committing.

Cheers,
Jói


On Wed, Nov 14, 2012 at 1:35 PM,  <mnissler@chromium.org> wrote:
> What you did seems right to me, so LGTM from my side. However, I defer to
> falken
> to check the font pref logic.
>
> https://codereview.chromium.org/11377131/

[falken, 2012-11-14 14:09:24]

font settings lgtm

I like how the code gets simpler!

http://codereview.chromium.org/11377131/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/font_settings/font_settings_api.h (right):

http://codereview.chromium.org/11377131/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.h:13: #include
<utility>
Can we get rid of <map> and <utility> now?

http://codereview.chromium.org/11377131/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.h:41: void
OnFontFamilyPrefChanged(const std::string& pref_name);
How about "OnFontFamilyMapPrefChanged"?

It's a bit confusing to have the functions named OnFontFamilyPrefChanged vs.
OnFontNamePrefChanged. There is not really a difference between a font family
and a font name (it's probably my fault for not being consistent).

[Jói, 2012-11-14 14:40:21]

Thanks!

http://codereview.chromium.org/11377131/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/font_settings/font_settings_api.h (right):

http://codereview.chromium.org/11377131/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.h:13: #include
<utility>
On 2012/11/14 14:09:24, falken wrote:
> Can we get rid of <map> and <utility> now?

Done.

http://codereview.chromium.org/11377131/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/font_settings/font_settings_api.h:41: void
OnFontFamilyPrefChanged(const std::string& pref_name);
On 2012/11/14 14:09:24, falken wrote:
> How about "OnFontFamilyMapPrefChanged"?
> 
> It's a bit confusing to have the functions named OnFontFamilyPrefChanged vs.
> OnFontNamePrefChanged. There is not really a difference between a font family
> and a font name (it's probably my fault for not being consistent).

Done.

[commit-bot: I haz the power, 2012-11-14 16:42:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11377131/8003

[commit-bot: I haz the power, 2012-11-14 16:42:47]

Presubmit check for 11377131-8003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/media_gallery
    chrome/browser/extensions

Presubmit checks took 1.6s to calculate.

[Jói, 2012-11-14 16:53:47]

TBR=estade@chromium.org,aa@chromium.org for OWNERS stamps for these directories:
    chrome/browser/media_gallery
    chrome/browser/extensions

This is for an API usage update, and already reviewed by mnissler@ who is an
OWNER for the API being changed.  Also, falken@ reviewed the
chrome/browser/extensions/api/font_settings/font_settings_api changes which we
were slightly unsure of.

Cheers,
Jói

[commit-bot: I haz the power, 2012-11-14 16:53:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/11377131/8003

[commit-bot: I haz the power, 2012-11-14 18:36:05]

Change committed as 167704