Set onc_source UI data parameter when importing ONC.

Set onc_source UI data parameter when importing ONC.

BUG=chromium-os:23124
TEST=Set up a policy-configured ONC network, observe that the network settings are locked down and the network cannot be removed.

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

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

This is based on Ken's ONC parsing refactoring CL. It's missing support for
parsing cert- and VPN-related UI data properties. Since that's hard to test for
me ATM, I figured I'd just do the WiFi bits now so we have the groundwork done.

Steven: Main reviewer
Ken: Parser-related changes
Julian: chrome/browser/policy

[pastarmovj, 2011-12-06 09:23:09]

LGTM as far as the policy code goes.

[Nikita (slow), 2011-12-06 09:35:16]

Refactoring CL was reverted due to clang errors.

[Mattias Nissler (ping if slow), 2011-12-06 11:59:19]

On 2011/12/06 09:35:16, Nikita Kostylev wrote:
> Refactoring CL was reverted due to clang errors.

Yep, I'm aware of that and tracking progress closely :)

[kmixter1, 2011-12-06 12:52:37]

I'm not sure I follow the intention of ui_data here.  From what I see at first
glance, it looks like it's compiled from ONC and contains properties are
recommended and their default values if applicable.

I was thinking it would be a simple dictionary like:
{ "Origin": "UserPolicy",
  "NetworkConfiguration": { "GUID": ..., rest of original NetConfig }
}

Then writing functions like IsRecommended("VPN.Host") and
GetRecommendedValue("VPN.Host") should be pretty straight-forward (in either JS
or C++ - not sure which would be required).

Then the UI would end up using the original ONC blob to compute what's
recommended or not.  That has the desirable trait that if/when we rewrite
network config ui's to be webui and talk ONC directly that this format is one
they'll already need to know.

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:40: ONC_SOURCE_USER_IMPORT,
Oh nice - so you will know which came from manual imports.

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:78: static const char
kPropertyVPNCaCertNss[];
why do we have to have copies of all the property names here?

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

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:47: { NULL }
Ouch, nice catch.

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:65:
NetworkUIData::kPropertyEAPAnonymousIdentity },
why can't these use the same name as in ONC?

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:657: !all_recommended &&
I was thinking "." meant something a little different - not so much that every
field was recommended (as a shorthand) but that the entity itself was
recommended.  So a recommended certificate can be removed and a recommended
network can be forgotten.  But properties within either can still be mandatory. 
Does that make sense?

[Mattias Nissler (ping if slow), 2011-12-06 13:37:59]

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_ui_data.h:78: static const char
kPropertyVPNCaCertNss[];
On 2011/12/06 12:52:37, kmixter1 wrote:
> why do we have to have copies of all the property names here?

Because I didn't want to leak ONC structural details into the internal data
representation, which at the moment still is PropertyIndex to value mappings. I
could use PropertyIndex values directly, but saving values directly will cause
trouble if the enum definition changes. Since I didn't want to use flimflam
property names either, I introduced new strings.

If/Once we switch to use ONC for the internal network config interchange format,
I'll happily change this to work on top of ONC directly. However, if we end up
doing so, this code will be most probably obsoleted anyway.

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

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:65:
NetworkUIData::kPropertyEAPAnonymousIdentity },
On 2011/12/06 12:52:37, kmixter1 wrote:
> why can't these use the same name as in ONC?

See other comment.

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:657: !all_recommended &&
On 2011/12/06 12:52:37, kmixter1 wrote:
> I was thinking "." meant something a little different - not so much that every
> field was recommended (as a shorthand) but that the entity itself was
> recommended.  So a recommended certificate can be removed and a recommended
> network can be forgotten.  But properties within either can still be
mandatory. 
> Does that make sense?

Ah, so I didn't understand what your intention with "." was. Does having
recommended certs and networks actually make sense? For networks, I guess it
doesn't, since the user can still select which network to use anyway. Then, the
only reason for recommended networks would be for the user to be able to remove
network entries, but do we really want to support this use case? Also how, to
bring recommended networks back once you've removed them? Regarding certs, I
cannot see a use case for recommended certs at all. Is there one?

[stevenjb, 2011-12-06 22:22:53]

I'm not following closely enough to have much perspective on the names or the
meaning of '.' (or the logic surrounding that bit of code), so I will leave it
to ken to review that part, but everything else lgtm.

[Mattias Nissler (ping if slow), 2011-12-07 19:10:47]

On 2011/12/06 22:22:53, stevenjb wrote:
> I'm not following closely enough to have much perspective on the names or the
> meaning of '.' (or the logic surrounding that bit of code), so I will leave it
> to ken to review that part, but everything else lgtm.

Regarding CL sequencing:

This one adds UIData parsing from ONC into the ui_data property. After our
discussion earlier, I'll redo this tomorrow, and I'll also need to change most
of the already-committed "Infrastructure for accessing and manipulating network
UI data" to not keep separate copies of the metadata in ui_data, but read ONC
directly.

"Add support for recommended settings to the internet details dialog" is on top
of that and adjusts the webui to handle user-editable settings properly.

"Proper management for policy-configured networks" currently also relies on this
CL to add and remove networks configured through policy correctly when policy
changes.

[kmixter1, 2011-12-07 23:08:29]

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

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:657: !all_recommended &&
On 2011/12/06 13:37:59, Mattias Nissler wrote:
> On 2011/12/06 12:52:37, kmixter1 wrote:
> > I was thinking "." meant something a little different - not so much that
every
> > field was recommended (as a shorthand) but that the entity itself was
> > recommended.  So a recommended certificate can be removed and a recommended
> > network can be forgotten.  But properties within either can still be
> mandatory. 
> > Does that make sense?
> 
> Ah, so I didn't understand what your intention with "." was. Does having
> recommended certs and networks actually make sense? For networks, I guess it
> doesn't, since the user can still select which network to use anyway. Then,
the
> only reason for recommended networks would be for the user to be able to
remove
> network entries, but do we really want to support this use case? Also how, to
> bring recommended networks back once you've removed them? Regarding certs, I
> cannot see a use case for recommended certs at all. Is there one?

I added "." just for completeness in the specification.  With the way we were
specifying the Recommended field, we could express a value of any scalar field
as being recommended but we had no way to make things in arrays be recommended. 
Conveniently most things in arrays that could be made recommended were objects,
and so adding "." closed that gap.  Whether or not anyone would ever want it, I
don't know.  I'm quite happy to punt on that capability indefinitely.

[stevenjb, 2011-12-08 02:26:35]

OK, I'm working on getting up to speed on all of this.

It's not clear to me yet whether or not we need to store UIData in flimflam, but
I definitely agree that we do not need another set of names for UI properties. I
think tying them directly to ONC fields is perfectly fine. Eventually I'd like
to see an ONC blob passed directly to web based configuration UI, with only
validation in between. 

Sorry I didn't notice this when reviewing
http://codereview.chromium.org/8728030/; somehow at the time I thought those
were the ONC definitions and not new ones.

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

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:474: const base::Value&
value,
|value| here is an ONC dictionary entry, yes? We should maybe name it better.
Since this has to be a dictionary, shouldn't we pass it as such? Also, this
appears to be a public function, but it seems like it should be private?

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:626: OncValueSignature*
signature) const {
|signature| is a list of signatures, yes, not a pointer to a single signature?
We should name it appropriately, e.g. signatures, or signature_list.

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:658:
recommended.find(signature[i].ui_data_key) == recommended.end();
I'm confused by this. Above, we appear to be filling in recommended<> from
dict["Recommended"], which is ONC data. Why are we searching for ui_data_key
here?

http://codereview.chromium.org/8804020/diff/3001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/onc_network_parser.cc:665:
ui_data.SetProperty(signature[i].ui_data_key, property_ui_data);
I can see why it is convenient to save this off, and if we're not going to parse
ONC data on every login, then it makes sense to store it in flimflam, but I
don't understand why we aren't just using signature[i].field.

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

Ken, Steven, I've change this to not parse UI meta data in the ONC and store it
in network->ui_data() as discussed last week. With that gone, I think the rest
of this is now good enough to land. PTAL.

[kmixter1, 2011-12-13 01:44:07]

In your last comment you meant "store it in network_->onc()" right?  Generally
looks good but need a little clarification of what ui_data continues to store.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_ui_data.h:93: DictionaryValue properties_;
I'm still not clear on what properties_ will contain here on out.  Is your plan
to do a compilation step over all properties from ONC and store in here that
they come from policy instead of looking up by consulting the onc_ field in the
network?

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

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:235: Network*
OncNetworkParser::ParseNetwork(int n) {
Why the reordering of ParseNetwork and ParseCertificates?  I generally think of
certificates coming first since networks can refer to them.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:315:
ui_data.FillDictionary(network->ui_data());
This name could be more descriptive.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:318:
network->onc()->MergeDictionary(&info);
Seems like merging isn't what you want.  Don't you want an exact match after
this?  Aka, if a property was previous specified but isn't from info, you don't
want it in the Network object.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:334:
network->UpdatePropertyMap(PROPERTY_INDEX_UI_DATA, ui_data_string_value);
What things are still stored in ui_data?

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:545: if (key ==
"Recommended")
Maybe a comment of where this key is handled?

[Mattias Nissler (ping if slow), 2011-12-13 13:52:19]

PTAL. It would be awesome if we could get this settled till Thursday, since
otherwise it'll have to wait till I'm back from vacation the week after
Christmas.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_ui_data.h:93: DictionaryValue properties_;
On 2011/12/13 01:44:07, kmixter1 wrote:
> I'm still not clear on what properties_ will contain here on out.  Is your
plan
> to do a compilation step over all properties from ONC and store in here that
> they come from policy instead of looking up by consulting the onc_ field in
the
> network?

Note that this gets removed in the other CL. This one is just about the
ONCNetworkParser side. I've now remove the changes from this file, since they
belong to the other CL.

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

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:235: Network*
OncNetworkParser::ParseNetwork(int n) {
On 2011/12/13 01:44:07, kmixter1 wrote:
> Why the reordering of ParseNetwork and ParseCertificates?  I generally think
of
> certificates coming first since networks can refer to them.

This was in an effort to re-synchronize the ordering in the header with that of
the implementation file (which the style guide says "should" be done "as much as
possible"). Turns out that there are much more inconsistencies, which I've fixed
as well now.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:315:
ui_data.FillDictionary(network->ui_data());
On 2011/12/13 01:44:07, kmixter1 wrote:
> This name could be more descriptive.

What name? FillDictionary? ui_data()? I'm not following.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:318:
network->onc()->MergeDictionary(&info);
On 2011/12/13 01:44:07, kmixter1 wrote:
> Seems like merging isn't what you want.  Don't you want an exact match after
> this?  Aka, if a property was previous specified but isn't from info, you
don't
> want it in the Network object.

We created a new network in line 310, so merging is perfectly fine. The
alternative would be to do:

scoped_ptr<DictionaryValue> onc(info.DeepCopy());
network->onc()->Swap(onc.get());

which seems more confusing.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:334:
network->UpdatePropertyMap(PROPERTY_INDEX_UI_DATA, ui_data_string_value);
On 2011/12/13 01:44:07, kmixter1 wrote:
> What things are still stored in ui_data?

Only the ONC source, which must go to ui_data, since when ONC drops a network,
we need to know which are the old policy networks to clean up.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:545: if (key ==
"Recommended")
On 2011/12/13 01:44:07, kmixter1 wrote:
> Maybe a comment of where this key is handled?

Done.

[Mattias Nissler (ping if slow), 2011-12-13 13:52:19]

PTAL. It would be awesome if we could get this settled till Thursday, since
otherwise it'll have to wait till I'm back from vacation the week after
Christmas.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_ui_data.h:93: DictionaryValue properties_;
On 2011/12/13 01:44:07, kmixter1 wrote:
> I'm still not clear on what properties_ will contain here on out.  Is your
plan
> to do a compilation step over all properties from ONC and store in here that
> they come from policy instead of looking up by consulting the onc_ field in
the
> network?

Note that this gets removed in the other CL. This one is just about the
ONCNetworkParser side. I've now remove the changes from this file, since they
belong to the other CL.

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

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:235: Network*
OncNetworkParser::ParseNetwork(int n) {
On 2011/12/13 01:44:07, kmixter1 wrote:
> Why the reordering of ParseNetwork and ParseCertificates?  I generally think
of
> certificates coming first since networks can refer to them.

This was in an effort to re-synchronize the ordering in the header with that of
the implementation file (which the style guide says "should" be done "as much as
possible"). Turns out that there are much more inconsistencies, which I've fixed
as well now.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:315:
ui_data.FillDictionary(network->ui_data());
On 2011/12/13 01:44:07, kmixter1 wrote:
> This name could be more descriptive.

What name? FillDictionary? ui_data()? I'm not following.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:318:
network->onc()->MergeDictionary(&info);
On 2011/12/13 01:44:07, kmixter1 wrote:
> Seems like merging isn't what you want.  Don't you want an exact match after
> this?  Aka, if a property was previous specified but isn't from info, you
don't
> want it in the Network object.

We created a new network in line 310, so merging is perfectly fine. The
alternative would be to do:

scoped_ptr<DictionaryValue> onc(info.DeepCopy());
network->onc()->Swap(onc.get());

which seems more confusing.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:334:
network->UpdatePropertyMap(PROPERTY_INDEX_UI_DATA, ui_data_string_value);
On 2011/12/13 01:44:07, kmixter1 wrote:
> What things are still stored in ui_data?

Only the ONC source, which must go to ui_data, since when ONC drops a network,
we need to know which are the old policy networks to clean up.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:545: if (key ==
"Recommended")
On 2011/12/13 01:44:07, kmixter1 wrote:
> Maybe a comment of where this key is handled?

Done.

[kmixter1, 2011-12-13 19:21:02]

lgtm

Just nits from me at this point.  Steven still needs to review.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/network_ui_data.h (right):

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_ui_data.h:93: DictionaryValue properties_;
On 2011/12/13 13:52:19, Mattias Nissler wrote:
> On 2011/12/13 01:44:07, kmixter1 wrote:
> > I'm still not clear on what properties_ will contain here on out.  Is your
> plan
> > to do a compilation step over all properties from ONC and store in here that
> > they come from policy instead of looking up by consulting the onc_ field in
> the
> > network?
> 
> Note that this gets removed in the other CL. This one is just about the
> ONCNetworkParser side. I've now remove the changes from this file, since they
> belong to the other CL.

OK - makes sense now that I've reviewed the other CL.

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

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:315:
ui_data.FillDictionary(network->ui_data());
On 2011/12/13 13:52:19, Mattias Nissler wrote:
> On 2011/12/13 01:44:07, kmixter1 wrote:
> > This name could be more descriptive.
> 
> What name? FillDictionary? ui_data()? I'm not following.

FillDictionary.  I think I didn't understand how the data was moving around
before.  Existing code is OK, but could FillDictionary first clear the
dictionary before setting its ONCSource field?  What's not entirely clear is if
you intend to ever use FillDictionary on a non-empty dictionary to do partial
updates.  I don't think you do, so clearing it would make that obvious.

Another option that I think is even clearer would be if the code was
set_ui_data(ui_data.Serialize()) and ui_data were just a JSON string instead of
a dictionary.  That's assuming nothing else needs to peek into the UIData
Dictionary.  But if it did, I'd think you want to put NetworkUIData in the
Network class instead of its intermediate representation (DictionaryValue).

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:318:
network->onc()->MergeDictionary(&info);
On 2011/12/13 13:52:19, Mattias Nissler wrote:
> On 2011/12/13 01:44:07, kmixter1 wrote:
> > Seems like merging isn't what you want.  Don't you want an exact match after
> > this?  Aka, if a property was previous specified but isn't from info, you
> don't
> > want it in the Network object.
> 
> We created a new network in line 310, so merging is perfectly fine. The
> alternative would be to do:
> 
> scoped_ptr<DictionaryValue> onc(info.DeepCopy());
> network->onc()->Swap(onc.get());
> 
> which seems more confusing.

Agreed - Didn't look far enough up.  Could you mention in the comment that the
existing ONC is expected to be empty?

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

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:854: // This contains the ONC
blob if this network was imported via ONC. This is
nit: This contains the deserialized ONC blob...

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/policy/netwo...
File chrome/browser/policy/network_configuration_updater.cc (right):

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/policy/netwo...
chrome/browser/policy/network_configuration_updater.cc:39:
chromeos::NetworkUIData::ONC_SOURCE_DEVICE_POLICY,
Could you add a TODO(crosbug.com/24152) to note that we still need to make it
possible to create shared/device networks (down through ConfigureService).

[stevenjb, 2011-12-14 00:27:38]

lgtm

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

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:362: // The list of
Recommended key is only of interest to the UI code and the UI
nit: s/The list of Recommended key is/Recommended keys are/

[Mattias Nissler (ping if slow), 2011-12-14 20:46:37]

It turns out that storing the ONC dictionary in the network object doesn't work
well, since we're recreating networks much to often. I've removed the |onc_|
field in this CL and the issue in the other CL.

Nits are fixed. I assume this one still looks good to you?

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

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:315:
ui_data.FillDictionary(network->ui_data());
On 2011/12/13 19:21:03, kmixter1 wrote:
> On 2011/12/13 13:52:19, Mattias Nissler wrote:
> > On 2011/12/13 01:44:07, kmixter1 wrote:
> > > This name could be more descriptive.
> > 
> > What name? FillDictionary? ui_data()? I'm not following.
> 
> FillDictionary.  I think I didn't understand how the data was moving around
> before.  Existing code is OK, but could FillDictionary first clear the
> dictionary before setting its ONCSource field?  What's not entirely clear is
if
> you intend to ever use FillDictionary on a non-empty dictionary to do partial
> updates.  I don't think you do, so clearing it would make that obvious.

Put in the Clear() call.

> Another option that I think is even clearer would be if the code was
> set_ui_data(ui_data.Serialize()) and ui_data were just a JSON string instead
of
> a dictionary.  That's assuming nothing else needs to peek into the UIData
> Dictionary.  But if it did, I'd think you want to put NetworkUIData in the
> Network class instead of its intermediate representation (DictionaryValue).

I admit what we currently have is a compromise. Not storing NetworkUIData in
Network is due to Steven's request to keep any UI-level semantics out of
NetworkLibrary (which I think is reasonable). Using a DictionaryValue instead of
a string avoid the need to re-parse the JSON string every time we want to access
UI data, which I think is a good idea too. So honestly I'd rather keep it the
way it is now.

http://codereview.chromium.org/8804020/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:318:
network->onc()->MergeDictionary(&info);
On 2011/12/13 19:21:03, kmixter1 wrote:
> On 2011/12/13 13:52:19, Mattias Nissler wrote:
> > On 2011/12/13 01:44:07, kmixter1 wrote:
> > > Seems like merging isn't what you want.  Don't you want an exact match
after
> > > this?  Aka, if a property was previous specified but isn't from info, you
> > don't
> > > want it in the Network object.
> > 
> > We created a new network in line 310, so merging is perfectly fine. The
> > alternative would be to do:
> > 
> > scoped_ptr<DictionaryValue> onc(info.DeepCopy());
> > network->onc()->Swap(onc.get());
> > 
> > which seems more confusing.
> 
> Agreed - Didn't look far enough up.  Could you mention in the comment that the
> existing ONC is expected to be empty?

Done.

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

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/network_library.h:854: // This contains the ONC
blob if this network was imported via ONC. This is
On 2011/12/13 19:21:03, kmixter1 wrote:
> nit: This contains the deserialized ONC blob...

Obsolete now...

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

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/onc_network_parser.cc:362: // The list of
Recommended key is only of interest to the UI code and the UI
On 2011/12/14 00:27:38, stevenjb wrote:
> nit: s/The list of Recommended key is/Recommended keys are/

Done.

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/policy/netwo...
File chrome/browser/policy/network_configuration_updater.cc (right):

http://codereview.chromium.org/8804020/diff/22001/chrome/browser/policy/netwo...
chrome/browser/policy/network_configuration_updater.cc:39:
chromeos::NetworkUIData::ONC_SOURCE_DEVICE_POLICY,
On 2011/12/13 19:21:03, kmixter1 wrote:
> Could you add a TODO(crosbug.com/24152) to note that we still need to make it
> possible to create shared/device networks (down through ConfigureService).

http://codereview.chromium.org/8804021/ does that. It's already LGTM'ed and is
only waiting for this CL to land :)

[kmixter1, 2011-12-14 22:17:39]

lgtm