media: Make media mojo services a component

media/mojo/services/BUILD.gn

Index: media/mojo/services/BUILD.gn
diff --git a/media/mojo/services/BUILD.gn b/media/mojo/services/BUILD.gn
index ae82fadcd31af4d18a278b995e90f00c8a265a4d..e51c3bcfcb56f212b57d55f37b1f264d036b9de1 100644
--- a/media/mojo/services/BUILD.gn
+++ b/media/mojo/services/BUILD.gn
@@ -84,19 +84,14 @@ source_set("proxy") {
   ]
 }
 
-source_set("cdm_service") {
-  deps = [
-    "//base",
-    "//media",
-    "//media/mojo/common",
-    "//media/mojo/interfaces",
-    "//mojo/common",
-    "//mojo/public/c/system:for_component",
-    "//services/shell/public/interfaces",
-    "//url",
-  ]
+component("services") {
+  output_name = "media_mojo_services"
 
   sources = [
+    "demuxer_stream_provider_shim.cc",

[ddorwin, 2016/05/11 23:02:27]

We now build everything regardless of whether a given platform/build
configuration needs/uses it?

[xhwang, 2016/05/16 05:41:20]

Yes. Previously I was trying to keep things separate so that we only build
things that we need. But it becomes a pain to maintain. Now I need to use
component (shared/static library). Having multiple components would be even
harder: we need to have different MEDIA_MOJO_XXX_EXPORT for each of them.

By building everything, we rely on the linker to remove symbols that are not
reachable. So it won't affect production build. It does increase build time a
little bit when mojo media is enabled, but makes the build file much easier to
manage.

Another option is to include things based on the config above, e.g. only include
audio decoder service when ENABLE_MOJO_AUDIO_DECODER is true. That's doable. But
again I am not sure whether it's worth it.

WDYT?

+    "demuxer_stream_provider_shim.h",
+    "mojo_audio_decoder_service.cc",
+    "mojo_audio_decoder_service.h",
     "mojo_cdm_allocator.cc",
     "mojo_cdm_allocator.h",
     "mojo_cdm_promise.cc",
@@ -107,103 +102,48 @@ source_set("cdm_service") {
     "mojo_cdm_service_context.h",
     "mojo_decryptor_service.cc",
     "mojo_decryptor_service.h",
-    "mojo_type_trait.h",
-  ]
-
-  if (is_android) {
-    sources += [
-      "mojo_provision_fetcher.cc",
-      "mojo_provision_fetcher.h",
-    ]
-  }
-
-  # TODO(xhwang): Needed because targets can depend on cdm_service directly,
-  # which is a bit hacky since we need to access CdmService directly
-  # from C++ code (AVDA). In the future we'll make those decoders part of
-  # MojoMediaApplication, then we won't need this.
-  public_configs = [ ":mojo_media_config" ]
-
-  configs += [ "//build/config/compiler:no_size_t_to_int_warning" ]
-}
-
-source_set("audio_decoder_service") {
-  sources = [
-    "mojo_audio_decoder_service.cc",
-    "mojo_audio_decoder_service.h",
-  ]
-
-  deps = [
-    ":cdm_service",
-    "//base",
-    "//media",
-    "//media:shared_memory_support",
-    "//media/mojo/common",
-    "//media/mojo/interfaces",
-    "//mojo/common",
-  ]
-}
-
-source_set("renderer_service") {
-  sources = [
-    "demuxer_stream_provider_shim.cc",
-    "demuxer_stream_provider_shim.h",
     "mojo_demuxer_stream_adapter.cc",
     "mojo_demuxer_stream_adapter.h",
-    "mojo_renderer_service.cc",
-    "mojo_renderer_service.h",
-  ]
-
-  deps = [
-    ":cdm_service",
-    "//base",
-    "//media",
-    "//media:shared_memory_support",
-    "//media/mojo/common",
-    "//media/mojo/interfaces",
-    "//mojo/common",
-  ]
-}
-
-source_set("application") {
-  sources = [
     "mojo_media_application.cc",
     "mojo_media_application.h",
+    "mojo_media_application_factory.cc",
+    "mojo_media_application_factory.h",
     "mojo_media_client.cc",
     "mojo_media_client.h",
+    "mojo_renderer_service.cc",
+    "mojo_renderer_service.h",
+    "mojo_type_trait.h",
     "service_factory_impl.cc",
     "service_factory_impl.h",
   ]
 
+  defines = [ "MEDIA_MOJO_IMPLEMENTATION" ]

[ddorwin, 2016/05/11 23:02:27]

This is new?

[xhwang, 2016/05/16 05:41:20]

Yes, this is needed since we are "component" now.

+  configs += [ "//build/config/compiler:no_size_t_to_int_warning" ]

[ddorwin, 2016/05/11 23:02:28]

Should we comment the reason for this?

[xhwang, 2016/05/16 05:41:20]

Done

   public_configs = [ ":mojo_media_config" ]

[ddorwin, 2016/05/11 23:02:27]

This was associated with a TODO about cdm_service. Is this no longer
incorrect/needing correction?

[xhwang, 2016/05/16 05:41:20]

cdm_service doesn't need this but has to have it for the reason documented in
the TODO. Now we have the app in this target and we should always have this
public_configs.

 
   deps = [
-    ":audio_decoder_service",
-    ":cdm_service",
-    ":renderer_service",
     "//base",
     "//media",
+    "//media:shared_memory_support",
+    "//media/mojo/common",
     "//media/mojo/interfaces",
+    "//mojo/common",
+    "//mojo/public/c/system:for_component",
     "//services/shell/public/cpp",
-  ]
-}
-
-source_set("application_factory") {
-  sources = [
-    "mojo_media_application_factory.cc",
-    "mojo_media_application_factory.h",
-  ]
-
-  public_configs = [ ":mojo_media_config" ]
-
-  deps = [
-    ":application",
-    "//base",
-    "//media",
     "//services/shell/public/cpp:sources",
+    "//services/shell/public/interfaces",
+    "//url",
   ]
 
+  if (is_android) {
+    sources += [
+      "mojo_provision_fetcher.cc",
+      "mojo_provision_fetcher.h",
+    ]
+  }
+
   if (enable_test_mojo_media_client) {

[ddorwin, 2016/05/11 23:02:28]

Does this mean we build all the files above multiple times - once for the
product and once for the test?

[xhwang, 2016/05/16 05:41:20]

This is used for test in chromium by developers, not in unittests. The files
above will only be built once.

-    defines = [ "ENABLE_TEST_MOJO_MEDIA_CLIENT" ]
+    defines += [ "ENABLE_TEST_MOJO_MEDIA_CLIENT" ]
     sources += [
       "test_mojo_media_client.cc",
       "test_mojo_media_client.h",
@@ -231,7 +171,7 @@ mojo_native_application("media") {
   ]
 
   deps = [
-    ":application",
+    ":services",
     "//base",
     "//media",
     "//mojo/logging",