Use Managed properties for Preferred and Provider.

Use Managed properties for Preferred and Provider.

This includes some necessary ONC translation fixes.

BUG=279351
R=armansito@chromium.org, pneubeck@chromium.org

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

[stevenjb, 2014-08-18 23:28:36]

armansito@ - please check the cellular serving operator changes.

Note: I recognize that there is extra work being done to simulate what
GetManagedProperties (should) return, but I think it is important that we go
through this step so that when we switched to calling GetManagedProperties (and
relying on it for all properties), we are only debugging that change and are
(hopefully) not making any JS/html changes at the same time.

[pneubeck (no reviews), 2014-08-19 16:24:03]

only reviewed .js so far.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:27: *              
       'recommended' - gets the recommended or active value
why should recommended fallback to the active value?
The caller could do this instead by calling
  value = getManagedValue(..., recommended)
  if (!recommended)
    value = getManagedValue(..., active)

to make this corner case (?) more explicit.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:32: if (!property)
be careful about the implicit conversion into boolean here!
please replace by data === undefined
or check 'key in data'.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:37: return
getEffectiveValue(property);
this doesn't look like it would return the recommended value if the user has
modified the property.
Should instead be:

function getRecommendedValue(propertyDict) {  // a'la getEffectValue
  if (property['UserEditable'])
    return property['UserPolicy']
  if (property['DeviceEditable'])
    return property['DevicePolicy']
  // No value recommended by policy.
  return undefined
}

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:40: if ('Active' in
property)
nit: add a comment before this line saying "Otherwise get the 'active' value".

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:66: * @return {*}
the property value or undefined.
'property value or dictionary if it exists. Otherwise undefined.'

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:71: var dictKey =
key.substr(0, index);
optional nit:
 dictKey -> keyComponent?

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:73: if (!data)
again, be careful about implicit conversion into boolean.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:76: index =
key.indexOf('.');
optional nit: could be merged with line 69 by
  while(true) {
    index = ...
    if (index < 0)
      break;
    ...
  }

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1191: assert('WiFi'
in data, 'WiFi network has no WiFi object' + networkName);
good catch!

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1256:
getManagedValue(data, 'Cellular.NetworkTechnology');
hm. You could have done a
  var cellular = getManagedProperty(data, 'Cellular')
  ...
    ...technology... = getManagedValue(data, 'NetworkTechnology')

to save repetitions of the 'Cellular' constant prefix.
(maybe similar for other network types)

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1394: if (propValue
== undefined)
even better catch. boolean conversion seems really evil ^^

[armansito, 2014-08-19 16:47:17]

serving operator bits lgtm

[stevenjb, 2014-08-19 18:41:47]

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:27: *              
       'recommended' - gets the recommended or active value
On 2014/08/19 16:24:02, pneubeck wrote:
> why should recommended fallback to the active value?
> The caller could do this instead by calling
>   value = getManagedValue(..., recommended)
>   if (!recommended)
>     value = getManagedValue(..., active)
> 
> to make this corner case (?) more explicit.

That is actually what it does, just the comment is wrong. Fixed.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:32: if (!property)
On 2014/08/19 16:24:02, pneubeck wrote:
> be careful about the implicit conversion into boolean here!
> please replace by data === undefined
> or check 'key in data'.
Actually, this check isn't necessary since typeof undefined == 'undefined' (i.e.
!= 'object). Removed.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:37: return
getEffectiveValue(property);
On 2014/08/19 16:24:02, pneubeck wrote:
> this doesn't look like it would return the recommended value if the user has
> modified the property.
> Should instead be:
> 
> function getRecommendedValue(propertyDict) {  // a'la getEffectValue
>   if (property['UserEditable'])
>     return property['UserPolicy']
>   if (property['DeviceEditable'])
>     return property['DevicePolicy']
>   // No value recommended by policy.
>   return undefined
> }

Bah, ugh, right, fixed.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:40: if ('Active' in
property)
On 2014/08/19 16:24:02, pneubeck wrote:
> nit: add a comment before this line saying "Otherwise get the 'active' value".

Done.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:66: * @return {*}
the property value or undefined.
On 2014/08/19 16:24:02, pneubeck wrote:
> 'property value or dictionary if it exists. Otherwise undefined.'

Done.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:71: var dictKey =
key.substr(0, index);
On 2014/08/19 16:24:02, pneubeck wrote:
> optional nit:
>  dictKey -> keyComponent?

Sure.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:73: if (!data)
On 2014/08/19 16:24:02, pneubeck wrote:
> again, be careful about implicit conversion into boolean.
Fixed.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:76: index =
key.indexOf('.');
On 2014/08/19 16:24:02, pneubeck wrote:
> optional nit: could be merged with line 69 by
>   while(true) {
>     index = ...
>     if (index < 0)
>       break;
>     ...
>   }
Yeah, I'm not a big fan of that pattern, but duplicated code also sucks.
Changed.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1191: assert('WiFi'
in data, 'WiFi network has no WiFi object' + networkName);
On 2014/08/19 16:24:02, pneubeck wrote:
> good catch!

Acknowledged.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1256:
getManagedValue(data, 'Cellular.NetworkTechnology');
On 2014/08/19 16:24:02, pneubeck wrote:
> hm. You could have done a
>   var cellular = getManagedProperty(data, 'Cellular')
>   ...
>     ...technology... = getManagedValue(data, 'NetworkTechnology')
> 
> to save repetitions of the 'Cellular' constant prefix.
> (maybe similar for other network types)
Yeah, I thought of that, but wanted to limit the changes here. Also, eventually
everything will use getManagedValue, so this will all be able to be a lot more
data driven, so I think I will leave it for now.

https://codereview.chromium.org/482243002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1394: if (propValue
== undefined)
On 2014/08/19 16:24:02, pneubeck wrote:
> even better catch. boolean conversion seems really evil ^^
I still kind of hate JS, but I'm slowly getting used to it...

[pneubeck (no reviews), 2014-08-19 19:26:38]

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:25: *              
       'active' (default)  - gets the active value
These type strings could be more enum like (e.g. use string constants) or to
simplify the semantics provide separate functions for each case: getActiveValue,
getRecommendedValue, ...

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1389: if
(recommended)
... recommended != undefined

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:349:
settings_dict->Set(settings_dict_key, dict);
nit:
maybe clarify at the comment of this function whether keys with '.' are
supported or not.
Accordingly this should indeed either be 'Set' as it is or
SetWithoutPathExpansion.

Maybe say something like 'base::DictionaryValue's path expansion is applied to
|settings_dict_key|.' ?

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:380: 
nit: drop empty line

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:452:
base::DictionaryValue* provider_dictionary = new base::DictionaryValue;
I have the impression that 'provider' is not the right term in this case, but
maybe I'm wrong.
Maybe something like vpn_subdictionary / vpn_details .. ?

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:453:
vpn_dictionary->Set(onc_provider_type, provider_dictionary);
This will not work: The possible subdictionaries are OpenVPN, L2TP, IPsec
There is a VPN type 'L2TP-IPsec' but no dictionary 'L2TP-IPsec'.

I mean, it will work for now, but not anymore when you change to
GetManagedProperties

Since you set only the username for L2TP below, this should be easy to fix

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:463:
provider_dictionary->SetString(::onc::vpn::kUsername, username);
The username is never set in IPsec but only in L2TP

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:470:
::onc::vpn::kHost,
optional nit: having the same key with different prefixes two times in a call is
rather confusing. could you add argument comments that one refers to the key in
the result and one to the path in ONC ?

OR:
just use the same path and don't pass in 'vpn_dictionary' but 'dictionary' ?
That would also ensure that the structure of the resulting dictionary matches
ONC.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:471: new
base::StringValue(provider_host),
memory leak, see other comment.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1381: new
base::FundamentalValue(priority),
this will leak the FundamentalValue.
Please, please change the functions SetManagedValueDictionary(Ex) to take a
(const Value& value) to prevent this mistake.

https://codereview.chromium.org/482243002/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/482243002/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:191: if (onc_field_name ==
::onc::openvpn::kRemoteCertKU ||
Why?
These exceptions are for OpenVPN properties that Shill returns not as strings
but as the actual types.

https://codereview.chromium.org/482243002/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:240: void
ShillToONCTranslator::TranslateVPN() {
I'll take a more thorough look at this part tomorrow.

[stevenjb, 2014-08-19 23:20:18]

Fixed unit tests, addressed feedback, PTAL.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:25: *              
       'active' (default)  - gets the active value
On 2014/08/19 19:26:37, pneubeck wrote:
> These type strings could be more enum like (e.g. use string constants) or to
> simplify the semantics provide separate functions for each case:
getActiveValue,
> getRecommendedValue, ...

The reason I didn't use separate functions was to avoid code duplication. I
could use wrapper functions I suppose, but I think an enum-like dictionary would
make sense here.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1389: if
(recommended)
On 2014/08/19 19:26:37, pneubeck wrote:
> ... recommended != undefined

Sigh. Thanks. Done.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc
(right):

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:349:
settings_dict->Set(settings_dict_key, dict);
On 2014/08/19 19:26:38, pneubeck wrote:
> nit:
> maybe clarify at the comment of this function whether keys with '.' are
> supported or not.
> Accordingly this should indeed either be 'Set' as it is or
> SetWithoutPathExpansion.
> 
> Maybe say something like 'base::DictionaryValue's path expansion is applied to
> |settings_dict_key|.' ?

This was not intended to support path expansion, settings_dict_key is expected
to be an ONC key. I added a comment and changed to SetWithout...

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:380: 
On 2014/08/19 19:26:37, pneubeck wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:452:
base::DictionaryValue* provider_dictionary = new base::DictionaryValue;
On 2014/08/19 19:26:37, pneubeck wrote:
> I have the impression that 'provider' is not the right term in this case, but
> maybe I'm wrong.
> Maybe something like vpn_subdictionary / vpn_details .. ?

Renamed provider_type_dictionary since that is what the key is. (Which, FWIW,
seems a little wonky to me, and certainly adds code complexity).

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:453:
vpn_dictionary->Set(onc_provider_type, provider_dictionary);
On 2014/08/19 19:26:38, pneubeck wrote:
> This will not work: The possible subdictionaries are OpenVPN, L2TP, IPsec
> There is a VPN type 'L2TP-IPsec' but no dictionary 'L2TP-IPsec'.
> 
> I mean, it will work for now, but not anymore when you change to
> GetManagedProperties
> 
> Since you set only the username for L2TP below, this should be easy to fix

Gah. OK, so... what? Let me look at the doc again...

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:463:
provider_dictionary->SetString(::onc::vpn::kUsername, username);
On 2014/08/19 19:26:37, pneubeck wrote:
> The username is never set in IPsec but only in L2TP
OK, I think I maybe have this right now...

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:470:
::onc::vpn::kHost,
On 2014/08/19 19:26:38, pneubeck wrote:
> optional nit: having the same key with different prefixes two times in a call
is
> rather confusing. could you add argument comments that one refers to the key
in
> the result and one to the path in ONC ?
> 
> OR:
> just use the same path and don't pass in 'vpn_dictionary' but 'dictionary' ?
> That would also ensure that the structure of the resulting dictionary matches
> ONC.

I really don't like using Value::Set with '.' separators, especially in
networking code where Shill sometimes uses '.' in key names.

I'll try to clarify the comments, but... there's no way this isn't going to be
confusing for now.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:471: new
base::StringValue(provider_host),
On 2014/08/19 19:26:38, pneubeck wrote:
> memory leak, see other comment.

Bah, this is what happens when... nevermind. Fixed.

https://codereview.chromium.org/482243002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc:1381: new
base::FundamentalValue(priority),
On 2014/08/19 19:26:38, pneubeck wrote:
> this will leak the FundamentalValue.
> Please, please change the functions SetManagedValueDictionary(Ex) to take a
> (const Value& value) to prevent this mistake.
Fixed.

https://codereview.chromium.org/482243002/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/482243002/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:191: if (onc_field_name ==
::onc::openvpn::kRemoteCertKU ||
On 2014/08/19 19:26:38, pneubeck wrote:
> Why?
> These exceptions are for OpenVPN properties that Shill returns not as strings
> but as the actual types.

SaveCredentials is a Service property, not a Service.OpenVPN property, so this
doesn't belong here for the Shill -> ONC translation. I fixed up the unit tests
to deal with the asymetric nature of Provider properties and added an explicit
check for the correct SaveCredentials type in TranslateVPN().

https://codereview.chromium.org/482243002/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:240: void
ShillToONCTranslator::TranslateVPN() {
On 2014/08/19 19:26:38, pneubeck wrote:
> I'll take a more thorough look at this part tomorrow.

OK, hopefully the need for these changes will be more clear with the updated
tests. (I had been testing with actual Shill results up to now).

[pneubeck (no reviews), 2014-08-20 08:16:16]

https://codereview.chromium.org/482243002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/482243002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:45: // Otherwise
get the Active value (defalt behavior).
typo: defalt -> default

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_signature.cc:293: { ::onc::network_config::kPriority,
&kIntegerSignature},
Please add some comment that this is not supported for policy or not part of the
ONC spec.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:253: LOG(ERROR) << "Shill
VPN object with no Provider.Type";
alternatively, instead of catching a missing input (which will be handled by
TranslateStringToONC as well),
you could check the result of TranslateStringToONC, to also catch unknown Shill
types.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:260: if
(provider->GetStringWithoutPathExpansion(shill::kHostProperty,
The translation tables were meant for the cases where the translation is
symmetric.

please comment out
  { ::onc::vpn::kHost, shill::kProviderHostProperty},
in
  vpn_fields in onc_translation_tables.cc
with a note
 // Is handled differently for Shill -> ONC, see onc_translator_*. )

and add the translation in the other direction explicitly to TranslateVPN() in
onc_translator_onc_to_shill.cc:

std::string host;
if (onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kHost, &host) {
  shill_dictionary_->SetStringWithoutPathExpansion(shill::kHostProperty, host);
}

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:267:
shill_dictionary_->GetWithoutPathExpansion(shill::kSaveCredentialsProperty,
like the Host property above,
please comment out in
  openvpn_fields and ipsec_fields

and add it explicitly to TranslateOpenVPN and to a new TranslateIPsec.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:279: SetNestedOncValue(
nit: wasn't set before and could be removed.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:446: // Note: this may
create an empty nested object which is necessary in case
Instead of potentially creating empty nested objects, SetNestedOncValue could
create the missing Dictionary.

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
File chromeos/test/data/network/shill_output_l2tpipsec.json (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
chromeos/test/data/network/shill_output_l2tpipsec.json:4: "Provider": {
When can they finally fix this in Shill... maybe I should just do it myself, I
guess...

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
File chromeos/test/data/network/shill_output_openvpn_with_errors.json (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
chromeos/test/data/network/shill_output_openvpn_with_errors.json:5: // String
instead of Int.
nit: Int -> Bool

[stevenjb, 2014-08-20 17:18:38]

I added Priority to the ONC spec, this seemed like a reasonable addition to me,
but we can discuss offline if you disagree.

PTAL

https://codereview.chromium.org/482243002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (right):

https://codereview.chromium.org/482243002/diff/80001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:45: // Otherwise
get the Active value (defalt behavior).
On 2014/08/20 08:16:15, pneubeck wrote:
> typo: defalt -> default

Done.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_signature.cc:293: { ::onc::network_config::kPriority,
&kIntegerSignature},
On 2014/08/20 08:16:15, pneubeck wrote:
> Please add some comment that this is not supported for policy or not part of
the
> ONC spec.

Hmm, we may need to chat about this. I would rather add this to the ONC spec as
a readonly property. (I could be convinced to use a bool 'Preferred', but I
think that any property we need for Settings should be part of the spec).
Alternately we could maybe add a 'UserSettings' dictionary that gets blindly
translated.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:253: LOG(ERROR) << "Shill
VPN object with no Provider.Type";
On 2014/08/20 08:16:16, pneubeck wrote:
> alternatively, instead of catching a missing input (which will be handled by
> TranslateStringToONC as well),
> you could check the result of TranslateStringToONC, to also catch unknown
Shill
> types.
Done.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:260: if
(provider->GetStringWithoutPathExpansion(shill::kHostProperty,
On 2014/08/20 08:16:16, pneubeck wrote:
> The translation tables were meant for the cases where the translation is
> symmetric.
> 
> please comment out
>   { ::onc::vpn::kHost, shill::kProviderHostProperty},
> in
>   vpn_fields in onc_translation_tables.cc
> with a note
>  // Is handled differently for Shill -> ONC, see onc_translator_*. )
> 
> and add the translation in the other direction explicitly to TranslateVPN() in
> onc_translator_onc_to_shill.cc:
> 
> std::string host;
> if (onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kHost, &host) {
>   shill_dictionary_->SetStringWithoutPathExpansion(shill::kHostProperty,
host);
> }

Ugh. Yeah, I understand your point, but this is tedious. It seems like we have
enough special cases at this point to provide some better support for asymmetry
and nested dictionaries. Another day. Done.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:267:
shill_dictionary_->GetWithoutPathExpansion(shill::kSaveCredentialsProperty,
On 2014/08/20 08:16:16, pneubeck wrote:
> like the Host property above,
> please comment out in
>   openvpn_fields and ipsec_fields
> 
> and add it explicitly to TranslateOpenVPN and to a new TranslateIPsec.

Done.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:279: SetNestedOncValue(
On 2014/08/20 08:16:16, pneubeck wrote:
> nit: wasn't set before and could be removed.

Done.

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:446: // Note: this may
create an empty nested object which is necessary in case
On 2014/08/20 08:16:16, pneubeck wrote:
> Instead of potentially creating empty nested objects, SetNestedOncValue could
> create the missing Dictionary.
I was trying to avoid code duplication since an empty dictionary should be
harmless, but done.

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
File chromeos/test/data/network/shill_output_l2tpipsec.json (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
chromeos/test/data/network/shill_output_l2tpipsec.json:4: "Provider": {
On 2014/08/20 08:16:16, pneubeck wrote:
> When can they finally fix this in Shill... maybe I should just do it myself, I
> guess...

Acknowledged.

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
File chromeos/test/data/network/shill_output_openvpn_with_errors.json (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/test/data/netwo...
chromeos/test/data/network/shill_output_openvpn_with_errors.json:5: // String
instead of Int.
On 2014/08/20 08:16:16, pneubeck wrote:
> nit: Int -> Bool

Done.

[pneubeck (no reviews), 2014-08-20 19:49:37]

lgtm with nits

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/482243002/diff/80001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:260: if
(provider->GetStringWithoutPathExpansion(shill::kHostProperty,
On 2014/08/20 17:18:37, stevenjb wrote:
> On 2014/08/20 08:16:16, pneubeck wrote:
> > The translation tables were meant for the cases where the translation is
> > symmetric.
> > 
> > please comment out
> >   { ::onc::vpn::kHost, shill::kProviderHostProperty},
> > in
> >   vpn_fields in onc_translation_tables.cc
> > with a note
> >  // Is handled differently for Shill -> ONC, see onc_translator_*. )
> > 
> > and add the translation in the other direction explicitly to TranslateVPN()
in
> > onc_translator_onc_to_shill.cc:
> > 
> > std::string host;
> > if (onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kHost, &host) {
> >   shill_dictionary_->SetStringWithoutPathExpansion(shill::kHostProperty,
> host);
> > }
> 
> Ugh. Yeah, I understand your point, but this is tedious. It seems like we have
> enough special cases at this point to provide some better support for
asymmetry
> and nested dictionaries. Another day. Done.

Yes, it's tedious. To get rid of it, I would prefer to work on the root cause
and align ONC and the format of Shill. Neither should be seen as static.

https://codereview.chromium.org/482243002/diff/140001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/482243002/diff/140001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:126: // Returns the name
provided in |shill_dictionary_| for debugging.
nit: 'name' -> 'name of the Shill Service'

https://codereview.chromium.org/482243002/diff/140001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:560: if (!shill_dictionary_)
optional nit: DCHECK
(the constructors don't allow for shill_dictionary_ == NULL)

https://codereview.chromium.org/482243002/diff/140001/components/onc/docs/onc...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/482243002/diff/140001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:409: Priority value for this network. May be
used by the system to determine
To be of any purpose to external users, this should explain the behavior more
detailed according to how Shill handles it currently.
Whether that is really the behavior that want to have for priorities in the
future. I don't know.

Do you agree to punt the documentation part for now, to reduce visibility...
just in case?
Let's keep a TODO or a bug as a reminder to discuss this outside this CL.

[stevenjb, 2014-08-20 20:09:17]

https://codereview.chromium.org/482243002/diff/140001/components/onc/docs/onc...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/482243002/diff/140001/components/onc/docs/onc...
components/onc/docs/onc_spec.html:409: Priority value for this network. May be
used by the system to determine
On 2014/08/20 19:49:37, pneubeck wrote:
> To be of any purpose to external users, this should explain the behavior more
> detailed according to how Shill handles it currently.
> Whether that is really the behavior that want to have for priorities in the
> future. I don't know.
> 
> Do you agree to punt the documentation part for now, to reduce visibility...
> just in case?
> Let's keep a TODO or a bug as a reminder to discuss this outside this CL.

I am wary of skipping the documentation, these things tend to get forgotten
otherwise. I intentionally kept it vague because we shouldn't really be
documenting Shill behavior here, just the purpose of the property, imho.

[stevenjb, 2014-08-20 20:40:04]

https://codereview.chromium.org/482243002/diff/140001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/482243002/diff/140001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:126: // Returns the name
provided in |shill_dictionary_| for debugging.
On 2014/08/20 19:49:36, pneubeck wrote:
> nit: 'name' -> 'name of the Shill Service'

Done.

https://codereview.chromium.org/482243002/diff/140001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_shill_to_onc.cc:560: if (!shill_dictionary_)
On 2014/08/20 19:49:36, pneubeck wrote:
> optional nit: DCHECK
> (the constructors don't allow for shill_dictionary_ == NULL)

Done.

[stevenjb, 2014-08-20 20:40:29]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-08-20 20:43:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/482243002/160001

[commit-bot: I haz the power, 2014-08-21 01:03:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[commit-bot: I haz the power, 2014-08-21 01:07:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-21 01:07:10]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[stevenjb, 2014-08-21 16:05:55]

Committed patchset #9 manually as 291085 (presubmit successful).