Update chrome.settingsPrivate to support CrOS-only settings.

Update chrome.settingsPrivate to support CrOS-only settings. These needed special handling since they are managed by the CrOSSettings rather than by one of the PrefService objects.

I also added a few such settings as used by the users page, to the prefs whitelist, and removed some dead code from the apitest.

BUG=496308
Committed: https://crrev.com/f13e590ff96fc4a53f42f5549c0ed3d0872ab68d
Cr-Commit-Position: refs/heads/master@{#333180}

[Oren Blasberg, 2015-06-03 20:38:59]

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

[Oren Blasberg, 2015-06-04 17:16:00]

I cleaned up the api test a bit too, as the TestDelegate wasn't really serving
any purpose as far as I could tell.

[Oren Blasberg, 2015-06-04 20:17:50]

As discussed via email, I'm going to make further changes so that PrefsUtil may
be used -- and stubbed out -- elsewhere, as in usersPrivate's implementation. 

For now, please hold off on reviewing and I'll reply again when it's ready.
Thanks!

[Oren Blasberg, 2015-06-04 22:02:37]

On 2015/06/04 20:17:50, Oren Blasberg wrote:
> As discussed via email, I'm going to make further changes so that PrefsUtil
may
> be used -- and stubbed out -- elsewhere, as in usersPrivate's implementation. 
> 
> For now, please hold off on reviewing and I'll reply again when it's ready.
> Thanks!

Ok, PrefsUtil is now a class with virtual public methods. In the follow-up CL
for usersPrivate, I'll add a stub implementation which usersPrivate may use in
its test, along with AppendToList and RemoveFromList methods on the pref util
interface.

[Oren Blasberg, 2015-06-04 22:03:28]

On 2015/06/04 22:02:37, Oren Blasberg wrote:
> On 2015/06/04 20:17:50, Oren Blasberg wrote:
> > As discussed via email, I'm going to make further changes so that PrefsUtil
> may
> > be used -- and stubbed out -- elsewhere, as in usersPrivate's
implementation. 
> > 
> > For now, please hold off on reviewing and I'll reply again when it's ready.
> > Thanks!

 
Ok, PrefsUtil is now a class with virtual public methods. In the follow-up CL
for usersPrivate, I'll add a stub implementation which usersPrivate may use in
its test, along with AppendToList and RemoveFromList methods on the pref util
interface.

This one should be ready now; PTAL

[stevenjb, 2015-06-05 02:23:50]

This is looking great, thanks.

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

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:98: CrosSettings*
cros_settings = CrosSettings::Get();
nit: Only used once, no need for local

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

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:20: using
TypedPrefMap = std::map<std::string, api::settings_private::PrefType>;
put this inside the class

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:25: virtual
~PrefsUtil() {}
We should avoid inlining virtual methods, even trivial destructors, except in
pure interface classes.

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:53: const
std::string& name, base::Value::Type type);
WS between all methods

[Oren Blasberg, 2015-06-05 20:12:22]

Thanks! PTAL.

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

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:98: CrosSettings*
cros_settings = CrosSettings::Get();
On 2015/06/05 02:23:50, stevenjb wrote:
> nit: Only used once, no need for local

Done.

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

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:20: using
TypedPrefMap = std::map<std::string, api::settings_private::PrefType>;
On 2015/06/05 02:23:50, stevenjb wrote:
> put this inside the class

Done.

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:25: virtual
~PrefsUtil() {}
On 2015/06/05 02:23:50, stevenjb wrote:
> We should avoid inlining virtual methods, even trivial destructors, except in
> pure interface classes.

Done.

https://codereview.chromium.org/1163593005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.h:53: const
std::string& name, base::Value::Type type);
On 2015/06/05 02:23:50, stevenjb wrote:
> WS between all methods

Done.

[stevenjb, 2015-06-05 20:31:11]

lgtm

[Oren Blasberg, 2015-06-05 20:33:36]

The CQ bit was checked by orenb@chromium.org

[commit-bot: I haz the power, 2015-06-05 20:37:35]

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

[commit-bot: I haz the power, 2015-06-05 20:59:26]

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

[commit-bot: I haz the power, 2015-06-05 20:59:27]

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

[Oren Blasberg, 2015-06-05 21:18:26]

The CQ bit was checked by orenb@chromium.org

[Oren Blasberg, 2015-06-05 21:18:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1163593005/#ps240001
(title: "Sync/merge")

[commit-bot: I haz the power, 2015-06-05 21:19:13]

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

[commit-bot: I haz the power, 2015-06-05 21:38:10]

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

[commit-bot: I haz the power, 2015-06-05 21:38:11]

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

[Oren Blasberg, 2015-06-05 22:49:44]

The CQ bit was checked by orenb@chromium.org

[Oren Blasberg, 2015-06-05 22:49:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1163593005/#ps260001
(title: "Fix issues preventing compilation on non-CrOS builds.")

[commit-bot: I haz the power, 2015-06-05 22:50:12]

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

[stevenjb, 2015-06-05 22:59:15]

Still lgtm, just a tiny entirely optional nit/FYI.

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

https://codereview.chromium.org/1163593005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc:104:
#endif
nit: This could be written as follows to avoid an empty if block for non
chromeos.

#if defined(OS_CHROMEOS)
  if (prefs_util_->IsCrosSetting(pref_name)) {
    ...
    continue;
  }
#endif
  FindRegistrarForPref(it.first)...

[commit-bot: I haz the power, 2015-06-05 23:55:16]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-06-05 23:56:30]

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

[Oren Blasberg, 2015-06-06 00:42:55]

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

https://codereview.chromium.org/1163593005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:112:
pref_object->type = GetType(name, value->GetType());
Somehow, this line segfaults if name is 'cros.accounts.users' (and if we also
add cros.accounts.users to the whitelist, of course).  According to gdb, the
value of type_ is 0x8 (a pointer to 0x8.... umm) Very weird :\

[stevenjb, 2015-06-06 00:48:17]

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

https://codereview.chromium.org/1163593005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:112:
pref_object->type = GetType(name, value->GetType());
On 2015/06/06 00:42:55, Oren Blasberg wrote:
> Somehow, this line segfaults if name is 'cros.accounts.users' (and if we also
> add cros.accounts.users to the whitelist, of course).  According to gdb, the
> value of type_ is 0x8 (a pointer to 0x8.... umm) Very weird :\

Is value maybe NULL?

[Oren Blasberg, 2015-06-06 01:02:11]

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

https://codereview.chromium.org/1163593005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/settings_private/prefs_util.cc:112:
pref_object->type = GetType(name, value->GetType());
On 2015/06/06 00:48:17, stevenjb wrote:
> On 2015/06/06 00:42:55, Oren Blasberg wrote:
> > Somehow, this line segfaults if name is 'cros.accounts.users' (and if we
also
> > add cros.accounts.users to the whitelist, of course).  According to gdb, the
> > value of type_ is 0x8 (a pointer to 0x8.... umm) Very weird :\
> 
> Is value maybe NULL?

Whoops, you're right, it's null.
However, I don't get why. I'm basically doing the same thing as what
accounts_options_handler.cc does [1], except that file is going through
GetList() which calls GetPref, and I'm just calling GetPref directly. GetList
calls "GetAsList" on the value but if the value's null that wouldn't make a
difference.

Maybe some sort of timing/lifetime issue.. basically since we load all prefs on
startup, adding 'cros.users.accounts' to the prefs_util.cc whitelist causes
md-settings to crash on load.

Hopefully the clarity of Monday morning will bring fresh insight.


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...