ppapi: PPB_VpnProvider: Define API

ppapi: PPB_VpnProvider: Define API
tbr=sky@chromium.org for trivial change to chrome/common/ppapi_utils.

BUG=506490
Committed: https://crrev.com/000bd4989875e1a1fd7faaaf07403cd1f9131d60
Cr-Commit-Position: refs/heads/master@{#393345}

[adrian.belgun, 2016-02-24 13:41:43]

adrian.belgun@intel.com changed reviewers:
+ bartfab@chromium.org, bbudge@chromium.org, dskaram@chromium.org,
robert.bradford@intel.com

[adrian.belgun, 2016-02-24 13:41:44]

Re-based old PPAPI VPN Provider proposal and split it into three separate
reviews.

ppapi:   https://codereview.chromium.org/1726303003/
naclsdk: https://codereview.chromium.org/1731273002/
browser: https://codereview.chromium.org/1735473002/

[bartfab (slow), 2016-02-24 13:46:00]

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

[bartfab (slow), 2016-02-24 13:46:00]

I am glad to see that work on this API resumed. However, I will not have the
cycles to review the CLs. Kevin, Maksim, can either one of you take a look?

[bbudge, 2016-02-24 19:34:22]

Initial comments on the IDL file and API shape.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider.idl
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:21: enum PP_VpnProvider_VpnConnectionState {
nit: PP_VpnProvider_ConnectionState to match enum values, and the other enums.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:22: // Only for compatibility with the JS API
Follow the existing commenting style for enums:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/pp_file_...

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:23: PP_VPN_PROVIDER_CONNECTION_STATE_NONE,
Pepper convention is to explicitly assign the value here. This isn't a normal
enum but an API contract, and we must ensure that actual enum values never
change. 

Also, enum value convention is all caps PP_<api>_<name> E.g.
  PP_VPNPROVIDER_CONNECTIONSTATENONE = 0,

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:28: PP_VPN_PROVIDER_CONNECTION_STATE_LAST =
PP_VPN_PROVIDER_CONNECTION_STATE_FAILURE
We don't usually provide a _LAST or _MAX value, since this enum is an API
contract. There are a few exceptions but the reason why this is bad is if a
value is added in a new version of the API (it happens!) then the numeric value
of the _LAST value will change, which may break a client that for some reason is
using this.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:131: * @return A <code>PP_Resource</code>
corresponding to a WebSocket if
WebSocket -> VpnProvider

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:134: PP_Resource Create( [in] PP_Instance
instance );
nit: eliminate excess spaces (Chromium style convention)

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:247: int32_t SetParameters(
Another approach is to have a SetOption(resource, option, value) function, with
an enum to specify which option is being set and a PP_Var holding the value.
This will version much more cleanly than a function with lots of parameters when
you need to (inevitably) add another parameter. See PPB_TCP_Socket or
PPB_UDP_Socket for good examples of SetOption.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:271: [in] PP_Var packet );
There must be some way (such as a callback) to throttle the plugin unless you
can have unlimited numbers of sent packets in-flight. In general this isn't
possible, since this must involve sending an IPC message to the browser, which
implies that the data must be copied into a message which is held until it can
be sent.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:352: int32_t GetPlatformMessage(
I think this API could be significantly simpler and easier to use if you
collapse the three message enums and request functions into a single message
enum and a single GetMessage function.

[adrian.belgun, 2016-03-09 11:07:08]

Updated IDL and synced up the rest of the code with it.

It's possible that is could take an other route, with a much more simpler API.
Need to clear some issues and will come back with another patch set.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider.idl
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:21: enum PP_VpnProvider_VpnConnectionState {
On 2016/02/24 19:34:21, bbudge wrote:
> nit: PP_VpnProvider_ConnectionState to match enum values, and the other enums.

Done.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:22: // Only for compatibility with the JS API
On 2016/02/24 19:34:22, bbudge wrote:
> Follow the existing commenting style for enums:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/pp_file_...

Done.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:23: PP_VPN_PROVIDER_CONNECTION_STATE_NONE,
On 2016/02/24 19:34:22, bbudge wrote:
> Pepper convention is to explicitly assign the value here. This isn't a normal
> enum but an API contract, and we must ensure that actual enum values never
> change. 
> 
> Also, enum value convention is all caps PP_<api>_<name> E.g.
>   PP_VPNPROVIDER_CONNECTIONSTATENONE = 0,

Done.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:28: PP_VPN_PROVIDER_CONNECTION_STATE_LAST =
PP_VPN_PROVIDER_CONNECTION_STATE_FAILURE
On 2016/02/24 19:34:21, bbudge wrote:
> We don't usually provide a _LAST or _MAX value, since this enum is an API
> contract. There are a few exceptions but the reason why this is bad is if a
> value is added in a new version of the API (it happens!) then the numeric
value
> of the _LAST value will change, which may break a client that for some reason
is
> using this.

Done.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:131: * @return A <code>PP_Resource</code>
corresponding to a WebSocket if
On 2016/02/24 19:34:22, bbudge wrote:
> WebSocket -> VpnProvider

Done.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:134: PP_Resource Create( [in] PP_Instance
instance );
On 2016/02/24 19:34:22, bbudge wrote:
> nit: eliminate excess spaces (Chromium style convention)

Done.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:247: int32_t SetParameters(
On 2016/02/24 19:34:21, bbudge (OOO until 3-18) wrote:
> Another approach is to have a SetOption(resource, option, value) function,
with
> an enum to specify which option is being set and a PP_Var holding the value.
> This will version much more cleanly than a function with lots of parameters
when
> you need to (inevitably) add another parameter. See PPB_TCP_Socket or
> PPB_UDP_Socket for good examples of SetOption.

Done.

Ended up with a SetParameter / CommitParameters combo because all parameters
must be setup at the same time in the underlying layer. Since some parameters
are optional, the user needs to explicitly commit them.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:271: [in] PP_Var packet );
On 2016/02/24 19:34:21, bbudge wrote:
> There must be some way (such as a callback) to throttle the plugin unless you
> can have unlimited numbers of sent packets in-flight. In general this isn't
> possible, since this must involve sending an IPC message to the browser, which
> implies that the data must be copied into a message which is held until it can
> be sent.

Done.

After adding the callback, the performance of this API was impacted due to
limiting to one the number of packets in flight. A packet queue could be added
to the resource in a further patch set to quickly release the callback. This
commit is only to further the discussion on the IDL.

https://codereview.chromium.org/1726303003/diff/1/ppapi/api/ppb_vpn_provider....
ppapi/api/ppb_vpn_provider.idl:352: int32_t GetPlatformMessage(
On 2016/02/24 19:34:21, bbudge wrote:
> I think this API could be significantly simpler and easier to use if you
> collapse the three message enums and request functions into a single message
> enum and a single GetMessage function.

Done.

[bartfab (slow), 2016-03-10 14:34:20]

IMHO, we should consider reducing the scope of this API: Instead of duplicating
the entire JS API in PPAPI, I would implement only the time-critical pieces
(packet transmission). VPN vendors would then use the JS API to set up a
connection and the PPAPI to transmit data over it.

This would not only reduce code complexity. It would also better align with the
way VPN vendors use the APIs: They have a NaCl blob that contains their
crypto/networking logic and that naturally should use the PPAPI. But the
UI/configuration code is all JS and for it to talk to the PPPAI, you would need
to pipe all calls through NaCl. It would feel much simpler and more natural to
have JS config APIs and a PPAPI packet transmission API.

[adrian.belgun, 2016-03-15 13:33:29]

On 2016/03/10 14:34:20, bartfab (slow) wrote:
> IMHO, we should consider reducing the scope of this API: Instead of
duplicating
> the entire JS API in PPAPI, I would implement only the time-critical pieces
> (packet transmission). VPN vendors would then use the JS API to set up a
> connection and the PPAPI to transmit data over it.
> 
> This would not only reduce code complexity. It would also better align with
the
> way VPN vendors use the APIs: They have a NaCl blob that contains their
> crypto/networking logic and that naturally should use the PPAPI. But the
> UI/configuration code is all JS and for it to talk to the PPPAI, you would
need
> to pipe all calls through NaCl. It would feel much simpler and more natural to
> have JS config APIs and a PPAPI packet transmission API.

Uploaded new patch set that keeps in NaCl just the packet API + helpers for
Bind/Unbind events.

Will push ASAP also the browser part.

[bbudge, 2016-03-22 22:38:33]

https://codereview.chromium.org/1726303003/diff/40001/ppapi/api/ppb_vpn_provi...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/40001/ppapi/api/ppb_vpn_provi...
ppapi/api/ppb_vpn_provider.idl:119: * SendPacket() sends an IP packet through
the tunnel created for the VPN
Looks like a duplicate of above.

https://codereview.chromium.org/1726303003/diff/40001/ppapi/api/ppb_vpn_provi...
ppapi/api/ppb_vpn_provider.idl:143: * GetPacket() receives an IP packet from the
tunnel for the VPN session.
ReceivePacket?  (Send / Receive, or Put / Get)

[adrian.belgun, 2016-03-29 15:45:44]

To prevent connection hijacking, this API can only be bound to connections
created by the same extension that contains the plugin instance.

https://codereview.chromium.org/1726303003/diff/40001/ppapi/api/ppb_vpn_provi...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/40001/ppapi/api/ppb_vpn_provi...
ppapi/api/ppb_vpn_provider.idl:119: * SendPacket() sends an IP packet through
the tunnel created for the VPN
On 2016/03/22 22:38:33, bbudge wrote:
> Looks like a duplicate of above.

Done.

https://codereview.chromium.org/1726303003/diff/40001/ppapi/api/ppb_vpn_provi...
ppapi/api/ppb_vpn_provider.idl:143: * GetPacket() receives an IP packet from the
tunnel for the VPN session.
On 2016/03/22 22:38:33, bbudge wrote:
> ReceivePacket?  (Send / Receive, or Put / Get)

Done.

[adrian.belgun, 2016-04-22 15:22:36]

Submitted new patch set that fixes performance issues introduced by the callback
for SendPacket. 

bbudge@chromium.org: Please review changes in current patch set. Once you're OK
with it, I would like to add reviewers for the files without reviewers assigned.

Thank you!

[bbudge, 2016-04-26 18:25:21]

I've posted comments  on the API parts.

This CL includes a lot of implementation too. The review might go faster if you
split the API from the implementation (messages, API definition, resource,
messages.)

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:68: * <code>PPB_VpnProvider</code>.
Comment seems to be from IsVpnProvider.

It's helpful to detail some of the possible error codes and their causes here if
possible.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:74: [in] PP_CompletionCallback callback);
nit: blank line

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:77: * configuration gets unbound.
Is it OK to call this before calling Bind? More documentation of behavior would
be helpful here.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:87: int32_t GetUnBindEvent([in]  PP_Resource
vpn_provider,
s/UnBind/Unbind

Here and everywhere else in this file.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:93: * module.
I'm guessing Bind() must have completed successfully first. It should be
documented here.

Also, can more than one Send be in-flight? In other words, will a call to
SendPacket before completion cause PP_ERROR_INPROGRESS?

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:112: * This interface only returns a single
packet. That is, this interface must
s/interface/function

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:113: * be called at least N times to receive N
packet, no matter the size of
s/packet/packets

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:114: * each packet.
Same comments as SendPacket regarding behavior.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:120: * <code>packet</code>. The
<code>packet</code> must remain valid until
nit: delete 'The' since you're referring to the function parameter.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:131: * <code>PP_OK</code> as if connection is
still established without errors.
Is this by design? Why not just abort all pending calls?

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider.h
File ppapi/cpp/vpn_provider.h (right):

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider...
ppapi/cpp/vpn_provider.h:30: explicit VpnProvider(Instance* instance);
Use InstanceHandle here.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider...
ppapi/cpp/vpn_provider.h:54: const std::string& configuration_name,
Why not use pp::Var's here? Won't these come from the web page as Vars?

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider...
ppapi/cpp/vpn_provider.h:62: int32_t GetUnBindEvent(const CompletionCallback&
callback);
s/UnBind/Unbind

Here and in comment

https://codereview.chromium.org/1726303003/diff/100001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1726303003/diff/100001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:2009:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_VpnProvider_OnUnBind)
Unbind is a word, so s/OnUnBind/OnUnbind

[adrian.belgun, 2016-04-27 14:55:21]

> This CL includes a lot of implementation too. The review might go faster if
you
> split the API from the implementation (messages, API definition, resource,
> messages.)

Uploaded a new patch set with the recommended approach, links below. 

PPAPI Definition: https://codereview.chromium.org/1726303003/
PPAPI Messages: https://codereview.chromium.org/1926703002/
PPAPI Resource: https://codereview.chromium.org/1931513002/
Resource Host: https://codereview.chromium.org/1735473002/
Bind to Service: https://codereview.chromium.org/1922873005/

Still in progress of responding to all review points. Patch contains addressed
ones.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:68: * <code>PPB_VpnProvider</code>.
On 2016/04/26 18:25:21, bbudge wrote:
> Comment seems to be from IsVpnProvider.
> 
> It's helpful to detail some of the possible error codes and their causes here
if
> possible.

Acknowledged.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:74: [in] PP_CompletionCallback callback);
On 2016/04/26 18:25:20, bbudge wrote:
> nit: blank line

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:87: int32_t GetUnBindEvent([in]  PP_Resource
vpn_provider,
On 2016/04/26 18:25:20, bbudge wrote:
> s/UnBind/Unbind
> 
> Here and everywhere else in this file.

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:112: * This interface only returns a single
packet. That is, this interface must
On 2016/04/26 18:25:20, bbudge wrote:
> s/interface/function

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:113: * be called at least N times to receive N
packet, no matter the size of
On 2016/04/26 18:25:20, bbudge wrote:
> s/packet/packets

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:120: * <code>packet</code>. The
<code>packet</code> must remain valid until
On 2016/04/26 18:25:21, bbudge wrote:
> nit: delete 'The' since you're referring to the function parameter.

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider.h
File ppapi/cpp/vpn_provider.h (right):

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider...
ppapi/cpp/vpn_provider.h:30: explicit VpnProvider(Instance* instance);
On 2016/04/26 18:25:21, bbudge wrote:
> Use InstanceHandle here.

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider...
ppapi/cpp/vpn_provider.h:54: const std::string& configuration_name,
On 2016/04/26 18:25:21, bbudge wrote:
> Why not use pp::Var's here? Won't these come from the web page as Vars?

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/cpp/vpn_provider...
ppapi/cpp/vpn_provider.h:62: int32_t GetUnBindEvent(const CompletionCallback&
callback);
On 2016/04/26 18:25:21, bbudge wrote:
> s/UnBind/Unbind
> 
> Here and in comment

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/proxy/ppapi_mess...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1726303003/diff/100001/ppapi/proxy/ppapi_mess...
ppapi/proxy/ppapi_messages.h:2009:
IPC_MESSAGE_CONTROL0(PpapiPluginMsg_VpnProvider_OnUnBind)
On 2016/04/26 18:25:21, bbudge wrote:
> Unbind is a word, so s/OnUnBind/OnUnbind

Done.

[bbudge, 2016-04-27 16:07:14]

Mostly documentation comments. Otherwise, this is getting close.

You probably have to add the API definition (ppb_vpn_provider_api.h) to make
this patch build.

You also have to update the gn build (BUILD.gn) to add the new files.

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:63: * the configuration gets bound.
nit: called on completion

It's possible that Bind can fail.

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:74: * configuration gets unbound.
Can this be called before Bind()?

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:90: * module.
Do you mean this will succeed only when the VpnProvider is bound?
Document that call must complete before it can be called again, or that multiple
sends can be in-flight simultaneously).

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:111: * each packet.
Can this be called before Bind()?
Document that call must complete before it can be called again, or that multiple
receives can be in-flight simultaneously).

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:128: * <code>PP_OK</code> as if connection is
still established without errors.
This error / disconnection behavior seems invisible to the plugin. Do you mean
that GetUnbindEvent may callback to the plugin with an error code, and the
plugin may continue to receive packets until the buffers are empty?

If not, I suggest removing this comment, since it is an implementation detail
that is invisible to the plugin.

[adrian.belgun, 2016-04-28 08:30:40]

On 2016/04/27 16:07:14, bbudge wrote:
> Mostly documentation comments. Otherwise, this is getting close.
> 
> You probably have to add the API definition (ppb_vpn_provider_api.h) to make
> this patch build.
> 
> You also have to update the gn build (BUILD.gn) to add the new files.

Added required files, and the boilerplate code that goes with them.

[adrian.belgun, 2016-04-28 10:26:59]

The CQ bit was checked by adrian.belgun@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-28 10:27:09]

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

[commit-bot: I haz the power, 2016-04-28 10:27:11]

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

[commit-bot: I haz the power, 2016-04-28 10:27:12]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[adrian.belgun, 2016-04-28 10:27:13]

The CQ bit was unchecked by adrian.belgun@intel.com

[bbudge, 2016-05-02 22:21:56]

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

[commit-bot: I haz the power, 2016-05-02 22:22:30]

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

[commit-bot: I haz the power, 2016-05-02 22:40:26]

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

[commit-bot: I haz the power, 2016-05-02 22:40:27]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[robert.bradford, 2016-05-03 11:36:12]

The CQ bit was checked by robert.bradford@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-03 11:36:23]

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

[commit-bot: I haz the power, 2016-05-03 12:42:56]

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

[commit-bot: I haz the power, 2016-05-03 12:42:57]

Dry run: This issue passed the CQ dry run.

[adrian.belgun, 2016-05-04 13:38:45]

Updated documentation.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:77: * configuration gets unbound.
On 2016/04/26 18:25:20, bbudge wrote:
> Is it OK to call this before calling Bind? More documentation of behavior
would
> be helpful here.

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:93: * module.
On 2016/04/26 18:25:20, bbudge wrote:
> I'm guessing Bind() must have completed successfully first. It should be
> documented here.
> 
> Also, can more than one Send be in-flight? In other words, will a call to
> SendPacket before completion cause PP_ERROR_INPROGRESS?

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:114: * each packet.
On 2016/04/26 18:25:21, bbudge wrote:
> Same comments as SendPacket regarding behavior.

Done.

https://codereview.chromium.org/1726303003/diff/100001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:131: * <code>PP_OK</code> as if connection is
still established without errors.
On 2016/04/26 18:25:21, bbudge wrote:
> Is this by design? Why not just abort all pending calls?

Done.

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:63: * the configuration gets bound.
On 2016/04/27 16:07:14, bbudge wrote:
> nit: called on completion
> 
> It's possible that Bind can fail.

Done.

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:74: * configuration gets unbound.
On 2016/04/27 16:07:14, bbudge wrote:
> Can this be called before Bind()?

The callback notifies the plugin that an Unbind event occurred (eg. the current
connection got disconnected). It's recommended that all callbacks should be
bound before calling bind to avoid the scenario:
Call Bind -> Bound -> Unbound on host -> Call GetUnbindEvent() -> No event

If called before:
Call GetUnbindEvent() -> Call Bind -> Bound -> Unbound on host -> Receive
GetUnbindEvent() callback

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:90: * module.
On 2016/04/27 16:07:13, bbudge wrote:
> Do you mean this will succeed only when the VpnProvider is bound?
> Document that call must complete before it can be called again, or that
multiple
> sends can be in-flight simultaneously).

Yes. The connection must be bound. 
As currently implemented, the Resource holds a shared memory buffer that will
cache in flight packets. If there is a spare location, it will return PP_OK and
run the callback immediately, else the callback will run after a packet has been
sent.
Anyway SendPacket must complete before it can be called again.

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:111: * each packet.
On 2016/04/27 16:07:14, bbudge wrote:
> Can this be called before Bind()?
> Document that call must complete before it can be called again, or that
multiple
> receives can be in-flight simultaneously).

Done.

https://codereview.chromium.org/1726303003/diff/120001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:128: * <code>PP_OK</code> as if connection is
still established without errors.
On 2016/04/27 16:07:13, bbudge wrote:
> This error / disconnection behavior seems invisible to the plugin. Do you mean
> that GetUnbindEvent may callback to the plugin with an error code, and the
> plugin may continue to receive packets until the buffers are empty?
> 
> If not, I suggest removing this comment, since it is an implementation detail
> that is invisible to the plugin.

Done.

[bbudge, 2016-05-05 21:45:51]

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:127: [in]  PP_CompletionCallback callback);
I forgot to bring this up before, but it might be simpler to just pass this
callback in as a parameter to Bind and eliminate this function. Are there any
use cases where a plugin wouldn't call GetUnbindEvent?

[adrian.belgun, 2016-05-06 02:40:01]

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:127: [in]  PP_CompletionCallback callback);
On 2016/05/05 21:45:50, bbudge wrote:
> I forgot to bring this up before, but it might be simpler to just pass this
> callback in as a parameter to Bind and eliminate this function. Are there any
> use cases where a plugin wouldn't call GetUnbindEvent?

I've thought about this use case also. The issue is that I haven't seen the
pattern of having a PPAPI method with two callbacks, so I've concluded that it
wasn't an acceptable pattern.

Bind will then look like:
+  int32_t Bind([in] PP_Resource vpn_provider,
+               [in] PP_Var configuration_id,
+               [in] PP_Var configuration_name,
+               [in] PP_CompletionCallback unbind_callback,
+               [in] PP_CompletionCallback callback);
Is this acceptable?

Regarding the GetUnbindEvent, it gets called when the connection gets unbound,
that is when VpnService issues a vpnProvider.onPlatformMessage. The plugin could
actually rely only on this JS API to track the state of the connection. Actually
my demo application does this and GetUnbindEvent is just for logging purposes. 

What would be best?
- Change Bind to add a second callback?
- Remove GetUnbindEvent completely and just expect the plugin to rely only on
vpnProvider.onPlatformMessage?
- Leave it as is?

Please let me know and I will send a new patch set.

[dskaram1, 2016-05-06 06:20:29]

On 2016/05/06 02:40:01, adrian.belgun wrote:
>
https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
> File ppapi/api/ppb_vpn_provider.idl (right):
> 
>
https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
> ppapi/api/ppb_vpn_provider.idl:127: [in]  PP_CompletionCallback callback);
> On 2016/05/05 21:45:50, bbudge wrote:
> > I forgot to bring this up before, but it might be simpler to just pass this
> > callback in as a parameter to Bind and eliminate this function. Are there
any
> > use cases where a plugin wouldn't call GetUnbindEvent?
> 
> I've thought about this use case also. The issue is that I haven't seen the
> pattern of having a PPAPI method with two callbacks, so I've concluded that it
> wasn't an acceptable pattern.
> 
> Bind will then look like:
> +  int32_t Bind([in] PP_Resource vpn_provider,
> +               [in] PP_Var configuration_id,
> +               [in] PP_Var configuration_name,
> +               [in] PP_CompletionCallback unbind_callback,
> +               [in] PP_CompletionCallback callback);
> Is this acceptable?
> 
> Regarding the GetUnbindEvent, it gets called when the connection gets unbound,
> that is when VpnService issues a vpnProvider.onPlatformMessage. The plugin
could
> actually rely only on this JS API to track the state of the connection.
Actually
> my demo application does this and GetUnbindEvent is just for logging purposes.


If it's any use, here is another example app which we are touting to partners as
an implementation benchmark. It also has the implementation for the updated set
of platform messages that we introduced in M51.

https://chromium.googlesource.com/apps/nacl-openconnect

[bbudge, 2016-05-06 17:03:17]

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

[commit-bot: I haz the power, 2016-05-06 17:03:37]

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

[bbudge, 2016-05-06 17:06:30]

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:127: [in]  PP_CompletionCallback callback);
On 2016/05/06 02:40:01, adrian.belgun wrote:
> On 2016/05/05 21:45:50, bbudge wrote:
> > I forgot to bring this up before, but it might be simpler to just pass this
> > callback in as a parameter to Bind and eliminate this function. Are there
any
> > use cases where a plugin wouldn't call GetUnbindEvent?
> 
> I've thought about this use case also. The issue is that I haven't seen the
> pattern of having a PPAPI method with two callbacks, so I've concluded that it
> wasn't an acceptable pattern.
> 
> Bind will then look like:
> +  int32_t Bind([in] PP_Resource vpn_provider,
> +               [in] PP_Var configuration_id,
> +               [in] PP_Var configuration_name,
> +               [in] PP_CompletionCallback unbind_callback,
> +               [in] PP_CompletionCallback callback);
> Is this acceptable?
> 
> Regarding the GetUnbindEvent, it gets called when the connection gets unbound,
> that is when VpnService issues a vpnProvider.onPlatformMessage. The plugin
could
> actually rely only on this JS API to track the state of the connection.
Actually
> my demo application does this and GetUnbindEvent is just for logging purposes.

> 
> What would be best?
> - Change Bind to add a second callback?
> - Remove GetUnbindEvent completely and just expect the plugin to rely only on
> vpnProvider.onPlatformMessage?
> - Leave it as is?
> 
> Please let me know and I will send a new patch set.

I think if it's possible to simplify the Pepper API by having the web page do
more, that's a good thing. So I'd be in favor of removing the GetUnbindEvent
function.

[commit-bot: I haz the power, 2016-05-06 18:12:39]

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

[commit-bot: I haz the power, 2016-05-06 18:12:41]

Dry run: This issue passed the CQ dry run.

[adrian.belgun, 2016-05-06 20:08:48]

Should we now add reviewers for the remaining files?

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
File ppapi/api/ppb_vpn_provider.idl (right):

https://codereview.chromium.org/1726303003/diff/200001/ppapi/api/ppb_vpn_prov...
ppapi/api/ppb_vpn_provider.idl:127: [in]  PP_CompletionCallback callback);
On 2016/05/06 17:06:30, bbudge wrote:
> On 2016/05/06 02:40:01, adrian.belgun wrote:
> > On 2016/05/05 21:45:50, bbudge wrote:
> > > I forgot to bring this up before, but it might be simpler to just pass
this
> > > callback in as a parameter to Bind and eliminate this function. Are there
> any
> > > use cases where a plugin wouldn't call GetUnbindEvent?
> > 
> > I've thought about this use case also. The issue is that I haven't seen the
> > pattern of having a PPAPI method with two callbacks, so I've concluded that
it
> > wasn't an acceptable pattern.
> > 
> > Bind will then look like:
> > +  int32_t Bind([in] PP_Resource vpn_provider,
> > +               [in] PP_Var configuration_id,
> > +               [in] PP_Var configuration_name,
> > +               [in] PP_CompletionCallback unbind_callback,
> > +               [in] PP_CompletionCallback callback);
> > Is this acceptable?
> > 
> > Regarding the GetUnbindEvent, it gets called when the connection gets
unbound,
> > that is when VpnService issues a vpnProvider.onPlatformMessage. The plugin
> could
> > actually rely only on this JS API to track the state of the connection.
> Actually
> > my demo application does this and GetUnbindEvent is just for logging
purposes.
> 
> > 
> > What would be best?
> > - Change Bind to add a second callback?
> > - Remove GetUnbindEvent completely and just expect the plugin to rely only
on
> > vpnProvider.onPlatformMessage?
> > - Leave it as is?
> > 
> > Please let me know and I will send a new patch set.
> 
> I think if it's possible to simplify the Pepper API by having the web page do
> more, that's a good thing. So I'd be in favor of removing the GetUnbindEvent
> function.

Done.

[bbudge, 2016-05-06 20:38:33]

bbudge@chromium.org changed reviewers:
+ binji@chromium.org, isherman@chromium.org

[bbudge, 2016-05-06 20:38:34]

Pepper LGTM
+isherman for tools/metrics/histograms
+binji for nacl_sdk

[bbudge, 2016-05-06 20:40:11]

-isherman who is OOO until 5-15
+holte for tools/metrics/histograms

[bbudge, 2016-05-06 20:41:23]

bbudge@chromium.org changed reviewers:
+ holte@chromium.org
- isherman@chromium.org

[bbudge, 2016-05-06 20:41:24]

+holte for tools/metrics/histograms

[Steven Holte, 2016-05-09 23:23:59]

lgtm

[binji, 2016-05-12 17:21:12]

native_client_sdk lgtm

[adrian.belgun, 2016-05-12 18:48:52]

The CQ bit was checked by adrian.belgun@intel.com

[commit-bot: I haz the power, 2016-05-12 18:49:15]

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

[commit-bot: I haz the power, 2016-05-12 18:59:21]

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

[commit-bot: I haz the power, 2016-05-12 18:59:23]

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...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[bbudge, 2016-05-12 19:14:35]

bbudge@chromium.org changed reviewers:
+ sky@chromium.org

[bbudge, 2016-05-12 19:14:36]

+sky for chrome/common/ppapi_utils.cc

[bbudge, 2016-05-12 20:08:51]

Description was changed from

==========
ppapi: PPB_VpnProvider: Define API

BUG=506490
==========

to

==========
ppapi: PPB_VpnProvider: Define API
tbr=sky@chromium.org for trivial change to chrome/common/ppapi_utils.

BUG=506490
==========

[bbudge, 2016-05-12 20:09:03]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2016-05-12 20:09:16]

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

[commit-bot: I haz the power, 2016-05-12 20:26:47]

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

[commit-bot: I haz the power, 2016-05-12 20:26:49]

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...)

[Lei Zhang, 2016-05-12 20:38:16]

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

[Lei Zhang, 2016-05-12 20:38:18]

lgtm

[bbudge, 2016-05-12 20:39:06]

The CQ bit was checked by bbudge@chromium.org

[commit-bot: I haz the power, 2016-05-12 20:39:22]

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

[commit-bot: I haz the power, 2016-05-12 20:44:21]

Description was changed from

==========
ppapi: PPB_VpnProvider: Define API
tbr=sky@chromium.org for trivial change to chrome/common/ppapi_utils.

BUG=506490
==========

to

==========
ppapi: PPB_VpnProvider: Define API
tbr=sky@chromium.org for trivial change to chrome/common/ppapi_utils.

BUG=506490
==========

[commit-bot: I haz the power, 2016-05-12 20:44:23]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-05-12 20:45:29]

Description was changed from

==========
ppapi: PPB_VpnProvider: Define API
tbr=sky@chromium.org for trivial change to chrome/common/ppapi_utils.

BUG=506490
==========

to

==========
ppapi: PPB_VpnProvider: Define API
tbr=sky@chromium.org for trivial change to chrome/common/ppapi_utils.

BUG=506490

Committed: https://crrev.com/000bd4989875e1a1fd7faaaf07403cd1f9131d60
Cr-Commit-Position: refs/heads/master@{#393345}
==========

[commit-bot: I haz the power, 2016-05-12 20:45:31]

Patchset 12 (id:??) landed as
https://crrev.com/000bd4989875e1a1fd7faaaf07403cd1f9131d60
Cr-Commit-Position: refs/heads/master@{#393345}