media: Support MojoMediaApplication in GPU process.

media: Support MojoMediaApplication in GPU process.

BUG=521731
TEST=Can play audio in GPU process with MojoMediaApplication.

Committed: https://crrev.com/fb22df1ea6881b5dfa72d3f9e2f467aaa7e77a05
Cr-Commit-Position: refs/heads/master@{#353893}

[xhwang, 2015-10-02 21:04:49]

Patchset #3 (id:40001) has been deleted

[xhwang, 2015-10-10 00:28:18]

xhwang@chromium.org changed reviewers:
+ piman@chromium.org, rockot@chromium.org

[xhwang, 2015-10-10 00:28:19]

This CL depends on https://chromiumcodereview.appspot.com/1297953004/.

PTAL!

[piman, 2015-10-10 00:33:09]

lgtm

https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
File content/gpu/gpu_process_control_impl.cc (right):

https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
content/gpu/gpu_process_control_impl.cc:23:
(*url_to_loader_map)[GURL("mojo:media")] =
nit: I see "mojo:media" is used in a lot of places, it sounds like it's worth
making it a constant somewhere?

[xhwang, 2015-10-12 18:05:02]

https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
File content/gpu/gpu_process_control_impl.cc (right):

https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
content/gpu/gpu_process_control_impl.cc:23:
(*url_to_loader_map)[GURL("mojo:media")] =
On 2015/10/10 00:33:08, piman (slow to review) wrote:
> nit: I see "mojo:media" is used in a lot of places, it sounds like it's worth
> making it a constant somewhere?

That is what I did in the old code. But it seems it's introducing some
unnecessary dependencies. I don't have a good solution now and it seems all
other mojo apps are using the string directly as well...

rockot: Any good recommendations from mandoline's POV?

[Ken Rockot(use gerrit already), 2015-10-12 18:18:24]

On 2015/10/12 18:05:02, xhwang wrote:
>
https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
> File content/gpu/gpu_process_control_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
> content/gpu/gpu_process_control_impl.cc:23:
> (*url_to_loader_map)[GURL("mojo:media")] =
> On 2015/10/10 00:33:08, piman (slow to review) wrote:
> > nit: I see "mojo:media" is used in a lot of places, it sounds like it's
worth
> > making it a constant somewhere?
> 
> That is what I did in the old code. But it seems it's introducing some
> unnecessary dependencies. I don't have a good solution now and it seems all
> other mojo apps are using the string directly as well...
> 
> rockot: Any good recommendations from mandoline's POV?

What we did with the devices app, and what seems like an OK pattern, is
introduce a public cpp target for things like constants that expose common
information about the app without introducing implementation dependencies. This
is kind of analogous to public interface targets that a component might export.

In your case this could just mean introducing a media/public/cpp (or I guess...
media/mojo/cpp or media/mojo/public/cpp) target which just defines the app URL
constant.

[xhwang, 2015-10-12 20:20:33]

On 2015/10/12 18:18:24, Ken Rockot wrote:
> On 2015/10/12 18:05:02, xhwang wrote:
> >
>
https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
> > File content/gpu/gpu_process_control_impl.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1320453003/diff/60001/content/gpu/gpu_...
> > content/gpu/gpu_process_control_impl.cc:23:
> > (*url_to_loader_map)[GURL("mojo:media")] =
> > On 2015/10/10 00:33:08, piman (slow to review) wrote:
> > > nit: I see "mojo:media" is used in a lot of places, it sounds like it's
> worth
> > > making it a constant somewhere?
> > 
> > That is what I did in the old code. But it seems it's introducing some
> > unnecessary dependencies. I don't have a good solution now and it seems all
> > other mojo apps are using the string directly as well...
> > 
> > rockot: Any good recommendations from mandoline's POV?
> 
> What we did with the devices app, and what seems like an OK pattern, is
> introduce a public cpp target for things like constants that expose common
> information about the app without introducing implementation dependencies.
This
> is kind of analogous to public interface targets that a component might
export.
> 
> In your case this could just mean introducing a media/public/cpp (or I
guess...
> media/mojo/cpp or media/mojo/public/cpp) target which just defines the app URL
> constant.

Thanks for the tip! I checked the device app and do see kDevicesMojoAppUrl [1].
But it seems we are not using it (according to code search)... And the test is
still using the raw string "mojo:devices" [2].

Nevertheless, I think it's a good idea and will do the same. Can I do that in a
separate CL though?

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/device/devices_app...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/device/devices_app...

[Ken Rockot(use gerrit already), 2015-10-13 16:33:21]

Yeah we're actually not using it now because of the decision to kill in-process
apps. LGTM

[xhwang, 2015-10-13 16:36:53]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-10-13 16:46:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320453003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320453003/60001

[commit-bot: I haz the power, 2015-10-13 16:54:19]

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

[commit-bot: I haz the power, 2015-10-13 16:54:20]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[xhwang, 2015-10-13 19:15:38]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-10-13 19:15:38]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
piman@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1320453003/#ps80001
(title: "Add missing import")

[commit-bot: I haz the power, 2015-10-13 19:19:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320453003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320453003/80001

[commit-bot: I haz the power, 2015-10-13 19:34:19]

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

[commit-bot: I haz the power, 2015-10-13 19:34:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xhwang, 2015-10-13 19:56:00]

sky: Please OWNERS review the addition of '+mojo/shell' in content/gpu/DEPS. I
need it for "mojo/shell/static_application_loader.h" in
content/gpu/gpu_process_control_impl.cc. Thanks!

[xhwang, 2015-10-13 19:56:53]

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

[xhwang, 2015-10-13 19:56:53]

(really add sky@ this time)

sky: Please OWNERS review the addition of '+mojo/shell' in content/gpu/DEPS. I
need it for "mojo/shell/static_application_loader.h" in
content/gpu/gpu_process_control_impl.cc. Thanks!

[sky, 2015-10-13 22:44:12]

LGTM

[xhwang, 2015-10-13 23:01:50]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2015-10-13 23:04:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1320453003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1320453003/80001

[commit-bot: I haz the power, 2015-10-13 23:13:55]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-13 23:14:36]

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