media: Add MojoCdmServiceContext.

media: Add MojoCdmServiceContext.

MojoCdmServiceContext will serve as a bridge between a MojoRendererService and a
MojoCdmService such that the renderer service can use the CDM service to decrypt
(and decode) encrypted contents.

With this change, now MojoCdmServiceContext owns and manages all MojoCdmService
instances so that MojoRendererService can find the correct CDM to use given a
|cdm_id|. As a result, MojoCdmService is not strongly bound to the pipe anymore.

An alternative to this is to have MojoCdmService still strongly bound to the
pipe, but notifies the MojoCdmServiceContext when the service is created and
destroyed. Here I was just following existing code like PermissionServiceContext
and GeolocationServiceContext.

TBR=sky@chromium.org
BUG=432998
Committed: https://crrev.com/ec48647eb0ed539e7436b3a299f8d5e5073c76f8
Cr-Commit-Position: refs/heads/master@{#334061}

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

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

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

This is split from the original CL (840473002) for easier discussion and review.
It covers the MojoCdmService creation/management part. See CL description for
details.

It also addresses parts of the comments in
https://codereview.chromium.org/840473002/#ps160001. I posted replies in that
issue. Sorry for the confusion.

PTAL

[sandersd (OOO until July 31), 2015-06-04 22:53:08]

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:17: // content decryption module (CDM) to decrypt (and
decode) encrypted buffers.
Add that CdmContext is only used for calling SetCdm().

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:27: // valid during the CDM's lifetime.
Add that if the return value will not be NULL if GetCdmId() is kInvalidCdm.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:32: // different process.
This sentence is confusing. I would say:

Returns an ID that identifies a CDM, or kInvalidCdmId. The interpretation is
implementation-specific; current implementations use the ID to locate a remote
CDM in the browser process.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:34: // GetDecryptor() should return a non-null
Decryptor.
I would say:

The return value will not be kInvalidCdm if GetDecryptor() returns NULL.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:44: // An interface that provides the access to a
CdmContext through |cdm_id|.
An interface for looking up CdmContext objects by |cdm_id|.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:45: // This is typically used when CDMs are hosted
remotely.
This is confusing because a CdmContextProvider only locates CDMs that are local
(to the caller). I would just remove it.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:55: // CdmContext should return kInvalidCdmId.
I would say:

Note: Calling GetCdmId() on the returned CdmContext returns kInvalidCdmId (in
all current cases) because the CDM will be local.

https://codereview.chromium.org/1165633004/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.cc (right):

https://codereview.chromium.org/1165633004/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:33: CdmContext*
MojoCdmServiceContext::GetCdmContext(int32_t cdm_id) {
The returned pointer can be deleted at any time. What do clients need to do to
be sure they don't try to dereference garbage?

[xhwang, 2015-06-05 17:55:59]

rebase only

[xhwang, 2015-06-05 18:14:11]

comments addressed

[xhwang, 2015-06-05 18:14:52]

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:17: // content decryption module (CDM) to decrypt (and
decode) encrypted buffers.
On 2015/06/04 22:53:07, sandersd wrote:
> Add that CdmContext is only used for calling SetCdm().

Done.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:27: // valid during the CDM's lifetime.
On 2015/06/04 22:53:07, sandersd wrote:
> Add that if the return value will not be NULL if GetCdmId() is kInvalidCdm.

Done.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:32: // different process.
On 2015/06/04 22:53:07, sandersd wrote:
> This sentence is confusing. I would say:
> 
> Returns an ID that identifies a CDM, or kInvalidCdmId. The interpretation is
> implementation-specific; current implementations use the ID to locate a remote
> CDM in the browser process.

Done with s/the browser/a different/ because we can host a CDM in any process
(e.g. gpu, utility...)

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:34: // GetDecryptor() should return a non-null
Decryptor.
On 2015/06/04 22:53:07, sandersd wrote:
> I would say:
> 
> The return value will not be kInvalidCdm if GetDecryptor() returns NULL.

Done.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:44: // An interface that provides the access to a
CdmContext through |cdm_id|.
On 2015/06/04 22:53:07, sandersd wrote:
> An interface for looking up CdmContext objects by |cdm_id|.

Done.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:45: // This is typically used when CDMs are hosted
remotely.
On 2015/06/04 22:53:07, sandersd wrote:
> This is confusing because a CdmContextProvider only locates CDMs that are
local
> (to the caller). I would just remove it.

Done.

https://codereview.chromium.org/1165633004/diff/20001/media/base/cdm_context....
media/base/cdm_context.h:55: // CdmContext should return kInvalidCdmId.
On 2015/06/04 22:53:07, sandersd wrote:
> I would say:
> 
> Note: Calling GetCdmId() on the returned CdmContext returns kInvalidCdmId (in
> all current cases) because the CDM will be local.

Done.

https://codereview.chromium.org/1165633004/diff/20001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.cc (right):

https://codereview.chromium.org/1165633004/diff/20001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:33: CdmContext*
MojoCdmServiceContext::GetCdmContext(int32_t cdm_id) {
On 2015/06/04 22:53:08, sandersd wrote:
> The returned pointer can be deleted at any time. What do clients need to do to
> be sure they don't try to dereference garbage?

Yes, that's in the plan. Added a TODO to make it clear.

[ddorwin, 2015-06-08 18:33:11]

LG. Mostly nits.

https://codereview.chromium.org/1165633004/diff/60001/components/html_viewer/...
File components/html_viewer/media_factory.cc (right):

https://codereview.chromium.org/1165633004/diff/60001/components/html_viewer/...
components/html_viewer/media_factory.cc:179: // TODO(xhwang): Replace
DefaultMediaPermission with something real when
Add NOTIMPLEMENTED?

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context....
media/base/cdm_context.h:34: // remote CDM in a different process. The return
value will not be kInvalidCdm
kInvalidCdmId

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context....
media/base/cdm_context.h:45: // An interface for looking up CdmContext objects
by |cdm_id|.
nit: Probably don't use variable names in a class defn.

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context....
media/base/cdm_context.h:53: // (in all current cases) because the CDM will be
local.
Local in the service side?

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:24: MojoCdmServiceContext* context,
Is this a pattern? Otherwise, it should be after const params.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:28: // TODO(xhwang): Use a CdmFactory to
create the CDM here. See
Would it be better to create AesDecryptor here now since that's how you intend
for this to work?

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service.h (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.h:27: // destructed when
OnConnectionError() happens.
nit: A function can be called but not "happen". A connection error can happen.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.h:60: // TODO(xhwang): Rename this to
GetDecryptor in the mojom interface.
Is this going to be used for the desktop case only?

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.cc (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
nit: year is inconsistent with .h. Is that correct?

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:27: MojoCdmService::Create(this,
key_system, request.Pass());
Should this function be returning a scoped_ptr?

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:33: // TODO(xhwang): The
returned CdmContext can be destroyed at any time if the
This seems worse than what is implied by the comments in the interface defn. You
might want to add this type of warning there.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.h (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:25: MojoCdmService* Create(
Should probably be CreateService. Create usually means one of "this" object.

[xhwang, 2015-06-08 22:12:09]

comments addressed

[xhwang, 2015-06-08 22:12:34]

PTAL again.

https://codereview.chromium.org/1165633004/diff/60001/components/html_viewer/...
File components/html_viewer/media_factory.cc (right):

https://codereview.chromium.org/1165633004/diff/60001/components/html_viewer/...
components/html_viewer/media_factory.cc:179: // TODO(xhwang): Replace
DefaultMediaPermission with something real when
On 2015/06/08 18:33:09, ddorwin wrote:
> Add NOTIMPLEMENTED?

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context....
media/base/cdm_context.h:34: // remote CDM in a different process. The return
value will not be kInvalidCdm
On 2015/06/08 18:33:10, ddorwin wrote:
> kInvalidCdmId

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context....
media/base/cdm_context.h:45: // An interface for looking up CdmContext objects
by |cdm_id|.
On 2015/06/08 18:33:10, ddorwin wrote:
> nit: Probably don't use variable names in a class defn.

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/base/cdm_context....
media/base/cdm_context.h:53: // (in all current cases) because the CDM will be
local.
On 2015/06/08 18:33:10, ddorwin wrote:
> Local in the service side?

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:24: MojoCdmServiceContext* context,
On 2015/06/08 18:33:10, ddorwin wrote:
> Is this a pattern? Otherwise, it should be after const params.

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.cc:28: // TODO(xhwang): Use a CdmFactory to
create the CDM here. See
On 2015/06/08 18:33:10, ddorwin wrote:
> Would it be better to create AesDecryptor here now since that's how you intend
> for this to work?

I need to create the AesDecryptor in MojoCdmService so that I can get to the
weak_ptr_. In the future, I will probably also use the CdmFactory within
MojoCdmService.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service.h (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.h:27: // destructed when
OnConnectionError() happens.
On 2015/06/08 18:33:10, ddorwin wrote:
> nit: A function can be called but not "happen". A connection error can happen.

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.h:60: // TODO(xhwang): Rename this to
GetDecryptor in the mojom interface.
On 2015/06/08 18:33:10, ddorwin wrote:
> Is this going to be used for the desktop case only?

Yes. I'll fix this TODO in the next CL.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.cc (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/06/08 18:33:10, ddorwin wrote:
> nit: year is inconsistent with .h. Is that correct?

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:27: MojoCdmService::Create(this,
key_system, request.Pass());
On 2015/06/08 18:33:10, ddorwin wrote:
> Should this function be returning a scoped_ptr?

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.cc:33: // TODO(xhwang): The
returned CdmContext can be destroyed at any time if the
On 2015/06/08 18:33:10, ddorwin wrote:
> This seems worse than what is implied by the comments in the interface defn.
You
> might want to add this type of warning there.

Done.

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.h (right):

https://codereview.chromium.org/1165633004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:25: MojoCdmService* Create(
On 2015/06/08 18:33:11, ddorwin wrote:
> Should probably be CreateService. Create usually means one of "this" object.

Done.

[xhwang, 2015-06-08 22:13:38]

https://codereview.chromium.org/1165633004/diff/80001/components/html_viewer/...
File components/html_viewer/media_factory.cc (left):

https://codereview.chromium.org/1165633004/diff/80001/components/html_viewer/...
components/html_viewer/media_factory.cc:177: 
This was accidentally introduced in the last CL. It's a duplicate of
l.146-l.160.

[xhwang, 2015-06-10 15:49:01]

ddorwin/sandersd: kindly ping :)

[sandersd (OOO until July 31), 2015-06-10 18:07:19]

lgtm

[ddorwin, 2015-06-11 19:09:44]

LGTM % comments. Thanks.

https://codereview.chromium.org/1165633004/diff/80001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/1165633004/diff/80001/media/base/cdm_context....
media/base/cdm_context.h:54: // GetCdmContext() is called.
Add this from the impl class?
The returned CdmContext can be destroyed at any time if the pipe is
disconnected.

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.h (right):

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:22: // Creates a MojoCdmService
for |key_system| and weakly bind it to |request|.
nit: binds

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:24: // if no MojoCdmService can
be created.
Perhaps add:
", resulting in a connection error"

Is that a common way to report creation errors?

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:33: // pipe is disconnected. When
implementing SetCdm(), make sure we never
I think the first sentence is documentation, and the second is the TODO. Is that
right?
Well, you can keep the whole thing as a TODO if you copy the first sentence to
the interface defn (see my earlier comment).

[xhwang, 2015-06-11 19:46:27]

rebase only

[xhwang, 2015-06-11 20:57:01]

comments addressed

[xhwang, 2015-06-11 20:58:01]

https://codereview.chromium.org/1165633004/diff/80001/media/base/cdm_context.h
File media/base/cdm_context.h (right):

https://codereview.chromium.org/1165633004/diff/80001/media/base/cdm_context....
media/base/cdm_context.h:54: // GetCdmContext() is called.
On 2015/06/11 19:09:43, ddorwin wrote:
> Add this from the impl class?
> The returned CdmContext can be destroyed at any time if the pipe is
> disconnected.

See responses above.

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service_context.h (right):

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:22: // Creates a MojoCdmService
for |key_system| and weakly bind it to |request|.
On 2015/06/11 19:09:43, ddorwin wrote:
> nit: binds

Done.

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:24: // if no MojoCdmService can
be created.
On 2015/06/11 19:09:43, ddorwin wrote:
> Perhaps add:
> ", resulting in a connection error"

Done.

> Is that a common way to report creation errors?

Yes. Another way is to use a factory class to do this, so you know whether it's
not supported, or actually an unexpected connection error happened. This is what
I did in the previous CL but I found it more complex and not worth the benefit.

https://codereview.chromium.org/1165633004/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service_context.h:33: // pipe is disconnected. When
implementing SetCdm(), make sure we never
On 2015/06/11 19:09:43, ddorwin wrote:
> I think the first sentence is documentation, and the second is the TODO. Is
that
> right?

Done.

> Well, you can keep the whole thing as a TODO if you copy the first sentence to
> the interface defn (see my earlier comment).

The interface is generic, where it knows nothing about a pipe. This comment only
applies for this particular implementation where we use a pipe.

[xhwang, 2015-06-11 20:59:11]

TBRing sky@chromium.org for trivial changes in
components/html_viewer/media_factory.cc

[xhwang, 2015-06-11 21:45:23]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-06-11 21:45:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
ddorwin@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1165633004/#ps120001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-06-11 21:47:26]

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

[commit-bot: I haz the power, 2015-06-11 22:38:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-11 22:40:37]

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