Extend vpnProvider API to support future enhancements

Extend vpnProvider API to support future enhancements

This CL modifies the vpnProvider extension API to support the following
usecases
1) Support internal create of VPN configurations by admin policy.
2) Deliver UI events to the VPN client from the native UI in the platform.
3) Support the use of a unique identifier in the future instead of name.

BUG=461256
Committed: https://crrev.com/3b40c46c3691bd3d5027b8bbe45d7fc1c463be5d
Cr-Commit-Position: refs/heads/master@{#320306}

[kaliamoorthi, 2015-03-11 14:42:04]

Patchset #1 (id:1) has been deleted

[kaliamoorthi, 2015-03-11 14:43:50]

kaliamoorthi@chromium.org changed reviewers:
+ bartfab@chromium.org, kalman@chromium.org

[kaliamoorthi, 2015-03-11 14:43:50]

PTAL

[not at google - send to devlin, 2015-03-11 17:56:09]

IDL lgtm with some comments

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:65: show_configure_dialog
Style should be camel case.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:145: // |data|: Configuration data
provided by the administrator.
So this will always be an object then?

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:148: // Triggered when there is an UI
event for the extension.
s/an UI/a UI/
and what does "UI event" mean?

[bartfab (slow), 2015-03-12 12:06:39]

lgtm

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
File extensions/browser/api/vpn_provider/vpn_provider_api.cc (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:190: this, params->name
/* name is id for now */),
Nit: As in the other file, I think this should be expanded. You can move the
comment above the base::Bind line if it gets too long. My suggestion would be:

"Use the configuration name as ID. In the future, a different ID scheme may be
used, requiring a mapping between the two."

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

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.cc:170: const std::string& id) {
Nit: Please make sure that the argument names in the header and implementation
files match.

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.cc:336: // id is configuration
name for now.
I think this sentence could be expanded a bit to make it clearer what you mean:

"The ID is the configuration name for now. This may change in the future."

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
File extensions/browser/api/vpn_provider/vpn_service.h (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.h:73: const std::string name);
Nit: s/name/configuration_id/

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.h:112: // Destroys the VPN
configuration with id |id| after verifying that it belongs
Nit 1: s/id/ID/
Nit 2: I think the sentence reads better if you drop the duplicate "id," i.e.
"...with |id| after...".

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.h:116: const std::string& id,
Nit: "id" is rather ambiguous. How about s/id/configuration_id/?

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:76: // error code if there is an error.
Nit: s/error/an error/

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:138: // |id|: ID of the configuration
removed.
Nit: s/configuration removed/removed configuration/

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:145: // |data|: Configuration data
provided by the administrator.
On 2015/03/11 17:56:09, kalman wrote:
> So this will always be an object then?

Yes, iIt will be a settings dictionary.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:150: // |id|: ID of the configuration for
which the UI event is triggered.
Nit: s/is/was/

[kaliamoorthi, 2015-03-12 15:20:44]

Patchset #2 (id:40001) has been deleted

[kaliamoorthi, 2015-03-12 15:23:48]

Thanks Benjamin, Bartosz. I have addressed your comments.

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
File extensions/browser/api/vpn_provider/vpn_provider_api.cc (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_provider_api.cc:190: this, params->name
/* name is id for now */),
On 2015/03/12 12:06:38, bartfab wrote:
> Nit: As in the other file, I think this should be expanded. You can move the
> comment above the base::Bind line if it gets too long. My suggestion would be:
> 
> "Use the configuration name as ID. In the future, a different ID scheme may be
> used, requiring a mapping between the two."

Done.

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

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.cc:170: const std::string& id) {
On 2015/03/12 12:06:38, bartfab wrote:
> Nit: Please make sure that the argument names in the header and implementation
> files match.

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.cc:336: // id is configuration
name for now.
On 2015/03/12 12:06:38, bartfab wrote:
> I think this sentence could be expanded a bit to make it clearer what you
mean:
> 
> "The ID is the configuration name for now. This may change in the future."

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
File extensions/browser/api/vpn_provider/vpn_service.h (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.h:73: const std::string name);
On 2015/03/12 12:06:38, bartfab wrote:
> Nit: s/name/configuration_id/

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.h:112: // Destroys the VPN
configuration with id |id| after verifying that it belongs
On 2015/03/12 12:06:38, bartfab wrote:
> Nit 1: s/id/ID/
> Nit 2: I think the sentence reads better if you drop the duplicate "id," i.e.
> "...with |id| after...".

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/browser/api/v...
extensions/browser/api/vpn_provider/vpn_service.h:116: const std::string& id,
On 2015/03/12 12:06:38, bartfab wrote:
> Nit: "id" is rather ambiguous. How about s/id/configuration_id/?

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:65: show_configure_dialog
On 2015/03/11 17:56:09, kalman wrote:
> Style should be camel case.

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:76: // error code if there is an error.
On 2015/03/12 12:06:38, bartfab wrote:
> Nit: s/error/an error/

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:138: // |id|: ID of the configuration
removed.
On 2015/03/12 12:06:38, bartfab wrote:
> Nit: s/configuration removed/removed configuration/

Done.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:148: // Triggered when there is an UI
event for the extension.
On 2015/03/11 17:56:09, kalman wrote:
> s/an UI/a UI/
> and what does "UI event" mean?

Done.
UI events are signals that request the app/extension to show some UI dialog.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:150: // |id|: ID of the configuration for
which the UI event is triggered.
On 2015/03/12 12:06:38, bartfab wrote:
> Nit: s/is/was/

Done.

[kaliamoorthi, 2015-03-12 15:40:44]

Patchset #2 (id:60001) has been deleted

[bartfab (slow), 2015-03-12 16:06:28]

Still LGTM.

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:148: // Triggered when there is an UI
event for the extension.
On 2015/03/12 15:23:48, kaliamoorthi wrote:
> On 2015/03/11 17:56:09, kalman wrote:
> > s/an UI/a UI/
> > and what does "UI event" mean?
> 
> Done.
> UI events are signals that request the app/extension to show some UI dialog.

I read Benjamin's comment as: Please add a sentence to the documentation that
explains what this is, for others' sake.

[kaliamoorthi, 2015-03-12 16:29:37]

The CQ bit was checked by kaliamoorthi@chromium.org

[kaliamoorthi, 2015-03-12 16:29:37]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
bartfab@chromium.org
Link to the patchset: https://codereview.chromium.org/999783002/#ps100001
(title: "Added documentation for UI events")

[commit-bot: I haz the power, 2015-03-12 16:30:16]

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

[not at google - send to devlin, 2015-03-12 16:31:10]

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
extensions/common/api/vpn_provider.idl:148: // Triggered when there is an UI
event for the extension.
On 2015/03/12 16:06:28, bartfab wrote:
> On 2015/03/12 15:23:48, kaliamoorthi wrote:
> > On 2015/03/11 17:56:09, kalman wrote:
> > > s/an UI/a UI/
> > > and what does "UI event" mean?
> > 
> > Done.
> > UI events are signals that request the app/extension to show some UI dialog.
> 
> I read Benjamin's comment as: Please add a sentence to the documentation that
> explains what this is, for others' sake.

Right, that is what I meant, sorry. Me confused --> probably other people too.

[kaliamoorthi, 2015-03-12 16:34:09]

On 2015/03/12 16:31:10, kalman wrote:
>
https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
> File extensions/common/api/vpn_provider.idl (right):
> 
>
https://codereview.chromium.org/999783002/diff/20001/extensions/common/api/vp...
> extensions/common/api/vpn_provider.idl:148: // Triggered when there is an UI
> event for the extension.
> On 2015/03/12 16:06:28, bartfab wrote:
> > On 2015/03/12 15:23:48, kaliamoorthi wrote:
> > > On 2015/03/11 17:56:09, kalman wrote:
> > > > s/an UI/a UI/
> > > > and what does "UI event" mean?
> > > 
> > > Done.
> > > UI events are signals that request the app/extension to show some UI
dialog.
> > 
> > I read Benjamin's comment as: Please add a sentence to the documentation
that
> > explains what this is, for others' sake.
> 
> Right, that is what I meant, sorry. Me confused --> probably other people too.

I address this in the new CL.

[commit-bot: I haz the power, 2015-03-12 17:16:09]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-03-12 17:17:03]

Patchset 3 (id:??) landed as
https://crrev.com/3b40c46c3691bd3d5027b8bbe45d7fc1c463be5d
Cr-Commit-Position: refs/heads/master@{#320306}