PPAPI: Whitelist PPB_CameraDevice_Private for some apps

PPAPI: Whitelist PPB_CameraDevice_Private for some apps

Also compile PPB_CameraDevice_Private and PPB_CameraCapabilities_Private in NACL.

BUG=387547
TEST=Test with a trusted plugin. (Haven't tested on NaCl)

Committed: https://crrev.com/586e3dbe02974966b7a3adaf0aebfbcc4d6983ba
Cr-Commit-Position: refs/heads/master@{#317984}

[Justin Chuang, 2015-02-20 16:18:17]

jchuang@chromium.org changed reviewers:
+ dmichael@chromium.org, wuchengli@chromium.org

[Justin Chuang, 2015-02-20 16:18:17]

Dave, PTAL

This CL fix basic things, but I don't know if it's sufficient to work. 

Do I need additional steps to get this private API whitelisted?

In the example of PPB_FileIO_Private, 
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Does it mean that alloy app will have
host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE)?

Thanks

[Justin Chuang, 2015-02-20 16:29:01]

The CQ bit was checked by jchuang@chromium.org

[commit-bot: I haz the power, 2015-02-20 16:29:50]

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

[commit-bot: I haz the power, 2015-02-20 16:29:51]

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

[commit-bot: I haz the power, 2015-02-20 16:29:52]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM 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.

[dmichael (off chromium), 2015-02-20 18:17:41]

On 2015/02/20 16:18:17, Justin Chuang wrote:
> Dave, PTAL
> 
> This CL fix basic things, but I don't know if it's sufficient to work. 
> 
> Do I need additional steps to get this private API whitelisted?
> 
> In the example of PPB_FileIO_Private, 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> Does it mean that alloy app will have
> host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE)?
> 
> Thanks

ARC, being a NaCl app, actually doesn't have PERMISSION_PRIVATE. In the example
above, permissions is granted to ARC because of the "allow_plugin" flag. That
gets passed through to here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
from the return of:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
which comes from:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
which checks the whitelist here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/plu...
Since this is ARC, it should already be in the whitelist...  you just have to
plumb the whitelist check in to your ImageCapture host.

Note...  I see now after copying/pasting all the above, I see that your host is
in the renderer, so the checking code will be different... I don't have time to
look it up right this second though.

Your patch as-is will grant access to this API to any NaCl app, which isn't what
we want (the interfaces_ppb_private_no_permissions.h file does no automatic
checking, so you have to check private permissions explicitly.) Your check will
be in the renderer, but should look kind of similar to what you see for
FileIO_Private. "If plugin has private permission OR is whitelisted, allow it"

[Justin Chuang, 2015-02-21 02:39:15]

On 2015/02/20 18:17:41, dmichael wrote:
> On 2015/02/20 16:18:17, Justin Chuang wrote:
> > Dave, PTAL
> > 
> > This CL fix basic things, but I don't know if it's sufficient to work. 
> > 
> > Do I need additional steps to get this private API whitelisted?
> > 
> > In the example of PPB_FileIO_Private, 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> > 
> > Does it mean that alloy app will have
> > host()->permissions().HasPermission(ppapi::PERMISSION_PRIVATE)?
> > 
> > Thanks
> 
> Your patch as-is will grant access to this API to any NaCl app, which isn't
what
> we want (the interfaces_ppb_private_no_permissions.h file does no automatic
> checking, so you have to check private permissions explicitly.) Your check
will
> be in the renderer, but should look kind of similar to what you see for
> FileIO_Private. "If plugin has private permission OR is whitelisted, allow it"

Thank you so much Dave, for the detailed explanation! Sorry I didn't figure out
by myself.
I change to the whitelist method in chrome_content_renderer_client.cc.
PTAL again.

[Justin Chuang, 2015-02-21 03:38:49]

jchuang@chromium.org changed reviewers:
+ jam@chromium.org

[Justin Chuang, 2015-02-21 03:38:49]

+jam, please review content_renderer_client.{cc|h} and
chrome_content_renderer_client.{cc|h}

[Justin Chuang, 2015-02-21 05:17:58]

jchuang@chromium.org changed reviewers:
+ avi@chromium.org
- jam@chromium.org

[Justin Chuang, 2015-02-21 05:17:59]

-jam, +avi for content/

[Avi (use Gerrit), 2015-02-21 05:59:19]

lgtm

stampity stamp

[Justin Chuang, 2015-02-21 05:59:33]

New patchsets have been uploaded after l-g-t-m from avi@chromium.org

[Justin Chuang, 2015-02-21 06:32:37]

Patchset #6 (id:100001) has been deleted

[Justin Chuang, 2015-02-21 06:33:32]

jchuang@chromium.org changed reviewers:
+ thakis@chromium.org

[Justin Chuang, 2015-02-21 06:33:33]

+ Nico for chrome/ thanks

[Nico, 2015-02-21 22:24:12]

rs-lgtm once dmichael is happy, but…

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:186:
"4EB74897CB187C7633357C2FE832E0AD6A44883A"   // see crbug.com/383937
can we please stop whitelisting random extension ids?

[Nico, 2015-02-21 23:21:02]

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:1570:
allowed_origins.insert(kPredefinedAllowedCameraDeviceOrigins[i]);
(fwiw, the other whitelists build the set once in the constructor instead of
every time, see lhs in
https://codereview.chromium.org/845823002/diff/40001/chrome/renderer/chrome_c...
. probably good to be self-consistent in this file)

[Justin Chuang, 2015-02-22 10:27:47]

New patchsets have been uploaded after l-g-t-m from thakis@chromium.org

[Justin Chuang, 2015-02-22 10:28:27]

Patchset #6 (id:120001) has been deleted

[Justin Chuang, 2015-02-22 12:06:26]

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:186:
"4EB74897CB187C7633357C2FE832E0AD6A44883A"   // see crbug.com/383937
On 2015/02/21 22:24:12, Nico wrote:
> can we please stop whitelisting random extension ids?

I don't know the whole story. 

Dave, would it help to add Alloy-specific PPAPI permission level (similar to
ppapi::PERMISSION_FLASH)? Or would it be overkill?

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:1570:
allowed_origins.insert(kPredefinedAllowedCameraDeviceOrigins[i]);
On 2015/02/21 23:21:02, Nico wrote:
> (fwiw, the other whitelists build the set once in the constructor instead of
> every time, see lhs in
>
https://codereview.chromium.org/845823002/diff/40001/chrome/renderer/chrome_c...
> . probably good to be self-consistent in this file)

Done.

[wuchengli, 2015-02-24 05:46:26]

wuchengli@chromium.org changed reviewers:
- wuchengli@chromium.org

[dmichael (off chromium), 2015-02-24 18:24:10]

lgtm

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:186:
"4EB74897CB187C7633357C2FE832E0AD6A44883A"   // see crbug.com/383937
On 2015/02/22 12:06:26, Justin Chuang wrote:
> On 2015/02/21 22:24:12, Nico wrote:
> > can we please stop whitelisting random extension ids?
> 
> I don't know the whole story. 
Sorry, Nico. This is born of wanting to provide APIs for Android-on-Chrome apps
in the most expedient way. This API should probably get on a track to be
available to all extensions. Similar to one Mozilla has:
https://developer.mozilla.org/en-US/docs/Web/API/Camera_API
...probably not acceptable at this point as a web API due to fingerprinting
concerns.

Longer term, we want to start plumbing these kinds of requests as mojo APIs so
that it's hopefully easier to get more sharing across different
languages/runtimes. As it is, there's very little zero shared code between
Pepper and web or chrome.* APIs.

> 
> Dave, would it help to add Alloy-specific PPAPI permission level (similar to
> ppapi::PERMISSION_FLASH)? Or would it be overkill?
Hmm, I like that idea. I think you could do that as a follow-up if you like.

https://codereview.chromium.org/944033002/diff/160001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/944033002/diff/160001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:184: const char* const
kPredefinedAllowedCameraDeviceOrigins[] = {
I think it would be helpful to add a comment at the top of this block that these
are for the Android Runtime for Chromium. I can't think of any reason not to be
open about that .

https://codereview.chromium.org/944033002/diff/160001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1617: return channel <=
chrome::VersionInfo::CHANNEL_DEV;
This isn't a Dev channel API, so let's not make it available to dev-channel.
Just replace these 2 lines with "return false;"

In fact, you can eliminate the #else and move the #endif up before the "return
false;" you already have.

https://codereview.chromium.org/944033002/diff/160001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/944033002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:65: PP_Instance
instance) {
nit: indentation off. You might want to run git cl format

[Justin Chuang, 2015-02-25 03:59:03]

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/944033002/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:186:
"4EB74897CB187C7633357C2FE832E0AD6A44883A"   // see crbug.com/383937
On 2015/02/24 18:24:09, dmichael wrote:
> > 
> > Dave, would it help to add Alloy-specific PPAPI permission level (similar to
> > ppapi::PERMISSION_FLASH)? Or would it be overkill?
> Hmm, I like that idea. I think you could do that as a follow-up if you like.

I can do that only if it's not urgent. I may not be able to return to this
within a month or longer.

https://codereview.chromium.org/944033002/diff/160001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/944033002/diff/160001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:184: const char* const
kPredefinedAllowedCameraDeviceOrigins[] = {
On 2015/02/24 18:24:10, dmichael wrote:
> I think it would be helpful to add a comment at the top of this block that
these
> are for the Android Runtime for Chromium. I can't think of any reason not to
be
> open about that .

Done.

https://codereview.chromium.org/944033002/diff/160001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:1617: return channel <=
chrome::VersionInfo::CHANNEL_DEV;
On 2015/02/24 18:24:10, dmichael wrote:
> This isn't a Dev channel API, so let's not make it available to dev-channel.
> Just replace these 2 lines with "return false;"
> 
> In fact, you can eliminate the #else and move the #endif up before the "return
> false;" you already have.

Done.

https://codereview.chromium.org/944033002/diff/160001/content/renderer/pepper...
File content/renderer/pepper/content_renderer_pepper_host_factory.cc (right):

https://codereview.chromium.org/944033002/diff/160001/content/renderer/pepper...
content/renderer/pepper/content_renderer_pepper_host_factory.cc:65: PP_Instance
instance) {
On 2015/02/24 18:24:10, dmichael wrote:
> nit: indentation off. You might want to run git cl format

Oops. Thanks

[Justin Chuang, 2015-02-25 04:25:51]

The CQ bit was checked by jchuang@chromium.org

[Justin Chuang, 2015-02-25 04:25:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmichael@chromium.org, avi@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/944033002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2015-02-25 04:27:10]

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

[commit-bot: I haz the power, 2015-02-25 05:18:05]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2015-02-25 05:19:01]

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