Introduce CdmFactory interface.

Introduce CdmFactory interface.

This is a preparation for media code to call into content code to create the CDMs, so that we could have EME support without depending on content code.

BUG=422730
Committed: https://crrev.com/00f7cdc52b6fcab8deda7c5e266c2db8ba60a28a
Cr-Commit-Position: refs/heads/master@{#300296}

[xhwang, 2014-10-15 23:33:58]

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

[xhwang, 2014-10-15 23:33:59]

This CL is based on https://codereview.chromium.org/651113002/.

It's another step towards moving a lot of stuff to media/.

PTAL!

[xhwang, 2014-10-15 23:48:11]

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

[ddorwin, 2014-10-16 17:32:52]

I like the reduction of ifdef's. Hopefully we can remove more (cdm_id) in a
future CL.

Is the description accurate/complete? Is this CL in preparation for the last
sentence?

https://codereview.chromium.org/660673002/diff/1/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/660673002/diff/1/content/content_renderer.gyp...
content/content_renderer.gypi:264:
'renderer/media/crypto/render_cdm_factory.cc',
Is there a .gn update missing too?

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:28: #include
"content/renderer/media/crypto/render_cdm_factory.h"
I really dislike "Render" as a noun. It is a verb except in a few uncommon cases
that are unrelated. I would say a renderer (n) is something that renders (v).
"renderer" is also consistent with other code (i.e. l25-26 above) and the
directory name. Is there precedent for this somewhere.?

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:36: bool
CdmSessionAdapter::Initialize(scoped_ptr<media::CdmFactory> cdm_factory,
It seems odd to pass ownership of a factory. It forces the caller to create a
new factory for each call, regardless of what makes sense. Since this function
is synchronous, we could pass a pointer and have the caller decide whether to
destroy it or not.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:40: // KeySystemNameForUMA out of
src/content so we can move CdmSessionAdapter to
We probably need to inject the key systems into media/.
UMA name is currently a special case, but I think we can replace the hard-coding
and use the same mechanism we use for adding other key system info.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:48: #if
defined(ENABLE_BROWSER_CDMS)
We've eliminated other ifdef's. Can we eliminate this  eventually?

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:17:
#include "content/renderer/pepper/pepper_webplugin_impl.h"
i don't think we need this.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:29:
#include "content/renderer/media/crypto/render_cdm_factory.h"
same comment as below

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:176: #if
defined(ENABLE_PEPPER_CDMS)
What if you only wanted to support Clear Key?
Should we be ifdef'ing the constructor's parameters like we used to ifdef the
Create() parameters?

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.cc:207: #if
defined(ENABLE_BROWSER_CDMS)
We could eliminate 2 of the 3 ifdef's if MediaKeys was responsible for this
value. I think maybe we had wanted to avoid that, but maybe we should
reconsider.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/render_cdm_factory.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
nit: I don't think we change this. Also, the .h was not changed.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/render_cdm_factory.cc:50: if
(CanUseAesDecryptor(key_system)) {
The TODOs are still valid, though you could address the second one. For the
first one, you can change it to make AesDecryptor take the origin.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/render_cdm_factory.h:38: // |create_pepper_cdm_cb|
will be called synchronously if necessary. The other
Does not apply to this function.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.h:11: #include
"media/base/cdm_factory.h"
Can both of these be fwd declared?

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.h:62:
blink::WebContentDecryptionModuleResult result);
ICWYU

https://codereview.chromium.org/660673002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:156: 
nit: There is a #elif here in most cases.

https://codereview.chromium.org/660673002/diff/1/media/base/cdm_factory.h
File media/base/cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/1/media/base/cdm_factory.h#new...
media/base/cdm_factory.h:27: int* cdm_id,  // Output parameter indicating the
CDM ID of the MediaKeys.
This comment seems weird. Why wouldn't the MK know its own CDM ID? (See also
earlier comment.)

[xhwang, 2014-10-16 20:08:11]

comments addressed

[xhwang, 2014-10-16 20:08:40]

Thanks for the comments. PTAL again!

https://codereview.chromium.org/660673002/diff/1/content/content_renderer.gypi
File content/content_renderer.gypi (right):

https://codereview.chromium.org/660673002/diff/1/content/content_renderer.gyp...
content/content_renderer.gypi:264:
'renderer/media/crypto/render_cdm_factory.cc',
On 2014/10/16 17:32:51, ddorwin wrote:
> Is there a .gn update missing too?

content_renderer doesn't have a gn. I don't know how that works though.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:28: #include
"content/renderer/media/crypto/render_cdm_factory.h"
On 2014/10/16 17:32:51, ddorwin wrote:
> I really dislike "Render" as a noun. It is a verb except in a few uncommon
cases
> that are unrelated. I would say a renderer (n) is something that renders (v).
> "renderer" is also consistent with other code (i.e. l25-26 above) and the
> directory name. Is there precedent for this somewhere.?

render is used more often than renderer, e.g. render_frame_impl,
render_process_impl, render_view, render_thread...

Here's a good example of how render/renderer are used in file names:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/&...

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:36: bool
CdmSessionAdapter::Initialize(scoped_ptr<media::CdmFactory> cdm_factory,
On 2014/10/16 17:32:51, ddorwin wrote:
> It seems odd to pass ownership of a factory. It forces the caller to create a
> new factory for each call, regardless of what makes sense. Since this function
> is synchronous, we could pass a pointer and have the caller decide whether to
> destroy it or not.

Good point. Done.

Currently we are still creating a new factory every time we create a new CDM,
this is because the |frame| is passed from Blink. We could have a
set_frame(frame) function on RenderCdmFactory, but since this is a simple class,
I am not sure whether it's worth it.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:40: // KeySystemNameForUMA out of
src/content so we can move CdmSessionAdapter to
On 2014/10/16 17:32:51, ddorwin wrote:
> We probably need to inject the key systems into media/.
> UMA name is currently a special case, but I think we can replace the
hard-coding
> and use the same mechanism we use for adding other key system info.

Acknowledged.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:48: #if
defined(ENABLE_BROWSER_CDMS)
On 2014/10/16 17:32:51, ddorwin wrote:
> We've eliminated other ifdef's. Can we eliminate this  eventually?

Totally agreed. I am thinking about it...

This is the history:
https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h#newc...

How about reintroduce GetCdmId() to MediaKeys, but put it in the #if
defined(ENABLE_BROWSER_CDMS)?

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:17:
#include "content/renderer/pepper/pepper_webplugin_impl.h"
On 2014/10/16 17:32:52, ddorwin wrote:
> i don't think we need this.

Done.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:176: #if
defined(ENABLE_PEPPER_CDMS)
On 2014/10/16 17:32:52, ddorwin wrote:
> What if you only wanted to support Clear Key?
> Should we be ifdef'ing the constructor's parameters like we used to ifdef the
> Create() parameters?

Not sure what you are asking exactly... A few points here:
1, I have moved the cdm_factory to InitializeCdm() so that we don't need to keep
it.
2, Currently ClearKey is also handled by RenderCdmFactory.
3, In the future, we'll need to be able to create the ClearKey CDM without using
content/. I am not sure what's the best way to do it yet. We can create
AesDecryptor directly, or have a media::CdmFactoryImpl to create AesDecryptor.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/proxy_decryptor.cc:207: #if
defined(ENABLE_BROWSER_CDMS)
On 2014/10/16 17:32:52, ddorwin wrote:
> We could eliminate 2 of the 3 ifdef's if MediaKeys was responsible for this
> value. I think maybe we had wanted to avoid that, but maybe we should
> reconsider.

Agreed. See comment above. I'll do that in another cleanup CL. WDYT?

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/render_cdm_factory.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/10/16 17:32:52, ddorwin wrote:
> nit: I don't think we change this. Also, the .h was not changed.

Reverted.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/render_cdm_factory.cc:50: if
(CanUseAesDecryptor(key_system)) {
On 2014/10/16 17:32:52, ddorwin wrote:
> The TODOs are still valid, though you could address the second one. For the
> first one, you can change it to make AesDecryptor take the origin.

These are accidentally removed. Thanks for catching it.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/render_cdm_factory.h:38: // |create_pepper_cdm_cb|
will be called synchronously if necessary. The other
On 2014/10/16 17:32:52, ddorwin wrote:
> Does not apply to this function.

Done.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.h:11: #include
"media/base/cdm_factory.h"
On 2014/10/16 17:32:52, ddorwin wrote:
> Can both of these be fwd declared?

When using scoped_ptr we can't. Now I am passing by ref and we can now!

https://codereview.chromium.org/660673002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:156: 
On 2014/10/16 17:32:52, ddorwin wrote:
> nit: There is a #elif here in most cases.

Done.

https://codereview.chromium.org/660673002/diff/1/media/base/cdm_factory.h
File media/base/cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/1/media/base/cdm_factory.h#new...
media/base/cdm_factory.h:27: int* cdm_id,  // Output parameter indicating the
CDM ID of the MediaKeys.
On 2014/10/16 17:32:52, ddorwin wrote:
> This comment seems weird. Why wouldn't the MK know its own CDM ID? (See also
> earlier comment.)

Acknowledged.

[ddorwin, 2014-10-16 20:33:20]

LG overall. Note there are a few comments in PS1.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:28: #include
"content/renderer/media/crypto/render_cdm_factory.h"
On 2014/10/16 20:08:39, xhwang wrote:
> On 2014/10/16 17:32:51, ddorwin wrote:
> > I really dislike "Render" as a noun. It is a verb except in a few uncommon
> cases
> > that are unrelated. I would say a renderer (n) is something that renders
(v).
> > "renderer" is also consistent with other code (i.e. l25-26 above) and the
> > directory name. Is there precedent for this somewhere.?
> 
> render is used more often than renderer, e.g. render_frame_impl,
> render_process_impl, render_view, render_thread...
> 
> Here's a good example of how render/renderer are used in file names:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/&...

It's about 40/60 in that directory. I would argue that Render classes are
related to rendering whereas Renderer classes are an implementation of some
interface in the renderer process (rather than the browser, GPU, etc.).

Maybe we should ask content/ owners or chromium-dev.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:48: #if
defined(ENABLE_BROWSER_CDMS)
On 2014/10/16 20:08:39, xhwang wrote:
> On 2014/10/16 17:32:51, ddorwin wrote:
> > We've eliminated other ifdef's. Can we eliminate this  eventually?
> 
> Totally agreed. I am thinking about it...
> 
> This is the history:
>
https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h#newc...
> 
> How about reintroduce GetCdmId() to MediaKeys, but put it in the #if
> defined(ENABLE_BROWSER_CDMS)?

Sounds good. Please make a separate CL after this lands so we can see how the
footprint changes.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:176: #if
defined(ENABLE_PEPPER_CDMS)
On 2014/10/16 20:08:39, xhwang wrote:
> On 2014/10/16 17:32:52, ddorwin wrote:
> > What if you only wanted to support Clear Key?
> > Should we be ifdef'ing the constructor's parameters like we used to ifdef
the
> > Create() parameters?
> 
> Not sure what you are asking exactly... A few points here:
> 1, I have moved the cdm_factory to InitializeCdm() so that we don't need to
keep
> it.
> 2, Currently ClearKey is also handled by RenderCdmFactory.
> 3, In the future, we'll need to be able to create the ClearKey CDM without
using
> content/. I am not sure what's the best way to do it yet. We can create
> AesDecryptor directly, or have a media::CdmFactoryImpl to create AesDecryptor.

Nevermind. I think the structure threw me off.

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:28:
#include "content/renderer/media/crypto/render_cdm_factory.h"
The factory must always be included.

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:51: // TODO(jrummell): Pass
|security_origin| to all constructors.
jrummell: I think this now just means AesDecryptor, which will eat it.

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:54: //
DCHECK(security_origin.is_valid());
jrummell: ping :)

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.h:56: const RendererCdmManager*
manager_;
Does this compile on Android? It seems unlikely we can create using a const
manager.

Either way, making Create() const may not be a good idea since it limits
implementations unnecessarily. (Unfortuantely, that means using a * instead of
const&

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:11: #include
"third_party/WebKit/public/platform/WebContentDecryptionModule.h"
What about this?
Replace with public/platform/WebContentDecryptionModuleResult.h ?

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:62:
blink::WebContentDecryptionModuleResult result);
ICWYU

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

https://codereview.chromium.org/660673002/diff/20001/media/base/cdm_factory.h...
media/base/cdm_factory.h:34: const SessionExpirationUpdateCB&
session_expiration_update_cb) const = 0;
const seems odd on create. See also my previous comment.

[xhwang, 2014-10-16 21:43:08]

rebase only

[jrummell, 2014-10-16 22:40:33]

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:51: // TODO(jrummell): Pass
|security_origin| to all constructors.
On 2014/10/16 20:33:20, ddorwin wrote:
> jrummell: I think this now just means AesDecryptor, which will eat it.

Looks that way.

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.cc:54: //
DCHECK(security_origin.is_valid());
On 2014/10/16 20:33:20, ddorwin wrote:
> jrummell: ping :)

I'll try this after this CL lands.

https://codereview.chromium.org/660673002/diff/40001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/660673002/diff/40001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:50: #endif  //
defined(ENABLE_PEPPER_CDMS)
comment doesn't line up with #if.

https://codereview.chromium.org/660673002/diff/40001/media/base/cdm_factory.h
File media/base/cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/40001/media/base/cdm_factory.h...
media/base/cdm_factory.h:35: 
Thought. Rather than passing optional "int* cdm_id" to Create(), would it be
simpler to add

#if defined(ENABLE_BROWSER_CDMS)
  int GetCdmId() { return cdm_id_; }
protected:
  int cdm_id_;
#endif

And then use that to get the cdmId in proxy_decryptor?

[xhwang, 2014-10-17 00:11:49]

Thanks, PTAL again.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:28: #include
"content/renderer/media/crypto/render_cdm_factory.h"
On 2014/10/16 20:33:20, ddorwin wrote:
> On 2014/10/16 20:08:39, xhwang wrote:
> > On 2014/10/16 17:32:51, ddorwin wrote:
> > > I really dislike "Render" as a noun. It is a verb except in a few uncommon
> > cases
> > > that are unrelated. I would say a renderer (n) is something that renders
> (v).
> > > "renderer" is also consistent with other code (i.e. l25-26 above) and the
> > > directory name. Is there precedent for this somewhere.?
> > 
> > render is used more often than renderer, e.g. render_frame_impl,
> > render_process_impl, render_view, render_thread...
> > 
> > Here's a good example of how render/renderer are used in file names:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/&...
> 
> It's about 40/60 in that directory. I would argue that Render classes are
> related to rendering whereas Renderer classes are an implementation of some
> interface in the renderer process (rather than the browser, GPU, etc.).
> 
> Maybe we should ask content/ owners or chromium-dev.

I think we had this discussion before and didn't get any result... sigh

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/cdm_s...
content/renderer/media/cdm_session_adapter.cc:48: #if
defined(ENABLE_BROWSER_CDMS)
On 2014/10/16 20:33:20, ddorwin wrote:
> On 2014/10/16 20:08:39, xhwang wrote:
> > On 2014/10/16 17:32:51, ddorwin wrote:
> > > We've eliminated other ifdef's. Can we eliminate this  eventually?
> > 
> > Totally agreed. I am thinking about it...
> > 
> > This is the history:
> >
>
https://codereview.chromium.org/193523002/diff/1/media/base/media_keys.h#newc...
> > 
> > How about reintroduce GetCdmId() to MediaKeys, but put it in the #if
> > defined(ENABLE_BROWSER_CDMS)?
> 
> Sounds good. Please make a separate CL after this lands so we can see how the
> footprint changes.

Acknowledged.

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/1/content/renderer/media/crypt...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:176: #if
defined(ENABLE_PEPPER_CDMS)
On 2014/10/16 20:33:20, ddorwin wrote:
> On 2014/10/16 20:08:39, xhwang wrote:
> > On 2014/10/16 17:32:52, ddorwin wrote:
> > > What if you only wanted to support Clear Key?
> > > Should we be ifdef'ing the constructor's parameters like we used to ifdef
> the
> > > Create() parameters?
> > 
> > Not sure what you are asking exactly... A few points here:
> > 1, I have moved the cdm_factory to InitializeCdm() so that we don't need to
> keep
> > it.
> > 2, Currently ClearKey is also handled by RenderCdmFactory.
> > 3, In the future, we'll need to be able to create the ClearKey CDM without
> using
> > content/. I am not sure what's the best way to do it yet. We can create
> > AesDecryptor directly, or have a media::CdmFactoryImpl to create
AesDecryptor.
> 
> Nevermind. I think the structure threw me off.

Acknowledged.

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:28:
#include "content/renderer/media/crypto/render_cdm_factory.h"
On 2014/10/16 20:33:20, ddorwin wrote:
> The factory must always be included.

Done.

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.h:56: const RendererCdmManager*
manager_;
On 2014/10/16 20:33:20, ddorwin wrote:
> Does this compile on Android? It seems unlikely we can create using a const
> manager.
> 
> Either way, making Create() const may not be a good idea since it limits
> implementations unnecessarily. (Unfortuantely, that means using a * instead of
> const&

I think you are right. But I do like passing const-ref around instead of a
pointer. It's seems pretty natural for me that the factory is const. A factory
doesn't really need to change it's state to create an object. It's kinda like
the create_pepper_cdm_cb callback we had, which is also passed as a const-ref.

I moved the position of "const" to make it work. It's a bit tricky, but seems
fine to me...

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:11: #include
"third_party/WebKit/public/platform/WebContentDecryptionModule.h"
On 2014/10/16 20:33:20, ddorwin wrote:
> What about this?
> Replace with public/platform/WebContentDecryptionModuleResult.h ?

I need this for blink::WebContentDecryptionModule...

https://codereview.chromium.org/660673002/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:62:
blink::WebContentDecryptionModuleResult result);
On 2014/10/16 20:33:20, ddorwin wrote:
> ICWYU

Done.

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

https://codereview.chromium.org/660673002/diff/20001/media/base/cdm_factory.h...
media/base/cdm_factory.h:34: const SessionExpirationUpdateCB&
session_expiration_update_cb) const = 0;
On 2014/10/16 20:33:20, ddorwin wrote:
> const seems odd on create. See also my previous comment.

Acknowledged.

[ddorwin, 2014-10-17 01:41:55]

I don't really like the const thing even though it simplifies a lot of code.
Non-const ref would be perfect here, but that's disallowed. :( How much code
would we need to change to make it a *? The factory couldn't be inline in the
call, but it shouldn't be too bad. WDYT?

https://codereview.chromium.org/660673002/diff/80001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/80001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.h:56: RendererCdmManager* const
manager_;
This is technically correct and will compile, but it's misleading and generally
discouraged to have access a modifiable object from a const function.

[xhwang, 2014-10-17 03:47:34]

comments addressed

[xhwang, 2014-10-17 03:54:21]

https://codereview.chromium.org/660673002/diff/80001/content/renderer/media/c...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/80001/content/renderer/media/c...
content/renderer/media/crypto/render_cdm_factory.h:56: RendererCdmManager* const
manager_;
On 2014/10/17 01:41:55, ddorwin wrote:
> This is technically correct and will compile, but it's misleading and
generally
> discouraged to have access a modifiable object from a const function.

Done.

[xhwang, 2014-10-17 03:56:30]

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

[xhwang, 2014-10-17 03:56:32]

jamesr@: Please OWNERS review changes in content/renderer/render_frame_impl.cc.

[xhwang, 2014-10-17 04:01:32]

Patchset #6 (id:100001) has been deleted

[ddorwin, 2014-10-17 16:32:04]

Thanks. LGTM with comments.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1528:
scoped_ptr<media::CdmFactory> cdm_factory(
You could also just have it on the stack and pass its address.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:183:
scoped_ptr<media::CdmFactory> cdm_factory(new RenderCdmFactory(
ditto

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:31: explicit
RenderCdmFactory(RendererCdmManager* manager);
What is the ownership story for the manager? How do we ensure it is still valid
when Create() is called?

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:56: RendererCdmManager*
manager_;
nit: It's okay for this to be const still if you want to indicate that the
manager cannot be replaced.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:1707: scoped_ptr<media::CdmFactory>
cdm_factory(new RenderCdmFactory(
ditto

[xhwang, 2014-10-17 18:54:53]

comments addressed

[xhwang, 2014-10-17 20:09:23]

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:1528:
scoped_ptr<media::CdmFactory> cdm_factory(
On 2014/10/17 16:32:04, ddorwin wrote:
> You could also just have it on the stack and pass its address.

Done.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/crypto/encrypted_media_player_support_impl.cc
(right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/encrypted_media_player_support_impl.cc:183:
scoped_ptr<media::CdmFactory> cdm_factory(new RenderCdmFactory(
On 2014/10/17 16:32:04, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:31: explicit
RenderCdmFactory(RendererCdmManager* manager);
On 2014/10/17 16:32:04, ddorwin wrote:
> What is the ownership story for the manager? How do we ensure it is still
valid
> when Create() is called?

It's a per-render-frame object owned by RenderFrameImpl.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:56: RendererCdmManager*
manager_;
On 2014/10/17 16:32:04, ddorwin wrote:
> nit: It's okay for this to be const still if you want to indicate that the
> manager cannot be replaced.

That's true. But usually we don't use const for this.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:1707: scoped_ptr<media::CdmFactory>
cdm_factory(new RenderCdmFactory(
On 2014/10/17 16:32:04, ddorwin wrote:
> ditto

Done.

[ddorwin, 2014-10-19 06:07:26]

LGTM with comment. Thanks.

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:31: explicit
RenderCdmFactory(RendererCdmManager* manager);
On 2014/10/17 20:09:22, xhwang wrote:
> On 2014/10/17 16:32:04, ddorwin wrote:
> > What is the ownership story for the manager? How do we ensure it is still
> valid
> > when Create() is called?
> 
> It's a per-render-frame object owned by RenderFrameImpl.

Maybe we should document this assumption here.

[xhwang, 2014-10-20 16:36:23]

comments addressed

[xhwang, 2014-10-20 16:36:29]

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
File content/renderer/media/crypto/render_cdm_factory.h (right):

https://codereview.chromium.org/660673002/diff/120001/content/renderer/media/...
content/renderer/media/crypto/render_cdm_factory.h:31: explicit
RenderCdmFactory(RendererCdmManager* manager);
On 2014/10/19 06:07:25, ddorwin wrote:
> On 2014/10/17 20:09:22, xhwang wrote:
> > On 2014/10/17 16:32:04, ddorwin wrote:
> > > What is the ownership story for the manager? How do we ensure it is still
> > valid
> > > when Create() is called?
> > 
> > It's a per-render-frame object owned by RenderFrameImpl.
> 
> Maybe we should document this assumption here.

Done.

[xhwang, 2014-10-20 16:38:01]

jamesr: Please OWNERS review changes in content/renderer/render_frame_impl.cc.
The change in that file is pretty simple and should take < 3 min to review.

[jamesr, 2014-10-20 17:09:57]

That's a lot of ifdefs.

content/renderer/render_frame_impl.cc lgtm

[xhwang, 2014-10-20 17:11:37]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2014-10-20 17:12:12]

On 2014/10/20 17:09:57, jamesr wrote:
> That's a lot of ifdefs.

Agreed :( I'll think about how we can unify these paths in the future.
 
> content/renderer/render_frame_impl.cc lgtm

[commit-bot: I haz the power, 2014-10-20 17:12:45]

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

[commit-bot: I haz the power, 2014-10-20 17:50:18]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2014-10-20 17:51:17]

Patchset 8 (id:??) landed as
https://crrev.com/00f7cdc52b6fcab8deda7c5e266c2db8ba60a28a
Cr-Commit-Position: refs/heads/master@{#300296}