media: Add MediaPermission interface and MediaPermissionDispatcher.

media: Add MediaPermission interface and MediaPermissionDispatcher.

MediaPermission is the interface for media code to check/request media related
permissions. MediaPermissionDispatcher is a MediaPermission implementation in
content/.

Plan of follow-up CL:
- Use MediaPermission in ProxyDecryptor and remove current protected media
  identifier info bar code in the browser process (on Android and ChromeOS).

BUG=446263
TEST=Tested on Android with encrypted-media-requestmediakeysystemaccess.html
layout test by calling MediaPermission methods manually in
webencryptedmediaclient_impl.cc.

Committed: https://crrev.com/36b1f3badd1e3f36d0fb5eb63328d2de4da051ae
Cr-Commit-Position: refs/heads/master@{#313303}

[xhwang, 2015-01-22 23:40:53]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, sandersd@chromium.org

[xhwang, 2015-01-22 23:40:53]

xhwang@chromium.org changed required reviewers:
+ ddorwin@chromium.org

[xhwang, 2015-01-22 23:40:54]

PTAL!

https://codereview.chromium.org/871663003/diff/1/media/blink/webencryptedmedi...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/871663003/diff/1/media/blink/webencryptedmedi...
media/blink/webencryptedmediaclient_impl.cc:155:
base::Bind(&OnPermissionStatus));
FYI: This is how I test this CL with
LayoutTests/media/encrypted-media/encrypted-media-requestmediakeysystemaccess.html
test.

https://codereview.chromium.org/871663003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/871663003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:22: return
PERMISSION_PROTECTED_MEDIA;
We have some inconsistency here:

PROTECTED_MEDIA_IDENTIFIER vs PROTECTED_MEDIA

We are using "identifier" in most cases and I think we should fix
"PERMISSION_PROTECTED_MEDIA". WDYT?

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_permission_dispatcher.cc (right):

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
content/renderer/media/media_permission_dispatcher.cc:20:
MediaPermissionDispatcher::~MediaPermissionDispatcher() {
Since |this| is a RenderFrameObserver, this is only called when the RenderFrame
is destroyed. In that case, the browser side will cancel the requests:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/pe...

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
content/renderer/media/media_permission_dispatcher.cc:76:
base::Unretained(this), request_id));
base::Unretained(this) is also used in other dispatchers:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/n...

I *think* it's safe because this class is a RenderFrameObserver. When the render
frame is destroyed, the browser side will get notified and will never return the
status back.

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_permission_dispatcher.h (right):

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
content/renderer/media/media_permission_dispatcher.h:20: class CONTENT_EXPORT
MediaPermissionDispatcher : public media::MediaPermission,
This file name is following geolocation_dispatcher and
notification_permission_dispatcher.

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
content/renderer/media/media_permission_dispatcher.h:28: const GURL&
security_origin,
I am using GURL for simplicity. If I use WebSecurityOrigin this class needs to
live in media/blink instead of media/base. Let me know if you have any concerns.

https://codereview.chromium.org/871663003/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/871663003/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3352: cdm_factory.Pass(),
media_permission_dispatcher_));
Note to myself: Need to fix HtmlDocument before committing this..........

[xhwang, 2015-01-22 23:43:53]

xhwang@chromium.org changed required reviewers:
+ sandersd@chromium.org
- ddorwin@chromium.org

[sandersd (OOO until July 31), 2015-01-24 00:49:22]

Gooks good overall, I still need to dive deeper into how permissions work to
verify that the GURLs provided from the renderer are not being overly trusted.

https://codereview.chromium.org/871663003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/871663003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:22: return
PERMISSION_PROTECTED_MEDIA;
On 2015/01/22 23:40:54, xhwang wrote:
> We have some inconsistency here:
> 
> PROTECTED_MEDIA_IDENTIFIER vs PROTECTED_MEDIA
> 
> We are using "identifier" in most cases and I think we should fix
> "PERMISSION_PROTECTED_MEDIA". WDYT?

I agree, consistency is good, and the identifier suffix is much clearer.

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_permission_dispatcher.h (right):

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
content/renderer/media/media_permission_dispatcher.h:28: const GURL&
security_origin,
On 2015/01/22 23:40:54, xhwang wrote:
> I am using GURL for simplicity. If I use WebSecurityOrigin this class needs to
> live in media/blink instead of media/base. Let me know if you have any
concerns.

I don't know the security implications of choosing one over the other, I
generally support whatever the simplest option is that provides the required
features. (WebSecurityOrigin seems to have a bit of bloat in it.)

[xhwang, 2015-01-26 21:21:58]

rebase only

[sandersd (OOO until July 31), 2015-01-26 21:24:04]

lgtm

[xhwang, 2015-01-26 21:49:06]

https://codereview.chromium.org/871663003/diff/20001/content/browser/permissi...
File content/browser/permissions/permission_service_impl.cc (right):

https://codereview.chromium.org/871663003/diff/20001/content/browser/permissi...
content/browser/permissions/permission_service_impl.cc:22: return
PERMISSION_PROTECTED_MEDIA;
On 2015/01/24 00:49:21, sandersd wrote:
> On 2015/01/22 23:40:54, xhwang wrote:
> > We have some inconsistency here:
> > 
> > PROTECTED_MEDIA_IDENTIFIER vs PROTECTED_MEDIA
> > 
> > We are using "identifier" in most cases and I think we should fix
> > "PERMISSION_PROTECTED_MEDIA". WDYT?
> 
> I agree, consistency is good, and the identifier suffix is much clearer.

sg; I'll update the name in a separate CL.

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
File content/renderer/media/media_permission_dispatcher.h (right):

https://codereview.chromium.org/871663003/diff/20001/content/renderer/media/m...
content/renderer/media/media_permission_dispatcher.h:28: const GURL&
security_origin,
On 2015/01/24 00:49:22, sandersd wrote:
> On 2015/01/22 23:40:54, xhwang wrote:
> > I am using GURL for simplicity. If I use WebSecurityOrigin this class needs
to
> > live in media/blink instead of media/base. Let me know if you have any
> concerns.
> 
> I don't know the security implications of choosing one over the other, I
> generally support whatever the simplest option is that provides the required
> features. (WebSecurityOrigin seems to have a bit of bloat in it.)

Yeah, as discussed offline, I'll keep the current implementation unless there's
a clear better way to enforce security origin check.

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:138: if (!media_permission_) {
Need this to suppress a unused memeber variable warning/error on
|media_mermission_|.

FYI, |media_permission| could be null. See the change in html_document.cc.

[xhwang, 2015-01-26 22:10:00]

xhwang@chromium.org changed reviewers:
+ aa@chromium.org, nasko@chromium.org

[xhwang, 2015-01-26 22:10:01]

ddorwin: Please let me know if you still want to do a high level review.

nasco: Please OWNERS review content/

aa: Please OWNERS review mojo/services/html_viewer/*. BTW, I am updating
content/common/permission_service.mojom and am using mojo in real Chromium for
the first time. You are welcome to review changes there as well!

[xhwang, 2015-01-26 22:12:39]

tsepez: Please OWNERS review the change in
content/common/permission_service.mojom

[xhwang, 2015-01-26 22:13:20]

xhwang@chromium.org changed reviewers:
+ tsepez@chromium.org

[xhwang, 2015-01-26 22:13:21]

tsepez: Please OWNERS review the change in
content/common/permission_service.mojom

[xhwang, 2015-01-26 23:23:44]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org
- nasko@chromium.org

[xhwang, 2015-01-26 23:23:44]

s/nasko/jam since nasko is busy.

jam@: Please OWNERS review changes in content/*.

[ddorwin, 2015-01-26 23:33:46]

I skimmed the code.

https://codereview.chromium.org/871663003/diff/80001/content/common/permissio...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/871663003/diff/80001/content/common/permissio...
content/common/permission_service.mojom:18: // start listening to permission
changes.
How does a client listen to permissions? Is that a missing API or do these
methods not actually match the behaviors described in MediaPermission?

https://codereview.chromium.org/871663003/diff/80001/content/common/permissio...
content/common/permission_service.mojom:19: interface PermissionService {
Separate CL: It would be nice if this file had detailed comments about user
interaction like you have in media_permission.h.

https://codereview.chromium.org/871663003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/871663003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3395:
DCHECK(!media_permission_dispatcher_);
Other than this DCHECK, do we need to save the pointer in this class? The
web_encrypted_media_client_ check on 3384 should be sufficient, right?

https://codereview.chromium.org/871663003/diff/80001/media/base/media_permiss...
File media/base/media_permission.h (right):

https://codereview.chromium.org/871663003/diff/80001/media/base/media_permiss...
media/base/media_permission.h:31: virtual void HasPermission(
"Has" seems wrong here. To make the intent clear, perhaps it should be
SilentlyCheckPermission or something like that.

Oh, this is a name from the permission service. That's more okay then (assuming
the behaviors are the same).

https://codereview.chromium.org/871663003/diff/80001/media/base/media_permiss...
media/base/media_permission.h:37: // interaction (e.g. permission info bar) if
the permission has never been
nit: s/info bar/prompt/ (or request) to be more general and avoid rotting.

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:138: if (!media_permission_) {
On 2015/01/26 21:49:06, xhwang wrote:
> Need this to suppress a unused memeber variable warning/error on
> |media_mermission_|.

Why not just an unused parameter macro? Or do you really want the logging?
Either way, the macro (with DVLOG_IF) would probably be better.
> 
> FYI, |media_permission| could be null. See the change in html_document.cc.

Are we going to need to check for null before calling? Should html_document.cc
provide a dummy impl instead?

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:140: }
TODO: Do something with the new pointer?

https://codereview.chromium.org/871663003/diff/80001/mojo/services/html_viewe...
File mojo/services/html_viewer/html_document.cc (right):

https://codereview.chromium.org/871663003/diff/80001/mojo/services/html_viewe...
mojo/services/html_viewer/html_document.cc:281: make_scoped_ptr(new
media::DefaultCdmFactory()), nullptr));
TODO?

[Tom Sepez, 2015-01-26 23:45:10]

RS LGTM on adding an enum constant.

[xhwang, 2015-01-27 00:01:10]

comments addressed

[xhwang, 2015-01-27 00:01:56]

xhwang@chromium.org changed reviewers:
+ mlamouri@chromium.org

[xhwang, 2015-01-27 00:01:58]

mlamouri: Please see the comment in permission_service.mojom

https://codereview.chromium.org/871663003/diff/80001/content/common/permissio...
File content/common/permission_service.mojom (right):

https://codereview.chromium.org/871663003/diff/80001/content/common/permissio...
content/common/permission_service.mojom:18: // start listening to permission
changes.
On 2015/01/26 23:33:46, ddorwin wrote:
> How does a client listen to permissions? Is that a missing API or do these
> methods not actually match the behaviors described in MediaPermission?

I don't think this file is complete. For example, there's no way to "revoke"
permission. Agreed that this file needs more comments and cleanups. +mlamouri
for comments.

https://codereview.chromium.org/871663003/diff/80001/content/common/permissio...
content/common/permission_service.mojom:19: interface PermissionService {
On 2015/01/26 23:33:46, ddorwin wrote:
> Separate CL: It would be nice if this file had detailed comments about user
> interaction like you have in media_permission.h.

ditto

https://codereview.chromium.org/871663003/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/871663003/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:3395:
DCHECK(!media_permission_dispatcher_);
On 2015/01/26 23:33:46, ddorwin wrote:
> Other than this DCHECK, do we need to save the pointer in this class? The
> web_encrypted_media_client_ check on 3384 should be sufficient, right?

Since |media_permission_dispatcher_| is a RenderFrameObserver, I feel it's
clearer to have RenderFrameImpl to own it. Otherwise the ownership (e.g. who
deletes it) will be harder to track.

https://codereview.chromium.org/871663003/diff/80001/media/base/media_permiss...
File media/base/media_permission.h (right):

https://codereview.chromium.org/871663003/diff/80001/media/base/media_permiss...
media/base/media_permission.h:31: virtual void HasPermission(
On 2015/01/26 23:33:46, ddorwin wrote:
> "Has" seems wrong here. To make the intent clear, perhaps it should be
> SilentlyCheckPermission or something like that.
> 
> Oh, this is a name from the permission service. That's more okay then
(assuming
> the behaviors are the same).

Acknowledged.

https://codereview.chromium.org/871663003/diff/80001/media/base/media_permiss...
media/base/media_permission.h:37: // interaction (e.g. permission info bar) if
the permission has never been
On 2015/01/26 23:33:46, ddorwin wrote:
> nit: s/info bar/prompt/ (or request) to be more general and avoid rotting.

Done.

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:138: if (!media_permission_) {
On 2015/01/26 23:33:46, ddorwin wrote:
> On 2015/01/26 21:49:06, xhwang wrote:
> > Need this to suppress a unused memeber variable warning/error on
> > |media_mermission_|.
> 
> Why not just an unused parameter macro? Or do you really want the logging?

I remember we had that macro before, but I can't find it now.

> Either way, the macro (with DVLOG_IF) would probably be better.

DVLOG_IF doesn't work because it doesn't check the condition is release builds.

> > FYI, |media_permission| could be null. See the change in html_document.cc.
> 
> Are we going to need to check for null before calling? Should html_document.cc
> provide a dummy impl instead?

I debated about that. Allowing null seems to be simpler. 

Also, there's no permission code hooked up in HtmlViewer yet. So the dummy impl
would pretty much be NOTIMPLEMENTED() and I am not sure whether it should return
true or false. Currently HTMLViewer only supports clear key, so we actually
don't need permission check for now. How about revisiting this later when we add
more key system support and hook up permission service with HTMLViewer?

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:140: }
On 2015/01/26 23:33:46, ddorwin wrote:
> TODO: Do something with the new pointer?

Done.

https://codereview.chromium.org/871663003/diff/80001/mojo/services/html_viewe...
File mojo/services/html_viewer/html_document.cc (right):

https://codereview.chromium.org/871663003/diff/80001/mojo/services/html_viewe...
mojo/services/html_viewer/html_document.cc:281: make_scoped_ptr(new
media::DefaultCdmFactory()), nullptr));
On 2015/01/26 23:33:46, ddorwin wrote:
> TODO?

Done.

[ddorwin, 2015-01-27 00:09:03]

LG

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:138: if (!media_permission_) {
On 2015/01/27 00:01:58, xhwang wrote:
> On 2015/01/26 23:33:46, ddorwin wrote:
> > On 2015/01/26 21:49:06, xhwang wrote:
> > > Need this to suppress a unused memeber variable warning/error on
> > > |media_mermission_|.
> > 
> > Why not just an unused parameter macro? Or do you really want the logging?
> 
> I remember we had that macro before, but I can't find it now.

Perhaps we just remove parameters (this is a member) in that case. However, when
we need a parameter for a DCHECK, we apparently automatically handle the opt
build case:
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=u....
That may not apply to DLOG is not, though.
> 
> > Either way, the macro (with DVLOG_IF) would probably be better.
> 
> DVLOG_IF doesn't work because it doesn't check the condition is release
builds.
> 
> > > FYI, |media_permission| could be null. See the change in html_document.cc.
> > 
> > Are we going to need to check for null before calling? Should
html_document.cc
> > provide a dummy impl instead?
> 
> I debated about that. Allowing null seems to be simpler. 
> 
> Also, there's no permission code hooked up in HtmlViewer yet. So the dummy
impl
> would pretty much be NOTIMPLEMENTED() and I am not sure whether it should
return
> true or false. Currently HTMLViewer only supports clear key, so we actually
> don't need permission check for now. How about revisiting this later when we
add
> more key system support and hook up permission service with HTMLViewer?

Ack

[xhwang, 2015-01-27 00:32:59]

comments addressed

[xhwang, 2015-01-27 00:34:05]

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
File media/blink/webencryptedmediaclient_impl.cc (right):

https://codereview.chromium.org/871663003/diff/80001/media/blink/webencrypted...
media/blink/webencryptedmediaclient_impl.cc:138: if (!media_permission_) {
On 2015/01/27 00:09:03, ddorwin wrote:
> On 2015/01/27 00:01:58, xhwang wrote:
> > On 2015/01/26 23:33:46, ddorwin wrote:
> > > On 2015/01/26 21:49:06, xhwang wrote:
> > > > Need this to suppress a unused memeber variable warning/error on
> > > > |media_mermission_|.
> > > 
> > > Why not just an unused parameter macro? Or do you really want the logging?
> > 
> > I remember we had that macro before, but I can't find it now.
> 
> Perhaps we just remove parameters (this is a member) in that case. 

Done.

However, when
> we need a parameter for a DCHECK, we apparently automatically handle the opt
> build case:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=u....
> That may not apply to DLOG is not, though.
> > 
> > > Either way, the macro (with DVLOG_IF) would probably be better.
> > 
> > DVLOG_IF doesn't work because it doesn't check the condition is release
> builds.
> > 
> > > > FYI, |media_permission| could be null. See the change in
html_document.cc.
> > > 
> > > Are we going to need to check for null before calling? Should
> html_document.cc
> > > provide a dummy impl instead?
> > 
> > I debated about that. Allowing null seems to be simpler. 
> > 
> > Also, there's no permission code hooked up in HtmlViewer yet. So the dummy
> impl
> > would pretty much be NOTIMPLEMENTED() and I am not sure whether it should
> return
> > true or false. Currently HTMLViewer only supports clear key, so we actually
> > don't need permission check for now. How about revisiting this later when we
> add
> > more key system support and hook up permission service with HTMLViewer?
> 
> Ack

[jam, 2015-01-27 16:26:14]

lgtm

[xhwang, 2015-01-27 17:48:52]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-01-27 17:49:59]

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

[commit-bot: I haz the power, 2015-01-27 17:54:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-27 17:55:33]

Patchset 7 (id:??) landed as
https://crrev.com/36b1f3badd1e3f36d0fb5eb63328d2de4da051ae
Cr-Commit-Position: refs/heads/master@{#313303}