Set onc_source UI data parameter when importing ONC.

chrome/browser/chromeos/cros/onc_network_parser.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/cros/onc_network_parser.h"
 
+#include <keyhi.h>
 #include <pk11pub.h>
-#include <keyhi.h>
 
 #include "base/base64.h"
 #include "base/json/json_value_serializer.h"
 #include "base/json/json_writer.h"  // for debug output only.
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/cros/native_network_constants.h"
 #include "chrome/browser/chromeos/cros/native_network_parser.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "net/base/cert_database.h"
@@ skipping 157 matching lines @@
 std::string ConvertValueToString(const base::Value& value) {
   std::string value_json;
   base::JSONWriter::Write(&value, false, &value_json);
   return value_json;
 }
 
 }  // namespace
 
 // -------------------- 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;
   JSONStringValueSerializer deserializer(onc_blob);
   deserializer.set_allow_trailing_comma(true);
   scoped_ptr<base::Value> root(deserializer.Deserialize(NULL, &parse_error_));
 
   if (!root.get() || root->GetType() != base::Value::TYPE_DICTIONARY) {
     LOG(WARNING) << "OncNetworkParser received bad ONC file: " << parse_error_;
   } else {
@@ skipping 25 matching lines @@
 }
 
 int OncNetworkParser::GetNetworkConfigsSize() const {
   return network_configs_ ? network_configs_->GetSize() : 0;
 }
 
 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_);
   CHECK(static_cast<size_t>(cert_index) < certificates_->GetSize());
   CHECK(cert_index >= 0);
   base::DictionaryValue* certificate = NULL;
   certificates_->GetDictionary(cert_index, &certificate);
   CHECK(certificate);
 
   if (VLOG_IS_ON(2)) {
@@ skipping 32 matching lines @@
   }
   if (cert_type == "Client") {
     return ParseClientCertificate(cert_index, guid, certificate);
   }
 
   LOG(WARNING) << "ONC File: certificate of unknown type: " << cert_type
                << " at index " << cert_index;
   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) {
   ConnectionType type = ParseTypeFromDictionary(info);
   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),
                          network_configuration_signature,
                          ParseNetworkConfigurationValue)) {
     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();
 }
 
 Network* OncNetworkParser::CreateNewNetwork(
     ConnectionType type, const std::string& service_path) {
   Network* network = NetworkParser::CreateNewNetwork(type, service_path);
   if (network) {
     if (type == TYPE_WIFI)
       network->SetNetworkParser(new OncWifiNetworkParser());
     else if (type == TYPE_VPN)
@@ skipping 183 matching lines @@
   if (!value.IsType(base::Value::TYPE_DICTIONARY)) {
     VLOG(1) << network->name() << ": expected object of type " << onc_type;
     return false;
   }
   VLOG(2) << "Parsing nested object of type " << onc_type;
   const DictionaryValue* dict = NULL;
   value.GetAsDictionary(&dict);
   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);
     int field_index;
     for (field_index = 0; signature[field_index].field != NULL; ++field_index) {
       if (key == signature[field_index].field)
         break;
     }
     if (signature[field_index].field == NULL) {
       VLOG(1) << network->name() << ": unexpected field: "
@@ skipping 10 matching lines @@
     }
     PropertyIndex index = signature[field_index].index;
     // We need to UpdatePropertyMap now since parser might want to
     // change the mapped value.
     network->UpdatePropertyMap(index, *inner_value);
     if (!parser(this, index, *inner_value, network)) {
       VLOG(1) << network->name() << ": field not parsed: " << key;
       any_errors = true;
       continue;
     }
+
     if (VLOG_IS_ON(2)) {
       std::string value_json;
       base::JSONWriter::Write(inner_value, true, &value_json);
       VLOG(2) << network->name() << ": Successfully parsed [" << key
               << "(" << index << ")] = " << value_json;
     }
   }
   return !any_errors;
 }
 
@@ skipping 585 matching lines @@
     // on the value of AuthenticationType.
     { "L2TP-IPsec", PROVIDER_TYPE_L2TP_IPSEC_PSK },
     { "OpenVPN", PROVIDER_TYPE_OPEN_VPN },
   };
   CR_DEFINE_STATIC_LOCAL(EnumMapper<ProviderType>, parser,
       (table, arraysize(table), PROVIDER_TYPE_MAX));
   return parser.Get(type);
 }
 
 }  // namespace chromeos