BrowserMediaPlayerManager manages MediaKeys objects.

BrowserMediaPlayerManager manages MediaKeys objects.

Changes in this CL:
- Add cdm_factory.h to support platform specific CDM creation.
- Detach BrowserMediaPlayerManager from MediaDrmBridge.
- Store the security origin of CDMs in the manager so that CDMs don't need to
  know it.
- Keep the CDM ID in the manager so that CDMs don't see it.

BUG=338910
TEST=Test page plays and tests still pass.

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

[xhwang, 2014-05-09 23:40:37]

ddorwin/damienv: PTAL

qinmin: FYI

https://codereview.chromium.org/276973005/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:400: }
This block is moved from media_drm_bridge.cc

https://codereview.chromium.org/276973005/diff/1/media/base/android/media_pla...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/276973005/diff/1/media/base/android/media_pla...
media/base/android/media_player_manager.h:94: const std::string&
destination_url) = 0;
This is changed because media::SessionMessageCB uses std::string.

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/media_k...

[damienv1, 2014-05-12 21:37:53]

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:664: init_data));
All this part seems to be an internal detail of the implementation of
MediaKeys::CreateSession.
And this would remove the need for a security_origin_map_ (the security_origin
should be an internal detail of the MediaKeys implementation).

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:688: if (iter ==
cdm_to_player_map_.end())
I don't think that's the right way: we shouldn't need this cdm_to_player_map_:
1) The BrowserMediaPlayerManager should not have to remember the CDM to player
association.
2) When we call SetCdm on the player, the player should register itself to the
MediaKey object to listen to some CDM events.
Then the CDM will invoke this callback.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
File media/base/android/cdm_factory_android.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory_android.cc:37:
->HasSwitch(switches::kMediaDrmEnableNonCompositing)) {
nit: looks weird to break the line on "->". Would rather break it after
HasSwitch:
CommandLine::ForCurrentProcess()->HasSwitch(
 ...)

[xhwang, 2014-05-12 21:58:43]

comments only. I'll wait for ddorwin's comment to make code/comment changes.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:664: init_data));
On 2014/05/12 21:37:53, damienv1 wrote:
> All this part seems to be an internal detail of the implementation of
> MediaKeys::CreateSession.
> And this would remove the need for a security_origin_map_ (the security_origin
> should be an internal detail of the MediaKeys implementation).

Well, I agree that the security origin should and the user permission prompt
should be an implementation detail of the MediaKeys. But to do that, we need to
pass the security origin in CreateCdm(). Also, we need some other plumbing so
that MediaDrmBridge can request protected media identifier permission. Maybe
pass a RequestPermissionCB() in CreateCdm(). I don't know what's the best way to
do that yet. How about a TODO?

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:688: if (iter ==
cdm_to_player_map_.end())
On 2014/05/12 21:37:53, damienv1 wrote:
> I don't think that's the right way: we shouldn't need this cdm_to_player_map_:
> 1) The BrowserMediaPlayerManager should not have to remember the CDM to player
> association.
> 2) When we call SetCdm on the player, the player should register itself to the
> MediaKey object to listen to some CDM events.
> Then the CDM will invoke this callback.

Agreed. Since this is from the old code and we'll have a new event to notify
"new key added". I'd like to do this in a later CL.

[damienv1, 2014-05-12 23:14:26]

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:403: routing_id(),
cdm_id, session_id, message, destination_gurl));
destination_gurl will be updated to a string once the renderer side is updated,
right ?

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

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory.h:27: #endif  //
MEDIA_BASE_ANDROID_CDM_FACTORY_H_
This file could already be part of media/base/ (at the same level as MediaKeys).
Nothing is android specific here.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:335:
DCHECK(!session_created_cb_.is_null());
We should have all these DCHECK in the constructor (same for
session_creeated_cb_, and other callbacks).
These are not checked later.

[ddorwin, 2014-05-13 00:59:37]

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:403: routing_id(),
cdm_id, session_id, message, destination_gurl));
On 2014/05/12 23:14:26, damienv1 wrote:
> destination_gurl will be updated to a string once the renderer side is
updated,
> right ?

Why would we move away from GURL? Especially for IPCs, we should use GURLs.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:643: DLOG(WARNING)
<< "No CDMs for ID: " << cdm_id << " found";
nit: s/CDMs/CDM/
nit: remove colon or move "found" before "for"

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:651:
DCHECK(ContainsKey(security_origin_map_, cdm_id));
If this fails, we crash at 658. Since this is the browser and it could be
attacked by the renderer, we should fail gracefully.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:664: init_data));
On 2014/05/12 21:58:44, xhwang wrote:
> On 2014/05/12 21:37:53, damienv1 wrote:
> > All this part seems to be an internal detail of the implementation of
> > MediaKeys::CreateSession.
> > And this would remove the need for a security_origin_map_ (the
security_origin
> > should be an internal detail of the MediaKeys implementation).
> 
> Well, I agree that the security origin should and the user permission prompt
> should be an implementation detail of the MediaKeys. But to do that, we need
to
> pass the security origin in CreateCdm(). Also, we need some other plumbing so
> that MediaDrmBridge can request protected media identifier permission. Maybe
> pass a RequestPermissionCB() in CreateCdm(). I don't know what's the best way
to
> do that yet. How about a TODO?

When and whether to check permissions is CDM-specific, but it is likely to occur
around createSession(), so it might make sense to include this in the general
framework. The individual implementation would then decide what to do.

It appears this is what would happen at 653. The BrowserContext implementation
will decide what to do and call the callback. We _could_ make it more explicit
about this request being related to session creation, but I don't think we have
use cases for that currently.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:673: DLOG(WARNING)
<< "No CDM for ID: " << cdm_id << " found";
ditto

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:701: DLOG(WARNING)
<< "No MediaKeys for ID: " << cdm_id << " found";
ditto

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:102: const
std::string& destination_url) OVERRIDE;
Why the change?
Should we pass GURLs around as much as possible?

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:167: // Adds a new
CDM for the given |key_system|, |cdm_id|, and |security_origin|.
Would it be more accurate to say this?
// Adds a new CDM [identified by] |cdm_id| for the given |key_system| and
|security_origin|.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:227: // Map from
CDM ID to CDM's security origin.
Why doesn't the CDM object maintain this? Should we keep a pointer to a subclass
of MediaKeys instead?

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:228: std::map<int,
GURL> security_origin_map_;
cdm_...

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

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory.h:18: CreateCdm(const std::string& key_system,
This name could conflict with other code in media. If this is really for
Android, maybe we should rename it CreateAndroidCdm. Otherwise, should we be
using the factory function for desktop too?

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
File media/base/android/cdm_factory_android.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory_android.cc:26: scoped_ptr<MediaDrmBridge>
cdm(MediaDrmBridge::Create(key_system,
Are we sure this will never fail? The returned value is not checked. To be safe,
we should probably check it since we may forget if that code is ever changed.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:335:
DCHECK(!session_created_cb_.is_null());
On 2014/05/12 23:14:26, damienv1 wrote:
> We should have all these DCHECK in the constructor (same for
> session_creeated_cb_, and other callbacks).
> These are not checked later.

I assume this is to make sure that this is not called after the second Create()
method. I was going to suggest this, but we should definitely have a comment in
the << stream.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:426: session_message_cb_.Run(session_id,
message, destination_url);
Ahh, did you make that GURL->string change because we're using MediaKeys CB
types? Should we (in another CL) change those CBs?

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:73: static scoped_ptr<MediaDrmBridge>
Create(const std::string& key_system);
For clarity (and because overloading is not allowed), call this
CreateSessionless or something like that.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:130: 
Note: My earlier comments wondered whether we should have the object manage the
removed values. Maybe we should have MediaKeysBrowserBase or something. The CBs
could be stored there too.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_player_android.h:76: // Pass a CDM to a player.
s/Pass/Provide/ (or attach or associate with).

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_player_manager.h:85: // Called when CDM determines a
SessionId.
creates a session?

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_player_manager.h:94: const std::string&
destination_url) = 0;
I think we should prefer GURL.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:387: // Only MediaDrmBridge will be
set on MediaSourcePlayer.
This seems strange. Why did we go to all that trouble to use MediaKeys then cast
back?

Should we have kept GetMediaBridge() in BrowserMediaPlayerManager?

[damienv1, 2014-05-13 17:07:06]

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

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory.h:18: CreateCdm(const std::string& key_system,
On 2014/05/13 00:59:38, ddorwin wrote:
> This name could conflict with other code in media. If this is really for
> Android, maybe we should rename it CreateAndroidCdm. Otherwise, should we be
> using the factory function for desktop too?

This is a static function that is invoked to create a platform specific CDM so
CreateAndroidCdm is not suitable (the same static function will be used to
create a Chromecast CDM).

[ddorwin, 2014-05-13 17:17:37]

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

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory.h:18: CreateCdm(const std::string& key_system,
On 2014/05/13 17:07:07, damienv1 wrote:
> On 2014/05/13 00:59:38, ddorwin wrote:
> > This name could conflict with other code in media. If this is really for
> > Android, maybe we should rename it CreateAndroidCdm. Otherwise, should we be
> > using the factory function for desktop too?
> 
> This is a static function that is invoked to create a platform specific CDM so
> CreateAndroidCdm is not suitable (the same static function will be used to
> create a Chromecast CDM).

If it's only used for platform-based (browser process) CDMs, maybe this should
be CreateBrowserCdm() or something.

[xhwang, 2014-05-14 16:42:06]

I think I addressed most comments. PTAL again.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:403: routing_id(),
cdm_id, session_id, message, destination_gurl));
On 2014/05/12 23:14:26, damienv1 wrote:
> destination_gurl will be updated to a string once the renderer side is
updated,
> right ?

See above. Added TODO to use GURL as much as we can.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:403: routing_id(),
cdm_id, session_id, message, destination_gurl));
On 2014/05/13 00:59:38, ddorwin wrote:
> On 2014/05/12 23:14:26, damienv1 wrote:
> > destination_gurl will be updated to a string once the renderer side is
> updated,
> > right ?
> 
> Why would we move away from GURL? Especially for IPCs, we should use GURLs.

ditto

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:643: DLOG(WARNING)
<< "No CDMs for ID: " << cdm_id << " found";
On 2014/05/13 00:59:38, ddorwin wrote:
> nit: s/CDMs/CDM/
> nit: remove colon or move "found" before "for"

Done.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:651:
DCHECK(ContainsKey(security_origin_map_, cdm_id));
On 2014/05/13 00:59:38, ddorwin wrote:
> If this fails, we crash at 658. Since this is the browser and it could be
> attacked by the renderer, we should fail gracefully.

Done.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:664: init_data));
On 2014/05/13 00:59:38, ddorwin wrote:
> On 2014/05/12 21:58:44, xhwang wrote:
> > On 2014/05/12 21:37:53, damienv1 wrote:
> > > All this part seems to be an internal detail of the implementation of
> > > MediaKeys::CreateSession.
> > > And this would remove the need for a security_origin_map_ (the
> security_origin
> > > should be an internal detail of the MediaKeys implementation).
> > 
> > Well, I agree that the security origin should and the user permission prompt
> > should be an implementation detail of the MediaKeys. But to do that, we need
> to
> > pass the security origin in CreateCdm(). Also, we need some other plumbing
so
> > that MediaDrmBridge can request protected media identifier permission. Maybe
> > pass a RequestPermissionCB() in CreateCdm(). I don't know what's the best
way
> to
> > do that yet. How about a TODO?
> 
> When and whether to check permissions is CDM-specific, but it is likely to
occur
> around createSession(), so it might make sense to include this in the general
> framework. The individual implementation would then decide what to do.
> 
> It appears this is what would happen at 653. The BrowserContext implementation
> will decide what to do and call the callback. We _could_ make it more explicit
> about this request being related to session creation, but I don't think we
have
> use cases for that currently.

I can think of it in both ways. What if on some platform, createsession doesn't
involve a protected-media-identifier? Then even this call is generic and can be
platform specific, the function name doesn't make sense anymore. Maybe we can
rename it as something like "RequestCreateSessionPermission". Or maybe we can
move all of these to MediaDrmBridge.

Anyway, I feel this is low priority and don't want to involve this in the
current refactoring work. WDYT?

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:673: DLOG(WARNING)
<< "No CDM for ID: " << cdm_id << " found";
On 2014/05/13 00:59:38, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:688: if (iter ==
cdm_to_player_map_.end())
On 2014/05/12 21:58:44, xhwang wrote:
> On 2014/05/12 21:37:53, damienv1 wrote:
> > I don't think that's the right way: we shouldn't need this
cdm_to_player_map_:
> > 1) The BrowserMediaPlayerManager should not have to remember the CDM to
player
> > association.
> > 2) When we call SetCdm on the player, the player should register itself to
the
> > MediaKey object to listen to some CDM events.
> > Then the CDM will invoke this callback.
> 
> Agreed. Since this is from the old code and we'll have a new event to notify
> "new key added". I'd like to do this in a later CL.

I filed a bug (http://crbug.com/373327) and added a TODO.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:701: DLOG(WARNING)
<< "No MediaKeys for ID: " << cdm_id << " found";
On 2014/05/13 00:59:38, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.h (right):

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:102: const
std::string& destination_url) OVERRIDE;
On 2014/05/13 00:59:38, ddorwin wrote:
> Why the change?
> Should we pass GURLs around as much as possible?

std::string is used in media_keys.h. Filed a bug and added TODO in media_keys.h.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:167: // Adds a new
CDM for the given |key_system|, |cdm_id|, and |security_origin|.
On 2014/05/13 00:59:38, ddorwin wrote:
> Would it be more accurate to say this?
> // Adds a new CDM [identified by] |cdm_id| for the given |key_system| and
> |security_origin|.

Done.

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:227: // Map from
CDM ID to CDM's security origin.
On 2014/05/13 00:59:38, ddorwin wrote:
> Why doesn't the CDM object maintain this? Should we keep a pointer to a
subclass
> of MediaKeys instead?

Because most CDM objects are not interested in this. Even MediaDrmBridge has no
idea what this is used for, unless we move
RequestProtectedMediaIdentifierPermission() call into MediaDrmBridge.

Bottom line, I think we should store the security origin in the same class where
we call context->RequestProtectedMediaIdentifierPermission().

https://codereview.chromium.org/276973005/diff/40001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.h:228: std::map<int,
GURL> security_origin_map_;
On 2014/05/13 00:59:38, ddorwin wrote:
> cdm_...

Done.

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

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory.h:18: CreateCdm(const std::string& key_system,
On 2014/05/13 17:17:37, ddorwin wrote:
> On 2014/05/13 17:07:07, damienv1 wrote:
> > On 2014/05/13 00:59:38, ddorwin wrote:
> > > This name could conflict with other code in media. If this is really for
> > > Android, maybe we should rename it CreateAndroidCdm. Otherwise, should we
be
> > > using the factory function for desktop too?
> > 
> > This is a static function that is invoked to create a platform specific CDM
so
> > CreateAndroidCdm is not suitable (the same static function will be used to
> > create a Chromecast CDM).
> 
> If it's only used for platform-based (browser process) CDMs, maybe this should
> be CreateBrowserCdm() or something.

Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory.h:27: #endif  //
MEDIA_BASE_ANDROID_CDM_FACTORY_H_
On 2014/05/12 23:14:26, damienv1 wrote:
> This file could already be part of media/base/ (at the same level as
MediaKeys).
> Nothing is android specific here.

Good point. Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
File media/base/android/cdm_factory_android.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory_android.cc:26: scoped_ptr<MediaDrmBridge>
cdm(MediaDrmBridge::Create(key_system,
On 2014/05/13 00:59:38, ddorwin wrote:
> Are we sure this will never fail? The returned value is not checked. To be
safe,
> we should probably check it since we may forget if that code is ever changed.

Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/cdm_f...
media/base/android/cdm_factory_android.cc:37:
->HasSwitch(switches::kMediaDrmEnableNonCompositing)) {
On 2014/05/12 21:37:53, damienv1 wrote:
> nit: looks weird to break the line on "->". Would rather break it after
> HasSwitch:
> CommandLine::ForCurrentProcess()->HasSwitch(
>  ...)

Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:335:
DCHECK(!session_created_cb_.is_null());
On 2014/05/13 00:59:38, ddorwin wrote:
> On 2014/05/12 23:14:26, damienv1 wrote:
> > We should have all these DCHECK in the constructor (same for
> > session_creeated_cb_, and other callbacks).
> > These are not checked later.
> 
> I assume this is to make sure that this is not called after the second
Create()
> method. I was going to suggest this, but we should definitely have a comment
in
> the << stream.

Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.cc:426: session_message_cb_.Run(session_id,
message, destination_url);
On 2014/05/13 00:59:38, ddorwin wrote:
> Ahh, did you make that GURL->string change because we're using MediaKeys CB
> types? Should we (in another CL) change those CBs?

yes :) I filed a bug to fix this.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_drm_bridge.h (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:73: static scoped_ptr<MediaDrmBridge>
Create(const std::string& key_system);
On 2014/05/13 00:59:38, ddorwin wrote:
> For clarity (and because overloading is not allowed), call this
> CreateSessionless or something like that.

Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_drm_bridge.h:130: 
On 2014/05/13 00:59:38, ddorwin wrote:
> Note: My earlier comments wondered whether we should have the object manage
the
> removed values. Maybe we should have MediaKeysBrowserBase or something. The
CBs
> could be stored there too.

See my reply above for discussion.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_player_android.h (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_player_android.h:76: // Pass a CDM to a player.
On 2014/05/13 00:59:38, ddorwin wrote:
> s/Pass/Provide/ (or attach or associate with).

Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_player_manager.h (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_player_manager.h:85: // Called when CDM determines a
SessionId.
On 2014/05/13 00:59:38, ddorwin wrote:
> creates a session?

Done.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_player_manager.h:94: const std::string&
destination_url) = 0;
On 2014/05/13 00:59:38, ddorwin wrote:
> I think we should prefer GURL.

I'll fix all destination_url in a separate CL.

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
File media/base/android/media_source_player.cc (right):

https://codereview.chromium.org/276973005/diff/40001/media/base/android/media...
media/base/android/media_source_player.cc:387: // Only MediaDrmBridge will be
set on MediaSourcePlayer.
On 2014/05/13 00:59:38, ddorwin wrote:
> This seems strange. Why did we go to all that trouble to use MediaKeys then
cast
> back?
> 
> Should we have kept GetMediaBridge() in BrowserMediaPlayerManager?

The purpose of this CL is that BrowserMediaPlayerManager can manage any CDMs,
e.g. a ChromeCast CDM. So I'd keep any platform specific class out of the
manager.

[damienv1, 2014-05-15 16:43:57]

Just a few additional remarks but overall looks good to me.

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:651: if
(!ContainsKey(cdm_security_origin_map_, cdm_id)) {
Not a big performance degradation but the map query is done twice.
Why not just:
cdm_security_origin_map_.find(cdm_id)
and then reuse the result here and when calling
RequestProtectedMediaIdentifierPermission ?

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:778:
&BrowserMediaPlayerManager::OnSessionCreated, weak_this, cdm_id),
Just want to confirm the threading model:
- BrowserMediaPlayerManager (in a future CL, BrowserCdmManager) and the
MediaKeys are assumed to run in the same thread, right ?
Which thread BrowserMediaPlayerManager is currently running on ?
- BrowserMediaPlayerManager is assumed to be destroyed on the same thread it is
running on, right ? (Otherwise, there are potentially issues with the weak
pointers).

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:781:
base::Bind(&BrowserMediaPlayerManager::OnSessionReady, weak_this, cdm_id),
nit: just to make it look better, I would go to a new line after base::Bind(
but this is just a personal taste ;)

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:824:
DCHECK(!ContainsKey(cdm_to_player_map_, cdm_id));
Do we have possibly the same security issue here ?

[ddorwin, 2014-05-15 20:06:04]

LGTM % comment.

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:824:
DCHECK(!ContainsKey(cdm_to_player_map_, cdm_id));
On 2014/05/15 16:43:57, damienv1 wrote:
> Do we have possibly the same security issue here ?

I originally thought not, but is it possible that the following overwrite would
eventually lead us to delete a CDM/Player that is in use by another Player/CDM?
Probably better to be safe than to assume it will always do the right thing.

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

https://codereview.chromium.org/276973005/diff/60001/media/base/cdm_factory.h...
media/base/cdm_factory.h:16: // Creates a CDM for |key_system|. Returns NULL if
the CDM cannot be created.
If we end up with an ifdef for browser-/IPC-based CDMs, we should add it here.

[xhwang, 2014-05-15 20:56:55]

comments addressed

[xhwang, 2014-05-15 20:59:29]

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:651: if
(!ContainsKey(cdm_security_origin_map_, cdm_id)) {
On 2014/05/15 16:43:57, damienv1 wrote:
> Not a big performance degradation but the map query is done twice.
> Why not just:
> cdm_security_origin_map_.find(cdm_id)
> and then reuse the result here and when calling
> RequestProtectedMediaIdentifierPermission ?

Done.

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:778:
&BrowserMediaPlayerManager::OnSessionCreated, weak_this, cdm_id),
On 2014/05/15 16:43:57, damienv1 wrote:
> Just want to confirm the threading model:
> - BrowserMediaPlayerManager (in a future CL, BrowserCdmManager) and the
> MediaKeys are assumed to run in the same thread, right ?
> Which thread BrowserMediaPlayerManager is currently running on ?
> - BrowserMediaPlayerManager is assumed to be destroyed on the same thread it
is
> running on, right ? (Otherwise, there are potentially issues with the weak
> pointers).

All browser side stuff lives on the UI thread. This could change but is not in
the scope of this CL. Yes, BrowserMediaPlayerManager is destroyed on the same
thread.

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:781:
base::Bind(&BrowserMediaPlayerManager::OnSessionReady, weak_this, cdm_id),
On 2014/05/15 16:43:57, damienv1 wrote:
> nit: just to make it look better, I would go to a new line after base::Bind(
> but this is just a personal taste ;)

Done.

https://codereview.chromium.org/276973005/diff/60001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:824:
DCHECK(!ContainsKey(cdm_to_player_map_, cdm_id));
On 2014/05/15 20:06:05, ddorwin wrote:
> On 2014/05/15 16:43:57, damienv1 wrote:
> > Do we have possibly the same security issue here ?
> 
> I originally thought not, but is it possible that the following overwrite
would
> eventually lead us to delete a CDM/Player that is in use by another
Player/CDM?
> Probably better to be safe than to assume it will always do the right thing.

Done.

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

https://codereview.chromium.org/276973005/diff/60001/media/base/cdm_factory.h...
media/base/cdm_factory.h:16: // Creates a CDM for |key_system|. Returns NULL if
the CDM cannot be created.
On 2014/05/15 20:06:05, ddorwin wrote:
> If we end up with an ifdef for browser-/IPC-based CDMs, we should add it here.

Added TODO.

[xhwang, 2014-05-15 21:05:50]

damienv: PTAL again.

[damienv1, 2014-05-16 00:27:52]

lgtm

[xhwang, 2014-05-16 06:59:45]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-16 07:00:08]

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

[commit-bot: I haz the power, 2014-05-16 08:37:42]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-16 09:40:15]

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

[commit-bot: I haz the power, 2014-05-16 09:40:15]

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

[xhwang, 2014-05-16 15:44:32]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-05-16 15:44:57]

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

[commit-bot: I haz the power, 2014-05-16 16:55:32]

Change committed as 271034