Implementation of ONC signature, validator and normalizer.

This CL provides a general ONC signature which is used for validation and normalization.

ONCValidator checks the existence of fields and types and may be extended further checks (e.g. format of IPAddresses or URLs can be checked there too). It supports removing unknown fields.
ONCNormalizer removes ignored/unnecessary fields.

This new code will not be in use, yet, but prepares for further changes to the network settings.

About the code organization:
My idea was to put all network settings related stuff into chromeos/network_settings and thus giving up on the monolithic NetworkLibrary.

In upcoming CLs, I will provide an ONC merger and a translator between ONC and Shill which will also use this signature (the Shill keys are already part of the signature).

BUG=chromium:148905
TBR=ben@chromium.org

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

[pneubeck (no reviews), 2012-09-18 12:17:38]

Please have an overview of the CL. Thanks.

[Mattias Nissler (ping if slow), 2012-09-18 16:29:08]

The last paragraph in the CL description should probably have been on the review
comment thread, please remove so it doesn't get committed.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_merger.cc (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:8: #include <string>
newline

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:19:
base::DictionaryValue* Merge(const base::DictionaryValue* user,
should return a scoped_ptr<base::DictionaryValue>

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:20: const
base::DictionaryValue* policy) {
This function is too damn long. Please break into more manageable pieces.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:26: return
Merge(&emptyDictionary, policy);
so you're making a recursive call? Why not just user = &emptyDictionary;

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:117: }
Algorithm-wise, wouldn't it be simpler to start with a copy of the user policy
and then overlay policy, making recursive calls for dictionaries?

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_merger.h (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:8: #include <string>
newline

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:13: }  // namespace base
namespace comment not necessary here per convention

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:18: class ONCMerger {
if you don't intend on adding state, this should become namespace onc_merger

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:20: static
base::DictionaryValue* MergeUserSettingsWithPolicy(
could use a description comment

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_merger_unittest.cc (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger_unittest.cc:7: #include
<string>
newline

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger_unittest.cc:78:
base::DictionaryValue* DeepDropRecommended(const base::DictionaryValue& dict) {
What about dictionaries in lists? you don't seem to handle them here.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger_unittest.cc:121: delete
result_;
I'd advise to use scoped_ptrs, but that's not possible for static members. Is
reading the files really so expensive we cannot do it for every test?

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.cc:7: #include <string>
newline

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.cc:13: void
RemoveEntryUnless(base::DictionaryValue* dict, const std::string path,
all parameters on separate lines unless they really really really belong
together.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.cc:25: void
ONCValidator::RemoveIgnoredFieldsFromIPsec(base::DictionaryValue* ipsec) {
If we do this, shouldn't we also remove keys that are not specified in ONC?

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.h:15: class ONCValidator
{
same comment about making this a namespace if it doesn't have instances and
state

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.h:19: static void
RemoveIgnoredFieldsFromNetwork(base::DictionaryValue* network);
The names don't exactly suggest this is validation?

[pneubeck (no reviews), 2012-10-02 15:03:02]

The implementations in the patches so far do not cover all of the ONC format.
When the reviewers confirm the principle implementation, I can complete the
missing parts immediately or step-by-step as necessary, if you are OK with that.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_merger.cc (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:8: #include <string>
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:19:
base::DictionaryValue* Merge(const base::DictionaryValue* user,
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> should return a scoped_ptr<base::DictionaryValue>

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:20: const
base::DictionaryValue* policy) {
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> This function is too damn long. Please break into more manageable pieces.

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:26: return
Merge(&emptyDictionary, policy);
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> so you're making a recursive call? Why not just user = &emptyDictionary;

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.cc:117: }
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> Algorithm-wise, wouldn't it be simpler to start with a copy of the user policy
> and then overlay policy, making recursive calls for dictionaries?
In that case, fields that only exist in user and are not recommended have to be
removed afterwards. The understandability should be very similar.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_merger.h (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:8: #include <string>
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:13: }  // namespace base
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> namespace comment not necessary here per convention

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:18: class ONCMerger {
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> if you don't intend on adding state, this should become namespace onc_merger

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger.h:20: static
base::DictionaryValue* MergeUserSettingsWithPolicy(
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> could use a description comment

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_merger_unittest.cc (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger_unittest.cc:7: #include
<string>
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger_unittest.cc:78:
base::DictionaryValue* DeepDropRecommended(const base::DictionaryValue& dict) {
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> What about dictionaries in lists? you don't seem to handle them here.
Added a comment explaining this.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_merger_unittest.cc:121: delete
result_;
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> I'd advise to use scoped_ptrs, but that's not possible for static members. Is
> reading the files really so expensive we cannot do it for every test?

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.cc:7: #include <string>
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.cc:13: void
RemoveEntryUnless(base::DictionaryValue* dict, const std::string path,
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> all parameters on separate lines unless they really really really belong
> together.

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.h:15: class ONCValidator
{
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> same comment about making this a namespace if it doesn't have instances and
> state

Done.

http://codereview.chromium.org/10944009/diff/1/chrome/browser/chromeos/networ...
chrome/browser/chromeos/network_settings/onc_validator.h:19: static void
RemoveIgnoredFieldsFromNetwork(base::DictionaryValue* network);
On 2012/09/18 16:29:08, Mattias Nissler wrote:
> The names don't exactly suggest this is validation?

Done.

[pneubeck (no reviews), 2012-10-02 15:42:39]

+stevenjb
Could you please review this CL?
This is my first CL preparing the discussed transition of network settings to
ONC.

+gspencer, +mnissler
FYI

The implementations in the patches so far do not cover all of the ONC format.
When you confirm the principle implementation, I can complete the missing parts
immediately or step-by-step as necessary, if you are OK with that.

[stevenjb, 2012-10-02 17:46:05]

Greg should really be the primary reviewer of this, since it may overlap with
what he is working on, but I will try to give it a look also.

[Greg Spencer (Chromium), 2012-10-03 21:25:06]

On 2012/10/02 17:46:05, stevenjb (chromium) wrote:
> Greg should really be the primary reviewer of this, since it may overlap with
> what he is working on, but I will try to give it a look also.

Philipp, since you've been unable to be at the meetings we've had, you should
know that we're actually leaning towards using protocol buffers to talk to
Shill, and not ONC in the future.  That doesn't mean this CL isn't useful: we
still need to parse ONC and validate it, and we'll need to use Shill
dictionaries for a while, but we might not be using it to talk to Shill in the
long term, and so the translation to Shill dictionaries might need to instead be
translation to protocol buffers someday, which might mean that we want to use
protocol buffer-based JSON in the javascript instead of ONC.

I'm happy to still review this if you want, since we'll still need it for the
near term.

[pneubeck (no reviews), 2012-10-04 14:55:08]

Yes, please review it. As you said protobuf is still a few steps ahead.

Just one more idea about the protobuf solution:
We could also think about generating a type signature as in onc_signature.* from
a protobuf definition. This would be the most light-weight solution without
generating too much stuff that is not used (the standard protobuf compiler
generates per type an accessor class that is rather bloated).

On 2012/10/03 21:25:06, Greg Spencer (Chromium) wrote:
> On 2012/10/02 17:46:05, stevenjb (chromium) wrote:
> > Greg should really be the primary reviewer of this, since it may overlap
with
> > what he is working on, but I will try to give it a look also.
> 
> Philipp, since you've been unable to be at the meetings we've had, you should
> know that we're actually leaning towards using protocol buffers to talk to
> Shill, and not ONC in the future.  That doesn't mean this CL isn't useful: we
> still need to parse ONC and validate it, and we'll need to use Shill
> dictionaries for a while, but we might not be using it to talk to Shill in the
> long term, and so the translation to Shill dictionaries might need to instead
be
> translation to protocol buffers someday, which might mean that we want to use
> protocol buffer-based JSON in the javascript instead of ONC.
> 
> I'm happy to still review this if you want, since we'll still need it for the
> near term.

[Greg Spencer (Chromium), 2012-10-08 19:21:08]

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.cc (right):

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.cc:63: { NULL }
You might want to use arraysize here instead of null termination: less error
prone.  If someone changes things they can't forget the NULL. (And on the arrays
below)

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:217: (type != "VPN"   
  || RequiredField(*result, kVPN)) &&
We usually don't align things like this with whitespace: they are too easy to
get out of alignment with name changes or insertion of new longer lines, and
cause extra lines to be changed unnecessarily in CLs, and so more merge
conflicts can occur than are necessary.

http://codereview.chromium.org/10944009/diff/17001/chrome/chrome_browser_chro...
File chrome/chrome_browser_chromeos.gypi (right):

http://codereview.chromium.org/10944009/diff/17001/chrome/chrome_browser_chro...
chrome/chrome_browser_chromeos.gypi:506:
'browser/chromeos/network_settings/onc_signature.cc',
Alphabetize these.

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

On Wed, Oct 3, 2012 at 11:25 PM, <gspencer@chromium.org> wrote:

> On 2012/10/02 17:46:05, stevenjb (chromium) wrote:
>
>> Greg should really be the primary reviewer of this, since it may overlap
>> with
>> what he is working on, but I will try to give it a look also.
>>
>
> Philipp, since you've been unable to be at the meetings we've had, you
> should
> know that we're actually leaning towards using protocol buffers to talk to
> Shill


May I ask for more context here? Are you planning on writing up design
thoughts? I'd like to make sure that policy fits into the picture more
cleanly instead of the bolted-on state that we currently have.


> , and not ONC in the future.  That doesn't mean this CL isn't useful: we
> still need to parse ONC and validate it, and we'll need to use Shill
> dictionaries for a while, but we might not be using it to talk to Shill in
> the
> long term, and so the translation to Shill dictionaries might need to
> instead be
> translation to protocol buffers someday, which might mean that we want to
> use
> protocol buffer-based JSON in the javascript instead of ONC.
>
> I'm happy to still review this if you want, since we'll still need it for
> the
> near term.
>
>
http://codereview.chromium.**org/10944009/<http://codereview.chromium.org/109...
>

[Greg Spencer (Chromium), 2012-10-09 15:34:04]

On Tue, Oct 9, 2012 at 6:06 AM, Mattias Nissler <mnissler@chromium.org>wrote:

> On Wed, Oct 3, 2012 at 11:25 PM, <gspencer@chromium.org> wrote:
>
>> Philipp, since you've been unable to be at the meetings we've had, you
>> should
>> know that we're actually leaning towards using protocol buffers to talk to
>> Shill
>
>
> May I ask for more context here? Are you planning on writing up design
> thoughts? I'd like to make sure that policy fits into the picture more
> cleanly instead of the bolted-on state that we currently have.
>

Of course.  Actually, there really isn't a lot more context to tell.  We've
discussed using protocol buffers in the meetings, but there hasn't been
much design work around it that I know of yet.  The advantages we talked
about are type safety and versioning, and the fact that we could eventually
jettison DBus entirely and just use sockets to talk to Shill.  The
disadvantage is that the JS implementations of protocol buffers are
verbose, and not widely used, so we may have to use ONC or some other JSON
representation in JS anyhow.

As far as policy goes, I would think that protocol buffers would be nice in
that we could design structures around values in the buffer that could
contain the policy information, and so have policy more directly integrated
into the structure: Shill could just store, but ignore, the policy pieces
of the buffer until it understands policy itself.

-Greg.

[pneubeck (no reviews), 2012-10-09 16:19:18]

Addressed or replied to comments.

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.cc (right):

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.cc:63: { NULL }
In the OncValueSignature only a pointer is stored without any information about
the array's size.

The only way to use arraysize that I see is:
Add an "size_t number_of_fields" member to OncValueSignature
and assign it the value "= arraysize(ipsec_fields)" at each declaration.

I'm fine with both variants.

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:217: (type != "VPN"   
  || RequiredField(*result, kVPN)) &&
On 2012/10/08 19:21:08, Greg Spencer (Chromium) wrote:
> We usually don't align things like this with whitespace: they are too easy to
> get out of alignment with name changes or insertion of new longer lines, and
> cause extra lines to be changed unnecessarily in CLs, and so more merge
> conflicts can occur than are necessary.

Done.

http://codereview.chromium.org/10944009/diff/17001/chrome/chrome_browser_chro...
File chrome/chrome_browser_chromeos.gypi (right):

http://codereview.chromium.org/10944009/diff/17001/chrome/chrome_browser_chro...
chrome/chrome_browser_chromeos.gypi:506:
'browser/chromeos/network_settings/onc_signature.cc',
On 2012/10/08 19:21:08, Greg Spencer (Chromium) wrote:
> Alphabetize these.

Done.

[Greg Spencer (Chromium), 2012-10-09 16:42:22]

LGTM

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.cc (right):

http://codereview.chromium.org/10944009/diff/17001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.cc:63: { NULL }
On 2012/10/09 16:19:19, pneubeck wrote:
> In the OncValueSignature only a pointer is stored without any information
about
> the array's size.
> 
> The only way to use arraysize that I see is:
> Add an "size_t number_of_fields" member to OncValueSignature
> and assign it the value "= arraysize(ipsec_fields)" at each declaration.
> 
> I'm fine with both variants.

Hmm.  I see what you mean. NULL terminated is fine.

[stevenjb, 2012-10-11 19:54:00]

Sorry for the late review. Mostly nits. I haven't thought too hard about how
this fits into everything, but it seems self-contained enough that it will be
useful wherever we end up putting the validation code.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:54: !found_unknown_field)
nit: {} around multi-line if

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:105: << fieldname <<
"'.";
nit: {}, alignment

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:120: NOTREACHED() <<
"Found missing onc_array_entry_signature.";
CHECK()?

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:129: *entry);
nit: align

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:62: base::DictionaryValue*
result);
Returning a bool here is inconsistent and confusing; could we return a scoped
Dictionary instead and either return an empty dictionary on error, pass multiple
bool*, or use a result code?

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_normalizer.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_normalizer.cc:69: std::string
auth_type = "";
nit: = "" is unnecessary here and elsewhere

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_normalizer.cc:71:
RemoveEntryUnless(ipsec, kClientCertType, auth_type == "Cert");
"Cert" and other strings should be consts declared at the top of the file (or in
a header if used elsewhere)

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.cc:14: const
base::Value::Type TYPE_DICTIONARY = base::Value::TYPE_DICTIONARY;
using base::Value

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.h (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.h:32: extern const
OncValueSignature recommended_signature;
These should use kRecommendedSignature style.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:101: } else {
nit: else unnecessary after early exit

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:145: std::string
error_cause = "";
nit: = "" unnecessary

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:181: }
nit: empty line between templates / classes

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:207: std::string type
= "";
nit: = "" unnecessary

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:209:
!IsAnyOf<std::string>(type, "Ethernet", "VPN", "WiFi"))
These should be consts declared somewhere

[pneubeck (no reviews), 2012-10-15 13:57:43]

Addressed all comments.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:54: !found_unknown_field)
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: {} around multi-line if

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:105: << fieldname <<
"'.";
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: {}, alignment

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:120: NOTREACHED() <<
"Found missing onc_array_entry_signature.";
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> CHECK()?
It's a fatal error but we can still ignore some input instead of crashing the
process.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:129: *entry);
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: align

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:62: base::DictionaryValue*
result);
I opted for a second bool argument. This keeps the flexibility of the passed
Dictionary* (there may be already some previously created fields) and makes the
meaning clearer.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_normalizer.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_normalizer.cc:69: std::string
auth_type = "";
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: = "" is unnecessary here and elsewhere

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_normalizer.cc:71:
RemoveEntryUnless(ipsec, kClientCertType, auth_type == "Cert");
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> "Cert" and other strings should be consts declared at the top of the file (or
in
> a header if used elsewhere)
I added constants for the possible field values to onc_constants.*

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.cc:14: const
base::Value::Type TYPE_DICTIONARY = base::Value::TYPE_DICTIONARY;
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> using base::Value

Would be nice, but unfortunately doesn't work.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.h (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.h:32: extern const
OncValueSignature recommended_signature;
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> These should use kRecommendedSignature style.

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:101: } else {
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: else unnecessary after early exit

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:145: std::string
error_cause = "";
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: = "" unnecessary

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:181: }
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: empty line between templates / classes

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:207: std::string type
= "";
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> nit: = "" unnecessary

Done.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:209:
!IsAnyOf<std::string>(type, "Ethernet", "VPN", "WiFi"))
On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> These should be consts declared somewhere

Done.

[stevenjb, 2012-10-15 17:23:53]

LGTM with one change.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.cc:14: const
base::Value::Type TYPE_DICTIONARY = base::Value::TYPE_DICTIONARY;
On 2012/10/15 13:57:44, pneubeck wrote:
> On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> > using base::Value
> 
> Would be nice, but unfortunately doesn't work.

You still have to use Value::TYPE_DICTIONARY, but you should do that anyway.
These extra const values are odd, we shouldn't use them. More important, we
should be consistent and not mix base::Value::TYPE_ with just TYPE_.

[pneubeck (no reviews), 2012-10-30 08:42:04]

I addressed the last comment of Steven and did several iterations on my own,
where I found some smaller flaws which I fixed.

Finally, I integrated the Validator into OncNetworkParser.
After this is committed, I will prepare a CL to add nice error feedback to the
Validator which can be shown on the about:policy page and at the ONC import.

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.cc (right):

http://codereview.chromium.org/10944009/diff/20003/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.cc:14: const
base::Value::Type TYPE_DICTIONARY = base::Value::TYPE_DICTIONARY;
On 2012/10/15 17:23:54, stevenjb (chromium) wrote:
> On 2012/10/15 13:57:44, pneubeck wrote:
> > On 2012/10/11 19:54:00, stevenjb (chromium) wrote:
> > > using base::Value
> > 
> > Would be nice, but unfortunately doesn't work.
> 
> You still have to use Value::TYPE_DICTIONARY, but you should do that anyway.
> These extra const values are odd, we shouldn't use them. More important, we
> should be consistent and not mix base::Value::TYPE_ with just TYPE_.
> 

Done.

[Mattias Nissler (ping if slow), 2012-11-02 10:09:59]

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_constants.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_constants.h:14: extern const char
kNetworkConfigurations[];
newline

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (left):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:136: { onc::vpn::kType,
PROPERTY_INDEX_PROVIDER_TYPE, TYPE_STRING },
Shouldn't this stay onc::vpn::kType? It's the type field for vpns after all.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:206: { onc::proxy::kType,
PROPERTY_INDEX_ONC_PROXY_TYPE, TYPE_STRING },
ditto

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:312: // names.
I think this comment should go before the is_managed definition in line 309.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:314: bool
error_on_wrong_recommended = false;
I don't understand what that means. Better name? Comment?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:1100: { onc::kNone,
CLIENT_CERT_TYPE_NONE },
same here. I'd prefer clarity of where to find constants over sharing.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:67: return
make_scoped_ptr(onc_primitive.DeepCopy());
Aha, so this is just an elaborate way to copy an ONC dictionary as long as it
corresponds to the spec. A note to that regard should be in the class comment.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:23: // Iterate over both a
hierarchical onc signature and an onc value. By comparing
nit: s/onc/ONC/g

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:25: // "onc_signature.h",
accurate signature-specific functions can be applied.  It
You should really explain what the term "map" means in this context.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:26: // is expected that
the hierarchy of the signature is complete.
what does this mean: "the hierarchy of the signature is complete"?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.h:29: const std::string&
onc_field_name) const;
Not entirely sure, but I think it's not OK to put functions into data structs.
I'd rather declare it separately.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_test_utils.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.cc:19: //
DictionaryValue. CHECKs if any error occurs.
Ah, here are the comments :) move to the header please.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_test_utils.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.h:21:
scoped_ptr<base::DictionaryValue> ReadTestDictionary(
comment

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.h:24:
::testing::AssertionResult Equals(const base::DictionaryValue* expected,
comment, what does this do differently than base::DictionaryValue::Equals()?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:147:
CHECK(recommended_list != NULL);
Does this mean we'll crash if somebody feeds bad ONC?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:152: if
(!managed_onc_) {
I would just drop this check. What additional benefit does it provide?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:200: bool
RequireAnyOf(const std::string &actual, const std::string &v1,
What's wrong with this:

bool RequireAnyOf(const std::string& str, const char** values) {
  for (; *values; ++values) {
    if (str == *values)
      return true;
  }

  return false;
}

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:289: allRequiredExist
= false;
you could write all these as &=, but that's a matter of personal taste.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:44: static
scoped_ptr<Validator> CreateLiberalValidator(bool managed_onc);
I think you only ever use these convenience helpers in a test, so no need to
litter the interface with them here.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:49: // encountered),
returns the repaired object, otherwise returns NULL.
What exactly does this "repair"?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator_unittest.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator_unittest.cc:118:
::testing::Bool());
newline

http://codereview.chromium.org/10944009/diff/81001/chrome/test/data/chromeos/...
File chrome/test/data/chromeos/network_settings/normalize.json (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/test/data/chromeos/...
chrome/test/data/chromeos/network_settings/normalize.json:1: {
"normalize.json" is a curious name. If this is the input to the normalizer, it
should probably be "to_be_normalized.json", if the is the expected result, it
should be "normalized.json".

[Mattias Nissler (ping if slow), 2012-11-02 10:09:59]

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_constants.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_constants.h:14: extern const char
kNetworkConfigurations[];
newline

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (left):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:136: { onc::vpn::kType,
PROPERTY_INDEX_PROVIDER_TYPE, TYPE_STRING },
Shouldn't this stay onc::vpn::kType? It's the type field for vpns after all.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:206: { onc::proxy::kType,
PROPERTY_INDEX_ONC_PROXY_TYPE, TYPE_STRING },
ditto

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:312: // names.
I think this comment should go before the is_managed definition in line 309.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:314: bool
error_on_wrong_recommended = false;
I don't understand what that means. Better name? Comment?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:1100: { onc::kNone,
CLIENT_CERT_TYPE_NONE },
same here. I'd prefer clarity of where to find constants over sharing.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:67: return
make_scoped_ptr(onc_primitive.DeepCopy());
Aha, so this is just an elaborate way to copy an ONC dictionary as long as it
corresponds to the spec. A note to that regard should be in the class comment.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:23: // Iterate over both a
hierarchical onc signature and an onc value. By comparing
nit: s/onc/ONC/g

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:25: // "onc_signature.h",
accurate signature-specific functions can be applied.  It
You should really explain what the term "map" means in this context.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:26: // is expected that
the hierarchy of the signature is complete.
what does this mean: "the hierarchy of the signature is complete"?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.h:29: const std::string&
onc_field_name) const;
Not entirely sure, but I think it's not OK to put functions into data structs.
I'd rather declare it separately.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_test_utils.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.cc:19: //
DictionaryValue. CHECKs if any error occurs.
Ah, here are the comments :) move to the header please.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_test_utils.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.h:21:
scoped_ptr<base::DictionaryValue> ReadTestDictionary(
comment

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.h:24:
::testing::AssertionResult Equals(const base::DictionaryValue* expected,
comment, what does this do differently than base::DictionaryValue::Equals()?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:147:
CHECK(recommended_list != NULL);
Does this mean we'll crash if somebody feeds bad ONC?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:152: if
(!managed_onc_) {
I would just drop this check. What additional benefit does it provide?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:200: bool
RequireAnyOf(const std::string &actual, const std::string &v1,
What's wrong with this:

bool RequireAnyOf(const std::string& str, const char** values) {
  for (; *values; ++values) {
    if (str == *values)
      return true;
  }

  return false;
}

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:289: allRequiredExist
= false;
you could write all these as &=, but that's a matter of personal taste.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:44: static
scoped_ptr<Validator> CreateLiberalValidator(bool managed_onc);
I think you only ever use these convenience helpers in a test, so no need to
litter the interface with them here.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:49: // encountered),
returns the repaired object, otherwise returns NULL.
What exactly does this "repair"?

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator_unittest.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator_unittest.cc:118:
::testing::Bool());
newline

http://codereview.chromium.org/10944009/diff/81001/chrome/test/data/chromeos/...
File chrome/test/data/chromeos/network_settings/normalize.json (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/test/data/chromeos/...
chrome/test/data/chromeos/network_settings/normalize.json:1: {
"normalize.json" is a curious name. If this is the input to the normalizer, it
should probably be "to_be_normalized.json", if the is the expected result, it
should be "normalized.json".

[pneubeck (no reviews), 2012-11-05 12:04:48]

Addressed Mattias' comments.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_constants.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_constants.h:14: extern const char
kNetworkConfigurations[];
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (left):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:136: { onc::vpn::kType,
PROPERTY_INDEX_PROVIDER_TYPE, TYPE_STRING },
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> Shouldn't this stay onc::vpn::kType? It's the type field for vpns after all.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:206: { onc::proxy::kType,
PROPERTY_INDEX_ONC_PROXY_TYPE, TYPE_STRING },
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> ditto

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:312: // names.
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> I think this comment should go before the is_managed definition in line 309.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:314: bool
error_on_wrong_recommended = false;
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> I don't understand what that means. Better name? Comment?

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:1100: { onc::kNone,
CLIENT_CERT_TYPE_NONE },
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> same here. I'd prefer clarity of where to find constants over sharing.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.cc:67: return
make_scoped_ptr(onc_primitive.DeepCopy());
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> Aha, so this is just an elaborate way to copy an ONC dictionary as long as it
> corresponds to the spec. A note to that regard should be in the class comment.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:23: // Iterate over both a
hierarchical onc signature and an onc value. By comparing
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> nit: s/onc/ONC/g

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:25: // "onc_signature.h",
accurate signature-specific functions can be applied.  It
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> You should really explain what the term "map" means in this context.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:26: // is expected that
the hierarchy of the signature is complete.
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> what does this mean: "the hierarchy of the signature is complete"?

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_signature.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_signature.h:29: const std::string&
onc_field_name) const;
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> Not entirely sure, but I think it's not OK to put functions into data structs.
> I'd rather declare it separately.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_test_utils.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.cc:19: //
DictionaryValue. CHECKs if any error occurs.
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> Ah, here are the comments :) move to the header please.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_test_utils.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.h:21:
scoped_ptr<base::DictionaryValue> ReadTestDictionary(
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> comment

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_test_utils.h:24:
::testing::AssertionResult Equals(const base::DictionaryValue* expected,
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> comment, what does this do differently than base::DictionaryValue::Equals()?

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:147:
CHECK(recommended_list != NULL);
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> Does this mean we'll crash if somebody feeds bad ONC?
This case should not occur as the type validation was applied before
(MapFields).
If it still happens, then the type validation is buggy.

However, a CHECK is as good as failing at the next access in line 159 or 162.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:152: if
(!managed_onc_) {
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> I would just drop this check. What additional benefit does it provide?

At least, I would keep the message and the removal of the unnecessary
Recommended field. This ensures, that the resulting ONC dictionary is really
what we expect.

I removed the flag and never throw errors on a Recommended field in unmanaged
ONC.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:200: bool
RequireAnyOf(const std::string &actual, const std::string &v1,
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> What's wrong with this:
> 
> bool RequireAnyOf(const std::string& str, const char** values) {
>   for (; *values; ++values) {
>     if (str == *values)
>       return true;
>   }
> 
>   return false;
> }

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:289: allRequiredExist
= false;
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> you could write all these as &=, but that's a matter of personal taste.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:44: static
scoped_ptr<Validator> CreateLiberalValidator(bool managed_onc);
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> I think you only ever use these convenience helpers in a test, so no need to
> litter the interface with them here.

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:49: // encountered),
returns the repaired object, otherwise returns NULL.
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> What exactly does this "repair"?

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator_unittest.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator_unittest.cc:118:
::testing::Bool());
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> newline

Done.

http://codereview.chromium.org/10944009/diff/81001/chrome/test/data/chromeos/...
File chrome/test/data/chromeos/network_settings/normalize.json (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/test/data/chromeos/...
chrome/test/data/chromeos/network_settings/normalize.json:1: {
On 2012/11/02 10:10:00, Mattias Nissler wrote:
> "normalize.json" is a curious name. If this is the input to the normalizer, it
> should probably be "to_be_normalized.json", if the is the expected result, it
> should be "normalized.json".

It contains both. Renamed to "settings_with_normalization.json".

Also renamed "invalid.json" to "invalid_settings_with_repairs.json"

[pneubeck (no reviews), 2012-11-05 12:12:07]

Hi William,

are you Ok with me adding the convenience function
  GetBooleanWithoutPathExpansion
in base/values.* ?
GetBoolean exists already, but this one was missing.

Thanks,
Philipp

[willchan no longer on Chromium, 2012-11-05 15:45:54]

lgtm

[Mattias Nissler (ping if slow), 2012-11-06 09:30:56]

Mostly good, just a couple more minor things.

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/81001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:147:
CHECK(recommended_list != NULL);
On 2012/11/05 12:04:48, pneubeck wrote:
> On 2012/11/02 10:10:00, Mattias Nissler wrote:
> > Does this mean we'll crash if somebody feeds bad ONC?
> This case should not occur as the type validation was applied before
> (MapFields).
> If it still happens, then the type validation is buggy.
> 
> However, a CHECK is as good as failing at the next access in line 159 or 162.

OK, makes sense.

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:317: new
onc::Validator(error_on_unknown_field,
nit: we commonly just write

false, /* error on unknown field */

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:95: 
private:

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:130: {
what is this scope good for?

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:237: static const
char* valid_types[] = { kEthernet, kVPN, kWiFi, NULL };
This should be named kValidTypes per the style guide (also below).

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:45: // "repaired"
object. Otherwise, on error, returns NULL.
This still doesn't make it very clear what "repairing" means. What are
"problematic fields"? Just give an actual example so people get an idea what
you're talking about.

[pneubeck (no reviews), 2012-11-06 13:32:22]

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/onc_network_parser.cc:317: new
onc::Validator(error_on_unknown_field,
On 2012/11/06 09:30:56, Mattias Nissler wrote:
> nit: we commonly just write
> 
> false, /* error on unknown field */

Done.

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:95: 
On 2012/11/06 09:30:56, Mattias Nissler wrote:
> private:

Done.

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:130: {
On 2012/11/06 09:30:56, Mattias Nissler wrote:
> what is this scope good for?

Done.

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:237: static const
char* valid_types[] = { kEthernet, kVPN, kWiFi, NULL };
On 2012/11/06 09:30:56, Mattias Nissler wrote:
> This should be named kValidTypes per the style guide (also below).

Done.

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/91007/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:45: // "repaired"
object. Otherwise, on error, returns NULL.
On 2012/11/06 09:30:56, Mattias Nissler wrote:
> This still doesn't make it very clear what "repairing" means. What are
> "problematic fields"? Just give an actual example so people get an idea what
> you're talking about.

Done.

[Mattias Nissler (ping if slow), 2012-11-06 13:49:18]

LGTM with nits.

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:498: const char*
valid_inner_values[] =
please convert these to static const char kValidInnerValues as well.

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:506: const char*
valid_outer_values[] = { kPEAP, kEAP_TLS, kEAP_TTLS, kLEAP,
ditto

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:53: // the invalid
fields and values and returns this "repaired"
nit: no need to break the line here?

[pneubeck (no reviews), 2012-11-06 14:45:21]

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.cc (right):

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:498: const char*
valid_inner_values[] =
On 2012/11/06 13:49:19, Mattias Nissler wrote:
> please convert these to static const char kValidInnerValues as well.

Done.

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.cc:506: const char*
valid_outer_values[] = { kPEAP, kEAP_TLS, kEAP_TTLS, kLEAP,
On 2012/11/06 13:49:19, Mattias Nissler wrote:
> ditto

Done.

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_validator.h (right):

http://codereview.chromium.org/10944009/diff/98008/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_validator.h:53: // the invalid
fields and values and returns this "repaired"
On 2012/11/06 13:49:19, Mattias Nissler wrote:
> nit: no need to break the line here?

Done.

[commit-bot: I haz the power, 2012-11-06 14:50:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/10944009/88037

[commit-bot: I haz the power, 2012-11-06 14:50:59]

Presubmit check for 10944009-88037 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.0s to calculate.

[commit-bot: I haz the power, 2012-11-06 14:52:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/10944009/88037

[commit-bot: I haz the power, 2012-11-06 16:46:59]

Change committed as 166222