media: Support CdmFactory in MojoMediaClient.

media: Support CdmFactory in MojoMediaClient.

The embedder can now use a customized CdmFactory.

BUG=495273
Committed: https://crrev.com/a6fd7d0cf75595cfbc3478d7bd8e6c5a1b17b4fe
Cr-Commit-Position: refs/heads/master@{#338169}

[xhwang, 2015-07-08 22:05:46]

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

[xhwang, 2015-07-08 22:05:47]

PTAL

[ddorwin, 2015-07-09 18:13:04]

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/default...
File media/mojo/services/default_mojo_media_client.cc (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/default...
media/mojo/services/default_mojo_media_client.cc:77: return make_scoped_ptr(new
DefaultCdmFactory());
It seems odd that this creates a new object each time it is called and returns a
scoped_ptr. s/Get/Create/?

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_service.cc:54: base::WeakPtr<MojoCdmService>
weak_this = weak_factory_.GetWeakPtr();
Totally optional, but I wonder if "auto" makes sense for weak pointers.
You already have the code, so this is fine. Something to consider in new code.

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_service.cc:56: key_system, GURL(security_origin),
CdmConfig(),
TODO: Something about the config?

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
File media/mojo/services/mojo_cdm_service.h (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_service.h:72: const std::string& error_message);
Why aren't these parameters part of the promise? It seems they apply to resolve
and reject, respectively.

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_client.h (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_client.h:55: // Returns the CdmFactory to be used
by MojoCdmService.
Ditto on creating a new instance.

[xhwang, 2015-07-09 21:16:06]

comments addressed

[xhwang, 2015-07-09 21:16:32]

PTAL again

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/default...
File media/mojo/services/default_mojo_media_client.cc (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/default...
media/mojo/services/default_mojo_media_client.cc:77: return make_scoped_ptr(new
DefaultCdmFactory());
On 2015/07/09 18:13:03, ddorwin wrote:
> It seems odd that this creates a new object each time it is called and returns
a
> scoped_ptr. s/Get/Create/?

This makes sense. But then most other methods need to be renamed as well. Can I
do that in a separate CL?

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
File media/mojo/services/mojo_cdm_service.cc (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_service.cc:54: base::WeakPtr<MojoCdmService>
weak_this = weak_factory_.GetWeakPtr();
On 2015/07/09 18:13:03, ddorwin wrote:
> Totally optional, but I wonder if "auto" makes sense for weak pointers.
> You already have the code, so this is fine. Something to consider in new code.

Done.

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_service.cc:56: key_system, GURL(security_origin),
CdmConfig(),
On 2015/07/09 18:13:03, ddorwin wrote:
> TODO: Something about the config?

This will be fixed this week: https://codereview.chromium.org/1227883002/

So I feel a TODO isn't necessary.

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
File media/mojo/services/mojo_cdm_service.h (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_cd...
media/mojo/services/mojo_cdm_service.h:72: const std::string& error_message);
On 2015/07/09 18:13:04, ddorwin wrote:
> Why aren't these parameters part of the promise? It seems they apply to
resolve
> and reject, respectively.

OnCdmCreated() needs to be bound into CdmCreatedCB. That's why we need the last
two parameters.

Actually as you can see, we bind
"mojo::Callback<void(mojo::CdmPromiseResultPtr)>&" into a SimpleMojoCdmPromise,
then bind SimpleMojoCdmPromise into a CdmCreatedCB. We could skip the
SimpleMojoCdmPromise step, but then we'll need to populate a CdmPromiseResultPtr
manually, similar to [1]. So this extra binding is really just for convenience.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_client.h (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_client.h:55: // Returns the CdmFactory to be used
by MojoCdmService.
On 2015/07/09 18:13:04, ddorwin wrote:
> Ditto on creating a new instance.

Acknowledged.

[ddorwin, 2015-07-09 21:19:20]

lgtm

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/default...
File media/mojo/services/default_mojo_media_client.cc (right):

https://codereview.chromium.org/1231623003/diff/1/media/mojo/services/default...
media/mojo/services/default_mojo_media_client.cc:77: return make_scoped_ptr(new
DefaultCdmFactory());
On 2015/07/09 21:16:31, xhwang wrote:
> On 2015/07/09 18:13:03, ddorwin wrote:
> > It seems odd that this creates a new object each time it is called and
returns
> a
> > scoped_ptr. s/Get/Create/?
> 
> This makes sense. But then most other methods need to be renamed as well. Can
I
> do that in a separate CL?

Yes.

[xhwang, 2015-07-09 21:31:59]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-07-09 21:32:52]

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

[commit-bot: I haz the power, 2015-07-09 21:37:45]

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

[commit-bot: I haz the power, 2015-07-09 21:37:46]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

(exceeded global retry quota)

[xhwang, 2015-07-09 21:44:21]

rebase only

[xhwang, 2015-07-09 21:50:55]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-07-09 21:50:56]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1231623003/#ps40001
(title: "rebase only")

[commit-bot: I haz the power, 2015-07-09 21:52:03]

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

[commit-bot: I haz the power, 2015-07-09 22:29:06]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-09 22:32:23]

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