PPAPI: Add permissions checking for PPB_NetworkProxy

PPAPI: Add permissions checking for PPB_NetworkProxy.

This CL is dependent on https://codereview.chromium.org/16819002/

BUG=247225
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208452

[yzshen1, 2013-06-21 06:59:40]

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:130:
SendFailureReply(PP_ERROR_ADDRESS_INVALID,
Strictly speaking, URL is not a network address. And the comments for
PP_ERROR_ADDRESS_INVALID is "This value indicates that the IP address or port
number is invalid."

I think BADARGUMENT may be more accurate. What do you think?

[teravest, 2013-06-21 14:46:29]

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:78:
result.context_getter = render_process_host->GetBrowserContext()->
What happens if context_getter isn't set? I don't see it being set to NULL in
the constructor, and I don't see any validity check in DidGetUIThreadData.

[dmichael (off chromium), 2013-06-21 16:19:40]

Thanks for the fast review, guys! PTAL

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:78:
result.context_getter = render_process_host->GetBrowserContext()->
On 2013/06/21 14:46:29, teravest wrote:
> What happens if context_getter isn't set? I don't see it being set to NULL in
> the constructor, and I don't see any validity check in DidGetUIThreadData.
Good catch. I suspect that doesn't actually happen, but I added the check in
DidGetUIThreadData.

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:130:
SendFailureReply(PP_ERROR_ADDRESS_INVALID,
On 2013/06/21 06:59:40, yzshen1 wrote:
> Strictly speaking, URL is not a network address.
It's a web address :-). So I think you could argue that it makes sense. But I
don't have a strong opinion.

> And the comments for
> PP_ERROR_ADDRESS_INVALID is "This value indicates that the IP address or port
> number is invalid."
I think it would be okay to use PP_ERROR_ADDRESS_INVALID if we just updated the
documentation. But I think BADARGUMENT is fine, too. I went with that.

> 
> I think BADARGUMENT may be more accurate. What do you think?

[teravest, 2013-06-21 16:22:29]

lgtm

[yzshen1, 2013-06-21 18:06:24]

LGTM

Thanks!

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/17094022/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:130:
SendFailureReply(PP_ERROR_ADDRESS_INVALID,
Yeah, if we update the document, I am fine with PP_ERROR_ADDRESS_INVALID.
But BADARGUMENT is fine too.

On 2013/06/21 16:19:40, dmichael wrote:
> On 2013/06/21 06:59:40, yzshen1 wrote:
> > Strictly speaking, URL is not a network address.
> It's a web address :-). So I think you could argue that it makes sense. But I
> don't have a strong opinion.
> 
> > And the comments for
> > PP_ERROR_ADDRESS_INVALID is "This value indicates that the IP address or
port
> > number is invalid."
> I think it would be okay to use PP_ERROR_ADDRESS_INVALID if we just updated
the
> documentation. But I think BADARGUMENT is fine, too. I went with that.
> 
> > 
> > I think BADARGUMENT may be more accurate. What do you think?
>

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:41:
BrowserPpapiHostImpl* host_impl =
You could directly pass in a BrowserPpapiHostImpl pointer. Because |host_| of 
 ContentBrowserPepperHostFactory is BrowserPpapiHostImpl*.

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.h (right):

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.h:47:
UIThreadData();
I suspect you will get a warning of not having a destructor defined. But I am
fine if the compiler doesn't complain. :)

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.h:79: // False
initially, but set to true once the values for proxy_service_ and
Is this comment inverted? It is set to false when |proxy_service_| and
|is_allowed_| are set.

[dmichael (off chromium), 2013-06-21 20:25:45]

Thanks again for the reviews. Now I just need to wait on the prior CL.

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.cc (right):

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.cc:41:
BrowserPpapiHostImpl* host_impl =
On 2013/06/21 18:06:24, yzshen1 wrote:
> You could directly pass in a BrowserPpapiHostImpl pointer. Because |host_| of 
>  ContentBrowserPepperHostFactory is BrowserPpapiHostImpl*.
Much better, thanks for noticing that!

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.h (right):

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.h:47:
UIThreadData();
On 2013/06/21 18:06:24, yzshen1 wrote:
> I suspect you will get a warning of not having a destructor defined. But I am
> fine if the compiler doesn't complain. :)
The compiler provides the destructor for you by default. It's usually only
important to provide one if you need to make it virtual or if you have to do
something special in the destructor, like delete a heap-allocated object. In
this case, the generated destructor invokes the destructor of the scoped_refptr,
so there's no need to write one.

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.h:79: // False
initially, but set to true once the values for proxy_service_ and
On 2013/06/21 18:06:24, yzshen1 wrote:
> Is this comment inverted? It is set to false when |proxy_service_| and
> |is_allowed_| are set.
Oops! Thanks, nice catch.

[yzshen1, 2013-06-21 20:31:35]

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
File content/browser/renderer_host/pepper/pepper_network_proxy_host.h (right):

https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
content/browser/renderer_host/pepper/pepper_network_proxy_host.h:47:
UIThreadData();
Yeah, I know.
I just vaguely remember that clang will complain that you don't have a 
destructor defined in certain cases. (But I don't remember which cases.)

I am fine if it doesn't complain. :)

[commit-bot: I haz the power, 2013-06-24 22:22:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/17094022/21001

[commit-bot: I haz the power, 2013-06-24 22:58:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[dmichael (off chromium), 2013-06-25 02:45:36]

On 2013/06/21 20:31:35, yzshen1 wrote:
>
https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
> File content/browser/renderer_host/pepper/pepper_network_proxy_host.h (right):
> 
>
https://codereview.chromium.org/17094022/diff/9001/content/browser/renderer_h...
> content/browser/renderer_host/pepper/pepper_network_proxy_host.h:47:
> UIThreadData();
> Yeah, I know.
> I just vaguely remember that clang will complain that you don't have a 
> destructor defined in certain cases. (But I don't remember which cases.)
> 
> I am fine if it doesn't complain. :)
Huh. You were absolutely right. I might have to look at the plugin to try to
understand why it's considered bad. Thanks.

[commit-bot: I haz the power, 2013-06-25 02:45:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/17094022/32002

[commit-bot: I haz the power, 2013-06-25 08:40:34]

Change committed as 208452