ppapi: PPB_VpnProvider: Implement Service Helper

ppapi: PPB_VpnProvider: Implement Service Helper

Glue code facilitating packet transfer between the
Resource Host and the VpnService.

BUG=506490
Committed: https://crrev.com/5b341c791c8d500786c18710a73c0524a32d7148
Cr-Commit-Position: refs/heads/master@{#401437}

[adrian.belgun, 2016-05-17 17:00:41]

Description was changed from

==========
ppapi: PPB_VpnProvider: Implement Service Helper

Glue code needed to faciliate packet transfer between the Resource Host
and the VpnService.

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

to

==========
ppapi: PPB_VpnProvider: Implement Service Helper

Glue code facilitating packet transfer between the
Resource Host and the VpnService.

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

[adrian.belgun, 2016-05-17 17:04:07]

adrian.belgun@intel.com changed reviewers:
+ bbudge@chromium.org, nick@chromium.org

[adrian.belgun, 2016-05-17 17:04:07]

adrian.belgun@intel.com changed required reviewers:
+ nick@chromium.org

[adrian.belgun, 2016-05-17 17:04:08]

nick@: Please review changes in content/

This patch adds the delegate handler needed for packet transfer between Resource
Host and the VpnService.
Both components register a delegate to this class.

[adrian.belgun, 2016-05-17 17:05:01]

> nick@: Please review changes in content/
> 
> This patch adds the delegate handler needed for packet transfer between
Resource
> Host and the VpnService.
> Both components register a delegate to this class.

Patches in this series:

Permission: https://codereview.chromium.org/1985093002/
Service Helper: https://codereview.chromium.org/1988613005/
Resource Stub: https://codereview.chromium.org/1931513002/
Resource Host: https://codereview.chromium.org/1735473002/
Vpn Service: https://codereview.chromium.org/1922873005/

[ncarter (slow), 2016-05-17 19:07:36]

This will require some rework to meet the content api guidelines, but I'm here
to help :)

Here's some comments based on my first pass of the header file.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
File content/public/browser/pepper_vpn_provider_service_helper.h (right):

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:6: #define
CONTENT_PUBLIC_BROWSER_PEPPER_VPN_PROVIDER_SERVICE_HELPER_H_
In general, content/public should have one class per file, and the filename
should match up to the class name (i.e., VpnProviderServiceHelper).

For the delegate interfaces, if they are kept as top-level classes in the
content namespace, they should be in their own file. Alternatively, you can use
a nested class with a shorter name to keep things in this one file (e.g.
VpnProviderServiceHelper::Delegate).

Example delegates in this directory:
https://code.google.com/p/chromium/codesearch#search/&q=Delegate%20file:conte...

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:24: class
CONTENT_EXPORT VpnMessageFilterDelegate {
Document these:
  Which threads will these methods be called on?
  What must the implementor do?

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:26: virtual
~VpnMessageFilterDelegate();
This class is called MessageFilterDelegate, but the API doesn't seem to really
be doing filtering, it's about sending data. Is there a better name?

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:28: virtual void
SendOnPacketReceived(const std::vector<char>& data) = 0;
It looks like this Delegate interface will be implemented by
content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos
? If so, then there is no reason for it to be part of content/public:
content/public is only for classes that need to be visible to the content
embedder.

For naming schemes, I typically expect the 'Foo' in FooDelegate to identify the
class that calls the delegate, not the class that implements the Delegate
interface. As in, "Foo delegates handling of an operation to its FooDelegate."

Also, once you are move this to content/browser/renderer_host you may not need a
delegate relationship (or a Register method) at all, since the two impl classes
might be able to see each other directly, and you would have the option of
tightly coupling them.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:32: class
CONTENT_EXPORT VpnServiceDelegate {
I see that the VpnService in extensions will actually implement the
VpnServiceDelegate. so this does belong in content/public.

Is this really best described as a Delegate? What does this class actually
represent, in this API? It looks like it represents something that can bind a
configuration and send packets. So does it actually represent something like a
tunnel/channel/connection/link/route, something like that?

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:37: using
StringCallback = base::Callback<void(const std::string& result)>;
This looks unused?

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:52: 
content/public, being the public api of this module, requires documentation.

Each class here should have a comment describing what it's for, and how it's
meant to be used. Please mention threading behavior. Methods should have
documentation too, that gives one an overall sense of how what behaviors need to
be implemented by content embedder, or what behaviors the content/
implementation must provide.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:61: static
VpnProviderServiceHelper* GetInstance();
VpnProviderServiceHelper ought to be a pure virtual interface with no data
members, implemented by a VpnProviderServiceHelperImpl in content/browser
(possibly content/browser/renderer_host/pepper).

content/public rules are documented here:
https://www.chromium.org/developers/content-module/content-api . See where it
mentions pure virtual interfaces.

Some examples of this pattern elsewhere, for reference:
https://code.google.com/p/chromium/codesearch#search/&q=file:content.public%2...

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:64: void
RegisterDelegate(const std::string& extension_id,
extensions don't exist at the content/ layer, so this is almost certainly not
the right way to express this. I can help you figure out the right way to
express it. What is the goal of these extension_ids?

note that extension_id are never used in content (what hits you see below are
mistakes):
https://code.google.com/p/chromium/codesearch#search/&q=extension_id%20file:s...

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:67:
std::unique_ptr<VpnServiceDelegate> delegate);
Is there only one of these per BrowserContext? Or can multiple delegates be
registered?

Another possible way to structure the registration handshake, would for content
to call out to the embedder via ContentBrowserClient interface, and have the
embedder return an instance that way. Sort of like this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

I'm not sure yet if that's a good idea in this case, it depends on the overall
behavior, which I don't quite understand yet.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:81: const
std::vector<char>& data);
Are all of these methods going to be called outside of content? Only expose in
content/public/ the ones that really need to be called by the consumer (which I
think is vpn_service.cc, in the extensions module?). The rest can be Impl-only
methods.

[adrian.belgun, 2016-05-17 20:10:45]

On 2016/05/17 19:07:36, ncarter wrote:
> This will require some rework to meet the content api guidelines, but I'm here
> to help :)
> 
> Here's some comments based on my first pass of the header file.
> 

Thanks for the help! It's a bit late here, so I will respond to the individual
comments tomorrow. 

Here's the high level overview for now:

We're in the process of implementing a new Pepper API to optimize the data path
of of https://developer.chrome.com/apps/vpnProvider .
The plugin binds to a connection created and configured by the JS API and
packets will be routed via Send/Receive Packet.

Long story short, we need to somehow connect
extensions/browser/api/vpn_provider/vpn_service.cc with
content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos.cc
without breaking any DEPS or the component build.

The MessageFilter need to somehow call the Bind() and SendPacket() methods of
the VpnService.
The VpnSerivice need to somehow call OnPacketReceived() and Unbind() of the
MessageFilter.

For security reasons Bind() can only be called from the same extension that
created the connection via JS.
The extension ID is obtained in the MessageFilter and is needed to be passed on
Bind() to make sure that the origin is the same.

[adrian.belgun, 2016-06-10 14:41:08]

Uploaded a new patch set. Responded to most of the issues raised. More details
inline. 

Please take another look at this. Thank you!

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
File content/public/browser/pepper_vpn_provider_service_helper.h (right):

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:6: #define
CONTENT_PUBLIC_BROWSER_PEPPER_VPN_PROVIDER_SERVICE_HELPER_H_
On 2016/05/17 19:07:36, ncarter wrote:
> In general, content/public should have one class per file, and the filename
> should match up to the class name (i.e., VpnProviderServiceHelper).
> 
> For the delegate interfaces, if they are kept as top-level classes in the
> content namespace, they should be in their own file. Alternatively, you can
use
> a nested class with a shorter name to keep things in this one file (e.g.
> VpnProviderServiceHelper::Delegate).
> 
> Example delegates in this directory:
>
https://code.google.com/p/chromium/codesearch#search/&q=Delegate%20file:conte...
> 

Done.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:24: class
CONTENT_EXPORT VpnMessageFilterDelegate {
On 2016/05/17 19:07:35, ncarter wrote:
> Document these:
>   Which threads will these methods be called on?
>   What must the implementor do?

Done.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:26: virtual
~VpnMessageFilterDelegate();
On 2016/05/17 19:07:36, ncarter wrote:
> This class is called MessageFilterDelegate, but the API doesn't seem to really
> be doing filtering, it's about sending data. Is there a better name?

Done. Renamed to PepperVpnProviderResourceHostProxy. the object that implements
this will only forward the calls to the PepperVpnProviderResourceHost object.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:28: virtual void
SendOnPacketReceived(const std::vector<char>& data) = 0;
On 2016/05/17 19:07:36, ncarter wrote:
> It looks like this Delegate interface will be implemented by
>
content/browser/renderer_host/pepper/pepper_vpn_provider_message_filter_chromeos
> ? If so, then there is no reason for it to be part of content/public:
> content/public is only for classes that need to be visible to the content
> embedder.
> 
> For naming schemes, I typically expect the 'Foo' in FooDelegate to identify
the
> class that calls the delegate, not the class that implements the Delegate
> interface. As in, "Foo delegates handling of an operation to its FooDelegate."
> 
> Also, once you are move this to content/browser/renderer_host you may not need
a
> delegate relationship (or a Register method) at all, since the two impl
classes
> might be able to see each other directly, and you would have the option of
> tightly coupling them.

Done. Split functionalty to Impl. 
Cannot avoid Register due to content/browser/renderer_host/pepper is dependent
on "enable_plugins" being set.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:32: class
CONTENT_EXPORT VpnServiceDelegate {
On 2016/05/17 19:07:36, ncarter wrote:
> I see that the VpnService in extensions will actually implement the
> VpnServiceDelegate. so this does belong in content/public.
> 
> Is this really best described as a Delegate? What does this class actually
> represent, in this API? It looks like it represents something that can bind a
> configuration and send packets. So does it actually represent something like a
> tunnel/channel/connection/link/route, something like that?

Done. Renamed to VpnServiceProxy. The object that implements this will only
forward the calls to a VpnService object.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:37: using
StringCallback = base::Callback<void(const std::string& result)>;
On 2016/05/17 19:07:36, ncarter wrote:
> This looks unused?

Done.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:52: 
On 2016/05/17 19:07:36, ncarter wrote:
> content/public, being the public api of this module, requires documentation.
> 
> Each class here should have a comment describing what it's for, and how it's
> meant to be used. Please mention threading behavior. Methods should have
> documentation too, that gives one an overall sense of how what behaviors need
to
> be implemented by content embedder, or what behaviors the content/
> implementation must provide.

Done.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:61: static
VpnProviderServiceHelper* GetInstance();
On 2016/05/17 19:07:36, ncarter wrote:
> VpnProviderServiceHelper ought to be a pure virtual interface with no data
> members, implemented by a VpnProviderServiceHelperImpl in content/browser
> (possibly content/browser/renderer_host/pepper).
> 
> content/public rules are documented here:
> https://www.chromium.org/developers/content-module/content-api . See where it
> mentions pure virtual interfaces.
> 
> Some examples of this pattern elsewhere, for reference:
>
https://code.google.com/p/chromium/codesearch#search/&q=file:content.public%2...

Done.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:64: void
RegisterDelegate(const std::string& extension_id,
On 2016/05/17 19:07:36, ncarter wrote:
> extensions don't exist at the content/ layer, so this is almost certainly not
> the right way to express this. I can help you figure out the right way to
> express it. What is the goal of these extension_ids?
> 
> note that extension_id are never used in content (what hits you see below are
> mistakes):
>
https://code.google.com/p/chromium/codesearch#search/&q=extension_id%20file:s...

Done. Replaced with host_id. The extension id is used for matching the Pepper
plugin with the extension that created the extension.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:67:
std::unique_ptr<VpnServiceDelegate> delegate);
On 2016/05/17 19:07:36, ncarter wrote:
> Is there only one of these per BrowserContext? Or can multiple delegates be
> registered?
> 
> Another possible way to structure the registration handshake, would for
content
> to call out to the embedder via ContentBrowserClient interface, and have the
> embedder return an instance that way. Sort of like this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
> 
> I'm not sure yet if that's a good idea in this case, it depends on the overall
> behavior, which I don't quite understand yet.

Yes. There is one VpnService per browser context. Currently leaving it as is.
For the next patch set I will look into this.

https://codereview.chromium.org/1988613005/diff/1/content/public/browser/pepp...
content/public/browser/pepper_vpn_provider_service_helper.h:81: const
std::vector<char>& data);
On 2016/05/17 19:07:36, ncarter wrote:
> Are all of these methods going to be called outside of content? Only expose in
> content/public/ the ones that really need to be called by the consumer (which
I
> think is vpn_service.cc, in the extensions module?). The rest can be Impl-only
> methods.

Done.

[adrian.belgun, 2016-06-14 14:06:53]

adrian.belgun@intel.com changed reviewers:
+ piman@chromium.org, sky@chromium.org

[adrian.belgun, 2016-06-14 14:06:53]

adrian.belgun@intel.com changed required reviewers:
+ piman@chromium.org, sky@chromium.org
- nick@chromium.org

[adrian.belgun, 2016-06-14 14:06:54]

sky@chromium.org: Please review changes in chrome/browser/

piman@chromium.org: Please review changes in content/public/browser/

As ncarter is currently out of office, adding piman as a reviewer.

Uploaded new patch set, minimizing the changes in content/. 

Dependent CLs:
https://codereview.chromium.org/1988613005/ - rest of changes in
chrome_content_browser_client_extensions_part.cc
https://codereview.chromium.org/1922873005/

[adrian.belgun, 2016-06-14 14:09:02]

On 2016/06/14 14:06:54, adrian.belgun wrote:
> mailto:sky@chromium.org: Please review changes in chrome/browser/
> 
> mailto:piman@chromium.org: Please review changes in content/public/browser/
> 
> As ncarter is currently out of office, adding piman as a reviewer.
> 
> Uploaded new patch set, minimizing the changes in content/. 

Correct links to dependent CLs:
https://codereview.chromium.org/1922873005/ - rest of changes in
> chrome_content_browser_client_extensions_part.cc
https://codereview.chromium.org/1735473002

[piman, 2016-06-16 00:27:09]

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:591: // Creates a new
VpnServiceProxy. The caller owns the returned value. It's
Please return a std::unique_ptr<VpnServiceProxy> to carry the ownership
semantics.

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
File content/public/browser/pepper_vpn_provider_resource_host_proxy.h (right):

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
content/public/browser/pepper_vpn_provider_resource_host_proxy.h:23: virtual
~PepperVpnProviderResourceHostProxy(){};
nit: space between ) and {

[adrian.belgun, 2016-06-16 12:05:29]

Uploaded new patch set with changes as per review.

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:591: // Creates a new
VpnServiceProxy. The caller owns the returned value. It's
On 2016/06/16 00:27:09, piman wrote:
> Please return a std::unique_ptr<VpnServiceProxy> to carry the ownership
> semantics.

Done.

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
File content/public/browser/pepper_vpn_provider_resource_host_proxy.h (right):

https://codereview.chromium.org/1988613005/diff/80001/content/public/browser/...
content/public/browser/pepper_vpn_provider_resource_host_proxy.h:23: virtual
~PepperVpnProviderResourceHostProxy(){};
On 2016/06/16 00:27:09, piman wrote:
> nit: space between ) and {

Done.

[piman, 2016-06-16 17:10:12]

lgtm

[ncarter (slow), 2016-06-20 20:01:56]

content lgtm

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
File content/public/browser/pepper_vpn_provider_resource_host_proxy.h (right):

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
content/public/browser/pepper_vpn_provider_resource_host_proxy.h:23: virtual
~PepperVpnProviderResourceHostProxy() {}
Within the public: section, the destructor should be declared before methods.

https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
File content/public/browser/vpn_service_proxy.h (right):

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
content/public/browser/vpn_service_proxy.h:41: virtual ~VpnServiceProxy() {}
dtor should be declared before Bind(), and after the typedefs.

[ncarter (slow), 2016-06-20 20:03:25]

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
File content/public/browser/vpn_service_proxy.h (right):

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
content/public/browser/vpn_service_proxy.h:33:
std::unique_ptr<PepperVpnProviderResourceHostProxy>) = 0;
The last parameter should have a name.

[adrian.belgun, 2016-06-21 08:08:16]

Uploaded new patch set with changes as per review.

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
File content/public/browser/pepper_vpn_provider_resource_host_proxy.h (right):

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
content/public/browser/pepper_vpn_provider_resource_host_proxy.h:23: virtual
~PepperVpnProviderResourceHostProxy() {}
On 2016/06/20 20:01:55, ncarter wrote:
> Within the public: section, the destructor should be declared before methods.
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order
> 

Done.

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
File content/public/browser/vpn_service_proxy.h (right):

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
content/public/browser/vpn_service_proxy.h:33:
std::unique_ptr<PepperVpnProviderResourceHostProxy>) = 0;
On 2016/06/20 20:03:25, ncarter wrote:
> The last parameter should have a name.

Done.

https://codereview.chromium.org/1988613005/diff/100001/content/public/browser...
content/public/browser/vpn_service_proxy.h:41: virtual ~VpnServiceProxy() {}
On 2016/06/20 20:01:55, ncarter wrote:
> dtor should be declared before Bind(), and after the typedefs.

Done.

[ncarter (slow), 2016-06-21 16:34:33]

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

[ncarter (slow), 2016-06-21 16:34:39]

lgtm

[commit-bot: I haz the power, 2016-06-21 16:34:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1988613005/120001

[commit-bot: I haz the power, 2016-06-21 16:37:20]

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

[commit-bot: I haz the power, 2016-06-21 16:37:21]

Dry run: Try jobs failed on following builders:
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[adrian.belgun, 2016-06-21 19:03:09]

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

[commit-bot: I haz the power, 2016-06-21 19:03:37]

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

[commit-bot: I haz the power, 2016-06-21 19:03:39]

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

[commit-bot: I haz the power, 2016-06-21 19:03:40]

Dry run: All required reviewers (with asterisk prefixes) have not yet approved
this CL.

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-06-21 19:08:14]

Uploaded new patch set with the rebase.
Unfortunately I do not have dry run access. Please restart the dry run. 

Thank You!

[bbudge, 2016-06-21 19:30:46]

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

[commit-bot: I haz the power, 2016-06-21 19:31:49]

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

[commit-bot: I haz the power, 2016-06-21 20:59:01]

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

[commit-bot: I haz the power, 2016-06-21 20:59:02]

Dry run: This issue passed the CQ dry run.

[robert.bradford, 2016-06-22 18:43:15]

robert.bradford@intel.com changed reviewers:
+ asargent@chromium.org, robert.bradford@intel.com

[robert.bradford, 2016-06-22 18:43:16]

robert.bradford@intel.com changed required reviewers:
+ asargent@chromium.org
- sky@chromium.org

[robert.bradford, 2016-06-22 18:43:16]

+asargent@ could you review the changes in chrome/browser/extensions?

[sky, 2016-06-22 19:04:34]

LGTM

[adrian.belgun, 2016-06-22 19:31:41]

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

[adrian.belgun, 2016-06-22 19:31:41]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1988613005/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-06-22 19:32:15]

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

[commit-bot: I haz the power, 2016-06-22 19:32:18]

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

[commit-bot: I haz the power, 2016-06-22 19:32:19]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

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-06-22 20:16:07]

adrian.belgun@intel.com changed required reviewers:
- asargent@chromium.org

[adrian.belgun, 2016-06-22 20:16:19]

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

[commit-bot: I haz the power, 2016-06-22 20:17:00]

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

[asargent_no_longer_on_chrome, 2016-06-22 20:47:25]

extensions lgtm

[piman, 2016-06-22 21:28:49]

lgtm

[commit-bot: I haz the power, 2016-06-22 22:24:39]

Description was changed from

==========
ppapi: PPB_VpnProvider: Implement Service Helper

Glue code facilitating packet transfer between the
Resource Host and the VpnService.

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

to

==========
ppapi: PPB_VpnProvider: Implement Service Helper

Glue code facilitating packet transfer between the
Resource Host and the VpnService.

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

[commit-bot: I haz the power, 2016-06-22 22:24:41]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-22 22:26:04]

Description was changed from

==========
ppapi: PPB_VpnProvider: Implement Service Helper

Glue code facilitating packet transfer between the
Resource Host and the VpnService.

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

to

==========
ppapi: PPB_VpnProvider: Implement Service Helper

Glue code facilitating packet transfer between the
Resource Host and the VpnService.

BUG=506490

Committed: https://crrev.com/5b341c791c8d500786c18710a73c0524a32d7148
Cr-Commit-Position: refs/heads/master@{#401437}
==========

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

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