ppapi: PPB_VpnProvider: Bind VpnService to the PPAPI Resource Host

ppapi: PPB_VpnProvider: Bind VpnService to the PPAPI Resource Host

BUG=506490
Committed: https://crrev.com/83fb583147081f2c3188028a0836512fb8ac53ea
Cr-Commit-Position: refs/heads/master@{#401515}

[adrian.belgun, 2016-04-27 15:00:11]

adrian.belgun@intel.com changed reviewers:
+ cernekee@chromium.org, emaxx@chromium.org

[adrian.belgun, 2016-04-27 15:00:12]

emaxx, cernekee: Please review changes for the VpnService

[ncarter (slow), 2016-05-17 17:15:40]

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

https://codereview.chromium.org/1922873005/diff/40001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_service.cc:108: // If the configrataion
was added via a PPAPI plugin
"configuration"

[adrian.belgun, 2016-05-17 17:18:56]

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

https://codereview.chromium.org/1922873005/diff/40001/extensions/browser/api/...
extensions/browser/api/vpn_provider/vpn_service.cc:108: // If the configrataion
was added via a PPAPI plugin
On 2016/05/17 17:15:40, ncarter wrote:
> "configuration"

Done.

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

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

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

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

This patch depends on plumbing added in
https://codereview.chromium.org/1988613005/

[sky, 2016-06-21 16:16:40]

https://codereview.chromium.org/1922873005/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/1922873005/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:34:
#if defined(OS_CHROMEOS)
conditional includes go after non-conditional includes (see style guide).

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

Patch set with fixed include order.

https://codereview.chromium.org/1922873005/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/1922873005/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:34:
#if defined(OS_CHROMEOS)
On 2016/06/21 16:16:39, sky wrote:
> conditional includes go after non-conditional includes (see style guide).

Done.

[sky, 2016-06-21 20:10:55]

LGTM

[emaxx, 2016-06-21 23:47:48]

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

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:69: void set_nacl_bound(
Is there a situation possible in which nacl_bound==true along with NULL
pepper_vpn_provider_proxy would be passed here?
If not, then it would be clearer to rid of the boolean variable and member.

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:118: // If the configuration
was added via a PPAPI plugin
nit: This comment looks unfinished, and describes only the the first branch of
the condition.

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:193:
VpnService::VpnServiceProxyImpl::~VpnServiceProxyImpl() {}
nit: Is this necessary?

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:690:
failure.Run(std::string(), std::string("Unauthorized access 3."));
Could you please make the error messages more informative?

[adrian.belgun, 2016-06-22 08:23:47]

Uploaded new patch set with changes as per review.

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

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:69: void set_nacl_bound(
On 2016/06/21 23:47:48, emaxx wrote:
> Is there a situation possible in which nacl_bound==true along with NULL
> pepper_vpn_provider_proxy would be passed here?
> If not, then it would be clearer to rid of the boolean variable and member.

Done. Removed superfluous nacl_bound. 
It was a remnant from the old logic, without pepper_vpn_provider_proxy.

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:118: // If the configuration
was added via a PPAPI plugin
On 2016/06/21 23:47:47, emaxx wrote:
> nit: This comment looks unfinished, and describes only the the first branch of
> the condition.

Done.

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:193:
VpnService::VpnServiceProxyImpl::~VpnServiceProxyImpl() {}
On 2016/06/21 23:47:47, emaxx wrote:
> nit: Is this necessary?

Removed it. Technically not necessary, but added it to be consistent with the
above VpnConfiguration.

https://codereview.chromium.org/1922873005/diff/140001/extensions/browser/api...
extensions/browser/api/vpn_provider/vpn_service.cc:690:
failure.Run(std::string(), std::string("Unauthorized access 3."));
On 2016/06/21 23:47:47, emaxx wrote:
> Could you please make the error messages more informative?

Done.

[emaxx, 2016-06-22 10:56:02]

lgtm

(and sorry for slow reviewing!)

[adrian.belgun, 2016-06-22 18:27:43]

On 2016/06/22 10:56:02, emaxx wrote:
> lgtm
> 
> (and sorry for slow reviewing!)

Thank you for the review! 
No problem related to the speed of the review, the patch on which this depends
on took a while to stabilize. 

This will land once I get the l_g_t_m for chrome/ on
https://codereview.chromium.org/1988613005/.

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

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

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

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1922873005/#ps160001
(title: "Remove redundant code.")

[commit-bot: I haz the power, 2016-06-23 01:05:33]

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

[commit-bot: I haz the power, 2016-06-23 01:52:56]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-23 01:57:18]

Description was changed from

==========
ppapi: PPB_VpnProvider: Bind VpnService to the PPAPI Resource Host

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

to

==========
ppapi: PPB_VpnProvider: Bind VpnService to the PPAPI Resource Host

BUG=506490

Committed: https://crrev.com/83fb583147081f2c3188028a0836512fb8ac53ea
Cr-Commit-Position: refs/heads/master@{#401515}
==========

[commit-bot: I haz the power, 2016-06-23 01:57:19]

Patchset 9 (id:??) landed as
https://crrev.com/83fb583147081f2c3188028a0836512fb8ac53ea
Cr-Commit-Position: refs/heads/master@{#401515}