Add new extension API for VPN

Add new extension API for VPN

This CL adds a new extension API that can be used to implement VPN clients by third party.

BUG=407541
Committed: https://crrev.com/f12e0ba44b4ec3279b3d88ca2bc3c345927ad96e
Cr-Commit-Position: refs/heads/master@{#302670}

[kaliamoorthi, 2014-10-23 09:44:24]

kaliamoorthi@chromium.org changed reviewers:
+ kalman@chromium.org, mpearson@google.com

[kaliamoorthi, 2014-10-23 09:44:25]

This CL adds vpnProvider extension API with the implementation stubbed.

PTAL.

[not at google - send to devlin, 2014-10-23 14:08:19]

I'm just reviewing the IDL and JSON of course.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_permission_features.json:1014: "channel": "dev",
FYI we use "dev" when the API has been implemented but not proven in the wild.
This might be in that state - in which case ignore this comment - but if it's
still under development after this CL then this should be "trunk".

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_permission_features.json:1015: "extension_types":
["extension"],
Why extension only, surely this should be opened up to (platform) apps too?

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_permission_features.json:1016: "platforms":
["chromeos"]
Are you planning to make this multi-platform at any point?

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/vpn_provider.idl (right):

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:7: [nodoc] namespace vpnProvider {
You don't need any of these nodocs. So long as you don't add an HTML file for
this API, it won't be documented.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:10: // IP address for the VPN
interface in CIDR notation.
Question, which might prompt you to consider commenting this: does IPV6 work
too?

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:12: // Broadcast address for the
VPN interface.
For optional arguments, it often makes sense to document what the default value
is, or what happens if it's omitted. It might not make sense, I'm not too
familiar with VPN.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:24: // currently supports only
one.
What's the timeline for supporting more than one? If it's a long time (e.g. if
you plan on releasing this API stable before it supports multiple) then just
make it a single string. If not, this is fine.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:38: error
Document all of these. In particular, what is the difference between
disconnected and underlyingNetworkDisconnected? If the underlying network is
disconnected how do you know when it has reconnected?

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:49: failure
Document each of these as well.

Something more descriptive than "failure" would be good, like an enumeration of
the failure states.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:55: callback CallCompleteCallback
= void ();
Make this inline_doc.

(like
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...)

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:61: // The API is used by the VPN
client to create a new VPN
Phrase these comments like actions: "Creates a new VPN configuration", etc.

[Mark P, 2014-10-23 18:38:48]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org
- mpearson@google.com

[Mark P, 2014-10-23 18:38:49]

It's not obvious why I'm listed as a reviewer here.  Removing my name.  Did you
mean to type someone else?  Otherwise, please tell me what you think I'm
qualified to review.

thanks,
mark

[Mark P, 2014-10-23 18:47:56]

mpearson@chromium.org changed reviewers:
- mpearson@chromium.org

[kaliamoorthi, 2014-10-27 14:40:18]

This CL address all previous comments. PTAL.

I assume that you can approve all these changes for adding a new extension API.
If not please suggest other reviewers.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_permission_features.json:1014: "channel": "dev",
On 2014/10/23 14:08:18, kalman wrote:
> FYI we use "dev" when the API has been implemented but not proven in the wild.
> This might be in that state - in which case ignore this comment - but if it's
> still under development after this CL then this should be "trunk".

Done.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_permission_features.json:1015: "extension_types":
["extension"],
On 2014/10/23 14:08:18, kalman wrote:
> Why extension only, surely this should be opened up to (platform) apps too?

The current thinking is VPN client would be an extension like SPDY. It is more
like a service that needs to run in the background then an app that would be
interacted with. An extension is the best model we have for this.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_permission_features.json:1016: "platforms":
["chromeos"]
On 2014/10/23 14:08:18, kalman wrote:
> Are you planning to make this multi-platform at any point?

VPN is typically controlled by the OS. So it does not make sense to have this
API in platforms other than chrome os.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/vpn_provider.idl (right):

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:7: [nodoc] namespace vpnProvider {
On 2014/10/23 14:08:19, kalman wrote:
> You don't need any of these nodocs. So long as you don't add an HTML file for
> this API, it won't be documented.

Done.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:10: // IP address for the VPN
interface in CIDR notation.
On 2014/10/23 14:08:19, kalman wrote:
> Question, which might prompt you to consider commenting this: does IPV6 work
> too?

Not in V0, the API can support it, but the implementation won't. It will be
supported in later releases.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:12: // Broadcast address for the
VPN interface.
On 2014/10/23 14:08:19, kalman wrote:
> For optional arguments, it often makes sense to document what the default
value
> is, or what happens if it's omitted. It might not make sense, I'm not too
> familiar with VPN.

I documented the defaults.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:24: // currently supports only
one.
On 2014/10/23 14:08:18, kalman wrote:
> What's the timeline for supporting more than one? If it's a long time (e.g. if
> you plan on releasing this API stable before it supports multiple) then just
> make it a single string. If not, this is fine.

No,before this API goes stable, the functionality would be added to the
platform.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:38: error
On 2014/10/23 14:08:19, kalman wrote:
> Document all of these. In particular, what is the difference between
> disconnected and underlyingNetworkDisconnected? If the underlying network is
> disconnected how do you know when it has reconnected?

I want to delay this till the API implementation is done, since new ones would
be introduced as the functionality is implemented.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:49: failure
On 2014/10/23 14:08:19, kalman wrote:
> Document each of these as well.
> 
> Something more descriptive than "failure" would be good, like an enumeration
of
> the failure states.

I want to delay this till the API implementation is done, since new ones would
be introduced as the functionality is implemented.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:55: callback CallCompleteCallback
= void ();
On 2014/10/23 14:08:19, kalman wrote:
> Make this inline_doc.
> 
> (like
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...)

Done.

https://codereview.chromium.org/671163002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/vpn_provider.idl:61: // The API is used by the VPN
client to create a new VPN
On 2014/10/23 14:08:19, kalman wrote:
> Phrase these comments like actions: "Creates a new VPN configuration", etc.

Done.

[not at google - send to devlin, 2014-10-30 17:46:47]

lgtm

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

https://codereview.chromium.org/671163002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/vpn_provider.idl:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
I saw a lot of "I will add this later..." responses to my comments. I think it
would help to add TODOs for each of these.

https://codereview.chromium.org/671163002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/vpn_provider.idl:7: [inline_doc] namespace
vpnProvider {
You don't need inline_doc either.

[kaliamoorthi, 2014-10-31 10:04:55]

This CL fixes the last set of comments.

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

https://codereview.chromium.org/671163002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/vpn_provider.idl:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2014/10/30 17:46:47, kalman wrote:
> I saw a lot of "I will add this later..." responses to my comments. I think it
> would help to add TODOs for each of these.

Done.

https://codereview.chromium.org/671163002/diff/40001/chrome/common/extensions...
chrome/common/extensions/api/vpn_provider.idl:7: [inline_doc] namespace
vpnProvider {
On 2014/10/30 17:46:47, kalman wrote:
> You don't need inline_doc either.

Done.

[kaliamoorthi, 2014-10-31 10:05:13]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-10-31 10:05:55]

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

[commit-bot: I haz the power, 2014-10-31 10:08:45]

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

[commit-bot: I haz the power, 2014-10-31 10:08:47]

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

[kaliamoorthi, 2014-10-31 10:36:58]

kaliamoorthi@chromium.org changed reviewers:
+ kalman; mpearson@chromium.org
- kalman@chromium.org

[kaliamoorthi, 2014-10-31 10:36:58]

Added mpearson@ as a reviewer for
extensions/browser/extension_function_histogram_value.h

Can you please LGTM?

[not at google - send to devlin, 2014-10-31 16:35:27]

kalman@chromium.org changed reviewers:
+ kalman@chromium.org

[not at google - send to devlin, 2014-10-31 16:35:28]

kaliamoorthi - that actually reminds me a couple of things, to fix while you're
waiting for a histogram lg.

(1) Could you add an OWNERS file in the
chrome/browser/extensions/api/vpn_provider directory? Ideally you + another
person on your team. The extensions team tries to avoid reviewing API
implementations, only the IDL files.

(2) Is there anything about the vpnProvider API that is likely to depend on
/chrome? It may be better to put this in /extensions if not.

[Mark P, 2014-10-31 16:48:20]

Please add your change to histograms.xml to this changelist.

--mark

[kaliamoorthi, 2014-11-03 13:12:19]

On 2014/10/31 16:48:20, Mark P wrote:
> Please add your change to histograms.xml to this changelist.
> 
> --mark

Mark, I updated histograms.xml and included it with this CL. PTAL.

Benjamin, Added an OWNERS file. I kept the files in chrome since I would have
dependencies on ui and net. At this point I don't know the exact files, but
based on how the API would interact with chrome, I assume these two would be
necessary.

[not at google - send to devlin, 2014-11-03 17:00:06]

kalman@chromium.org changed reviewers:
+ mpearson@chromium.org, rockot@chromium.org
- kalman; mpearson@chromium.org

[not at google - send to devlin, 2014-11-03 17:00:07]

On 2014/11/03 13:12:19, kaliamoorthi wrote:
> On 2014/10/31 16:48:20, Mark P wrote:
> > Please add your change to histograms.xml to this changelist.
> > 
> > --mark
> 
> Mark, I updated histograms.xml and included it with this CL. PTAL.
> 
> Benjamin, Added an OWNERS file. I kept the files in chrome since I would have
> dependencies on ui and net. At this point I don't know the exact files, but
> based on how the API would interact with chrome, I assume these two would be
> necessary.

net is fine, I thought ui is fine but double check with rockot@.

[Ken Rockot(use gerrit already), 2014-11-03 17:19:27]

On 2014/11/03 17:00:07, kalman wrote:
> On 2014/11/03 13:12:19, kaliamoorthi wrote:
> > On 2014/10/31 16:48:20, Mark P wrote:
> > > Please add your change to histograms.xml to this changelist.
> > > 
> > > --mark
> > 
> > Mark, I updated histograms.xml and included it with this CL. PTAL.
> > 
> > Benjamin, Added an OWNERS file. I kept the files in chrome since I would
have
> > dependencies on ui and net. At this point I don't know the exact files, but
> > based on how the API would interact with chrome, I assume these two would be
> > necessary.
> 
> net is fine, I thought ui is fine but double check with rockot@.

net is indeed fine, and ui depends on how deep the dependencies go. Ideally this
would live in src/extensions and heavy ui stuff could be delegated to embedder
code (e.g. a ui/views implementation of something might live in src/chrome).

[Mark P, 2014-11-03 21:47:19]

extension_function_histogram_value.h and histograms.xml lgtm

[kaliamoorthi, 2014-11-04 12:48:24]

Patchset #6 (id:100001) has been deleted

[kaliamoorthi, 2014-11-04 15:08:24]

Patchset #6 (id:120001) has been deleted

[kaliamoorthi, 2014-11-04 17:27:28]

PTAL. Moved the implementation to extension/

The API would need to access chromeos/dbus/. Is this allowed?

[Ken Rockot(use gerrit already), 2014-11-04 17:58:08]

On 2014/11/04 17:27:28, kaliamoorthi wrote:
> PTAL. Moved the implementation to extension/
> 
> The API would need to access chromeos/dbus/. Is this allowed?

Yes, //chromeos/dbus dependencies will be allowed in //extensions/browser.

[kaliamoorthi, 2014-11-04 18:12:03]

On 2014/11/04 17:58:08, Ken Rockot wrote:
> On 2014/11/04 17:27:28, kaliamoorthi wrote:
> > PTAL. Moved the implementation to extension/
> > 
> > The API would need to access chromeos/dbus/. Is this allowed?
> 
> Yes, //chromeos/dbus dependencies will be allowed in //extensions/browser.

Thanks for the confirmation.

[not at google - send to devlin, 2014-11-04 18:19:35]

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

[not at google - send to devlin, 2014-11-04 18:19:36]

lgtm, it doesn't look like you've introduced the chromeos dependencies in this
patch, but when you do you'll need to update the DEPS file(s). +jamescook as an
FYI.

[James Cook, 2014-11-04 18:49:13]

On 2014/11/04 18:19:36, kalman wrote:
> lgtm, it doesn't look like you've introduced the chromeos dependencies in this
> patch, but when you do you'll need to update the DEPS file(s). +jamescook as
an
> FYI.

Yes, //chromeos/dbus is fine.

[kaliamoorthi, 2014-11-04 19:19:47]

On 2014/11/04 18:49:13, James Cook wrote:
> On 2014/11/04 18:19:36, kalman wrote:
> > lgtm, it doesn't look like you've introduced the chromeos dependencies in
this
> > patch, but when you do you'll need to update the DEPS file(s). +jamescook as
> an
> > FYI.
> 
> Yes, //chromeos/dbus is fine.

Thanks for the lgtm and the confirmation. I did not introduce the dependency in
this CL. I'll do it in subsequent CLs.

[kaliamoorthi, 2014-11-04 19:19:55]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-11-04 19:22:13]

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

[commit-bot: I haz the power, 2014-11-04 20:46:16]

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

[commit-bot: I haz the power, 2014-11-04 20:46:17]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kaliamoorthi, 2014-11-04 20:48:53]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-11-04 20:50:25]

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

[commit-bot: I haz the power, 2014-11-04 21:19:45]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2014-11-04 21:20:24]

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