Hook up PpapiPermissions in more places.

Hook up PpapiPermissions in more places.

This doesn't actually do much more checking of the permissions, but it should wire it up everywhere we'll need it. It will also at least only return public interfaces via GetInterface in the proxy now unless other bits are supplied.

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

[brettw, 2012-09-28 22:19:45]

+John for content/public addition.

[jam, 2012-09-28 23:15:16]

https://codereview.chromium.org/10984094/diff/4001/content/public/common/pepp...
File content/public/common/pepper_permissions_config.cc (right):

https://codereview.chromium.org/10984094/diff/4001/content/public/common/pepp...
content/public/common/pepper_permissions_config.cc:24: original_perms.GetBits()
| additional_permissions);
i'm not sure I follow, why is this file needed? the function is not really doing
anything other than the test switch. is more stuff going to be added?

[brettw, 2012-09-30 02:32:27]

I didn't use to have this file in this patch but I found myself doing the same
command line checking code in three places in three difference places
(content/browser, content/renderer, and chrome/browser). It's important that
everybody agree what the permissions are so this didn't seem like a good
situation. And I was worried we may add more flags or change what the testing
flag means in terms of permissions (I actually changed this part way though this
patch).

[brettw, 2012-09-30 02:33:47]

Oh, I also wanted to minimize the number of places where we assume that
permissions are an int32 since I'm worried we may need to make the permissions
more rich in the future. With this file, the only places we need to check this
are when making a WebPluginInfo and in this one file.

[bbudge, 2012-10-01 17:32:01]

LGTM

http://codereview.chromium.org/10984094/diff/4001/chrome/renderer/pepper/ppb_...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10984094/diff/4001/chrome/renderer/pepper/ppb_...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:202: PP_Bool
StartPpapiProxy(PP_Instance instance) {
This is called by the NaCl plugin, which knows whether 'Dev' interfaces should
be enabled. We can add a bool to pass this information down.

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/interface_list.h
File ppapi/proxy/interface_list.h (right):

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/interface_list....
ppapi/proxy/interface_list.h:79: // Perms are the permission bits in
ppapi::Permissions that are required to
Not clear what 'Perms' refers to. How about:
// The permission bits in ppapi::Permissions that ...

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/plugin_dispatch...
File ppapi/proxy/plugin_dispatcher.h (right):

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/plugin_dispatch...
ppapi/proxy/plugin_dispatcher.h:86: // Note about permissions: On the host side,
the dispatcher and the plugin
s/host/plugin?

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/ppb_flash_proxy.cc
File ppapi/proxy/ppb_flash_proxy.cc (right):

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/ppb_flash_proxy...
ppapi/proxy/ppb_flash_proxy.cc:92: return false;
I'm wondering if there is any reason to expose the dispatcher's permissions
directly. Would it be better if the dispatcher instead had a HasPermission()
method?

[jam, 2012-10-01 18:25:52]

On 2012/09/30 02:32:27, brettw wrote:
> I didn't use to have this file in this patch but I found myself doing the same
> command line checking code in three places in three difference places
> (content/browser, content/renderer, and chrome/browser). It's important that
> everybody agree what the permissions are so this didn't seem like a good
> situation. And I was worried we may add more flags or change what the testing
> flag means in terms of permissions (I actually changed this part way though
this
> patch).


On 2012/09/30 02:33:47, brettw wrote:
> Oh, I also wanted to minimize the number of places where we assume that
> permissions are an int32 since I'm worried we may need to make the permissions
> more rich in the future. With this file, the only places we need to check this
> are when making a WebPluginInfo and in this one file.

how about putting this knowledge in ppapi/shared_impl/ppapi_permissions.h then
instead of content?

[brettw, 2012-10-01 19:16:39]

John: that's a good idea, I'll move the command line flag from webkit/plugins to
ppapi.

[brettw, 2012-10-01 23:04:48]

I addressed the other stuff.

http://codereview.chromium.org/10984094/diff/4001/chrome/renderer/pepper/ppb_...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10984094/diff/4001/chrome/renderer/pepper/ppb_...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:202: PP_Bool
StartPpapiProxy(PP_Instance instance) {
We also need to hook this up in the browser process. Let's do that in a separate
pass.

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/ppb_flash_proxy.cc
File ppapi/proxy/ppb_flash_proxy.cc (right):

http://codereview.chromium.org/10984094/diff/4001/ppapi/proxy/ppb_flash_proxy...
ppapi/proxy/ppb_flash_proxy.cc:92: return false;
That would work and would be fine with just this one function. I was worried
about having potentially more functions and doing a bunch of plumbing. This
seemed a bit cleaner (a dispatcher has permissions, and you use the permissions
API to access it).