chrome.settingsPrivate: Implement onPrefsChanged event handling.

Previously, onPrefsChanged was a no op. This CL implements functionality for notifying listeners on the event when a pref has changed.

In the process:
- pull out shared logic into a prefs util namespace.
- simplify the whitelisting logic so there is only one GetWhitelistedPrefs method and not a CrOS-specific one.

BUG=468093
Committed: https://crrev.com/a22ce7ae22a39dd3c1693a082b727a4d725fe306
Cr-Commit-Position: refs/heads/master@{#324083}

[Oren Blasberg, 2015-04-06 20:43:02]

orenb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Oren Blasberg, 2015-04-06 20:43:52]

Hey Steven,

The code is working and ready -- while I work on tests can you take a first pass
at reviewing the implementation? Thanks!

[stevenjb, 2015-04-06 22:40:59]

Nice. Looking good.

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:54: bool
IsPrefUserModifiable(Profile* profile, const std::string& pref_name) {
Order in C++ should match .h (or vice versa)

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.h (right):

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:24: const
TypedPrefMap& GetWhitelistedKeys();
All of these functions should have comments.

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:26: // Gets the
value of the pref with the given |name|.
Describe behavior if |name| is not found. (i.e. Returns a default pref object.
as opposed to a null ptr value which might be expected).

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc
(right):

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc:117:
new api::settings_private::PrefObject();
?

[Oren Blasberg, 2015-04-06 23:27:07]

Thanks! I also added a test that exercises the event router code path. I had to
write to the real PrefService to do so. I am welcome to ideas on how to improve
the test.

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:54: bool
IsPrefUserModifiable(Profile* profile, const std::string& pref_name) {
On 2015/04/06 22:40:59, stevenjb wrote:
> Order in C++ should match .h (or vice versa)
> 

Done.

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.h (right):

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:24: const
TypedPrefMap& GetWhitelistedKeys();
On 2015/04/06 22:40:59, stevenjb wrote:
> All of these functions should have comments.

Done.

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:26: // Gets the
value of the pref with the given |name|.
On 2015/04/06 22:40:59, stevenjb wrote:
> Describe behavior if |name| is not found. (i.e. Returns a default pref object.
> as opposed to a null ptr value which might be expected).
> 

Done.

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc
(right):

https://codereview.chromium.org/1061613002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc:117:
new api::settings_private::PrefObject();
On 2015/04/06 22:40:59, stevenjb wrote:
> ?

Oops. That was some leftover code. Removed.

[Oren Blasberg, 2015-04-06 23:28:16]

orenb@chromium.org changed reviewers:
+ kalman@chromium.org

[Oren Blasberg, 2015-04-06 23:29:24]

+Ben: Could you please take a look at the IDL? It's a one line change. I'd
simply made a mistake originally when writing the argument for the event -- it
should be the type of the object passed to the callback, not the callback
itself. Thanks!

[stevenjb, 2015-04-06 23:33:38]

lgtm

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:119: }
nit: no {}

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.h (right):

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:29: // empty
PrefObject if no pref is found for |name|.)
nit: No () around 'Returns...', that is a key part of the comment.

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/settings_private_apitest.cc
(right):

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/settings_private_apitest.cc:31:
pref_service->Set(name.c_str(), *value);
This seems fine since it needs to be a browser test anyway and
profile_->GetPrefs() exists.

[not at google - send to devlin, 2015-04-06 23:37:34]

lgtm

[Oren Blasberg, 2015-04-06 23:41:30]

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.cc (right):

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:119: }
On 2015/04/06 23:33:38, stevenjb wrote:
> nit: no {}

Done. (Thanks. Old habits from Javascript world die hard)

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/prefs_util.h (right):

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:29: // empty
PrefObject if no pref is found for |name|.)
On 2015/04/06 23:33:38, stevenjb wrote:
> nit: No () around 'Returns...', that is a key part of the comment.

Done.

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/settings_private/settings_private_apitest.cc
(right):

https://codereview.chromium.org/1061613002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/settings_private_apitest.cc:31:
pref_service->Set(name.c_str(), *value);
On 2015/04/06 23:33:38, stevenjb wrote:
> This seems fine since it needs to be a browser test anyway and
> profile_->GetPrefs() exists.

Acknowledged.

[Oren Blasberg, 2015-04-06 23:41:39]

The CQ bit was checked by orenb@chromium.org

[Oren Blasberg, 2015-04-06 23:41:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1061613002/#ps160001
(title: "Respond to nits")

[commit-bot: I haz the power, 2015-04-06 23:42:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061613002/160001

[commit-bot: I haz the power, 2015-04-07 02:09:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-07 02:10:01]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Oren Blasberg, 2015-04-07 18:18:44]

The CQ bit was checked by orenb@chromium.org

[Oren Blasberg, 2015-04-07 18:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1061613002/#ps180001
(title: "Fix a direct memory leak")

[commit-bot: I haz the power, 2015-04-07 18:19:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1061613002/180001

[commit-bot: I haz the power, 2015-04-07 19:10:45]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-07 19:11:41]

Patchset 10 (id:??) landed as
https://crrev.com/a22ce7ae22a39dd3c1693a082b727a4d725fe306
Cr-Commit-Position: refs/heads/master@{#324083}