Elim 'Translated' from Managed ONC dictionary

Elim 'Translated' from Managed ONC dictionary

Instead of providing a 'Translated' value in managed ONC dictionaries,
use a well understood naming convention.

This also moves the onc helper methods to their own JS module.

BUG=279351
For proxy_settings.html:
TBR=xiyuan@chromium.org

Committed: https://crrev.com/53077e70bdcc0b651c297eb4d176edf6a86480a3
Cr-Commit-Position: refs/heads/master@{#293412}

[stevenjb, 2014-09-03 17:22:12]

stevenjb@chromium.org changed reviewers:
+ armansito@chromium.org, pneubeck@chromium.org

[stevenjb, 2014-09-03 17:22:13]

I think this is probably a simpler way to handle translation, at least for now.

[armansito, 2014-09-03 21:42:03]

Mostly nits.

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

https://codereview.chromium.org/539573002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
nit: I think we omit the "(c)" bit now.

https://codereview.chromium.org/539573002/diff/40001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/539573002/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:801:
provider_properties2.SetString(shill::kHostProperty, "vpn_host2");
nit: Add an empty line here for consistency with the previous block.

https://codereview.chromium.org/539573002/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:810: }
nit: You already declared |provider_properties| and |provider_properties2|; are
the nested scopes necessary?

[pneubeck (no reviews), 2014-09-04 12:50:13]

pneubeck@chromium.org changed reviewers:
+ benchan@chromium.org

[pneubeck (no reviews), 2014-09-04 12:50:14]

+Ben
please take a look at the comment at onc_translator_shill_to_onc.cc:244
Thanks!

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

https://codereview.chromium.org/539573002/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:244:
shill_dictionary_->GetStringWithoutPathExpansion(
+Ben who ran into this issue before, see this comment in
network_connection_handler.cc:

      // TODO(benchan): Modify shill to specify the authentication type via
      // the kL2tpIpsecAuthenticationType property, so that Chrome doesn't need
      // to deduce the authentication type based on the
      // kL2tpIpsecClientCertIdProperty here (and also in VPNConfigView).

Ben, could we fix this for 39?
How would Shill deduce the Authentication Type? or would Shill just report back
what Chrome previously configured?

[stevenjb, 2014-09-04 15:34:11]

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

https://codereview.chromium.org/539573002/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/09/03 21:42:02, armansito wrote:
> nit: I think we omit the "(c)" bit now.

Done.

https://codereview.chromium.org/539573002/diff/40001/chromeos/dbus/fake_shill...
File chromeos/dbus/fake_shill_manager_client.cc (right):

https://codereview.chromium.org/539573002/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:801:
provider_properties2.SetString(shill::kHostProperty, "vpn_host2");
On 2014/09/03 21:42:02, armansito wrote:
> nit: Add an empty line here for consistency with the previous block.

Done.

https://codereview.chromium.org/539573002/diff/40001/chromeos/dbus/fake_shill...
chromeos/dbus/fake_shill_manager_client.cc:810: }
On 2014/09/03 21:42:03, armansito wrote:
> nit: You already declared |provider_properties| and |provider_properties2|;
are
> the nested scopes necessary?

It's far too easily to accidentally use 'provider_properties' instead of
'provider_properties2'. I guess I could name them better and remove the scoping.
Done.

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

https://codereview.chromium.org/539573002/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:244:
shill_dictionary_->GetStringWithoutPathExpansion(
On 2014/09/04 12:50:14, pneubeck wrote:
> +Ben who ran into this issue before, see this comment in
> network_connection_handler.cc:
> 
>       // TODO(benchan): Modify shill to specify the authentication type via
>       // the kL2tpIpsecAuthenticationType property, so that Chrome doesn't
need
>       // to deduce the authentication type based on the
>       // kL2tpIpsecClientCertIdProperty here (and also in VPNConfigView).
> 
> Ben, could we fix this for 39?
> How would Shill deduce the Authentication Type? or would Shill just report
back
> what Chrome previously configured?

I'd rather not block this on a Shill change, we can always fix this later if we
add the property to Shill. Is there anything inherently wrong with this logic
(it's pretty much just copied from internet_options_handler_strings.cc.

[armansito, 2014-09-04 15:38:53]

lgtm

[pneubeck (no reviews), 2014-09-04 18:05:21]

lgtm with nits

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

https://codereview.chromium.org/539573002/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:244:
shill_dictionary_->GetStringWithoutPathExpansion(
On 2014/09/04 15:34:11, stevenjb wrote:
> On 2014/09/04 12:50:14, pneubeck wrote:
> > +Ben who ran into this issue before, see this comment in
> > network_connection_handler.cc:
> > 
> >       // TODO(benchan): Modify shill to specify the authentication type via
> >       // the kL2tpIpsecAuthenticationType property, so that Chrome doesn't
> need
> >       // to deduce the authentication type based on the
> >       // kL2tpIpsecClientCertIdProperty here (and also in VPNConfigView).
> > 
> > Ben, could we fix this for 39?
> > How would Shill deduce the Authentication Type? or would Shill just report
> back
> > what Chrome previously configured?
> 
> I'd rather not block this on a Shill change, we can always fix this later if
we
> add the property to Shill. Is there anything inherently wrong with this logic
> (it's pretty much just copied from internet_options_handler_strings.cc.

I didn't intend to suggest blocking this. This should be clarified
independently.

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (left):

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1306:
assert('Cellular' in data,
optional nit: you could probably add this assert where you set the 'type'
member.

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:68: // For now, just
uupdate the active value. TODO(stevenjb): Eventually we
typo: uupdate -> update

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:84: // Otherwise get the
Active value (defalt behavior).
typo: defalt -> default

[stevenjb, 2014-09-04 19:41:17]

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/internet_detail.js (left):

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/internet_detail.js:1306:
assert('Cellular' in data,
On 2014/09/04 18:05:20, pneubeck wrote:
> optional nit: you could probably add this assert where you set the 'type'
> member.

Well, before we were accessing data.Cellular.Foo, which would lead to a runtime
error if Cellular were missing. Now we use getActiveValue which will fail
gracefully if Cellular is absent, i.e. the assert is unnecessary.

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/onc_data.js (right):

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:68: // For now, just
uupdate the active value. TODO(stevenjb): Eventually we
On 2014/09/04 18:05:20, pneubeck wrote:
> typo: uupdate -> update

Done.

https://codereview.chromium.org/539573002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/onc_data.js:84: // Otherwise get the
Active value (defalt behavior).
On 2014/09/04 18:05:20, pneubeck wrote:
> typo: defalt -> default

Done.

[stevenjb, 2014-09-04 19:41:41]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-09-04 19:44:00]

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

[stevenjb, 2014-09-04 23:30:41]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-09-04 23:34:15]

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

[commit-bot: I haz the power, 2014-09-05 02:55:11]

Committed patchset #6 (id:100001) as 96f0dd681e3426764953fe9b3a1831e84e6ad798

[commit-bot: I haz the power, 2014-09-10 03:35:59]

Patchset 6 (id:??) landed as
https://crrev.com/53077e70bdcc0b651c297eb4d176edf6a86480a3
Cr-Commit-Position: refs/heads/master@{#293412}