Add ONC VPN support for OpenVPN and L2TP/IPsec VPNs

Add ONC VPN support for OpenVPN and L2TP/IPsec VPNs

Also simplifies some of the parsing code, adds type validation, brings
the WiFi security parsing up to spec, and improves the unit tests for
wifi so that they all verify the property_map_ fields (the only ones
actually used to configure the network).  Also fixed a general parser
problem where certificates wouldn't be parsed if network configs were.

BUG=chromium-os:23476
chromium-os:23477
TEST=manual import of ONC file with openvpn network, unit_tests

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

[kmixter1, 2011-12-01 02:51:51]

This code doesn't work yet (doesn't even compile yet) but I wanted to give you
an early look at how I'm refactoring things as I add VPN support.  I value any
early review.

[Mattias Nissler (ping if slow), 2011-12-01 13:09:10]

In general, I like the direction. I put some implementation comments. One thing
I dislike is that the current code adds PROPERTY_INDEX_ONC_XYZ, which is an ONC
implementation detail that shouldn't be visible once we're talking about parse
properties.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/ne...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/ne...
chrome/browser/chromeos/cros/network_library.h:120: PROPERTY_INDEX_ONC_VPN,  //
Used internally for ONC parsing
I'm not sold on the idea to have these here. Can we somehow forward the data in
the ONC parser internally?

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:34: const base::Value::Type
TYPE_BOOLEAN = base::Value::TYPE_BOOLEAN;
why not just:

using base::Value::TYPE_BOOLEAN;

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:410: if (value.GetType() !=
Value::TYPE_DICTIONARY) {
Shortcut: value.IsType(base::Value::TYPE_DICTIONARY)

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:410: if (value.GetType() !=
Value::TYPE_DICTIONARY) {
also, Value is in namespace base nowadays, so you want to make sure new code has
the correct namespace prefix here and below.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:421: DCHECK(res);
I'd just remove the if and make this a CHECK(). If it ever triggers, that means
the DictionaryValue code is severely broken and we have larger problems anyway.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:427: if (key ==
signature[field].field) break;
I actually missed that break and were going to complain about unnecessary
curlies, so I'd like to see the break on the next line.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:439: }
I think you can just collapse these nested conditionals to:

if (signature[field].field != NULL &&
    inner_value->GetType() != signature[field].type &&
    parser(this, index, inner_value, network) {
  // success
} else {
  // report error
}

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:440:
network->UpdatePropertyMap(index, *inner_value);
Do we really want to update the map for unrecognized fields?

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:663: if (value.GetType() !=
Value::TYPE_DICTIONARY) {
there's value.IsType() for this.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:667: const DictionaryValue&
dict = static_cast<const DictionaryValue&>(value);
To avoid the cast, you could replace lines 663-667 with:

const DictionaryValue* dict = NULL;
if (!value.GetAsDictionary(&dict)) {
  // error
}

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:676: break;
Lines 668-676 shorter:

std::string provider_type_string;
if (!dict.GetString("Type", &provider_type_string))
  break;

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.h:86: // Parses a nested ONC
object with the given mapper and parser function.
You should mention how it handles errors and such.

[Charlie Lee, 2011-12-01 18:53:18]

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/na...
File chrome/browser/chromeos/cros/native_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/na...
chrome/browser/chromeos/cros/native_network_parser.cc:134: 
remove extra line

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:421: DCHECK(res);
Probably don't even need a CHECK(). If it ever fails, there will just be a null
pointer exception.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:425: int field;
should probably name this something other than field to avoid confusion.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:426: for (field = 0;
signature[field].field != NULL; ++field) {
consider using a map instead of an array so it's more efficient. similar to our
property maps, it will be signature maps.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:464:
OncWifiNetworkParser::OncWifiNetworkParser() {
nit: probably unnecessary

[kmixter1, 2011-12-01 19:48:39]

Addressed existing comments.  Status is that it builds but doesn't yet work...

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/ne...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/ne...
chrome/browser/chromeos/cros/network_library.h:120: PROPERTY_INDEX_ONC_VPN,  //
Used internally for ONC parsing
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> I'm not sold on the idea to have these here. Can we somehow forward the data
in
> the ONC parser internally?

As we discussed off-thread, the only alternative without a more large rewrite
would be to define a region of PropertyIndex which is local to individual
parsers and then define these values in the onc_network_parser.h.  I'm open to
that, but you suggested that was hacky, so leaving as is for now.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:34: const base::Value::Type
TYPE_BOOLEAN = base::Value::TYPE_BOOLEAN;
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> why not just:
> 
> using base::Value::TYPE_BOOLEAN;

using doesn't work with things inside of classes.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:410: if (value.GetType() !=
Value::TYPE_DICTIONARY) {
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> Shortcut: value.IsType(base::Value::TYPE_DICTIONARY)

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:410: if (value.GetType() !=
Value::TYPE_DICTIONARY) {
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> also, Value is in namespace base nowadays, so you want to make sure new code
has
> the correct namespace prefix here and below.

Added the prefix to be exact but somewhere else it must have already been pulled
into scope using "using", because it built without it.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:421: DCHECK(res);
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> I'd just remove the if and make this a CHECK(). If it ever triggers, that
means
> the DictionaryValue code is severely broken and we have larger problems
anyway.

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:421: DCHECK(res);
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> I'd just remove the if and make this a CHECK(). If it ever triggers, that
means
> the DictionaryValue code is severely broken and we have larger problems
anyway.

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:425: int field;
On 2011/12/01 18:53:18, Charlie Lee wrote:
> should probably name this something other than field to avoid confusion.

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:426: for (field = 0;
signature[field].field != NULL; ++field) {
On 2011/12/01 18:53:18, Charlie Lee wrote:
> consider using a map instead of an array so it's more efficient. similar to
our
> property maps, it will be signature maps.

Yeah, I suspect what we'll want to do is construct something similar to a
"Mapper" from the tables and have multiple ones of them in the class.  Then we
can do things in the WiFi and VirtualNetworkParser initializers like create the
appropriate ONCObjectParsers for the different ONC typed objects specific to
those kinds of networks, but also add more complex validation rules like what
string values are allowed for JSON-style enums (string must be either "WEP-PSK"
or ...) and bit-fields (list of strings which each must be one of N constants). 
We can also add "required" and "optional" checks at that stage, especially those
"required if" checks (like EAP is required if Security is WPA-EAP). At that
point adding hash maps is probably the way to go.  I wanted something simple
now, and even 400 strcmps isn't very expensive when parsing the most complex
objects.

At that point we can probably make the Parse{VPN,EAP}Value functions specific to
those individual ONC object parsers.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:427: if (key ==
signature[field].field) break;
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> I actually missed that break and were going to complain about unnecessary
> curlies, so I'd like to see the break on the next line.

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:439: }
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> I think you can just collapse these nested conditionals to:
> 
> if (signature[field].field != NULL &&
>     inner_value->GetType() != signature[field].type &&
>     parser(this, index, inner_value, network) {
>   // success
> } else {
>   // report error
> }

I preferred to give more specific error messages for the different kinds of
errors.  So I've moved things around to that end.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:440:
network->UpdatePropertyMap(index, *inner_value);
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> Do we really want to update the map for unrecognized fields? 

No, but we do need to update it before we run the parser function because the
parser functions unfortunately want to have the last say in translating those
values.  So the new code does this.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:464:
OncWifiNetworkParser::OncWifiNetworkParser() {
On 2011/12/01 18:53:18, Charlie Lee wrote:
> nit: probably unnecessary

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:663: if (value.GetType() !=
Value::TYPE_DICTIONARY) {
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> there's value.IsType() for this.

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:667: const DictionaryValue&
dict = static_cast<const DictionaryValue&>(value);
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> To avoid the cast, you could replace lines 663-667 with:
> 
> const DictionaryValue* dict = NULL;
> if (!value.GetAsDictionary(&dict)) {
>   // error
> }

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.cc:676: break;
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> Lines 668-676 shorter:
> 
> std::string provider_type_string;
> if (!dict.GetString("Type", &provider_type_string))
>   break;

Done.

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8759014/diff/1/chrome/browser/chromeos/cros/on...
chrome/browser/chromeos/cros/onc_network_parser.h:86: // Parses a nested ONC
object with the given mapper and parser function.
On 2011/12/01 13:09:10, Mattias Nissler wrote:
> You should mention how it handles errors and such.

Done.

[kmixter1, 2011-12-02 07:54:18]

PTAL - current status is that unit tests pass, wifi configuration works, but
parts of VPN configuration are lost somewhere in Chrome before being sent to
flimflam.  Would like to land this on Friday even if the VPN support isn't fully
baked.

[Mattias Nissler (ping if slow), 2011-12-02 12:23:30]

LGTM. Added Steven to the reviewers list, since we'll need OWNERS approval.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:136: Network* wifi_network);
nit: as far as my experience goes, we mostly put newlines before visibility
labels.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:149: // Base for virtual
private networks.
nit: Is it really a base class?

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:159: Network* network);
newline before start of comment

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:166: const std::string&
auth_type);
ditto

[Charlie Lee, 2011-12-02 21:21:20]

lgtm

[stevenjb, 2011-12-02 21:34:45]

I didn't look too closely at the ONC parser code, but I do have a couple
comments.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:550: Network* network) {
We should keep the check before doing the static cast. Really, we should do a
proper test and return false if |network| is not TYPE_WIFI.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:553: bool bool_value = false;
nit: I'm not a fan of extracting something as two different types. I'd rather
have lightweight wrappers, e.g. std::string ValueAsString(value) and
ValueAsBool() and call those instead.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:599: Network* network) {
Same comment about static cast.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:605:
value.GetAsBoolean(&bool_value);
same comment about multiple extraction.

[kmixter1, 2011-12-03 00:20:34]

Comments resolved, unit tests pass

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:550: Network* network) {
On 2011/12/02 21:34:45, Steven Bennetts wrote:
> We should keep the check before doing the static cast. Really, we should do a
> proper test and return false if |network| is not TYPE_WIFI.

Done.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:553: bool bool_value = false;
On 2011/12/02 21:34:45, Steven Bennetts wrote:
> nit: I'm not a fan of extracting something as two different types. I'd rather
> have lightweight wrappers, e.g. std::string ValueAsString(value) and
> ValueAsBool() and call those instead.

Done.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:599: Network* network) {
On 2011/12/02 21:34:45, Steven Bennetts wrote:
> Same comment about static cast.

Done.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:605:
value.GetAsBoolean(&bool_value);
On 2011/12/02 21:34:45, Steven Bennetts wrote:
> same comment about multiple extraction.

Done.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:136: Network* wifi_network);
On 2011/12/02 12:23:30, Mattias Nissler wrote:
> nit: as far as my experience goes, we mostly put newlines before visibility
> labels.

Done.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:149: // Base for virtual
private networks.
On 2011/12/02 12:23:30, Mattias Nissler wrote:
> nit: Is it really a base class?

Done.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:159: Network* network);
On 2011/12/02 12:23:30, Mattias Nissler wrote:
> newline before start of comment

Done.

http://codereview.chromium.org/8759014/diff/5019/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.h:166: const std::string&
auth_type);
On 2011/12/02 12:23:30, Mattias Nissler wrote:
> ditto

Done.

[kmixter1, 2011-12-03 01:37:45]

PTAL - it is now working to configure flimflam with the proper configureservice
for openvpn.  I can get far enough to start trying to connect to VPN but have
found that we're missing the functionality on flimflam side to pull openvpn
certs from NSS.

[Mattias Nissler (ping if slow), 2011-12-05 09:33:32]

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:510: 
we usually put "// static" comments in the implementation in the line before the
implementation of a static member function (also below).

[stevenjb, 2011-12-05 18:38:18]

For future reference: Patchset comments would help identify which deltas have
significant changes, vs. rebases, vs. addressing nits.

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:288: LOG(WARNING) << "Network
" << network->name() << " had problems.";
Maybe "failed to parse nested object" instead of "had problems"?

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:503: if (expected !=
network->type()) {
Since this is unexpected, we should really log a warning here, not just a vlog.

http://codereview.chromium.org/8759014/diff/22001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8759014/diff/22001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.h:116: static std::string
ConvertValueToString(const base::Value& value);
Do any of these need to be static members, or could they just be in an anonymous
namespace in the file?

[kmixter1, 2011-12-05 20:34:42]

PTAL - current state is that openvpn is still not connecting, but the next
hurdle is on the flimflam side, so I'm planning to land this change as an
incremental improvement.

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:288: LOG(WARNING) << "Network
" << network->name() << " had problems.";
On 2011/12/05 18:38:18, Steven Bennetts wrote:
> Maybe "failed to parse nested object" instead of "had problems"?

I get caught up on the "parse" verb used here and throughout this code because I
think of parsing as turning text into an internal representation, but we're
really partially translating it at the same time we're doing that.  So I'd think
of type-mismatch or unknown field as a parse error, but the ones that happen in
ParseNestedObject as translation errors.  It's a nit, I admit, so I'll just
change it to be consistent with the rest and say parse.

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:503: if (expected !=
network->type()) {
On 2011/12/05 18:38:18, Steven Bennetts wrote:
> Since this is unexpected, we should really log a warning here, not just a
vlog.

Done.

http://codereview.chromium.org/8759014/diff/20002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:510: 
On 2011/12/05 09:33:32, Mattias Nissler wrote:
> we usually put "// static" comments in the implementation in the line before
the
> implementation of a static member function (also below).

Done.

http://codereview.chromium.org/8759014/diff/22001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.h (right):

http://codereview.chromium.org/8759014/diff/22001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.h:116: static std::string
ConvertValueToString(const base::Value& value);
On 2011/12/05 18:38:18, Steven Bennetts wrote:
> Do any of these need to be static members, or could they just be in an
anonymous
> namespace in the file?

Done.

[Mattias Nissler (ping if slow), 2011-12-05 21:03:44]

http://codereview.chromium.org/8759014/diff/22006/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/22006/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:492: if
(!inner_value->IsType(signature[field_index].type)) {
Do we crash here if inner_value is NULL (i.e. field not present)?

[kmixter1, 2011-12-05 21:37:39]

http://codereview.chromium.org/8759014/diff/22006/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/onc_network_parser.cc (right):

http://codereview.chromium.org/8759014/diff/22006/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:492: if
(!inner_value->IsType(signature[field_index].type)) {
On 2011/12/05 21:03:44, Mattias Nissler wrote:
> Do we crash here if inner_value is NULL (i.e. field not present)?

That would be an internal error but I've added a CHECK above to make the cause
more clear.

[stevenjb, 2011-12-05 22:18:54]

LGTM

[commit-bot: I haz the power, 2011-12-05 23:41:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmixter@chromium.org/8759014/23010

[commit-bot: I haz the power, 2011-12-06 06:14:21]

Change committed as 113120