Extend vpnProvider to allow reconnections

Extend vpnProvider to allow reconnections

Upcoming changes in the Chrome OS networking daemon (shill) will allow
third party VPNs to transition "backwards" from Online->Configuring if
the default physical connection changes.  Add a "reconnect" flag so
that VPN apps can signal their compatibility with this new scheme,
and add the necessary UI changes so that Chrome can identify
reconnections and present them to the user in a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343
Committed: https://crrev.com/7288268a683040968054a3dcfedef28d9151bc1d
Cr-Commit-Position: refs/heads/master@{#381566}

[Kevin Cernekee, 2016-03-12 06:24:38]

Description was changed from

==========
Extend vpnProvider API to allow reconnections

Currently, if the system's physical network connection changes while a
VPN is connected (wifi->wifi, wifi->mobile, wifi->DOWN->ethernet, etc.)
the VPN will drop and the user will be forced to log in again.  But
many VPN protocols are robust enough to handle reconnections without
restarting the entire process from scratch.

Add a couple of new shill->Chrome events that inform the extension
about events that affect the VPN connection, gated by a new "reconnect"
parameter that extensions will use to indicate that they support the
new API.

BUG=514343
==========

to

==========
Extend vpnProvider to allow reconnections

Upcoming shill changes will allow third party VPNs to transition
"backwards" from Online->Configuring if the default physical connection
changes.  Add a "reconnect" flag so that VPN apps can signal their
compatibility with this new scheme, and add the necessary UI changes so
that Chrome can identify reconnections and present them to the user in
a sensible way.

BUG=514343
==========

[Kevin Cernekee, 2016-03-12 06:28:10]

cernekee@chromium.org changed reviewers:
+ bartfab@chromium.org, rockot@chromium.org, stevenjb@chromium.org

[Kevin Cernekee, 2016-03-12 06:53:29]

Description was changed from

==========
Extend vpnProvider to allow reconnections

Upcoming shill changes will allow third party VPNs to transition
"backwards" from Online->Configuring if the default physical connection
changes.  Add a "reconnect" flag so that VPN apps can signal their
compatibility with this new scheme, and add the necessary UI changes so
that Chrome can identify reconnections and present them to the user in
a sensible way.

BUG=514343
==========

to

==========
Extend vpnProvider to allow reconnections

Upcoming shill changes will allow third party VPNs to transition
"backwards" from Online->Configuring if the default physical connection
changes.  Add a "reconnect" flag so that VPN apps can signal their
compatibility with this new scheme, and add the necessary UI changes so
that Chrome can identify reconnections and present them to the user in
a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343
==========

[Kevin Cernekee, 2016-03-14 22:14:27]

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

[commit-bot: I haz the power, 2016-03-14 22:14:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1722453002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1722453002/20001

[commit-bot: I haz the power, 2016-03-15 01:29:11]

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

[commit-bot: I haz the power, 2016-03-15 01:29:12]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2016-03-15 23:15:39]

lgtm w/ nits

https://codereview.chromium.org/1722453002/diff/20001/extensions/browser/api/...
File extensions/browser/api/vpn_provider/vpn_service.cc (right):

https://codereview.chromium.org/1722453002/diff/20001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_service.cc:122:
vpn_service_->SetActiveConfiguration(nullptr);
{} around all clauses of multi-line if

[Kevin Cernekee, 2016-03-16 00:54:44]

cernekee@chromium.org changed reviewers:
+ emaxx@chromium.org

[Kevin Cernekee, 2016-03-16 00:54:44]

emaxx - could you please take a look at the VPN files when you have a moment?

https://codereview.chromium.org/1722453002/diff/20001/extensions/browser/api/...
File extensions/browser/api/vpn_provider/vpn_service.cc (right):

https://codereview.chromium.org/1722453002/diff/20001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_service.cc:122:
vpn_service_->SetActiveConfiguration(nullptr);
On 2016/03/15 23:15:39, stevenjb wrote:
> {} around all clauses of multi-line if

Done.

[emaxx, 2016-03-16 04:05:34]

On 2016/03/16 00:54:44, Kevin Cernekee wrote:
> emaxx - could you please take a look at the VPN files when you have a moment?

lgtm with nits

[emaxx, 2016-03-16 04:05:58]

https://codereview.chromium.org/1722453002/diff/40001/extensions/common/api/v...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/1722453002/diff/40001/extensions/common/api/v...
extensions/common/api/vpn_provider.idl:55: DOMString? reconnect;
Shouldn't it have the boolean type?

https://codereview.chromium.org/1722453002/diff/40001/extensions/common/api/v...
extensions/common/api/vpn_provider.idl:70: link_down,
I'm not sure about the guidelines regarding naming style here, but UIEvent enum
values defined later in this file are in the lowerCamelCase.

https://codereview.chromium.org/1722453002/diff/40001/ui/chromeos/network/net...
File ui/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/1722453002/diff/40001/ui/chromeos/network/net...
ui/chromeos/network/network_icon.cc:842: // network.
The comment should also reflect the changes.

[Kevin Cernekee, 2016-03-16 18:56:14]

https://codereview.chromium.org/1722453002/diff/40001/extensions/common/api/v...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/1722453002/diff/40001/extensions/common/api/v...
extensions/common/api/vpn_provider.idl:55: DOMString? reconnect;
On 2016/03/16 04:05:58, emaxx wrote:
> Shouldn't it have the boolean type?

These values are always passed to dbus as strings; when I tried to pass a bool
at that layer, it got dropped.

I could use a bool here and manually translate it to a string before it
traverses dbus, but that would be inconsistent with how other args are handled. 
See |mtu| above, which is an integer value; the API also takes a DOMString.

https://codereview.chromium.org/1722453002/diff/40001/extensions/common/api/v...
extensions/common/api/vpn_provider.idl:70: link_down,
On 2016/03/16 04:05:58, emaxx wrote:
> I'm not sure about the guidelines regarding naming style here, but UIEvent
enum
> values defined later in this file are in the lowerCamelCase.

I saw a mix of styles in other APIs, but since UIEvent is camelcase, I'll rename
these to camelcase.

https://codereview.chromium.org/1722453002/diff/40001/ui/chromeos/network/net...
File ui/chromeos/network/network_icon.cc (right):

https://codereview.chromium.org/1722453002/diff/40001/ui/chromeos/network/net...
ui/chromeos/network/network_icon.cc:842: // network.
On 2016/03/16 04:05:58, emaxx wrote:
> The comment should also reflect the changes.

Done.

[Kevin Cernekee, 2016-03-16 21:11:57]

The CQ bit was checked by cernekee@chromium.org

[Kevin Cernekee, 2016-03-16 21:11:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/1722453002/#ps80001
(title: "update comment")

[commit-bot: I haz the power, 2016-03-16 21:12:22]

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

[commit-bot: I haz the power, 2016-03-16 21:22:57]

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

[commit-bot: I haz the power, 2016-03-16 21:22:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kevin Cernekee, 2016-03-16 21:36:00]

cernekee@chromium.org changed reviewers:
+ asargent@chromium.org

[Kevin Cernekee, 2016-03-16 21:36:01]

Hi asargent or rockot - could you please take a look at
extensions/common/api/vpn_provider.idl to finish getting this approved?

Thanks!

[asargent_no_longer_on_chrome, 2016-03-16 22:04:08]

CL description nit: what does "shill changes" mean?

vpn_provider.idl lgtm

[Kevin Cernekee, 2016-03-16 22:12:03]

Description was changed from

==========
Extend vpnProvider to allow reconnections

Upcoming shill changes will allow third party VPNs to transition
"backwards" from Online->Configuring if the default physical connection
changes.  Add a "reconnect" flag so that VPN apps can signal their
compatibility with this new scheme, and add the necessary UI changes so
that Chrome can identify reconnections and present them to the user in
a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343
==========

to

==========
Extend vpnProvider to allow reconnections

Upcoming changes in the Chrome OS networking daemon (shill) will allow
third party VPNs to transition "backwards" from Online->Configuring if
the default physical connection changes.  Add a "reconnect" flag so
that VPN apps can signal their compatibility with this new scheme,
and add the necessary UI changes so that Chrome can identify
reconnections and present them to the user in a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343
==========

[Kevin Cernekee, 2016-03-16 22:12:12]

The CQ bit was checked by cernekee@chromium.org

[commit-bot: I haz the power, 2016-03-16 22:12:37]

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

[commit-bot: I haz the power, 2016-03-16 22:19:08]

Description was changed from

==========
Extend vpnProvider to allow reconnections

Upcoming changes in the Chrome OS networking daemon (shill) will allow
third party VPNs to transition "backwards" from Online->Configuring if
the default physical connection changes.  Add a "reconnect" flag so
that VPN apps can signal their compatibility with this new scheme,
and add the necessary UI changes so that Chrome can identify
reconnections and present them to the user in a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343
==========

to

==========
Extend vpnProvider to allow reconnections

Upcoming changes in the Chrome OS networking daemon (shill) will allow
third party VPNs to transition "backwards" from Online->Configuring if
the default physical connection changes.  Add a "reconnect" flag so
that VPN apps can signal their compatibility with this new scheme,
and add the necessary UI changes so that Chrome can identify
reconnections and present them to the user in a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343
==========

[commit-bot: I haz the power, 2016-03-16 22:19:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-16 22:22:26]

Description was changed from

==========
Extend vpnProvider to allow reconnections

Upcoming changes in the Chrome OS networking daemon (shill) will allow
third party VPNs to transition "backwards" from Online->Configuring if
the default physical connection changes.  Add a "reconnect" flag so
that VPN apps can signal their compatibility with this new scheme,
and add the necessary UI changes so that Chrome can identify
reconnections and present them to the user in a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343
==========

to

==========
Extend vpnProvider to allow reconnections

Upcoming changes in the Chrome OS networking daemon (shill) will allow
third party VPNs to transition "backwards" from Online->Configuring if
the default physical connection changes.  Add a "reconnect" flag so
that VPN apps can signal their compatibility with this new scheme,
and add the necessary UI changes so that Chrome can identify
reconnections and present them to the user in a sensible way.

Also, change the UI so that users can cancel VPN reconnections
(and connections) using the "Disconnect" button.

BUG=514343

Committed: https://crrev.com/7288268a683040968054a3dcfedef28d9151bc1d
Cr-Commit-Position: refs/heads/master@{#381566}
==========

[commit-bot: I haz the power, 2016-03-16 22:22:27]

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