Separate RendererCdmManager from RendererMediaPlayerManager.

Separate RendererCdmManager from RendererMediaPlayerManager.
    
This CL also updates the prefixed EME implementation on Android to use the new
RendererCdmManager.

CLs to follow:
- RendererCdmManager will also be used by the unprefixed EME implementation.
- Separate BrowserCdmManager from BrowserMediaPlayerManager.

TBR=damienv@chromium.org
BUG=338910
TEST=Test page still works.

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

[xhwang, 2014-05-23 22:11:47]

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

This CL only changes the render side. In the next CL I'll split the browser
side.

PTAL.

[xhwang, 2014-05-23 22:13:42]

CC'ing qinmin and gmorgan. FYI.

[ddorwin, 2014-05-23 23:28:37]

lgtm % comments

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/renderer_cdm_manager.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/renderer_cdm_manager.h:81: std::map<int,
ProxyMediaKeys*> media_keys_;
In a future CL, it'd be nice to rename this. When it appears in code, it seems
like it might be a MediaKeys.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/renderer_media_player_manager.cc (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/renderer_media_player_manager.cc:243:
NOTREACHED();
OOC, what changed? Oh, the WMPA code?

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:1339:
player_manager_->SetCdm(player_id_, proxy_decryptor_->GetCdmId());
This handles the Clear Key case, right? Should we add a comment? i.e. "Non-IPC
CDMs may not have an ID." Otherwise, it looks like an error we should handle.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:343: // Manager for
managing CDMs on the Render Thread.
nit: "Manager for managing" is redundant. "Manages CDMs.."?
Same above.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:346: RendererCdmManager*
cdm_manager_;
Should we explain who owns these?

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/content_decryption_module_factory.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/content_decryption_module_factory.h:33:
RendererCdmManager* manager,
Step n: Unify these parameters? :)

https://codereview.chromium.org/295103015/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.h:609: // Manages all media players and CDMs
in this render frame for communicating
nit: These manage...

[damienv1, 2014-05-27 04:53:09]

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:6: #define
CONTENT_RENDERER_MEDIA_ANDROID_PROXY_MEDIA_KEYS_H_
This should be moved to content/renderer/media/crypto.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/renderer_cdm_manager.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/renderer_cdm_manager.h:5: #ifndef
CONTENT_RENDERER_MEDIA_ANDROID_RENDERER_CDM_MANAGER_H_
Shouldn't it be part of content/renderer/media/crypto instead ?
This is not android specific anymore.

[xhwang, 2014-05-27 22:57:40]

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.h:6: #define
CONTENT_RENDERER_MEDIA_ANDROID_PROXY_MEDIA_KEYS_H_
On 2014/05/27 04:53:09, damienv1 wrote:
> This should be moved to content/renderer/media/crypto.

Done.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/renderer_cdm_manager.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/renderer_cdm_manager.h:5: #ifndef
CONTENT_RENDERER_MEDIA_ANDROID_RENDERER_CDM_MANAGER_H_
On 2014/05/27 04:53:09, damienv1 wrote:
> Shouldn't it be part of content/renderer/media/crypto instead ?
> This is not android specific anymore.

Done.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/renderer_cdm_manager.h:81: std::map<int,
ProxyMediaKeys*> media_keys_;
On 2014/05/23 23:28:37, ddorwin wrote:
> In a future CL, it'd be nice to rename this. When it appears in code, it seems
> like it might be a MediaKeys.

Yeah, this is bad naming. Done in this CL.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/renderer_media_player_manager.cc (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/renderer_media_player_manager.cc:243:
NOTREACHED();
On 2014/05/23 23:28:37, ddorwin wrote:
> OOC, what changed? Oh, the WMPA code?

Yeah. Now WMPA checks the validity of Cdm ID. I feel the original code is a bit
confusing.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.cc:1339:
player_manager_->SetCdm(player_id_, proxy_decryptor_->GetCdmId());
On 2014/05/23 23:28:37, ddorwin wrote:
> This handles the Clear Key case, right? Should we add a comment? i.e. "Non-IPC
> CDMs may not have an ID." Otherwise, it looks like an error we should handle.

Done.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:343: // Manager for
managing CDMs on the Render Thread.
On 2014/05/23 23:28:37, ddorwin wrote:
> nit: "Manager for managing" is redundant. "Manages CDMs.."?
> Same above.

Done.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/a...
content/renderer/media/android/webmediaplayer_android.h:346: RendererCdmManager*
cdm_manager_;
On 2014/05/23 23:28:37, ddorwin wrote:
> Should we explain who owns these?

Done.

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/c...
File content/renderer/media/crypto/content_decryption_module_factory.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/media/c...
content/renderer/media/crypto/content_decryption_module_factory.h:33:
RendererCdmManager* manager,
On 2014/05/23 23:28:37, ddorwin wrote:
> Step n: Unify these parameters? :)

n++ :)

https://codereview.chromium.org/295103015/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/295103015/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.h:609: // Manages all media players and CDMs
in this render frame for communicating
On 2014/05/23 23:28:37, ddorwin wrote:
> nit: These manage...

Done.

[xhwang, 2014-05-27 23:00:27]

jam@: Please OWNERS review content/renderer/render_frame_impl.*

damienv@: Please take a look again.

[jam, 2014-05-27 23:45:24]

On 2014/05/27 23:00:27, xhwang wrote:
> jam@: Please OWNERS review content/renderer/render_frame_impl.*
> 
> damienv@: Please take a look again.

lgtm

[xhwang, 2014-05-28 05:11:00]

TBRing damienv@...

[xhwang, 2014-05-28 05:16:19]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-28 05:17:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/295103015/100001

[xhwang, 2014-05-28 05:29:29]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-28 05:30:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/295103015/120001

[commit-bot: I haz the power, 2014-05-28 09:04:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-05-28 10:04:21]

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

[commit-bot: I haz the power, 2014-05-28 10:04:22]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)

[damienv1, 2014-05-28 11:58:54]

Seems like you forgot to move ProxyMediaKeys to content/renderer/media/crypto

[damienv1, 2014-05-28 13:26:02]

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

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:26: // Class for managing
all the WebMediaPlayerAndroid objects in the same
Comment should be updated.

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:54: // Registers a
ProxyMediaKeys object. There must be a WebMediaPlayerAndroid
Comment should be updated.

[damienv1, 2014-05-28 13:44:51]

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

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:126: cdm_id_(next_cdm_id_++),
The CDM id should be initialized and generated by the CDM manager (as part of a
regular member variable).
I don't think you should need a static "next_cdm_id_".

[damienv1, 2014-05-28 14:16:06]

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

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:80: // ProxyMediaKeys for
each available WebMediaPlayerAndroid.
ditto.

[xhwang, 2014-05-28 17:21:08]

comments addressed

[damienv1, 2014-05-28 17:53:03]

lgtm

[damienv1, 2014-05-28 18:56:28]

https://codereview.chromium.org/295103015/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/renderer_cdm_manager.h (right):

https://codereview.chromium.org/295103015/diff/160001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:57: ProxyMediaKeys*
GetMediaKeys(int cdm_id);
Note: this is public but does not seem to be used outside of this class.

[xhwang, 2014-05-28 21:17:09]

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

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/android/proxy_media_keys.cc:126: cdm_id_(next_cdm_id_++),
On 2014/05/28 13:44:52, damienv1 wrote:
> The CDM id should be initialized and generated by the CDM manager (as part of
a
> regular member variable).
> I don't think you should need a static "next_cdm_id_".

Agreed. I'll do it in a separate CL.

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

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:26: // Class for managing
all the WebMediaPlayerAndroid objects in the same
On 2014/05/28 13:26:03, damienv1 wrote:
> Comment should be updated.

Done.

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:54: // Registers a
ProxyMediaKeys object. There must be a WebMediaPlayerAndroid
On 2014/05/28 13:26:03, damienv1 wrote:
> Comment should be updated.

Done.

https://codereview.chromium.org/295103015/diff/120001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:80: // ProxyMediaKeys for
each available WebMediaPlayerAndroid.
On 2014/05/28 14:16:06, damienv1 wrote:
> ditto.

Done.

https://codereview.chromium.org/295103015/diff/160001/content/renderer/media/...
File content/renderer/media/crypto/renderer_cdm_manager.h (right):

https://codereview.chromium.org/295103015/diff/160001/content/renderer/media/...
content/renderer/media/crypto/renderer_cdm_manager.h:57: ProxyMediaKeys*
GetMediaKeys(int cdm_id);
On 2014/05/28 18:56:28, damienv1 wrote:
> Note: this is public but does not seem to be used outside of this class.

Done.

[xhwang, 2014-05-28 21:19:06]

dcheng@: Please OWNERS review a one line change in
content/common/content_message_generator.h. Basically the cdm_messages.h will be
shared by multiple platforms. Previously it's only used by Android. In a later
CL, I will introduce a new ifdef (e.g. USE_BROWSER_CDM) so that I'll move this
to that ifdef block.

[xhwang, 2014-05-28 21:19:24]

PTAL

[dcheng, 2014-05-28 21:20:33]

lgtm

[xhwang, 2014-05-28 21:21:19]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-28 21:22:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/295103015/200001

[xhwang, 2014-05-28 21:48:03]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-28 21:48:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/295103015/220001

[xhwang, 2014-05-28 21:51:05]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-28 21:52:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/295103015/220001

[commit-bot: I haz the power, 2014-05-29 01:57:09]

Change committed as 273435