Extending ONC validator's logging. Completing toplevel validation.

Extending ONC validator's logging. Completing toplevel validation.

Adding also more validation tests.
The logging is in line with https://codereview.chromium.org/11299236/.
BUG=162802
TEST=Unit tests.
TBR=stevenjb@chromium.org,eroman@chromium.org (whitespace change in net_interals.cc)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173393

[pneubeck (no reviews), 2012-12-07 12:20:01]

Hi Steven, hi Greg,

I reworked my last CL about logging in ONC validator. It should be in line with
the CL that Greg is currently preparing (no translation in base code, few error
codes, use logging).

Feel free to commit Greg's CL at first and I will rebase. If you prefer it, I
can also use NET_LOG_* in the validator.

Thanks!
-- Philipp

[Greg Spencer (Chromium), 2012-12-10 22:59:01]

https://codereview.chromium.org/11469026/diff/4001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

https://codereview.chromium.org/11469026/diff/4001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:56: bool* error);
Seems kind of backwards to return the scoped_ptr, and take a bool* to get the
success/failure.  Would it make sense to pass in a base::DictionaryValue*
instead and return a bool?  The ownership model in the classic calling
convention (passing in the pointer) is pretty well defined, and doesn't need a
scoped_ptr then.

What was the reason to change from returning NULL for failure?  That seemed a
lot simpler...

[pneubeck (no reviews), 2012-12-11 08:25:22]

Greg indicated what I already feared: The Mapper abstraction is borderline...
Currently I hope that this was the last adaption of it, but if it again happens
that we have to adapt the Mapper base class, then we may think about a better
solution.

https://codereview.chromium.org/11469026/diff/4001/chrome/browser/chromeos/ne...
File chrome/browser/chromeos/network_settings/onc_mapper.h (right):

https://codereview.chromium.org/11469026/diff/4001/chrome/browser/chromeos/ne...
chrome/browser/chromeos/network_settings/onc_mapper.h:56: bool* error);
Here, the |error| argument indicates something different than the return value.
Before this patch, I was just not consistent and added the |error| flag only to
a few methods.

The idea was that the Mapper class doesn't know about what a derived class
considers to be a fatal error. E.g. if we find an unknown field then we might
either see that as an fatal error (e.g. strict validation) or ignore the problem
(e.g. liberal validation).

Thus, the scheme I followed here is: |error| is set to true if anywhere in this
subtree an error occurred. The return value is a "repaired" object (with
problematic fields removed).

I think, the choice return object/error argument is fine here because the
primary usage of this function is the mapping and the resulting object. The
|error| value is an additional detail.

This usage is also similar to what was discussed on chromium-dev a while ago.
The seemed to prefer
  scoped_ptr<X> MyFunction(/* optional outparam for detailed error code */);
over
  RetValue MyFunction(scoped_ptr<X>* p);

https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/scoped...

On 2012/12/10 22:59:01, Greg Spencer (Chromium) wrote:
> Seems kind of backwards to return the scoped_ptr, and take a bool* to get the
> success/failure.  Would it make sense to pass in a base::DictionaryValue*
> instead and return a bool?  The ownership model in the classic calling
> convention (passing in the pointer) is pretty well defined, and doesn't need a
> scoped_ptr then.
> 
> What was the reason to change from returning NULL for failure?  That seemed a
> lot simpler...

[pneubeck (no reviews), 2012-12-12 14:06:56]

Rebased. Catching up with Greg's latest CL.

[Joao da Silva, 2012-12-13 09:51:34]

Just a drive-by, very high-level review. Feel free to proceed without an
explicit ACK from my side :-)

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1104: <<
onc::GetSourceAsString(source);
nit: indent

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1125: << " from " <<
onc::GetSourceAsString(source);
nit: indent

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:74:
errors->AddError(policy_name(), IDS_POLICY_NETWORK_CONFIG_INVALID);
I'm not entirely sure if this one should return false or not. Is it possible
that INVALID is returned but some networks or certificates can still be loaded?

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/net...
File chrome/browser/policy/network_configuration_updater_unittest.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/net...
chrome/browser/policy/network_configuration_updater_unittest.cc:28:
chromeos::onc::kEmptyUnencryptedConfiguration;
Why not just remove kEmptyConfiguration and replace its uses with
chromeos::onc::kEmptyUnencryptedConfiguration?

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_mapper.h (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_mapper.h:14: class Value;
nit: order

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:94: LOG(ERROR) << ErrorHeader() << "Found
value '" << ValueToString(onc_value)
ValueToString is not needed, base/values.h has operator<<(ostream, Value&).

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:522: kValidAuthentications) |
||

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:562: FieldExistsAndHasNoValueOf(*result,
kClientCertType, kValidCertTypes) |
||

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:651: FieldExistsAndHasNoValueOf(*result,
kOuter, kValidOuterValues) |
||

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:8: #include <vector>
Also <string>

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:16: class DictionaryValue;
nit: order

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:48: std::pair<std::string, const
OncValueSignature*> > {
#include <utility>

[pastarmovj, 2012-12-13 10:15:44]

Some more comments from me. Some might be repeating with Joao  though.

https://codereview.chromium.org/11469026/diff/13002/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6054: +        The network configuration is
valid but contains non-critical errors.
What are non-critical errors. It seems to me that this message is not very good.
Why not use a similar template as with the ...PARSE_ERROR version where you give
detail where is that non-critical error. Also if it is valid than it should have
no errors maybe "The nc could be parsed but ..." that is only MHO though.

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1042: return true;
Is this correct change?

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1104: <<
onc::GetSourceAsString(source);
Align the <<

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1124: LOG(ERROR) <<
"Error during ONC parsing network at index " << i
Ditto.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_mapper.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_mapper.cc:99: DCHECK(field_signature->value_signature
!= NULL)
Leave just DCHECK(pointer) it's equivalent.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_mapper.cc:114:
DCHECK(array_signature.onc_array_entry_signature != NULL)
Ditto.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:42: DCHECK(static_cast<size_t>(type) <
arraysize(strings));
Make this a CHECK you'll segfault if you go beyond this point with an invalid
index.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:78: DCHECK_EQ(result_value.get() == NULL,
*result == INVALID);
Please put a comment here that the value should be empty iff the result is
invalid. It took me a few seconds to parse this line and you can spare those
seconds on your next reviewer ;)

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:116: if (&signature ==
&kToplevelConfigurationSignature)
It's a pity we can't do cases on strings :(

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:186: scoped_ptr<base::Value> result =
Mapper::MapEntry(index, signature,
I'd rather split this after the =

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:297: const char** valid_values) {
I think the way you pass this array is somewhat error prone. I'd rather add a
next param that is |int valid_values_count| and you can use the macro to get the
size of the array when you invoke this function to make sure you will not forget
to put the NULL .

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:562: FieldExistsAndHasNoValueOf(*result,
kClientCertType, kValidCertTypes) |
On 2012/12/13 09:51:35, Joao da Silva wrote:
> ||

Twice!

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:651: FieldExistsAndHasNoValueOf(*result,
kOuter, kValidOuterValues) |
On 2012/12/13 09:51:35, Joao da Silva wrote:
> ||

x2

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:32: // Creates a Validator that searches
for the following invalid cases:
This seems more like a documentation on the class rather than on the
constructor. Can you maybe move it before the class line?

[pneubeck (no reviews), 2012-12-13 14:10:02]

Addressed your comments. See a few answers below.

https://codereview.chromium.org/11469026/diff/13002/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6054: +        The network configuration is
valid but contains non-critical errors.
On 2012/12/13 10:15:44, pastarmovj wrote:
> What are non-critical errors. It seems to me that this message is not very
good.
> Why not use a similar template as with the ...PARSE_ERROR version where you
give
> detail where is that non-critical error. Also if it is valid than it should
have
> no errors maybe "The nc could be parsed but ..." that is only MHO though.

The above message was actually not up-to-date. Fixed that.

Please check the new messages for clarity.

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1042: return true;
On 2012/12/13 10:15:44, pastarmovj wrote:
> Is this correct change?
Yes. That case occurs regularly (depending on which is loading faster: Chrome or
Shill) and shouldn't throw a warning or error (at the call site).

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1104: <<
onc::GetSourceAsString(source);
On 2012/12/13 10:15:44, pastarmovj wrote:
> Align the <<

Done.

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1104: <<
onc::GetSourceAsString(source);
On 2012/12/13 09:51:35, Joao da Silva wrote:
> nit: indent

Done.

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1124: LOG(ERROR) <<
"Error during ONC parsing network at index " << i
On 2012/12/13 10:15:44, pastarmovj wrote:
> Ditto.

Done.

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1125: << " from " <<
onc::GetSourceAsString(source);
On 2012/12/13 09:51:35, Joao da Silva wrote:
> nit: indent

Done.

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/con...
File chrome/browser/policy/configuration_policy_handler_chromeos.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/con...
chrome/browser/policy/configuration_policy_handler_chromeos.cc:74:
errors->AddError(policy_name(), IDS_POLICY_NETWORK_CONFIG_INVALID);
On 2012/12/13 09:51:35, Joao da Silva wrote:
> I'm not entirely sure if this one should return false or not. Is it possible
> that INVALID is returned but some networks or certificates can still be
loaded?

Currently not. I apply the same validation again at the actual import
(NetworkLibrary::LoadOncNetworks) and abort there.

As long as these two validations don't get out-of-sync, a return false should be
fine.

Added return false for now, but still feeling a bit uncomfortable with that.

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/net...
File chrome/browser/policy/network_configuration_updater_unittest.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chrome/browser/policy/net...
chrome/browser/policy/network_configuration_updater_unittest.cc:28:
chromeos::onc::kEmptyUnencryptedConfiguration;
On 2012/12/13 09:51:35, Joao da Silva wrote:
> Why not just remove kEmptyConfiguration and replace its uses with
> chromeos::onc::kEmptyUnencryptedConfiguration?
(Because it's longer.) Replaced.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_mapper.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_mapper.cc:99: DCHECK(field_signature->value_signature
!= NULL)
On 2012/12/13 10:15:44, pastarmovj wrote:
> Leave just DCHECK(pointer) it's equivalent.
For consistency with the other null-pointer checks, I would better like to keep
it that way.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_mapper.cc:114:
DCHECK(array_signature.onc_array_entry_signature != NULL)
On 2012/12/13 10:15:44, pastarmovj wrote:
> Ditto.
see above.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_mapper.h (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_mapper.h:14: class Value;
On 2012/12/13 09:51:35, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:42: DCHECK(static_cast<size_t>(type) <
arraysize(strings));
On 2012/12/13 10:15:44, pastarmovj wrote:
> Make this a CHECK you'll segfault if you go beyond this point with an invalid
> index.
Fixed also in the original source file.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:78: DCHECK_EQ(result_value.get() == NULL,
*result == INVALID);
On 2012/12/13 10:15:44, pastarmovj wrote:
> Please put a comment here that the value should be empty iff the result is
> invalid. It took me a few seconds to parse this line and you can spare those
> seconds on your next reviewer ;)

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:94: LOG(ERROR) << ErrorHeader() << "Found
value '" << ValueToString(onc_value)
On 2012/12/13 09:51:35, Joao da Silva wrote:
> ValueToString is not needed, base/values.h has operator<<(ostream, Value&).

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:116: if (&signature ==
&kToplevelConfigurationSignature)
On 2012/12/13 10:15:44, pastarmovj wrote:
> It's a pity we can't do cases on strings :(
These are actually not strings but pointers, still cases don't work as they're
not available at compile-time.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:186: scoped_ptr<base::Value> result =
Mapper::MapEntry(index, signature,
On 2012/12/13 10:15:44, pastarmovj wrote:
> I'd rather split this after the =

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:297: const char** valid_values) {
On 2012/12/13 10:15:44, pastarmovj wrote:
> I think the way you pass this array is somewhat error prone. I'd rather add a
> next param that is |int valid_values_count| and you can use the macro to get
the
> size of the array when you invoke this function to make sure you will not
forget
> to put the NULL .

We already had a discussion about that previously and both solutions have their
advantages over each other.

I'm inclined to add a tiny template wrapper around this function doing the same
as the arraysize macro (which also uses a template). That would prohibit any
wrong usage of this function.
But that again creates some mental overhead at the definition...

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:522: kValidAuthentications) |
On 2012/12/13 09:51:35, Joao da Silva wrote:
> ||

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:562: FieldExistsAndHasNoValueOf(*result,
kClientCertType, kValidCertTypes) |
On 2012/12/13 10:15:44, pastarmovj wrote:
> On 2012/12/13 09:51:35, Joao da Silva wrote:
> > ||
> 
> Twice!

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:562: FieldExistsAndHasNoValueOf(*result,
kClientCertType, kValidCertTypes) |
On 2012/12/13 09:51:35, Joao da Silva wrote:
> ||

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:651: FieldExistsAndHasNoValueOf(*result,
kOuter, kValidOuterValues) |
On 2012/12/13 09:51:35, Joao da Silva wrote:
> ||

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:651: FieldExistsAndHasNoValueOf(*result,
kOuter, kValidOuterValues) |
On 2012/12/13 10:15:44, pastarmovj wrote:
> On 2012/12/13 09:51:35, Joao da Silva wrote:
> > ||
> 
> x2

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:8: #include <vector>
On 2012/12/13 09:51:35, Joao da Silva wrote:
> Also <string>

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:16: class DictionaryValue;
On 2012/12/13 09:51:35, Joao da Silva wrote:
> nit: order

Done.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:32: // Creates a Validator that searches
for the following invalid cases:
On 2012/12/13 10:15:44, pastarmovj wrote:
> This seems more like a documentation on the class rather than on the
> constructor. Can you maybe move it before the class line?
Done and added a missing case in the comment.

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/11469026/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator_unittest.cc:48: std::pair<std::string, const
OncValueSignature*> > {
On 2012/12/13 09:51:35, Joao da Silva wrote:
> #include <utility>

Done.

[pastarmovj, 2012-12-13 14:25:34]

LGTM in gnereral and particularly owner approval for the policy directory.

[stevenjb, 2012-12-14 01:27:40]

The NetworkLibrary changes look OK to me, but this really needs Greg's
expertise.

[Greg Spencer (Chromium), 2012-12-14 18:28:43]

LGTM with nits...

https://codereview.chromium.org/11469026/diff/26004/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/11469026/diff/26004/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6054: +        The network configuration
doesn't comply to the ONC standard. Parts of the configuration may others may
not be imported.
"may others may not be imported." -> "may not have been imported."

https://codereview.chromium.org/11469026/diff/26004/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11469026/diff/26004/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:296: bool
Validator::FieldExistsAndHasNoValueOf(const base::DictionaryValue& object,
Maybe "FieldExistsAndHasNoValidValue"?

[pneubeck (no reviews), 2012-12-14 19:44:45]

Addressed Greg's comments.

https://codereview.chromium.org/11469026/diff/26004/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/11469026/diff/26004/chrome/app/generated_reso...
chrome/app/generated_resources.grd:6054: +        The network configuration
doesn't comply to the ONC standard. Parts of the configuration may others may
not be imported.
On 2012/12/14 18:28:43, Greg Spencer (Chromium) wrote:
> "may others may not be imported." -> "may not have been imported."

Done.

https://codereview.chromium.org/11469026/diff/26004/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/11469026/diff/26004/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:296: bool
Validator::FieldExistsAndHasNoValueOf(const base::DictionaryValue& object,
On 2012/12/14 18:28:43, Greg Spencer (Chromium) wrote:
> Maybe "FieldExistsAndHasNoValidValue"?

Done.

[commit-bot: I haz the power, 2012-12-16 12:17:18]

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

[commit-bot: I haz the power, 2012-12-16 12:17:32]

Presubmit check for 11469026-39001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chromeos
    chrome/browser/ui/webui/net_internals
    chrome/browser/chromeos/cros

Presubmit checks took 2.5s to calculate.

[commit-bot: I haz the power, 2012-12-16 12:21:28]

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

[commit-bot: I haz the power, 2012-12-16 20:41:49]

Change committed as 173393