chromeos: Move ownership of ash VPN provider list from chrome into ash

chromeos: Move ownership of ash VPN provider list from chrome into ash

This will make it easier to convert VPNDelegate to a mojo interface for
mustash.

The ash VPNDelegate maintains the list of providers. It is responsible for
adding the built-in OpenVPN provider. Chrome only provides the list of
extension-backed VPN providers.

Collpase VPNProvider::Key and VPNProvider into a single struct because
there is a 1:1 mapping between the two structs, so there is no need for
a separate key.

BUG=651148
TEST=added VPNDelegateTest to ash_unittests

Committed: https://crrev.com/2dd804b7babf9e02ecfec278fb628e67dcbaa837
Cr-Commit-Position: refs/heads/master@{#433745}

[James Cook, 2016-11-18 18:45:05]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 18:46:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-11-18 18:47:10]

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

[James Cook, 2016-11-18 18:47:11]

Steven, please take a look.

I've got an in-progress follow-up CL that converts things to mojo. I'm planning
to rename VPNDelegate to VpnList (since it's a model/controller in ash) and
VPNDelegateChromeOS to VpnListClient (since it will be the "client" interface in
Chrome that shows the add network pages). Lemme know if you would prefer
different names.

[commit-bot: I haz the power, 2016-11-18 21:48:35]

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

[commit-bot: I haz the power, 2016-11-18 21:48:35]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-11-18 23:22:41]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 23:23:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 00:42:39]

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

[commit-bot: I haz the power, 2016-11-19 00:42:44]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-11-21 19:17:30]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 19:18:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-11-21 19:23:30]

Steven, please take a look.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.h:86: virtual void
ShowAddPage(const VPNProvider::Key& key) = 0;
I'm going to move ShowAddPage() to the mojo SystemTrayClient interface in the
next CL. I didn't do it in this CL because I would end up with a
VPNDelegateChromeOS that had no delegate methods.

[stevenjb, 2016-11-21 20:23:18]

https://codereview.chromium.org/2510083006/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/2510083006/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.cc:33: }
This appears to be only used in vpn_list_view.cc, can we move it there and
remove the network_state.h dependency here? It makes it a little more clear that
this is really just UI glue code.

https://codereview.chromium.org/2510083006/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.cc:70: AddBuiltInProvider();
nit: Even though I'm not sure that the order matters here, it makes more sense
to me to add the built in provider first.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.cc:84:
l10n_util::GetStringUTF8(IDS_ASH_STATUS_TRAY_VPN_BUILT_IN_PROVIDER)));
Can we make this an empty string and modify the View code to use this string
when key.third_party == false? Since this is really model code, it shouldn't be
dependent on strings.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.h:50: // Human-readable name.
// Human-readable name if key.third_party is true, otherwise empty.
Also s/name/third_party_provider_name/ to ensure that we aren't using it
directly without checking key.third_party.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.h:86: virtual void
ShowAddPage(const VPNProvider::Key& key) = 0;
On 2016/11/21 19:23:30, James Cook wrote:
> I'm going to move ShowAddPage() to the mojo SystemTrayClient interface in the
> next CL. I didn't do it in this CL because I would end up with a
> VPNDelegateChromeOS that had no delegate methods.

Acknowledged.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:453: }
std::string vpn_name = key.third_party ? name :
l10n_util::GetStringUTF8(IDS_ASH_STATUS_TRAY_VPN_BUILT_IN_PROVIDER)

[commit-bot: I haz the power, 2016-11-21 20:30:30]

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

[commit-bot: I haz the power, 2016-11-21 20:30:31]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-11-21 22:05:16]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 22:05:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-11-21 23:32:45]

Patchset #4 (id:60001) has been deleted

[James Cook, 2016-11-21 23:56:27]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 23:57:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-11-21 23:57:32]

Description was changed from

==========
chromeos: Move ownership of ash VPN provider list from chrome into ash

This will make it easier to convert VPNDelegate to a mojo interface for
mustash.

The ash VPNDelegate maintains the list of providers. It is responsible for
adding the built-in OpenVPN provider. Chrome only provides the list of
extension-backed VPN providers.

BUG=651148
TEST=added VPNDelegateTest to ash_unittests
==========

to

==========
chromeos: Move ownership of ash VPN provider list from chrome into ash

This will make it easier to convert VPNDelegate to a mojo interface for
mustash.

The ash VPNDelegate maintains the list of providers. It is responsible for
adding the built-in OpenVPN provider. Chrome only provides the list of
extension-backed VPN providers.

Collpase VPNProvider::Key and VPNProvider into a single struct because
there is a 1:1 mapping between the two structs, so there is no need for
a separate key.

BUG=651148
TEST=added VPNDelegateTest to ash_unittests
==========

[James Cook, 2016-11-21 23:58:38]

Steven, please take another look.

I also collapsed VPNProvider::Key and VPNProvider into a single struct. There is
a 1:1 mapping between the two, so having the key by itself is not useful (and I
found having a separate key to be confusing, as it implies the mapping might not
be 1:1).

https://codereview.chromium.org/2510083006/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/2510083006/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.cc:33: }
On 2016/11/21 20:23:17, stevenjb wrote:
> This appears to be only used in vpn_list_view.cc, can we move it there and
> remove the network_state.h dependency here? It makes it a little more clear
that
> this is really just UI glue code.
> 

Done.

https://codereview.chromium.org/2510083006/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.cc:70: AddBuiltInProvider();
On 2016/11/21 20:23:17, stevenjb wrote:
> nit: Even though I'm not sure that the order matters here, it makes more sense
> to me to add the built in provider first.

Done.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_delegate.cc (right):

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.cc:84:
l10n_util::GetStringUTF8(IDS_ASH_STATUS_TRAY_VPN_BUILT_IN_PROVIDER)));
On 2016/11/21 20:23:18, stevenjb wrote:
> Can we make this an empty string and modify the View code to use this string
> when key.third_party == false? Since this is really model code, it shouldn't
be
> dependent on strings.

Done.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_delegate.h (right):

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_delegate.h:50: // Human-readable name.
On 2016/11/21 20:23:18, stevenjb wrote:
> // Human-readable name if key.third_party is true, otherwise empty.
> Also s/name/third_party_provider_name/ to ensure that we aren't using it
> directly without checking key.third_party.

Done.

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2510083006/diff/40001/ash/common/system/chrom...
ash/common/system/chromeos/network/vpn_list_view.cc:453: }
On 2016/11/21 20:23:18, stevenjb wrote:
> std::string vpn_name = key.third_party ? name :
> l10n_util::GetStringUTF8(IDS_ASH_STATUS_TRAY_VPN_BUILT_IN_PROVIDER)

Done. (I just passed in the whole provider.)

[commit-bot: I haz the power, 2016-11-22 00:07:21]

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

[commit-bot: I haz the power, 2016-11-22 00:07:21]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stevenjb, 2016-11-22 00:08:36]

Thanks for doing the extra cleanup, lgtm!

https://codereview.chromium.org/2510083006/diff/100001/ash/common/system/chro...
File ash/common/system/chromeos/network/vpn_list_view.cc (right):

https://codereview.chromium.org/2510083006/diff/100001/ash/common/system/chro...
ash/common/system/chromeos/network/vpn_list_view.cc:79: }
Thanks for moving this, I meant to suggest that before.

[James Cook, 2016-11-22 00:14:05]

jamescook@chromium.org changed reviewers:
+ bruthig@chromium.org

[James Cook, 2016-11-22 00:14:06]

+bruthig just FYI, since I see you have an open bug for MD tray VPN. This
touches VpnListView, but not the list items themselves, so hopefully it
shouldn't conflict with your work.

[James Cook, 2016-11-22 00:14:19]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-11-22 00:14:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 02:15:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479773659295100,
"parent_rev": "a6bf3045ab5c1bbec2333cbbe62715fa900f63a7", "commit_rev":
"a4ba41ed21c576c07b0a9d28320591b3fe53460b"}

[commit-bot: I haz the power, 2016-11-22 02:16:23]

Description was changed from

==========
chromeos: Move ownership of ash VPN provider list from chrome into ash

This will make it easier to convert VPNDelegate to a mojo interface for
mustash.

The ash VPNDelegate maintains the list of providers. It is responsible for
adding the built-in OpenVPN provider. Chrome only provides the list of
extension-backed VPN providers.

Collpase VPNProvider::Key and VPNProvider into a single struct because
there is a 1:1 mapping between the two structs, so there is no need for
a separate key.

BUG=651148
TEST=added VPNDelegateTest to ash_unittests
==========

to

==========
chromeos: Move ownership of ash VPN provider list from chrome into ash

This will make it easier to convert VPNDelegate to a mojo interface for
mustash.

The ash VPNDelegate maintains the list of providers. It is responsible for
adding the built-in OpenVPN provider. Chrome only provides the list of
extension-backed VPN providers.

Collpase VPNProvider::Key and VPNProvider into a single struct because
there is a 1:1 mapping between the two structs, so there is no need for
a separate key.

BUG=651148
TEST=added VPNDelegateTest to ash_unittests
==========

[commit-bot: I haz the power, 2016-11-22 02:16:24]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-11-22 02:20:58]

Description was changed from

==========
chromeos: Move ownership of ash VPN provider list from chrome into ash

This will make it easier to convert VPNDelegate to a mojo interface for
mustash.

The ash VPNDelegate maintains the list of providers. It is responsible for
adding the built-in OpenVPN provider. Chrome only provides the list of
extension-backed VPN providers.

Collpase VPNProvider::Key and VPNProvider into a single struct because
there is a 1:1 mapping between the two structs, so there is no need for
a separate key.

BUG=651148
TEST=added VPNDelegateTest to ash_unittests
==========

to

==========
chromeos: Move ownership of ash VPN provider list from chrome into ash

This will make it easier to convert VPNDelegate to a mojo interface for
mustash.

The ash VPNDelegate maintains the list of providers. It is responsible for
adding the built-in OpenVPN provider. Chrome only provides the list of
extension-backed VPN providers.

Collpase VPNProvider::Key and VPNProvider into a single struct because
there is a 1:1 mapping between the two structs, so there is no need for
a separate key.

BUG=651148
TEST=added VPNDelegateTest to ash_unittests

Committed: https://crrev.com/2dd804b7babf9e02ecfec278fb628e67dcbaa837
Cr-Commit-Position: refs/heads/master@{#433745}
==========

[commit-bot: I haz the power, 2016-11-22 02:20:59]

Patchset 5 (id:??) landed as
https://crrev.com/2dd804b7babf9e02ecfec278fb628e67dcbaa837
Cr-Commit-Position: refs/heads/master@{#433745}