Add an ONC property for the third-party VPN provider extension ID

Add an ONC property for the third-party VPN provider extension ID

This CL maps the third-party VPN provider extension ID (which is
stored in shill's |Provider.Host| field) to an ONC property.

BUG=460428
TEST=Extended unit tests and API test

Committed: https://crrev.com/82bb909334491a927f4e77ae5e2f0326fe4da0e2
Cr-Commit-Position: refs/heads/master@{#321238}

[bartfab (slow), 2015-03-18 18:27:15]

bartfab@chromium.org changed reviewers:
+ pneubeck@chromium.org, sadrul@chromium.org, tbarzic@chromium.org

[bartfab (slow), 2015-03-18 18:27:16]

Hi Philipp,

Could you please look at:

chromeos/network/*
chromeos/test/data/network/*
components/onc/*

Hi Sadrul,

Could you please look at:

ash/system/chromeos/network/vpn_delegate.cc

Hi Toni,

Could you please look at:

chrome/browser/extensions/api/networking_private/
networking_private_chromeos_apitest.cc

[tbarzic, 2015-03-18 18:31:16]

lgtm for networking_private_chromeos_apitest.cc

[sadrul, 2015-03-18 18:40:44]

lgtm

[pneubeck (no reviews), 2015-03-18 18:48:27]

https://codereview.chromium.org/1019033002/diff/1/ash/system/chromeos/network...
File ash/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/1019033002/diff/1/ash/system/chromeos/network...
ash/system/chromeos/network/vpn_delegate.cc:16: : third_party(true),
extension_id(extension_id) {
  extension_id(extension_id)
jesus christ... does this even work?

[bartfab (slow), 2015-03-18 19:00:47]

https://codereview.chromium.org/1019033002/diff/1/ash/system/chromeos/network...
File ash/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/1019033002/diff/1/ash/system/chromeos/network...
ash/system/chromeos/network/vpn_delegate.cc:16: : third_party(true),
extension_id(extension_id) {
On 2015/03/18 18:48:26, pneubeck wrote:
>   extension_id(extension_id)
> jesus christ... does this even work?

Sure. We have lots of code like that. The outer one is the member, the inner one
is the argument.

[pneubeck (no reviews), 2015-03-18 19:04:42]

I'm not through yet, but the test is wrong so sending what i have so far.

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:361:
AddService("stub_vpn2", "vpn2", shill::kTypeVPN, shill::kStateOnline);
maybe don't set every VPN to online, instead at most one. (but maybe it just
doesn't make a difference anywhere)

https://codereview.chromium.org/1019033002/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1019033002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:257:
"Type": "OpenVPN",
this is wrong.
type should be "ThirdPartyVPN"

[bartfab (slow), 2015-03-18 19:21:17]

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:361:
AddService("stub_vpn2", "vpn2", shill::kTypeVPN, shill::kStateOnline);
On 2015/03/18 19:04:42, pneubeck wrote:
> maybe don't set every VPN to online, instead at most one. (but maybe it just
> doesn't make a difference anywhere)

Done.

https://codereview.chromium.org/1019033002/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/1019033002/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:257:
"Type": "OpenVPN",
On 2015/03/18 19:04:42, pneubeck wrote:
> this is wrong.
> type should be "ThirdPartyVPN"

Done.

[pneubeck (no reviews), 2015-03-18 19:35:29]

lgtm, but I can also take another look later

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:361:
AddService("stub_vpn2", "vpn2", shill::kTypeVPN, shill::kStateOnline);
maybe don't set every vpn to Online but at most one. (but probably it just
doesn't make a difference anywhere)

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.cc:170:
third_party_vpn_provider_extension_id_.clear();
I found the reason why I said we must be cautious with the clear()s here.
See the documentation of this function:
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...

If you return false, the state _must not_ have changed.
Please move this clear into the
 if (vpn_provider_type_ != shill::kProviderThirdPartyVpn)
case

Then there's still one corner-case left if vpn_provider_type_ changed to 3rd
party and fails in line 187.

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.cc:240: scoped_ptr<base::DictionaryValue>
provider_property(
nit: drop a comment before this line, like:

"Shill is sending VPN provider properties in a nested dictionary (but expects to
receive a flat dictionary). |dictionary| must have the same structure."

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_unittest.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_unittest.cc:64: bool SetProperty(const
std::string& key, scoped_ptr<base::Value> value) {
nit: SetStringProperty above could call this function.

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
chromeos/network/onc/onc_translator_onc_to_shill.cc:192: if
(onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kType, &onc_type))
nit: missing {}

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
chromeos/network/onc/onc_translator_shill_to_onc.cc:275: std::string
shill_extension_id;
nit: same comment as in the other direction:
 // For third-party VPNs, |shill::kProviderHostProperty| is used to store the
 // provider's extension ID.

https://codereview.chromium.org/1019033002/diff/1/components/onc/docs/onc_spe...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/1019033002/diff/1/components/onc/docs/onc_spe...
components/onc/docs/onc_spec.html:843: <dt class="field">OpenVPN</dt>
like this field, the description for ThirdPartyVPN is missing.

https://codereview.chromium.org/1019033002/diff/1/components/onc/docs/onc_spe...
components/onc/docs/onc_spec.html:864: <span class="value">ThirdPartyVPN</span>.
You're right!
The constant is called kThirdPartyVpn
but its value is "ThirdPartyVPN"   !!!

[bartfab (slow), 2015-03-18 20:02:14]

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/1019033002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:361:
AddService("stub_vpn2", "vpn2", shill::kTypeVPN, shill::kStateOnline);
On 2015/03/18 19:35:29, pneubeck wrote:
> maybe don't set every vpn to Online but at most one. (but probably it just
> doesn't make a difference anywhere)

Eh, done :).

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.cc:170:
third_party_vpn_provider_extension_id_.clear();
On 2015/03/18 19:35:29, pneubeck wrote:
> I found the reason why I said we must be cautious with the clear()s here.
> See the documentation of this function:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/network/m...
> 
> If you return false, the state _must not_ have changed.
> Please move this clear into the
>  if (vpn_provider_type_ != shill::kProviderThirdPartyVpn)
> case
> 
> Then there's still one corner-case left if vpn_provider_type_ changed to 3rd
> party and fails in line 187.

Fixed, including the corner case.

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
chromeos/network/network_state.cc:240: scoped_ptr<base::DictionaryValue>
provider_property(
On 2015/03/18 19:35:29, pneubeck wrote:
> nit: drop a comment before this line, like:
> 
> "Shill is sending VPN provider properties in a nested dictionary (but expects
to
> receive a flat dictionary). |dictionary| must have the same structure."

Done.

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
File chromeos/network/network_state_unittest.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/network_st...
chromeos/network/network_state_unittest.cc:64: bool SetProperty(const
std::string& key, scoped_ptr<base::Value> value) {
On 2015/03/18 19:35:29, pneubeck wrote:
> nit: SetStringProperty above could call this function.

Done.

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
chromeos/network/onc/onc_translator_onc_to_shill.cc:192: if
(onc_object_->GetStringWithoutPathExpansion(::onc::vpn::kType, &onc_type))
On 2015/03/18 19:35:29, pneubeck wrote:
> nit: missing {}

Done.

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/1019033002/diff/1/chromeos/network/onc/onc_tr...
chromeos/network/onc/onc_translator_shill_to_onc.cc:275: std::string
shill_extension_id;
On 2015/03/18 19:35:29, pneubeck wrote:
> nit: same comment as in the other direction:
>  // For third-party VPNs, |shill::kProviderHostProperty| is used to store the
>  // provider's extension ID.

Done.

https://codereview.chromium.org/1019033002/diff/1/components/onc/docs/onc_spe...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/1019033002/diff/1/components/onc/docs/onc_spe...
components/onc/docs/onc_spec.html:843: <dt class="field">OpenVPN</dt>
On 2015/03/18 19:35:29, pneubeck wrote:
> like this field, the description for ThirdPartyVPN is missing.

Done.

https://codereview.chromium.org/1019033002/diff/1/components/onc/docs/onc_spe...
components/onc/docs/onc_spec.html:864: <span class="value">ThirdPartyVPN</span>.
On 2015/03/18 19:35:29, pneubeck wrote:
> You're right!
> The constant is called kThirdPartyVpn
> but its value is "ThirdPartyVPN"   !!!

Acknowledged.

[bartfab (slow), 2015-03-18 20:09:37]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-18 20:09:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
tbarzic@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1019033002/#ps60001
(title: "Updated ONC normalizer.")

[commit-bot: I haz the power, 2015-03-18 20:09:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019033002/60001

[bartfab (slow), 2015-03-18 20:22:01]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-18 20:22:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
tbarzic@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1019033002/#ps80001
(title: "Fix typo.")

[commit-bot: I haz the power, 2015-03-18 20:24:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019033002/80001

[bartfab (slow), 2015-03-18 20:25:29]

bartfab@chromium.org changed reviewers:
+ stevenjb@chromium.org

[bartfab (slow), 2015-03-18 20:25:30]

Hi Steven,

Could you do a post-land review once you are back?

[bartfab (slow), 2015-03-18 21:05:49]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-18 21:05:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
tbarzic@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1019033002/#ps100001
(title: "Fix typo.")

[commit-bot: I haz the power, 2015-03-18 21:06:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019033002/100001

[bartfab (slow), 2015-03-18 22:50:17]

The CQ bit was checked by bartfab@chromium.org

[bartfab (slow), 2015-03-18 22:50:18]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
tbarzic@chromium.org, pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1019033002/#ps120001
(title: "Fixed browser tests.")

[commit-bot: I haz the power, 2015-03-18 22:50:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1019033002/120001

[commit-bot: I haz the power, 2015-03-18 23:59:07]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-18 23:59:47]

Patchset 7 (id:??) landed as
https://crrev.com/82bb909334491a927f4e77ae5e2f0326fe4da0e2
Cr-Commit-Position: refs/heads/master@{#321238}