Set onc_source UI data parameter when importing ONC.

chrome/browser/chromeos/cros/onc_network_parser.cc

Index: chrome/browser/chromeos/cros/onc_network_parser.cc
diff --git a/chrome/browser/chromeos/cros/onc_network_parser.cc b/chrome/browser/chromeos/cros/onc_network_parser.cc
index 55f8e9a4c017fff0ece9c1fd5b8833c15e61f8d1..d1d21b58d05ffc6ccde48ce47b29e51c7ce01f32 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser.cc
@@ -4,8 +4,8 @@
 
 #include "chrome/browser/chromeos/cros/onc_network_parser.h"
 
-#include <pk11pub.h>
 #include <keyhi.h>
+#include <pk11pub.h>
 
 #include "base/base64.h"
 #include "base/json/json_value_serializer.h"
@@ -183,8 +183,10 @@ std::string ConvertValueToString(const base::Value& value) {
 
 // -------------------- OncNetworkParser --------------------
 
-OncNetworkParser::OncNetworkParser(const std::string& onc_blob)
+OncNetworkParser::OncNetworkParser(const std::string& onc_blob,
+                                   NetworkUIData::ONCSource onc_source)
     : NetworkParser(get_onc_mapper()),
+      onc_source_(onc_source),
       network_configs_(NULL),
       certificates_(NULL) {
   VLOG(2) << __func__ << ": OncNetworkParser called on " << onc_blob;
@@ -230,6 +232,23 @@ int OncNetworkParser::GetCertificatesSize() const {
   return certificates_ ? certificates_->GetSize() : 0;
 }
 
+Network* OncNetworkParser::ParseNetwork(int n) {

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

Why the reordering of ParseNetwork and ParseCertificates?  I generally think of
certificates coming first since networks can refer to them.

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

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.

+  if (!network_configs_)
+    return NULL;
+  DictionaryValue* info = NULL;
+  if (!network_configs_->GetDictionary(n, &info))
+    return NULL;
+  if (VLOG_IS_ON(2)) {
+    std::string network_json;
+    base::JSONWriter::Write(static_cast<base::Value*>(info),
+                            true, &network_json);
+    VLOG(2) << "Parsing network at index " << n
+            << ": " << network_json;
+  }
+
+  return CreateNetworkFromInfo(std::string(), *info);
+}
+
 scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
     int cert_index) {
   CHECK(certificates_);
@@ -282,23 +301,6 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
   return NULL;
 }
 
-Network* OncNetworkParser::ParseNetwork(int n) {
-  if (!network_configs_)
-    return NULL;
-  DictionaryValue* info = NULL;
-  if (!network_configs_->GetDictionary(n, &info))
-    return NULL;
-  if (VLOG_IS_ON(2)) {
-    std::string network_json;
-    base::JSONWriter::Write(static_cast<base::Value*>(info),
-                            true, &network_json);
-    VLOG(2) << "Parsing network at index " << n
-            << ": " << network_json;
-  }
-
-  return CreateNetworkFromInfo(std::string(), *info);
-}
-
 Network* OncNetworkParser::CreateNetworkFromInfo(
     const std::string& service_path,
     const DictionaryValue& info) {
@@ -306,6 +308,16 @@ Network* OncNetworkParser::CreateNetworkFromInfo(
   if (type == TYPE_UNKNOWN)  // Return NULL if cannot parse network type.
     return NULL;
   scoped_ptr<Network> network(CreateNewNetwork(type, service_path));
+
+  // Initialize UI data.
+  NetworkUIData ui_data;
+  ui_data.set_onc_source(onc_source_);
+  ui_data.FillDictionary(network->ui_data());

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

This name could be more descriptive.

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

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

[kmixter1, 2011/12/13 19:21:03]

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).

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

Put in the Clear() call.
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.

+
+  // Copy ONC to the network object.
+  network->onc()->MergeDictionary(&info);

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

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.

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

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.

[kmixter1, 2011/12/13 19:21:03]

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

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

Done.

+
+  // Parse all properties recursively.
   if (!ParseNestedObject(network.get(),
                          "NetworkConfiguration",
                          static_cast<const base::Value&>(info),
@@ -314,11 +326,19 @@ Network* OncNetworkParser::CreateNetworkFromInfo(
     LOG(WARNING) << "Network " << network->name() << " failed to parse.";
     return NULL;
   }
+
+  // Update the UI data property.
+  std::string ui_data_json;
+  base::JSONWriter::Write(network->ui_data(), false, &ui_data_json);
+  base::StringValue ui_data_string_value(ui_data_json);
+  network->UpdatePropertyMap(PROPERTY_INDEX_UI_DATA, ui_data_string_value);

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

What things are still stored in ui_data?

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

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.

+
   if (VLOG_IS_ON(2)) {
     VLOG(2) << "Created Network '" << network->name()
             << "' from info. Path:" << service_path
             << " Type:" << ConnectionTypeToString(type);
   }
+
   return network.release();
 }
 
@@ -522,6 +542,9 @@ bool OncNetworkParser::ParseNestedObject(Network* network,
   for (DictionaryValue::key_iterator iter = dict->begin_keys();
        iter != dict->end_keys(); ++iter) {
     const std::string& key = *iter;
+    if (key == "Recommended")

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

Maybe a comment of where this key is handled?

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

Done.

+      continue;
+
     base::Value* inner_value = NULL;
     dict->GetWithoutPathExpansion(key, &inner_value);
     CHECK(inner_value != NULL);
@@ -552,6 +575,7 @@ bool OncNetworkParser::ParseNestedObject(Network* network,
       any_errors = true;
       continue;
     }
+
     if (VLOG_IS_ON(2)) {
       std::string value_json;
       base::JSONWriter::Write(inner_value, true, &value_json);