Extract CdmManager from MojoCdmService.

Extract CdmManager from MojoCdmService.

This allows us to remove the conditional dependency from //media/gpu to
//media/mojo/services, thus allowing future dependencies in the other
direction.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2857953006
Cr-Commit-Position: refs/heads/master@{#469544}
Committed: https://chromium.googlesource.com/chromium/src/+/2c899c792ecd7fd3c2f0e778f256e919e778d73f

[sandersd (OOO until July 31), 2017-05-04 00:30:16]

Description was changed from

==========
Extract CdmManager from MojoCdmService.

This allows us to remove the conditional dependency from //media/gpu to
//media/mojo/services, thus allowing future dependencies in the other
direction.
==========

to

==========
Extract CdmManager from MojoCdmService.

This allows us to remove the conditional dependency from //media/gpu to
//media/mojo/services, thus allowing future dependencies in the other
direction.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[sandersd (OOO until July 31), 2017-05-04 00:30:28]

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

[xhwang, 2017-05-04 01:41:41]

https://codereview.chromium.org/2857953006/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2857953006/diff/20001/media/BUILD.gn#newcode521
media/BUILD.gn:521: static_library("cdm_manager") {
In a component build this will still cause multiple copies of the |manager|.
You'll want a component("cdm_manager") here. You can check this by looking at
the symbols in libmedia.so and libcontent.so, or something like that, or by
always print the address of |manager| when it's accessed.

https://codereview.chromium.org/2857953006/diff/20001/media/cdm/cdm_manager.h
File media/cdm/cdm_manager.h (right):

https://codereview.chromium.org/2857953006/diff/20001/media/cdm/cdm_manager.h...
media/cdm/cdm_manager.h:21: // future. (MojoCdmContext serves the same purpose
for Media Mojo services,
nit: MojoCdmContext -> MojoCdmServiceContext

[sandersd (OOO until July 31), 2017-05-04 19:56:11]

https://codereview.chromium.org/2857953006/diff/20001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2857953006/diff/20001/media/BUILD.gn#newcode521
media/BUILD.gn:521: static_library("cdm_manager") {
On 2017/05/04 01:41:41, xhwang wrote:
> In a component build this will still cause multiple copies of the |manager|.
> You'll want a component("cdm_manager") here. You can check this by looking at
> the symbols in libmedia.so and libcontent.so, or something like that, or by
> always print the address of |manager| when it's accessed.

Done.

https://codereview.chromium.org/2857953006/diff/20001/media/cdm/cdm_manager.h
File media/cdm/cdm_manager.h (right):

https://codereview.chromium.org/2857953006/diff/20001/media/cdm/cdm_manager.h...
media/cdm/cdm_manager.h:21: // future. (MojoCdmContext serves the same purpose
for Media Mojo services,
On 2017/05/04 01:41:41, xhwang wrote:
> nit: MojoCdmContext -> MojoCdmServiceContext

Done.

[xhwang, 2017-05-04 20:46:11]

https://codereview.chromium.org/2857953006/diff/40001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2857953006/diff/40001/media/BUILD.gn#newcode526
media/BUILD.gn:526: configs += [ ":media_implementation" ]
Typically we only use this on the "media" target. Now, since we are depending on
"media", when we include a header file with MEDIA_EXPORT in "media", since we
have "media implementation", the MEDIA_EXPORT will be treated as "import"
instead of "export". I remember this will cause issues on some bots/platforms.

To avoid that, the right thing to do is to have a cdm_manager_export.h. It seems
overkill, but I don't know any better way to work around it.

[sandersd (OOO until July 31), 2017-05-04 21:03:36]

https://codereview.chromium.org/2857953006/diff/40001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2857953006/diff/40001/media/BUILD.gn#newcode526
media/BUILD.gn:526: configs += [ ":media_implementation" ]
On 2017/05/04 20:46:11, xhwang wrote:
> Typically we only use this on the "media" target. Now, since we are depending
on
> "media", when we include a header file with MEDIA_EXPORT in "media", since we
> have "media implementation", the MEDIA_EXPORT will be treated as "import"
> instead of "export". I remember this will cause issues on some bots/platforms.
> 
> To avoid that, the right thing to do is to have a cdm_manager_export.h. It
seems
> overkill, but I don't know any better way to work around it.

I have restricted the visibility and added a comment to explain this.

[xhwang, 2017-05-04 21:06:31]

https://codereview.chromium.org/2857953006/diff/60001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2857953006/diff/60001/media/BUILD.gn#newcode524
media/BUILD.gn:524: # cdm_manager_export.h, with its own independent config.
But we are already depending on ":media" which also uses "media_implementation.
For example, we include media/base/content_decryption_module.h, which uses
MEDIA_EXPORT...

[sandersd (OOO until July 31), 2017-05-04 21:17:47]

https://codereview.chromium.org/2857953006/diff/60001/media/BUILD.gn
File media/BUILD.gn (right):

https://codereview.chromium.org/2857953006/diff/60001/media/BUILD.gn#newcode524
media/BUILD.gn:524: # cdm_manager_export.h, with its own independent config.
On 2017/05/04 21:06:31, xhwang wrote:
> But we are already depending on ":media" which also uses
"media_implementation.
> For example, we include media/base/content_decryption_module.h, which uses
> MEDIA_EXPORT...

Done.

[xhwang, 2017-05-04 21:25:05]

Could you please add a TODO/BUG to make sure this is removed in the future?
Previously this was indicated by the "Legacy" naming, but now the name is gone.

Otherwise LGTM, thanks for working on this!

[sandersd (OOO until July 31), 2017-05-04 21:30:26]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 21:31:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 21:35:00]

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

[commit-bot: I haz the power, 2017-05-04 21:35:01]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[sandersd (OOO until July 31), 2017-05-04 21:39:29]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 21:40:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 00:13:32]

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

[commit-bot: I haz the power, 2017-05-05 00:13:32]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2017-05-05 00:22:39]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2017-05-05 00:22:39]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2857953006/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-05-05 00:23:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 00:28:28]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493943759036360,
"parent_rev": "8d03afaa6214f583f526587f3865b4ccad45cad3", "commit_rev":
"2c899c792ecd7fd3c2f0e778f256e919e778d73f"}

[commit-bot: I haz the power, 2017-05-05 00:29:00]

Description was changed from

==========
Extract CdmManager from MojoCdmService.

This allows us to remove the conditional dependency from //media/gpu to
//media/mojo/services, thus allowing future dependencies in the other
direction.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Extract CdmManager from MojoCdmService.

This allows us to remove the conditional dependency from //media/gpu to
//media/mojo/services, thus allowing future dependencies in the other
direction.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2857953006
Cr-Commit-Position: refs/heads/master@{#469544}
Committed:
https://chromium.googlesource.com/chromium/src/+/2c899c792ecd7fd3c2f0e778f256...
==========

[commit-bot: I haz the power, 2017-05-05 00:29:01]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/2c899c792ecd7fd3c2f0e778f256...