Move SessionIdAdapter out of WebContentDecryptionModuleImpl

Move SessionIdAdapter out of WebContentDecryptionModuleImpl

In order to manage the lifetime of the CDM, have WebContentDecryptionModuleImpl
and WebContentDecryptionModuleSessionImpl both keep RefPtrs to SessionIdAdapter
(now renamed CdmSessionAdapter). CdmSessionAdapter owns the MediaKeys object,
so the CDM gets destroyed when there is no more need for it.

BUG=341567
TEST=EME content tests pass

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

[jrummell, 2014-02-18 18:51:19]

PTAL.

[ddorwin, 2014-02-18 20:59:42]

The changes look fine (I skipped the new class for now since a rename will mess
with history), but we need to find a better name for the adapter.

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

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:13: #include
"content/renderer/media/cdm_session_id_adapter.h"
We need a new name for this object. It's no longer just adapting - it owns the
CDM, and part of that is directing events at the correct Blink objects.

I can't come up with a good name, though, which is always a bad sign. Part of
the problem is that this is another layer around MediaKeys.h (should be CDM.h).
However, we don't want to force this type of logic into each implementation of
that interface.

Some ideas:
CdmProxy - ambiguous; is it really a proxy?
CdmHost - not really hosting
CdmWrapper - already used

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:47: uint32 session_id
= adapter_->GenerateSessionId();
The adapter could generate the ID itself when Add[New]Session() is called. Maybe
that function returns the session_id.

Another option to look at: Maybe the adapter should create the
WebCDMSessionImpl, providing it with a session_id in the constructor.

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:44:
adapter_->CreateSession(
It really is the CDM interface! All the more reason to give it a more meaningful
name.

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

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:51: // Owned
pointers.
Not owned - ref counted. We don't usually call this out. I think we can remove
this comment. See below.

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:54: // Non-owned
pointers.
Remove the 's'

[jrummell, 2014-02-19 00:20:29]

Updated, new class renamed CdmSessionAdapter.

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

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:13: #include
"content/renderer/media/cdm_session_id_adapter.h"
On 2014/02/18 20:59:42, ddorwin wrote:
> We need a new name for this object. It's no longer just adapting - it owns the
> CDM, and part of that is directing events at the correct Blink objects.
> 
> I can't come up with a good name, though, which is always a bad sign. Part of
> the problem is that this is another layer around MediaKeys.h (should be
CDM.h).
> However, we don't want to force this type of logic into each implementation of
> that interface.
> 
> Some ideas:
> CdmProxy - ambiguous; is it really a proxy?
> CdmHost - not really hosting
> CdmWrapper - already used

How about CdmSessionAdapter?

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodule_impl.cc:47: uint32 session_id
= adapter_->GenerateSessionId();
On 2014/02/18 20:59:42, ddorwin wrote:
> The adapter could generate the ID itself when Add[New]Session() is called.
Maybe
> that function returns the session_id.
> 
> Another option to look at: Maybe the adapter should create the
> WebCDMSessionImpl, providing it with a session_id in the constructor.

Done.

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

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:51: // Owned
pointers.
On 2014/02/18 20:59:42, ddorwin wrote:
> Not owned - ref counted. We don't usually call this out. I think we can remove
> this comment. See below.

Done.

https://codereview.chromium.org/171073002/diff/1/content/renderer/media/webco...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:54: // Non-owned
pointers.
On 2014/02/18 20:59:42, ddorwin wrote:
> Remove the 's'

Done.

[ddorwin, 2014-02-19 01:41:52]

LG overall.

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

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:49: if (!media_keys_)
collapse these 4 lines.

OR log the failure if we don't have good logging elsewhere.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:99: if (session)
I wonder if we should:
DLOG_IF(session) << __FUNCTION__ << " for unknown session " << session_id);

Same below.

This could also be done in GetSession(), though we'd lose the __FUNCTION__ part.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:14: #include
"third_party/WebKit/public/platform/WebContentDecryptionModule.h"
...Session.h?

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:19: // Forwards the session
ID-based callbacks of the MediaKeys interface to the
It also makes calls from the session objects to the CDM.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:20: // appropriate session object.
Both WebContentDecryptionModuleImpl and
I would say that this object owns the CDM instance and that higher levels should
hold references to it as long as they need the CDM instance. (It's generally
best to avoid talking about other class implementations explicitly, though maybe
it's okay in this case.) I'd specifically call out MK and MKS, but those aren't
the actual classes, and CDM and CDM Session doesn't make sense since the former
is confusing.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:26: // On success returns true.
nit: Returns true on success.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:30: // created session.
RemoveSession() must be called when destroying it.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:31:
WebContentDecryptionModuleSessionImpl* NewSession(
Create...
"New" is somewhat redundant, so we could just go with CreateSession(), which is
consistent with other functions.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:38: void CreateSession(uint32
session_id,
I guess we're going to need to rename this to initializeNewSession() like the
higher levels.
This also allows us to make this comment reference |session_id| in a way that
makes sense (similar to the functions below).

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:43: // Updates a session specified
by |session_id| with |response|.
s/a/the/

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:52: // Decryptor associated with
the MediaKeys object.
nit: _is_ associated

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:55: media::Decryptor*
GetDecryptor();
Who is calling this? We shouldn't need this in the Blink layers, right?

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:77: // Keep ownership of MediaKeys.
Redundant.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:90:
adapter_->RemoveSession(session_id_);
Is there any advantage in de-registering the session before we're destroyed? If
not, it seems better to simplify the logic and remove a member. WDYT?

[jrummell, 2014-02-19 20:42:10]

Updated.

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

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:49: if (!media_keys_)
On 2014/02/19 01:41:53, ddorwin wrote:
> collapse these 4 lines.
> 
> OR log the failure if we don't have good logging elsewhere.

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.cc:99: if (session)
On 2014/02/19 01:41:53, ddorwin wrote:
> I wonder if we should:
> DLOG_IF(session) << __FUNCTION__ << " for unknown session " << session_id);
> 
> Same below.
> 
> This could also be done in GetSession(), though we'd lose the __FUNCTION__
part.

Good idea. Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:14: #include
"third_party/WebKit/public/platform/WebContentDecryptionModule.h"
On 2014/02/19 01:41:53, ddorwin wrote:
> ...Session.h?

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:19: // Forwards the session
ID-based callbacks of the MediaKeys interface to the
On 2014/02/19 01:41:53, ddorwin wrote:
> It also makes calls from the session objects to the CDM.

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:20: // appropriate session object.
Both WebContentDecryptionModuleImpl and
On 2014/02/19 01:41:53, ddorwin wrote:
> I would say that this object owns the CDM instance and that higher levels
should
> hold references to it as long as they need the CDM instance. (It's generally
> best to avoid talking about other class implementations explicitly, though
maybe
> it's okay in this case.) I'd specifically call out MK and MKS, but those
aren't
> the actual classes, and CDM and CDM Session doesn't make sense since the
former
> is confusing.

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:26: // On success returns true.
On 2014/02/19 01:41:53, ddorwin wrote:
> nit: Returns true on success.

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:30: // created session.
On 2014/02/19 01:41:53, ddorwin wrote:
> RemoveSession() must be called when destroying it.

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:31:
WebContentDecryptionModuleSessionImpl* NewSession(
On 2014/02/19 01:41:53, ddorwin wrote:
> Create...
> "New" is somewhat redundant, so we could just go with CreateSession(), which
is
> consistent with other functions.

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:38: void CreateSession(uint32
session_id,
On 2014/02/19 01:41:53, ddorwin wrote:
> I guess we're going to need to rename this to initializeNewSession() like the
> higher levels.
> This also allows us to make this comment reference |session_id| in a way that
> makes sense (similar to the functions below).

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:43: // Updates a session specified
by |session_id| with |response|.
On 2014/02/19 01:41:53, ddorwin wrote:
> s/a/the/

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:52: // Decryptor associated with
the MediaKeys object.
On 2014/02/19 01:41:53, ddorwin wrote:
> nit: _is_ associated

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:55: media::Decryptor*
GetDecryptor();
On 2014/02/19 01:41:53, ddorwin wrote:
> Who is calling this? We shouldn't need this in the Blink layers, right?

It is WebContentDecryptionModuleImpl, which no longer owns the CDM. Ultimately
WebMediaPlayerImpl::setContentDecryptionModule() and
WebMediaPlayerImpl::SetDecryptorReadyCB() call it.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/c...
content/renderer/media/cdm_session_adapter.h:77: // Keep ownership of MediaKeys.
On 2014/02/19 01:41:53, ddorwin wrote:
> Redundant.

Done.

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodulesession_impl.cc (right):

https://codereview.chromium.org/171073002/diff/60001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodulesession_impl.cc:90:
adapter_->RemoveSession(session_id_);
On 2014/02/19 01:41:53, ddorwin wrote:
> Is there any advantage in de-registering the session before we're destroyed?
If
> not, it seems better to simplify the logic and remove a member. WDYT?

Not really. I just added it here in case the Blink side doesn't bother to go
away until later. Removed.

[ddorwin, 2014-02-19 21:09:14]

LGTM when comments are fixed.

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.cc:98: DLOG_IF(WARNING, session) <<
__FUNCTION__ << " for unknown session "
Oops. These should be !session.

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.h:14: #include
"third_party/WebKit/public/platform/WebContentDecryptionModule.h"
This one isn't used, right?

[jrummell, 2014-02-19 21:23:14]

Updated.

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.cc (right):

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.cc:98: DLOG_IF(WARNING, session) <<
__FUNCTION__ << " for unknown session "
On 2014/02/19 21:09:15, ddorwin wrote:
> Oops. These should be !session.

Done.

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/171073002/diff/180001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.h:14: #include
"third_party/WebKit/public/platform/WebContentDecryptionModule.h"
On 2014/02/19 21:09:15, ddorwin wrote:
> This one isn't used, right?

Done.

[xhwang, 2014-02-19 21:58:04]

lgtm % nits. You can address them later.

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.h:16: namespace content {
nit: an empty line here

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:31:
scoped_refptr<CdmSessionAdapter> adapter);
nit: in media code, we pass refptrs by const reference:
const scoped_refptr<Foo>& foo

[jrummell, 2014-02-19 22:51:26]

Updated for nits.

+jamesr for OWNERS review of content_renderer.gypi

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
File content/renderer/media/cdm_session_adapter.h (right):

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
content/renderer/media/cdm_session_adapter.h:16: namespace content {
On 2014/02/19 21:58:04, xhwang wrote:
> nit: an empty line here

Done.

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
File content/renderer/media/webcontentdecryptionmodulesession_impl.h (right):

https://codereview.chromium.org/171073002/diff/270001/content/renderer/media/...
content/renderer/media/webcontentdecryptionmodulesession_impl.h:31:
scoped_refptr<CdmSessionAdapter> adapter);
On 2014/02/19 21:58:04, xhwang wrote:
> nit: in media code, we pass refptrs by const reference:
> const scoped_refptr<Foo>& foo

Done.

[jamesr, 2014-02-19 22:56:51]

content_renderer.gypi lgtm

(maybe we should have a * rule for the gypis for file additions/removals like
chrome/OWNERS has)?

[jrummell, 2014-02-19 22:58:31]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-02-19 23:19:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/171073002/250002

[commit-bot: I haz the power, 2014-02-20 01:12:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/171073002/250002

[commit-bot: I haz the power, 2014-02-20 04:58:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/171073002/250002

[commit-bot: I haz the power, 2014-02-20 08:56:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/171073002/250002

[commit-bot: I haz the power, 2014-02-20 12:24:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/171073002/250002

[commit-bot: I haz the power, 2014-02-20 13:47:56]

Change committed as 252226