This adds Cellular to the Shill to ONC translation.

This adds Cellular to the Shill to ONC translation.

It also populates the Name property when translating from Shill to ONC, and adds a ConnectionState property in ONC that indicates the current connection state of the network.

(TBR'ing OWNERS because it's just refactoring: moving symbols into more specific namespaces)

TBR=zelidrag@chromium.org,glotov@chromium.org,mnissler@chromium.org
BUG=none

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

[Greg Spencer (Chromium), 2013-01-17 22:50:23]

Hi Phillipp, here are some changes I have so that I can use your ONC conversions
to convert from Shill to ONC for the networking API.

One question I have: when a property is read-only in Shill, do you just map it
in the signature, or do you have to translate it manually so that you don't set
it when going ONC->Shill, but do populate it when going the other way.  An
example of this is the Name field: I need to see it going Shill->ONC, but need
to not to set it when going ONC->Shill unless it's a VPN.

[pneubeck (no reviews), 2013-01-18 10:27:41]

The latter, as I did for the name field:
 - don't set the shill name in the signature but write a comment
 - translate it manually only in Shill->ONC
You did it already right for the status.

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

https://codereview.chromium.org/11962048/diff/12001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1094:
root_dict->GetStringWithoutPathExpansion(onc::network_config::kType,
onc::toplevel_config::kType

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer.cc:77: if
(!certificate.GetString(network_config::kGUID, &guid) || guid.empty()) {
(onc::)kGUID, see comment in onc_constant.h

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_certificate_importer_unittest.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_certificate_importer_unittest.cc:90:
certificate->GetStringWithoutPathExpansion(network_config::kGUID, guid);
(onc::)kGUID, see comment in onc_constant.h

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_constants.h (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:22: // Top Level ONC.
remove old comment
add:

// ToplevelConfiguration.
namespace toplevel_config {
  ... kType[];
...
} ...

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:40: CHROMEOS_EXPORT extern const char
kGUID[];
should really be identical for both certificates and networks -> keep in the
chromeos::onc namespace

(otherwise add certificate::kGUID)

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:53: namespace network_type {
We didn't differentiate type values from field names in other cases, too. E.g.
in vpn:: kOpenVPN is both field and value.
A nice side-effect of that was, that you can do (onc_validator.cc:437)
  result->GetStringWithoutPathExpansion(kType, &type);
  RequireField(*result, type);


If we still want to change it, I would prefer to
 - rename to kType*, e.g. kCellular to kTypeCellular
 - and move them into network_config namespace

I can adapt the remaining types in a follow-up CL.

I'm fine with both variants, where differentiating types values/fields names is
formally more correct.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:168: CHROMEOS_EXPORT extern const char
kType[];
that one slipped in because of the workaround in the onc spec. Should be
replaced by the above toplevel_config::kType

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_normalizer.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_normalizer.cc:159: RemoveEntryUnless(network,
network_type::kEthernet,
the second arg has to be a field name, thus
  network_config::kEthernet

same for the next two lines.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:217: const OncFieldSignature
cellular_fields[] = {
We'll need documentation for these fields. I'll set up an addendum document
besides the onc_spec.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:291: // This field is converted during
translation, see onc_translator_*.
mention that it is read-only and/or converted only in Shill->ONC
(onc_translator_shill_to_onc.*)

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:298: { network_config::kGUID, NULL,
&kStringSignature },
(onc::)kGUID

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:310: { encrypted::kType, NULL,
&kStringSignature },
toplevel_config::kType

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translation_tables.h (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translation_tables.h:22: CHROMEOS_EXPORT extern const
StringTranslationEntry kNetworkTypeTable[];
What for would we export these?

The idea was to translate parts of ONC by using (e.g. for the WiFi sub-object):
TranslateShillServiceToONCPart(shill_dict, &kWiFiSignature);
TranslateONCObjectToShill(onc_wifi, &kWiFiSignature);

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_onc_to_shill.cc:125:
onc_object_->GetStringWithoutPathExpansion(network_config::kType, &type);
vpn::kType

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:90: if (onc_signature_ ==
&kNetworkConfigurationSignature)
to make it consistent with further down, add braces here too.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:151: network_config::kType);
vpn::kType

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:155: if
(onc_object_->GetStringWithoutPathExpansion(network_config::kType,
vpn::kType

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:187: // Since Name is a read
only field in Shill unless it's a VPN, is is copied
is is -> it is

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:197:
onc_object_->SetString(network_config::kState, status::kConnected);
better use a helper variable shill_state and set once after the if

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:71: !root.GetString(network_config::kType,
&onc_type) ||
toplevel_config::kType

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

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:392: kValidTypes))
toplevel_config::kType

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:400:
result->GetStringWithoutPathExpansion(network_config::kType, &type);
here too

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:421: if
(!ValidateObjectDefault(kNetworkConfigurationSignature,
add
  using namespace onc::network_config;

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:456: allRequiredExist &= type.empty() ||
RequireField(*result, type);
  RequireField(*result, type)
doesn't work anymore.
Will have to be
 if (type == network_type::kWifi)
    allRequiredExist &= RequireField(*result, network_config::kWifi);
 else if ...

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:749: bool allRequiredExist =
RequireField(*result, network_config::kGUID);
kGUID without namespace (with the 'using namespace', will be certificate::kGUID)

[Greg Spencer (Chromium), 2013-01-18 22:27:43]

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_constants.h (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:22: // Top Level ONC.
On 2013/01/18 10:27:41, pneubeck wrote:
> remove old comment
> add:
> 
> // ToplevelConfiguration.
> namespace toplevel_config {
>   ... kType[];
> ...
> } ...

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:40: CHROMEOS_EXPORT extern const char
kGUID[];
On 2013/01/18 10:27:41, pneubeck wrote:
> should really be identical for both certificates and networks -> keep in the
> chromeos::onc namespace
> 
> (otherwise add certificate::kGUID)

Added certificate::kGUID

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:53: namespace network_type {
On 2013/01/18 10:27:41, pneubeck wrote:
> If we still want to change it, I would prefer to
>  - rename to kType*, e.g. kCellular to kTypeCellular
>  - and move them into network_config namespace

Well, but the network_type namespace nicely groups them so that you know what
"all" set kAllTypes could possibly refer to.
 
> I can adapt the remaining types in a follow-up CL.
> 
> I'm fine with both variants, where differentiating types values/fields names
is
> formally more correct.

I'd like to make them separate: it's formally more correct, and is less prone to
error if the string for a value changes but a field name doesn't (for instance).
 Granted, we have control over both through the spec, but it seems better to let
them vary independently and understand which thing (value or field) is being
referred to in the code.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:168: CHROMEOS_EXPORT extern const char
kType[];
On 2013/01/18 10:27:41, pneubeck wrote:
> that one slipped in because of the workaround in the onc spec. Should be
> replaced by the above toplevel_config::kType

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_normalizer.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_normalizer.cc:159: RemoveEntryUnless(network,
network_type::kEthernet,
On 2013/01/18 10:27:41, pneubeck wrote:
> the second arg has to be a field name, thus
>   network_config::kEthernet
> 
> same for the next two lines.

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:217: const OncFieldSignature
cellular_fields[] = {
On 2013/01/18 10:27:41, pneubeck wrote:
> We'll need documentation for these fields. I'll set up an addendum document
> besides the onc_spec.

OK.  I think almost all of them are read-only fields.

Given that, and the fact that I think read-only is a pretty common thing among
these fields, do you think it makes sense to have a bitfield in
OncFieldSignature that indicate which fields are writable?  Then we could
probably eliminate some functions in the translators where they're only handling
the read-only aspects and not actual translation.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:291: // This field is converted during
translation, see onc_translator_*.
On 2013/01/18 10:27:41, pneubeck wrote:
> mention that it is read-only and/or converted only in Shill->ONC
> (onc_translator_shill_to_onc.*)

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:298: { network_config::kGUID, NULL,
&kStringSignature },
On 2013/01/18 10:27:41, pneubeck wrote:
> (onc::)kGUID

Changed to certificate::kGUID.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:310: { encrypted::kType, NULL,
&kStringSignature },
On 2013/01/18 10:27:41, pneubeck wrote:
> toplevel_config::kType

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translation_tables.h (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translation_tables.h:22: CHROMEOS_EXPORT extern const
StringTranslationEntry kNetworkTypeTable[];
On 2013/01/18 10:27:41, pneubeck wrote:
> What for would we export these?
> 
> The idea was to translate parts of ONC by using (e.g. for the WiFi
sub-object):
> TranslateShillServiceToONCPart(shill_dict, &kWiFiSignature);
> TranslateONCObjectToShill(onc_wifi, &kWiFiSignature);

I needed to have these, and the functions below because the JavaScript is
passing in an ONC "Type" for the type of networks it wants to get back from
"getVisibleNetworks", and so I need to translate the Shill type into an ONC type
in order to filter it properly.

See line 195 here for an example usage: 
https://codereview.chromium.org/11975015
diff/27001/chrome/browser/chromeos/extensions/networking_private_api.cc

For this I need to translate the value and not the entire object.

I could rearrange the code in that example to translate the Shill service object
first and then see if it's the right type, but that seems inefficient, since I
can avoid the entire translation if I just look at the type.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_onc_to_shill.cc:125:
onc_object_->GetStringWithoutPathExpansion(network_config::kType, &type);
On 2013/01/18 10:27:41, pneubeck wrote:
> vpn::kType

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:90: if (onc_signature_ ==
&kNetworkConfigurationSignature)
On 2013/01/18 10:27:41, pneubeck wrote:
> to make it consistent with further down, add braces here too.

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:151: network_config::kType);
On 2013/01/18 10:27:41, pneubeck wrote:
> vpn::kType

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:155: if
(onc_object_->GetStringWithoutPathExpansion(network_config::kType,
On 2013/01/18 10:27:41, pneubeck wrote:
> vpn::kType

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:187: // Since Name is a read
only field in Shill unless it's a VPN, is is copied
On 2013/01/18 10:27:41, pneubeck wrote:
> is is -> it is

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:197:
onc_object_->SetString(network_config::kState, status::kConnected);
On 2013/01/18 10:27:41, pneubeck wrote:
> better use a helper variable shill_state and set once after the if

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:71: !root.GetString(network_config::kType,
&onc_type) ||
On 2013/01/18 10:27:41, pneubeck wrote:
> toplevel_config::kType

Done.

[pneubeck (no reviews), 2013-01-21 08:32:18]

LGTM
if you address the remaining comments (also the previous ones in
chromeos/network/onc/onc_validator.cc)

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_translation_tables.h (right):

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translation_tables.h:22: CHROMEOS_EXPORT extern const
StringTranslationEntry kNetworkTypeTable[];
On 2013/01/18 22:27:44, Greg Spencer (Chromium) wrote:
> On 2013/01/18 10:27:41, pneubeck wrote:
> > What for would we export these?
> > 
> > The idea was to translate parts of ONC by using (e.g. for the WiFi
> sub-object):
> > TranslateShillServiceToONCPart(shill_dict, &kWiFiSignature);
> > TranslateONCObjectToShill(onc_wifi, &kWiFiSignature);
> 
> I needed to have these, and the functions below because the JavaScript is
> passing in an ONC "Type" for the type of networks it wants to get back from
> "getVisibleNetworks", and so I need to translate the Shill type into an ONC
type
> in order to filter it properly.
> 
> See line 195 here for an example usage: 
> https://codereview.chromium.org/11975015
> diff/27001/chrome/browser/chromeos/extensions/networking_private_api.cc
> 
> For this I need to translate the value and not the entire object.
> 
> I could rearrange the code in that example to translate the Shill service
object
> first and then see if it's the right type, but that seems inefficient, since I
> can avoid the entire translation if I just look at the type.

The nice thing is, that the signatures work not only for dictionaries but also
for simple values.
So, no need to change a lot:

Just add a new OncValueSignature for the relevant value to onc_signature.cc and
.h (e.g. instead of kStringSignature).
Afterwards you can apply the TranslateONCObjectToShill /
TranslateShillServiceToONCPart functions with that signature to the single value
/ or extract from a Shill dict only that value.

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:197: if
(shill_dictionary_->GetString(flimflam::kStateProperty, &state)) {
GetStringWithoutPathExpansion (I know that it's ugly :-/ )

[pneubeck (no reviews), 2013-01-21 09:02:33]

One last thing:

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:57: CHROMEOS_EXPORT extern const char
kAllTypes[];
This should be near its usage in networking_private_api.*
It's never used as part of a configuration.

[pneubeck (no reviews), 2013-01-21 09:26:40]

Ok, I lied :-) Still found something.

Not directly related to the read-only flag, but I'm currently thinking about the
more critical problem that this CL extends the 'original' ONC (which describes
what we accept from CPanel) about status fields and thereby changes the
validation of such configurations.

It would be nice to at least keep the validation to the pure configuration part
of ONC.
Even better would be a parameter to select the extension to verify for.

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.cc:36: const char kState[] = "State";
In line with chromeos/network/network_state.h would be
"ConnectionState".
As this name will go into an ONC extension spec, I'd prefer the more
verbose/unambiguous name.

Btw. below you still have the namespace "status" (instead of state)

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:207: { wifi::kBSSID, flimflam::kWifiBSsid,
&kStringSignature },
the BSSID should be read-only (according to shill's service-api.txt)

[Greg Spencer (Chromium), 2013-01-22 19:10:54]

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

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:392: kValidTypes))
On 2013/01/18 10:27:41, pneubeck wrote:
> toplevel_config::kType

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:400:
result->GetStringWithoutPathExpansion(network_config::kType, &type);
On 2013/01/18 10:27:41, pneubeck wrote:
> here too

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:421: if
(!ValidateObjectDefault(kNetworkConfigurationSignature,
On 2013/01/18 10:27:41, pneubeck wrote:
> add
>   using namespace onc::network_config;

Done.

https://codereview.chromium.org/11962048/diff/12001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.cc:456: allRequiredExist &= type.empty() ||
RequireField(*result, type);
On 2013/01/18 10:27:41, pneubeck wrote:
>   RequireField(*result, type)
> doesn't work anymore.
> Will have to be
>  if (type == network_type::kWifi)
>     allRequiredExist &= RequireField(*result, network_config::kWifi);
>  else if ...

Done.

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.cc:36: const char kState[] = "State";
On 2013/01/21 09:26:40, pneubeck wrote:
> In line with chromeos/network/network_state.h would be
> "ConnectionState".
> As this name will go into an ONC extension spec, I'd prefer the more
> verbose/unambiguous name.

Yeah, you're right.  Fixed.

> 
> Btw. below you still have the namespace "status" (instead of state)

Fixed.

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_constants.h:57: CHROMEOS_EXPORT extern const char
kAllTypes[];
On 2013/01/21 09:02:34, pneubeck wrote:
> This should be near its usage in networking_private_api.*
> It's never used as part of a configuration.

Well, but what if we start doing validation for ONC that comes from the UI (even
if it only happens in DEBUG mode)?  The validation code is going to need to know
about the possible values for that type.

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_signature.cc:207: { wifi::kBSSID, flimflam::kWifiBSsid,
&kStringSignature },
On 2013/01/21 09:26:40, pneubeck wrote:
> the BSSID should be read-only (according to shill's service-api.txt)

Done.

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

https://codereview.chromium.org/11962048/diff/13002/chromeos/network/onc/onc_...
chromeos/network/onc/onc_translator_shill_to_onc.cc:197: if
(shill_dictionary_->GetString(flimflam::kStateProperty, &state)) {
On 2013/01/21 08:32:18, pneubeck wrote:
> GetStringWithoutPathExpansion (I know that it's ugly :-/ )

Done.

[commit-bot: I haz the power, 2013-01-22 22:28:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11962048/26046

[commit-bot: I haz the power, 2013-01-23 00:00:51]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-23 00:55:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gspencer@chromium.org/11962048/19003

[commit-bot: I haz the power, 2013-01-23 03:32:35]

Change committed as 178228