Use a schema to decode 3rd party policy on windows, when present.

Use a schema to decode 3rd party policy on windows, when present.

BUG=108994
TEST=unit_tests green


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

[Joao da Silva, 2012-06-26 16:47:13]

Here's the other half. Please review, thanks!

[Mattias Nissler (ping if slow), 2012-06-27 12:07:17]

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/configur...
File chrome/browser/policy/configuration_policy_provider_test.cc (right):

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_provider_test.cc:278: for (int i = 0;
i < 5; ++i) {
two should be enough?

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:299: 
what about TYPE_LIST and TYPE_INTEGER? Should they have conversion handlers here
as well?

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:300: default:
I'd list the remaining enum constants here and remove the default branch in
favor of a NOTREACHED() below.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:319: default:
same comment regarding remaining constants, default branch and NOTREACHED().

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:424: const base::DictionaryValue*
sub_schema = GetEntry(schema, name);
What about the case of a dictionary with arbitrary keys but a common type for
all entries? Content settings could use that.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:427: ReadComponentSimpleValue(&key,
name16, it.Type(), type);
Why can't this be an encoded dictionary or list?

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win_unittest.cc:129: return "{ \"type\":
\"null\" }";
It might be more readable if you were to build this as a base::Value structure
and then serialize it. Not sure though.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win_unittest.cc:501: 
It would be good to have a test case that cover reading JSON-encoded
dictionaries (and maybe lists) as well.

[Joao da Silva, 2012-06-27 14:31:15]

Please have another look

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/configur...
File chrome/browser/policy/configuration_policy_provider_test.cc (right):

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/configur...
chrome/browser/policy/configuration_policy_provider_test.cc:278: for (int i = 0;
i < 5; ++i) {
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> two should be enough?

Done.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:299: 
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> what about TYPE_LIST and TYPE_INTEGER? Should they have conversion handlers
here
> as well?

Sure, done. Also added a conversion to TYPE_BOOLEAN.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:300: default:
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> I'd list the remaining enum constants here and remove the default branch in
> favor of a NOTREACHED() below.

Done.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:319: default:
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> same comment regarding remaining constants, default branch and NOTREACHED().

Done.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:424: const base::DictionaryValue*
sub_schema = GetEntry(schema, name);
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> What about the case of a dictionary with arbitrary keys but a common type for
> all entries? Content settings could use that.

Great idea, and json-schema even has something for that case
(http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.4). Done.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win.cc:427: ReadComponentSimpleValue(&key,
name16, it.Type(), type);
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> Why can't this be an encoded dictionary or list?

Because we're using a RegistryValueIterator, so this can only be a REG_SZ or
REG_DWORD, not a subkey. ReadComponentSimpleValue() will decode a REG_SZ as a
dictionary or list, if the schema says so.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win_unittest.cc:129: return "{ \"type\":
\"null\" }";
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> It might be more readable if you were to build this as a base::Value structure
> and then serialize it. Not sure though.

Good idea, done.

http://codereview.chromium.org/10656046/diff/1/chrome/browser/policy/policy_l...
chrome/browser/policy/policy_loader_win_unittest.cc:501: 
On 2012/06/27 12:07:17, Mattias Nissler wrote:
> It would be good to have a test case that cover reading JSON-encoded
> dictionaries (and maybe lists) as well.

Done.

[Mattias Nissler (ping if slow), 2012-06-27 14:52:10]

LGTM w/ nits

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider_test.h:145: protected:
indentation

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
chrome/browser/policy/policy_loader_win.cc:308: if (type ==
base::Value::TYPE_BOOLEAN)
int: I would put these two into separate case branches, given that that solution
is easier to read and insignificantly more code.

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
chrome/browser/policy/policy_loader_win_unittest.cc:52: const char kItems[] =
"items";
nit: You could convert these to static members in PolicyLoaderWin to avoid the
duplicate constants.

[Joao da Silva, 2012-06-27 16:55:28]

PTAL

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_provider_test.h (right):

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_provider_test.h:145: protected:
On 2012/06/27 14:52:10, Mattias Nissler wrote:
> indentation

Done.

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_loader_win.cc (right):

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
chrome/browser/policy/policy_loader_win.cc:308: if (type ==
base::Value::TYPE_BOOLEAN)
On 2012/06/27 14:52:10, Mattias Nissler wrote:
> int: I would put these two into separate case branches, given that that
solution
> is easier to read and insignificantly more code.

Done.

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
chrome/browser/policy/policy_loader_win_unittest.cc:52: const char kItems[] =
"items";
On 2012/06/27 14:52:10, Mattias Nissler wrote:
> nit: You could convert these to static members in PolicyLoaderWin to avoid the
> duplicate constants.

I've added them to a namespace within ::policy, so that I can just "using
namespace X" to import them instead of having to qualify everything with
PolicyLoaderWin::. WDYT?

[Mattias Nissler (ping if slow), 2012-06-28 10:00:28]

Still LGTM.

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
File chrome/browser/policy/policy_loader_win_unittest.cc (right):

http://codereview.chromium.org/10656046/diff/10001/chrome/browser/policy/poli...
chrome/browser/policy/policy_loader_win_unittest.cc:52: const char kItems[] =
"items";
On 2012/06/27 16:55:28, Joao da Silva wrote:
> On 2012/06/27 14:52:10, Mattias Nissler wrote:
> > nit: You could convert these to static members in PolicyLoaderWin to avoid
the
> > duplicate constants.
> 
> I've added them to a namespace within ::policy, so that I can just "using
> namespace X" to import them instead of having to qualify everything with
> PolicyLoaderWin::. WDYT?

I think that's a fair solution. I'm not sure whether you noted, but there's also
chrome/common/json_schema_validator.h, and some of the strings are duplicated in
there. The solution to that would be to create file that declares the json
schema constants, e.g. chrome/common/json_schema_constants.h.

[commit-bot: I haz the power, 2012-06-28 10:06:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/10656046/11009

[commit-bot: I haz the power, 2012-06-28 11:22:08]

Change committed as 144703