Mojo framework for AndroidOverlay.

Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds the first instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
TEST=mojo_android_overlay_unittest

Review-Url: https://codereview.chromium.org/2688193002
Cr-Commit-Position: refs/heads/master@{#457964}
Committed: https://chromium.googlesource.com/chromium/src/+/3df040e473be7b838c1648fc2599287d1e3e3549

[liberato (no reviews please), 2017-02-11 20:27:45]

Description was changed from

==========
Mojo framework for AndroidOverlay.

BUG=
==========

to

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Also adds Java-only mojo interfaces to the GPU registry.

BUG=691283
==========

[liberato (no reviews please), 2017-02-11 20:30:47]

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

[commit-bot: I haz the power, 2017-02-11 20:30:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-02-11 20:31:38]

Description was changed from

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Also adds Java-only mojo interfaces to the GPU registry.

BUG=691283
==========

to

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds first the instance of a Java-only mojo interfaces to the GPU
interface registry.

BUG=691283
==========

[commit-bot: I haz the power, 2017-02-11 20:32:32]

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

[commit-bot: I haz the power, 2017-02-11 20:32:33]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[liberato (no reviews please), 2017-02-11 20:36:24]

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

[commit-bot: I haz the power, 2017-02-11 20:36:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-02-11 20:41:40]

Description was changed from

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds first the instance of a Java-only mojo interfaces to the GPU
interface registry.

BUG=691283
==========

to

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds first the instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
==========

[commit-bot: I haz the power, 2017-02-11 20:49:27]

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

[commit-bot: I haz the power, 2017-02-11 20:49:28]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[liberato (no reviews please), 2017-02-12 05:45:17]

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

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

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-02-12 06:47:11]

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

[commit-bot: I haz the power, 2017-02-12 06:47:12]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[liberato (no reviews please), 2017-02-14 21:34:45]

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

[commit-bot: I haz the power, 2017-02-14 21:35:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-02-14 21:35:27]

Description was changed from

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds first the instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
==========

to

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds first the instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
TEST=mojo_android_overlay_unittest
==========

[liberato (no reviews please), 2017-02-14 21:35:27]

liberato@chromium.org changed reviewers:
+ boliu@chromium.org, dcheng@chromium.org

[commit-bot: I haz the power, 2017-02-14 21:38:24]

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

[commit-bot: I haz the power, 2017-02-14 21:38:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[liberato (no reviews please), 2017-02-14 21:45:50]

liberato@chromium.org changed reviewers:
+ watk@chromium.org, xhwang@chromium.org

[liberato (no reviews please), 2017-02-14 21:45:51]

boliu: i've not included any code that might do something useful, but that will
be mostly dropping in DialogSurfaceImpl / DialogSurfaceCore from the other CL. 
also needs to integrate with SurfaceLookup.  i'd like to land that separately,
though.

xhwang, dcheng: PTAL at lots of mojo.  I've never used mojo before, so it's
entirely possible that i've got this all wrong.  :)

all: of particular concern are the changes in gpu_process_host.cc -- i tried to
do something similar to what RenderProcessHostImpl does.  however, i had to post
to UI to call GetGlobalJavaInterfaces.  for whatever reason, i still had to
provide the UI task runner to AddInterface.

I've no idea what that actually does.  i do know that it didn't work without it.
 dcheck on wrong thread, i think.

tests: i've included a unit test for the client side.  i've not tried to add
tests to the service side, mostly since it does nothing interesting yet.  i have
verified via manual testing that one can actually create an overlay in the gpu
process, and have the browser process send callbacks.

thanks
-fl

[liberato (no reviews please), 2017-02-14 23:03:15]

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

[commit-bot: I haz the power, 2017-02-14 23:03:54]

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-02-14 23:30:21]

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

[commit-bot: I haz the power, 2017-02-14 23:30:22]

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...)

[liberato (no reviews please), 2017-02-14 23:40:44]

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

[commit-bot: I haz the power, 2017-02-14 23:41:57]

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-02-15 01:11:45]

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

[commit-bot: I haz the power, 2017-02-15 01:11:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[liberato (no reviews please), 2017-02-15 20:13:34]

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

[commit-bot: I haz the power, 2017-02-15 20:14:31]

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-02-15 22:11:47]

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

[commit-bot: I haz the power, 2017-02-15 22:11:48]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[boliu, 2017-02-16 22:24:18]

boliu@chromium.org changed reviewers:
+ rockot@chromium.org

[boliu, 2017-02-16 22:24:19]

looks much simpler than binder, although I guess all the implementation is not
actually here yet

+rockot for mojo help. This is trying have gpu c++ code talk to browser java
code on the UI thread, which I think is a first. Any advice on how to set this
up?

https://codereview.chromium.org/2688193002/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2688193002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:326: void
AddUIInterfaceHelper(service_manager::InterfaceRegistry* registry) {
Hmm, I don't know mojo at all.. my immediate question here is what's the
lifetime and thread safety guarantees of registry here.

Plus this class is all on the IO thread, can this be done somewhere else? Maybe
GpuProcessHostUIShim?

[Ken Rockot(use gerrit already), 2017-02-17 06:29:57]

This all looks very reasonable to me. See inline comments about fixing the
GpuProcessHost code.

I don't see client code for the GPU process but in general if you use
GpuChildThread's GetRemoteInterfaces() as your InterfaceProvider this should all
just work (maybe you've done that already in your manual testing?)

https://codereview.chromium.org/2688193002/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2688193002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:337:
base::Bind(&AddUIInterfaceHelper<InterfaceType>, registry));
This is not safe, and CreateInterfaceFactory() while convenient is unfortunately
not sufficient for this case. |registry| lives on the IO thread where the
ConnectionFilter lives, so you can't just post to the UI thread to register a
binder on it there. Instead, you want to register a binder on the IO thread
which itself posts to the UI thread to handle each individual request, like so:

template <typename Interface>
void BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
  DCHECK(/* on the UI thread */);
  content::GetGlobalJavaInterfaces()->GetInterface(std::move(request));
}

template <typename Interface>
void BindJavaInterfaceOnUIThread(mojo::InterfaceRequest<Interface> request) {
  BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)->PostTask(
      FROM_HERE, base::Bind(&BindJavaInterface<Interface>,
base::Passed(&request));
}

/// ... in OnConnect

registry->AddInterface(base::Bind(
    &BindJavaInterfaceOnUIThread<media::mojom::AndroidOverlayProvider>));

https://codereview.chromium.org/2688193002/diff/200001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay.cc (right):

https://codereview.chromium.org/2688193002/diff/200001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay.cc:107: // TODO(liberato): ask binder
for the surface.
Side note in case this is interesting: we're actively looking into enabling
wrapping of Parcelable  objects as generic Mojo handles so they can be
transmitted seamlessly over message pipes. No ETA yet, but there should be more
developments soon.

[liberato (no reviews please), 2017-02-17 17:35:24]

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

[commit-bot: I haz the power, 2017-02-17 17:36:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-02-17 17:45:21]

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

[commit-bot: I haz the power, 2017-02-17 17:45:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-02-17 17:47:13]

rockot: i really appreciate your detailed help.

i've been testing by adding debug code in ChildProcessServiceImpl.  it does
round trip initialization of an overlay with gpu => browser => gpu (callback).

boliu: this PS also changes the AndroidOverlay interface slightly.  in a
different CL, i'm modifiying AndroidVideoDecodeAccelerator to use the
AndroidOverlay interface.  it's based on a ContentVideoView wrapper impl rather
than mojo.  the interface changes here come out of what i learned there.

also removes some example code from the java impl which caused a bot failure.

thanks
-fl

thanks
-fl

https://codereview.chromium.org/2688193002/diff/200001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2688193002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:326: void
AddUIInterfaceHelper(service_manager::InterfaceRegistry* registry) {
On 2017/02/16 22:24:19, boliu wrote:
> Hmm, I don't know mojo at all.. my immediate question here is what's the
> lifetime and thread safety guarantees of registry here.
> 
> Plus this class is all on the IO thread, can this be done somewhere else?
Maybe
> GpuProcessHostUIShim?

(fixed per rockot@'s suggestions.)

https://codereview.chromium.org/2688193002/diff/200001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:337:
base::Bind(&AddUIInterfaceHelper<InterfaceType>, registry));
On 2017/02/17 06:29:56, Ken Rockot wrote:
> This is not safe, and CreateInterfaceFactory() while convenient is
unfortunately
> not sufficient for this case. |registry| lives on the IO thread where the
> ConnectionFilter lives, so you can't just post to the UI thread to register a
> binder on it there. Instead, you want to register a binder on the IO thread
> which itself posts to the UI thread to handle each individual request, like
so:
> 
> template <typename Interface>
> void BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
>   DCHECK(/* on the UI thread */);
>   content::GetGlobalJavaInterfaces()->GetInterface(std::move(request));
> }
> 
> template <typename Interface>
> void BindJavaInterfaceOnUIThread(mojo::InterfaceRequest<Interface> request) {
>   BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)->PostTask(
>       FROM_HERE, base::Bind(&BindJavaInterface<Interface>,
> base::Passed(&request));
> }
> 
> /// ... in OnConnect
> 
> registry->AddInterface(base::Bind(
>     &BindJavaInterfaceOnUIThread<media::mojom::AndroidOverlayProvider>));

thank you very much!

https://codereview.chromium.org/2688193002/diff/200001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay.cc (right):

https://codereview.chromium.org/2688193002/diff/200001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay.cc:107: // TODO(liberato): ask binder
for the surface.
On 2017/02/17 06:29:56, Ken Rockot wrote:
> Side note in case this is interesting: we're actively looking into enabling
> wrapping of Parcelable  objects as generic Mojo handles so they can be
> transmitted seamlessly over message pipes. No ETA yet, but there should be
more
> developments soon.

that is very exciting news!

is there a design doc?  i'm interested in how that will work.

i can think of only two ways: (a) do something magic with parceled file
descriptors, or (b) use binder under the hood.

[Ken Rockot(use gerrit already), 2017-02-17 17:55:55]

On Fri, Feb 17, 2017 at 9:47 AM, <liberato@chromium.org> wrote:

> rockot: i really appreciate your detailed help.
>
> i've been testing by adding debug code in ChildProcessServiceImpl. it does
> round trip initialization of an overlay with gpu => browser => gpu
> (callback).
>
> boliu: this PS also changes the AndroidOverlay interface slightly. in a
> different CL, i'm modifiying AndroidVideoDecodeAccelerator to use the
> AndroidOverlay interface. it's based on a ContentVideoView wrapper impl
> rather
> than mojo. the interface changes here come out of what i learned there.
>
> also removes some example code from the java impl which caused a bot
> failure.
>
> thanks
> -fl
>
> thanks
> -fl
>
>
> https://codereview.chromium.org/2688193002/diff/200001/
> content/browser/gpu/gpu_process_host.cc
> File content/browser/gpu/gpu_process_host.cc (right):
>
> https://codereview.chromium.org/2688193002/diff/200001/
> content/browser/gpu/gpu_process_host.cc#newcode326
> content/browser/gpu/gpu_process_host.cc:326: void
> AddUIInterfaceHelper(service_manager::InterfaceRegistry* registry) {
> On 2017/02/16 22:24:19, boliu wrote:
> > Hmm, I don't know mojo at all.. my immediate question here is what's
> the
> > lifetime and thread safety guarantees of registry here.
> >
> > Plus this class is all on the IO thread, can this be done somewhere
> else? Maybe
> > GpuProcessHostUIShim?
>
> (fixed per rockot@'s suggestions.)
>
> https://codereview.chromium.org/2688193002/diff/200001/
> content/browser/gpu/gpu_process_host.cc#newcode337
> content/browser/gpu/gpu_process_host.cc:337:
> base::Bind(&AddUIInterfaceHelper<InterfaceType>, registry));
> On 2017/02/17 06:29:56, Ken Rockot wrote:
> > This is not safe, and CreateInterfaceFactory() while convenient is
> unfortunately
> > not sufficient for this case. |registry| lives on the IO thread where
> the
> > ConnectionFilter lives, so you can't just post to the UI thread to
> register a
> > binder on it there. Instead, you want to register a binder on the IO
> thread
> > which itself posts to the UI thread to handle each individual request,
> like so:
> >
> > template <typename Interface>
> > void BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
> > DCHECK(/* on the UI thread */);
> >
> content::GetGlobalJavaInterfaces()->GetInterface(std::move(request));
> > }
> >
> > template <typename Interface>
> > void BindJavaInterfaceOnUIThread(mojo::InterfaceRequest<Interface>
> request) {
> > BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)->PostTask(
> > FROM_HERE, base::Bind(&BindJavaInterface<Interface>,
> > base::Passed(&request));
> > }
> >
> > /// ... in OnConnect
> >
> > registry->AddInterface(base::Bind(
> >
> &BindJavaInterfaceOnUIThread<media::mojom::AndroidOverlayProvider>));
>
> thank you very much!
>
> https://codereview.chromium.org/2688193002/diff/200001/
> media/mojo/clients/mojo_android_overlay.cc
> File media/mojo/clients/mojo_android_overlay.cc (right):
>
> https://codereview.chromium.org/2688193002/diff/200001/
> media/mojo/clients/mojo_android_overlay.cc#newcode107
> media/mojo/clients/mojo_android_overlay.cc:107: // TODO(liberato): ask
> binder for the surface.
> On 2017/02/17 06:29:56, Ken Rockot wrote:
> > Side note in case this is interesting: we're actively looking into
> enabling
> > wrapping of Parcelable objects as generic Mojo handles so they can be
> > transmitted seamlessly over message pipes. No ETA yet, but there
> should be more
> > developments soon.
>
> that is very exciting news!
>
> is there a design doc? i'm interested in how that will work.
>
> i can think of only two ways: (a) do something magic with parceled file
> descriptors, or (b) use binder under the hood.
>

no design doc atm, but tl;dr is we want to have a binder side-channel
accompany the OS pipe we use for general message transit between any two
processes. a parcelable will be serialized over the pipe as a simple GUID,
and we'll use the binder interface to broker the GUID/parcelable mapping.

ultimately we'd love to move the low-level mojo transport on android
completely over to binder in lieu of using any OS pipes, but there are some
rather large hurdles there.


> https://codereview.chromium.org/2688193002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-02-17 20:17:38]

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

[commit-bot: I haz the power, 2017-02-17 20:17:39]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[boliu, 2017-02-17 21:20:51]

https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:326: void
BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
if we end up creating a separate registry for gpu, then this could move to that
code

if not, I think gpu_process_host_ui_shim might be better, since that's like the
"UI" part of GpuProcessHost

https://codereview.chromium.org/2688193002/diff/240001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
(right):

https://codereview.chromium.org/2688193002/diff/240001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:66:
registry.addInterface(AndroidOverlayProvider.MANAGER,
this makes Overlay accessible to renderer as well, right? do we need a separate
registrar just for gpu?

[boliu, 2017-02-17 21:24:10]

And getting off topic..

> ultimately we'd love to move the low-level mojo transport on android
completely over to binder in lieu of using any OS pipes, but there are some
rather large hurdles there.

So one early concern there. For webview, the pool of binder threads is shared
with the app. So potentially app could clog up those threads, or (probably more
realistically) introduce undesired thread blocking behavior

[liberato (no reviews please), 2017-02-22 16:25:01]

sorry for the delay.  thanks for the comments!

-fl

https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host.cc:326: void
BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
On 2017/02/17 21:20:51, boliu wrote:
> if we end up creating a separate registry for gpu, then this could move to
that
> code
> 
> if not, I think gpu_process_host_ui_shim might be better, since that's like
the
> "UI" part of GpuProcessHost

moved to UI shim, per offline discussion.

https://codereview.chromium.org/2688193002/diff/240001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
(right):

https://codereview.chromium.org/2688193002/diff/240001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:66:
registry.addInterface(AndroidOverlayProvider.MANAGER,
On 2017/02/17 21:20:51, boliu wrote:
> this makes Overlay accessible to renderer as well, right? do we need a
separate
> registrar just for gpu?

as far as i can tell, it does not make it available.  one must also add the
interface in remote interfaces in RenderProcessHostImpl, and update the service
manifest to allow connections from the renderer.

i'm still a little sketchy, but it seems like this makes it available in the
browser.  the other steps advertise it to the other processes.

[Ken Rockot(use gerrit already), 2017-02-22 16:30:10]

On 2017/02/22 at 16:25:01, liberato wrote:
> sorry for the delay.  thanks for the comments!
> 
> -fl
> 
>
https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gp...
> File content/browser/gpu/gpu_process_host.cc (right):
> 
>
https://codereview.chromium.org/2688193002/diff/240001/content/browser/gpu/gp...
> content/browser/gpu/gpu_process_host.cc:326: void
BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
> On 2017/02/17 21:20:51, boliu wrote:
> > if we end up creating a separate registry for gpu, then this could move to
that
> > code
> > 
> > if not, I think gpu_process_host_ui_shim might be better, since that's like
the
> > "UI" part of GpuProcessHost
> 
> moved to UI shim, per offline discussion.
> 
>
https://codereview.chromium.org/2688193002/diff/240001/content/public/android...
> File
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
(right):
> 
>
https://codereview.chromium.org/2688193002/diff/240001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:66:
registry.addInterface(AndroidOverlayProvider.MANAGER,
> On 2017/02/17 21:20:51, boliu wrote:
> > this makes Overlay accessible to renderer as well, right? do we need a
separate
> > registrar just for gpu?
> 
> as far as i can tell, it does not make it available.  one must also add the
interface in remote interfaces in RenderProcessHostImpl, and update the service
manifest to allow connections from the renderer.
> 
> i'm still a little sketchy, but it seems like this makes it available in the
browser.  the other steps advertise it to the other processes.

That's correct - the java interfaces bag is only directly accessible to the
browser. In order to make one of its registered interfaces reachable from
another process, you have to register a forwarding binder somewhere that's
accessible to that process (as you've done in GpuProcessHost's InterfaceRegistry
for the GPU process to access)

[boliu, 2017-02-22 18:03:50]

only minor things since this CL is not really complete

https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host_ui_shim.cc:231: void
BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
these two should be in an anonymous namespace

https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host_ui_shim.cc:246: void
GpuProcessHostUIShim::RegisterUIThreadMojoInterfaces(
This needs to be OS_ANDROID as well?

https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java
(right):

https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java:14:
* Default AndroidOverlay impl.  Will use a separate (shared) overlay-ui thread
to own a Dialog
why not just use the UI thread since it's simpler. Are there going to be
blocking/slow operations here?

https://codereview.chromium.org/2688193002/diff/260001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay.h (right):

https://codereview.chromium.org/2688193002/diff/260001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay.h:23: class MojoAndroidOverlay : public
AndroidOverlay {
I guess it's a bit odd to me that MojoFoo isn't actually a mojo type here and in
the factory. But not a big deal I guess

[liberato (no reviews please), 2017-02-22 19:10:50]

incomplete: intentionally.  my new goal is to make incremental progress after
abandoning https://codereview.chromium.org/2178973004/ .

it does have the bits that are missing, though, minus the surface lookup.  i'd
expect DSImpl and DSCore to be brought over largely unchanged.

thanks
-fl

https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gp...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host_ui_shim.cc:231: void
BindJavaInterface(mojo::InterfaceRequest<Interface> request) {
On 2017/02/22 18:03:49, boliu wrote:
> these two should be in an anonymous namespace

Done.

https://codereview.chromium.org/2688193002/diff/260001/content/browser/gpu/gp...
content/browser/gpu/gpu_process_host_ui_shim.cc:246: void
GpuProcessHostUIShim::RegisterUIThreadMojoInterfaces(
On 2017/02/22 18:03:49, boliu wrote:
> This needs to be OS_ANDROID as well?

Done.

https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java
(right):

https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java:14:
* Default AndroidOverlay impl.  Will use a separate (shared) overlay-ui thread
to own a Dialog
On 2017/02/22 18:03:49, boliu wrote:
> why not just use the UI thread since it's simpler. Are there going to be
> blocking/slow operations here?

SurfaceDestroyed is synchronous on this thread.  it's caused us many problems
when it's on the ui thread.  latency is one.  another is that it risks deadlock
with the gpu main thread while the sync callback is going on.

https://codereview.chromium.org/2688193002/diff/260001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay.h (right):

https://codereview.chromium.org/2688193002/diff/260001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay.h:23: class MojoAndroidOverlay : public
AndroidOverlay {
On 2017/02/22 18:03:50, boliu wrote:
> I guess it's a bit odd to me that MojoFoo isn't actually a mojo type here and
in
> the factory. But not a big deal I guess

i agree, done.

originally i went back and forth on this.  it was more obviously doing what i
intended this way.  seems better now that i tried again.

[boliu, 2017-02-22 19:16:00]

content lgtm

https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java
(right):

https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java:14:
* Default AndroidOverlay impl.  Will use a separate (shared) overlay-ui thread
to own a Dialog
On 2017/02/22 19:10:50, liberato wrote:
> On 2017/02/22 18:03:49, boliu wrote:
> > why not just use the UI thread since it's simpler. Are there going to be
> > blocking/slow operations here?
> 
> SurfaceDestroyed is synchronous on this thread.  it's caused us many problems
> when it's on the ui thread.  latency is one.  another is that it risks
deadlock
> with the gpu main thread while the sync callback is going on.

CompositorView::SurfaceDestroyed doesn't block.. But if you say media stuff is
less robust to this, then ok

[liberato (no reviews please), 2017-02-22 19:23:38]

On 2017/02/22 19:16:00, boliu wrote:
> content lgtm
> 
>
https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java
> (right):
> 
>
https://codereview.chromium.org/2688193002/diff/260001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayImpl.java:14:
> * Default AndroidOverlay impl.  Will use a separate (shared) overlay-ui thread
> to own a Dialog
> On 2017/02/22 19:10:50, liberato wrote:
> > On 2017/02/22 18:03:49, boliu wrote:
> > > why not just use the UI thread since it's simpler. Are there going to be
> > > blocking/slow operations here?
> > 
> > SurfaceDestroyed is synchronous on this thread.  it's caused us many
problems
> > when it's on the ui thread.  latency is one.  another is that it risks
> deadlock
> > with the gpu main thread while the sync callback is going on.
> 
> CompositorView::SurfaceDestroyed doesn't block.. But if you say media stuff is
> less robust to this, then ok

CompositorView: oh, sorry, i should have clarified 'ContentVideoView'.

dcheng: PTAL @ content_browser_manifest.json and android_overlay.mojom

thanks
-fl

[dcheng, 2017-02-28 05:22:04]

Sorry for the delayed review =(

https://codereview.chromium.org/2688193002/diff/280001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java
(right):

https://codereview.chromium.org/2688193002/diff/280001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:25:
public void createOverlay(AndroidOverlayClient client, AndroidOverlayConfig
config) {
Normally, it's best to implement the interface in the same CL as the one that
adds the interface methods, to make it easier to security review things
end-to-end. This is the preferred option, so if that's possible here, that would
be best.

If that's not possible, can you make sure to include an IPC reviewer on the
followup CL that implements this.

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay.h (right):

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay.h:32:
MojoAndroidOverlay(mojom::AndroidOverlayProviderPtr provider_ptr,
Since InterfaceProvider is a mojo interface, can't this be mocked out in the
unit test?

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay_factory.h (right):

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay_factory.h:34:
service_manager::mojom::InterfaceProvider* interface_provider_;
Nit: I personally prefer to mark these const to make sure they aren't
accidentally mutated.

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay_factory.h:35: int render_frame_id_;
This feels like a layering violation. //media can't depend on //content since
//content depends on //media, but this embeds the concept of render frames
inside //media.

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:26: OnInitialized(AndroidOverlay
overlay);
Can we remove the need for this method by plumbing it through CreateOverlay as
well? AndroidOverlay& is useful for this purpose (it passes an interface request
which the other side will bind)

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:42: uint32 renderer_pid;
We shouldn't be passing render frame ID or the renderer PID over IPC, as we
can't trust the renderer.

[liberato (no reviews please), 2017-03-06 07:51:05]

thanks for the comments, everybody.

-fl

https://codereview.chromium.org/2688193002/diff/280001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java
(right):

https://codereview.chromium.org/2688193002/diff/280001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:25:
public void createOverlay(AndroidOverlayClient client, AndroidOverlayConfig
config) {
On 2017/02/28 05:22:03, dcheng wrote:
> Normally, it's best to implement the interface in the same CL as the one that
> adds the interface methods, to make it easier to security review things
> end-to-end. This is the preferred option, so if that's possible here, that
would
> be best.
> 
> If that's not possible, can you make sure to include an IPC reviewer on the
> followup CL that implements this.

i'll pick option 2.  :)

the reason is that this is a rewrite of
https://codereview.chromium.org/2178973004/ to us mojo, since boliu@ correctly
pointed out that it's more chrom-y than binder.  much of the remaining bits
there will be more or less identical to what will be added in later CLs to the
mojo-based overlay framework.

however, that CL was also unreasonably large in my opinion, so i'd really,
really like to keep this one simpler.  after abandoning a ~6 month CL, i'm all
about incremental progress right now.  :)

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay.h (right):

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay.h:32:
MojoAndroidOverlay(mojom::AndroidOverlayProviderPtr provider_ptr,
On 2017/02/28 05:22:03, dcheng wrote:
> Since InterfaceProvider is a mojo interface, can't this be mocked out in the
> unit test?

good point.  done.

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
File media/mojo/clients/mojo_android_overlay_factory.h (right):

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay_factory.h:34:
service_manager::mojom::InterfaceProvider* interface_provider_;
On 2017/02/28 05:22:03, dcheng wrote:
> Nit: I personally prefer to mark these const to make sure they aren't
> accidentally mutated.

Done.

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/clients/moj...
media/mojo/clients/mojo_android_overlay_factory.h:35: int render_frame_id_;
On 2017/02/28 05:22:03, dcheng wrote:
> This feels like a layering violation. //media can't depend on //content since
> //content depends on //media, but this embeds the concept of render frames
> inside //media.

yeah, it is.  i'd like to remove them both; they're just implementation details
anyway that the client shouldn't know or care about.  what i'd really like to
see is render frame create the factory in the browser, and hand that off to the
gpu process.  however, we can't since there's still IPC involved renderer -> GPU
that won't let us hand off mojo objects.  plus, i think it might route messages
from gpu -> renderer -> browser if we do that (+rockot: am i correct about
that?)

one could probably have the render frame get an UnguessableToken from the
browser, rather than the mojo interface, and send that to the gpu process.  i
tried something similar in a previous CL (TL;DR: adds no security, none needed
anyway), but it wasn't in the context of layering, just security.

any way one looks at it, it's just trading one int for another -- we'd name
(|render_frame_id_|, |renderer_pid_|) to something new (|token|), and add some
maps on the browser side to map it between the token and the original data. 
nothing would really be all that different; it'd be a differently named opaque
value.

granted, we could remove the need for the gpu process to get the pid from the
ipc channel.  it'll also work better for other future use-cases for overlays in
which there is no render frame.  the client doesn't need to know that.

so, how about this: i'll replace these two ints with one unguessable token now. 
this token will be created, in the next CL, by the provider in response to some
call from RenderFrameHostImpl.  the token will be sent by RFHI to the
RenderFrame, and passed to the gpu process over IPC.  gpu will put it back into
this config when talking to the provider.

later, if RFHI isn't the only thing that can create an overlay, that'll fit
right in with no client changes.

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:26: OnInitialized(AndroidOverlay
overlay);
On 2017/02/28 05:22:04, dcheng wrote:
> Can we remove the need for this method by plumbing it through CreateOverlay as
> well? AndroidOverlay& is useful for this purpose (it passes an interface
request
> which the other side will bind)

thanks, done!

https://codereview.chromium.org/2688193002/diff/280001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:42: uint32 renderer_pid;
On 2017/02/28 05:22:04, dcheng wrote:
> We shouldn't be passing render frame ID or the renderer PID over IPC, as we
> can't trust the renderer.

this came up earlier.  this isn't exposed to the renderer; the gpu process will
be using it.  i'll dig up the thread with chrome-security, but TL;DR was "gpu
can do what it likes anyway. it can get the pid from the ipc channel, so the
renderer only sends the frame id.  it's allowed to lie about that, since the pid
is reliable.".

that said, i'd really like to remove both frame id and pid from here.  please
see my comments about the layering issue.

[liberato (no reviews please), 2017-03-11 21:39:36]

sorry about the delay.  i've finally gotten around to trying out removing (frame
id, pid) entirely.  turns out that it works better than i expected, since the
non-mojo implementation of AndroidOverlay that AVDA uses also needs an entirely
different opaque token.

thanks
-fl

https://codereview.chromium.org/2688193002/diff/360001/media/base/android/and...
File media/base/android/android_overlay.h (right):

https://codereview.chromium.org/2688193002/diff/360001/media/base/android/and...
media/base/android/android_overlay.h:59: base::UnguessableToken routing_token;
it turns out that not only is (frame id, pid) a layering violation, it's also
implementation specific.  CVV-based overlay (currently used in AVDA) requires a
different integer token to work.  i hadn't thought of it that way before doing
the CVV => AndroidOverlay work last week.

so, i've removed (frame id, pid) from the mojo-specific code, and made a generic
token part of the AndroidOverlay config.

RenderFrameImpl will negotiate with RFHI to get a token for the mojo impl of
AndroidOverlay, which hides all of it from media and removes the need for
anybody to worry about the pid.

[dcheng, 2017-03-16 08:51:40]

sorry for the review delay. mojo lgtm, but if you don't mind, can you include
security reviewers on followups that finish implementing some of the not-fully
implemented mojo methods?

https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:38:
mojo.common.mojom.UnguessableToken routing_token;
Nit: please add some comments that describe why routing_token is necessary. In
Mojo, it's ideal to represent identity implicitly with the message pipe between
the sender and the receiver, and anytime where that isn't true, it's best to
document why that's not sufficient.

[dcheng, 2017-03-16 08:54:25]

https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:11: interface AndroidOverlayProvider
{
Also, some interface level comments that describe the high-level purpose would
be nice. I can tell by reading this CL that AndroidOverlayProvider and
AndroidOverlay live in the browser process (and are presumably used from less
privileged processes), but I'm not sure that will be so obvious a year or two
from now.

[liberato (no reviews please), 2017-03-16 16:50:32]

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

[commit-bot: I haz the power, 2017-03-16 16:50:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-03-16 16:53:53]

dcheng: i'll be sure to add you to the rest of the impl  CLs.  thanks for being
flexible about this.

thanks
-fl

https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:11: interface AndroidOverlayProvider
{
On 2017/03/16 08:54:25, dcheng wrote:
> Also, some interface level comments that describe the high-level purpose would
> be nice. I can tell by reading this CL that AndroidOverlayProvider and
> AndroidOverlay live in the browser process (and are presumably used from less
> privileged processes), but I'm not sure that will be so obvious a year or two
> from now.

Done.

https://codereview.chromium.org/2688193002/diff/360001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:38:
mojo.common.mojom.UnguessableToken routing_token;
On 2017/03/16 08:51:39, dcheng wrote:
> Nit: please add some comments that describe why routing_token is necessary. In
> Mojo, it's ideal to represent identity implicitly with the message pipe
between
> the sender and the receiver, and anytime where that isn't true, it's best to
> document why that's not sufficient.

Done.  TL;DR: it must be sent via IPC for now.

[liberato (no reviews please), 2017-03-16 18:48:56]

Description was changed from

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds first the instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
TEST=mojo_android_overlay_unittest
==========

to

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds the first instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
TEST=mojo_android_overlay_unittest
==========

[xhwang, 2017-03-16 19:12:50]

LGTM with tiny nits

https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:19: CreateOverlay(AndroidOverlay&
overlay, AndroidOverlayClient client, AndroidOverlayConfig config);
nit: 80 char limit?

https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:56:
mojo.common.mojom.UnguessableToken routing_token;
nit: empty line here

[liberato (no reviews please), 2017-03-16 19:53:20]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-16 19:53:20]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org,
dcheng@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2688193002/#ps400001
(title: "cl feedback")

[commit-bot: I haz the power, 2017-03-16 19:54:04]

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

[liberato (no reviews please), 2017-03-16 19:54:14]

thanks for the feedback!


-fl

https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/...
File media/mojo/interfaces/android_overlay.mojom (right):

https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:19: CreateOverlay(AndroidOverlay&
overlay, AndroidOverlayClient client, AndroidOverlayConfig config);
On 2017/03/16 19:12:50, xhwang_slow wrote:
> nit: 80 char limit?

thanks.  didn't notice that 'cl format' skips these.

https://codereview.chromium.org/2688193002/diff/380001/media/mojo/interfaces/...
media/mojo/interfaces/android_overlay.mojom:56:
mojo.common.mojom.UnguessableToken routing_token;
On 2017/03/16 19:12:50, xhwang_slow wrote:
> nit: empty line here

Done.

[commit-bot: I haz the power, 2017-03-16 23:46:19]

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

[commit-bot: I haz the power, 2017-03-16 23:46:21]

Failed to apply patch for
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:
While running git apply --index -p1;
  error: patch failed:
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:8
  error:
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java:
patch does not apply

Patch:      
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
Index:
content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
diff --git
a/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
b/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
index
ec7a1258fa1e827ed995a14005f4793383d660ce..dcd01b057972852558f706501b33dc6f79306d79
100644
---
a/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
+++
b/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
@@ -8,6 +8,7 @@ import android.content.Context;
 
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
+import org.chromium.content.browser.androidoverlay.AndroidOverlayProviderImpl;
 import org.chromium.content.browser.shapedetection.FaceDetectionProviderImpl;
 import org.chromium.content_public.browser.InterfaceRegistrar;
 import org.chromium.content_public.browser.WebContents;
@@ -16,6 +17,7 @@ import org.chromium.device.battery.BatteryMonitorFactory;
 import org.chromium.device.mojom.VibrationManager;
 import org.chromium.device.nfc.mojom.Nfc;
 import org.chromium.device.vibration.VibrationManagerImpl;
+import org.chromium.media.mojom.AndroidOverlayProvider;
 import org.chromium.mojo.system.impl.CoreImpl;
 import org.chromium.services.service_manager.InterfaceRegistry;
 import org.chromium.shape_detection.mojom.FaceDetectionProvider;
@@ -61,6 +63,8 @@ class InterfaceRegistrarImpl {
                     BatteryMonitor.MANAGER, new
BatteryMonitorFactory(applicationContext));
             registry.addInterface(FaceDetectionProvider.MANAGER,
                     new FaceDetectionProviderImpl.Factory(applicationContext));
+            registry.addInterface(AndroidOverlayProvider.MANAGER,
+                    new
AndroidOverlayProviderImpl.Factory(applicationContext));
             // TODO(avayvod): Register the PresentationService implementation
here.
         }
     }

[liberato (no reviews please), 2017-03-17 05:54:56]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-17 05:54:56]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
boliu@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2688193002/#ps420001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-17 05:55:37]

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-03-17 08:31:42]

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

[commit-bot: I haz the power, 2017-03-17 08:31:44]

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

[liberato (no reviews please), 2017-03-17 16:48:18]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2017-03-17 16:48:44]

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-03-17 18:07:10]

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

[commit-bot: I haz the power, 2017-03-17 18:07:11]

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

[liberato (no reviews please), 2017-03-17 23:05:58]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-17 23:05:59]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
boliu@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2688193002/#ps440001
(title: "fixed mojo manifest bug introduced by rebase")

[commit-bot: I haz the power, 2017-03-17 23:06:37]

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-03-18 01:01:03]

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

[commit-bot: I haz the power, 2017-03-18 01:01:06]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[liberato (no reviews please), 2017-03-18 06:14:30]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2017-03-18 06:14:45]

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-03-18 08:06:57]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1489817670456060,
"parent_rev": "92435a5750ed8008b6498e1d8b2ca050dc974e13", "commit_rev":
"3df040e473be7b838c1648fc2599287d1e3e3549"}

[commit-bot: I haz the power, 2017-03-18 08:07:57]

Description was changed from

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds the first instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
TEST=mojo_android_overlay_unittest
==========

to

==========
Mojo framework for AndroidOverlay.

Basic implementation of the AndroidOverlay framework.  Requests to
create an overlay will create a do-nothing overlay impl.

Adds the first instance of a Java-only mojo interface to the GPU
interface registry.

BUG=691283
TEST=mojo_android_overlay_unittest

Review-Url: https://codereview.chromium.org/2688193002
Cr-Commit-Position: refs/heads/master@{#457964}
Committed:
https://chromium.googlesource.com/chromium/src/+/3df040e473be7b838c1648fc2599...
==========

[commit-bot: I haz the power, 2017-03-18 08:07:59]

Committed patchset #23 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/3df040e473be7b838c1648fc2599...