Add embedder api for participating in gpu logic

Add embedder api for participating in gpu logic

With new ContentGpuClient hook and GetGpuServiceRegistry, embedder can
register service in GPU process and query service registry in browser
process.

BUG=b/25057601

Committed: https://crrev.com/08377adbc7200d4a7e0e9821cfea0809ce61cd82
Cr-Commit-Position: refs/heads/master@{#376217}

[kcwu, 2016-01-28 14:57:10]

kcwu@chromium.org changed reviewers:
+ jam@chromium.org, owenlin@chromium.org, posciak@chromium.org

[kcwu, 2016-01-28 14:57:11]

Hi @jam,
I added content client hooks as discussed. Could you please take a look?
Thanks.

[jam, 2016-01-28 16:47:37]

https://codereview.chromium.org/1646853002/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1646853002/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:135: base::LazyInstance<ContentGpuClient>
g_empty_content_gpu_client =
nit: follow the rest of the lines and indent g_empty_content_gpu_client like
them

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS
File content/public/gpu/DEPS (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS#new...
content/public/gpu/DEPS:2: "+content/browser/gpu",
huh? why is this? content/gpu can't depend on content/browser

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.cc (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.cc:15: return
host->GetServiceRegistry();
this is code in the gpu process. GpuProcessHost is in the browser process. this
won't work.

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.h (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.h:15: CONTENT_EXPORT ServiceRegistry*
GetGpuServiceRegistry();
why is this needed? i.e. why isn't the ContentGpuClient method enough?

it's hard to see how this will be used since the change is just a skeleton. can
you add the arc++ code that will use this?

[kcwu, 2016-01-28 17:55:30]

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS
File content/public/gpu/DEPS (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS#new...
content/public/gpu/DEPS:2: "+content/browser/gpu",
On 2016/01/28 16:47:36, jam wrote:
> huh? why is this? content/gpu can't depend on content/browser

This is for implementation of gpu_service_registry.cc.
So, I'm wondering maybe gpu_service_registry.{cc,h} should be in
content/public/browser ?

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.cc (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.cc:15: return
host->GetServiceRegistry();
On 2016/01/28 16:47:36, jam wrote:
> this is code in the gpu process. GpuProcessHost is in the browser process.
this
> won't work.

I did want to call this in the browser process.

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.h (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.h:15: CONTENT_EXPORT ServiceRegistry*
GetGpuServiceRegistry();
On 2016/01/28 16:47:36, jam wrote:
> why is this needed? i.e. why isn't the ContentGpuClient method enough?
> 
> it's hard to see how this will be used since the change is just a skeleton.
can
> you add the arc++ code that will use this?

Since there is already a mojo ipc channel with ServiceRegistry established
between GpuProcessHost on browser process and GpuChildThread on GPU process. I
want to register a mojo service in the GPU process (via
ServiceRegistry::AddService in GpuChildThread) and then use the registered
service in the browser process (get the service via
ServiceRegistry::ConnectToRemoteService). So this function is needed and is
called in browser process.

The code to utilize them is not ready yet.

[jam, 2016-01-28 21:13:39]

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS
File content/public/gpu/DEPS (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS#new...
content/public/gpu/DEPS:2: "+content/browser/gpu",
On 2016/01/28 17:55:30, kcwu wrote:
> On 2016/01/28 16:47:36, jam wrote:
> > huh? why is this? content/gpu can't depend on content/browser
> 
> This is for implementation of gpu_service_registry.cc.
> So, I'm wondering maybe gpu_service_registry.{cc,h} should be in
> content/public/browser ?

Where do you want the code in gpu_service_registry.cc to run? If you want it to
run in the browser process, it should be in content/browser (or
content/public/browser). If you want it to run in the gpu process, it should be
in content/gpu (or content/public/gpu).

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.cc (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.cc:15: return
host->GetServiceRegistry();
On 2016/01/28 17:55:30, kcwu wrote:
> On 2016/01/28 16:47:36, jam wrote:
> > this is code in the gpu process. GpuProcessHost is in the browser process.
> this
> > won't work.
> 
> I did want to call this in the browser process.

ok that should be in content/public/browser then.

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.h (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.h:15: CONTENT_EXPORT ServiceRegistry*
GetGpuServiceRegistry();
On 2016/01/28 17:55:30, kcwu wrote:
> On 2016/01/28 16:47:36, jam wrote:
> > why is this needed? i.e. why isn't the ContentGpuClient method enough?
> > 
> > it's hard to see how this will be used since the change is just a skeleton.
> can
> > you add the arc++ code that will use this?
> 
> Since there is already a mojo ipc channel with ServiceRegistry established
> between GpuProcessHost on browser process and GpuChildThread on GPU process. I
> want to register a mojo service in the GPU process (via
> ServiceRegistry::AddService in GpuChildThread) and then use the registered
> service in the browser process (get the service via
> ServiceRegistry::ConnectToRemoteService). So this function is needed and is
> called in browser process.

If it's called in the browser process, then it should be in
content/public/browser.

> 
> The code to utilize them is not ready yet.

Please upload one version that works. At that point, we can split it out into
smaller patches. But right now, preliminary patches that don't have a working
system aren't ready for review because code is in the wrong processes.

[kcwu, 2016-01-29 17:22:44]

https://codereview.chromium.org/1646853002/diff/1/content/app/content_main_ru...
File content/app/content_main_runner.cc (right):

https://codereview.chromium.org/1646853002/diff/1/content/app/content_main_ru...
content/app/content_main_runner.cc:135: base::LazyInstance<ContentGpuClient>
g_empty_content_gpu_client =
On 2016/01/28 16:47:36, jam wrote:
> nit: follow the rest of the lines and indent g_empty_content_gpu_client like
> them

Done.

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS
File content/public/gpu/DEPS (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/DEPS#new...
content/public/gpu/DEPS:2: "+content/browser/gpu",
On 2016/01/28 21:13:39, jam wrote:
> On 2016/01/28 17:55:30, kcwu wrote:
> > On 2016/01/28 16:47:36, jam wrote:
> > > huh? why is this? content/gpu can't depend on content/browser
> > 
> > This is for implementation of gpu_service_registry.cc.
> > So, I'm wondering maybe gpu_service_registry.{cc,h} should be in
> > content/public/browser ?
> 
> Where do you want the code in gpu_service_registry.cc to run? If you want it
to
> run in the browser process, it should be in content/browser (or
> content/public/browser). If you want it to run in the gpu process, it should
be
> in content/gpu (or content/public/gpu).

Done.

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.cc (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.cc:15: return
host->GetServiceRegistry();
On 2016/01/28 21:13:39, jam wrote:
> On 2016/01/28 17:55:30, kcwu wrote:
> > On 2016/01/28 16:47:36, jam wrote:
> > > this is code in the gpu process. GpuProcessHost is in the browser process.
> > this
> > > won't work.
> > 
> > I did want to call this in the browser process.
> 
> ok that should be in content/public/browser then.

Done.

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
File content/public/gpu/gpu_service_registry.h (right):

https://codereview.chromium.org/1646853002/diff/1/content/public/gpu/gpu_serv...
content/public/gpu/gpu_service_registry.h:15: CONTENT_EXPORT ServiceRegistry*
GetGpuServiceRegistry();
On 2016/01/28 21:13:39, jam wrote:
> On 2016/01/28 17:55:30, kcwu wrote:
> > On 2016/01/28 16:47:36, jam wrote:
> > > why is this needed? i.e. why isn't the ContentGpuClient method enough?
> > > 
> > > it's hard to see how this will be used since the change is just a
skeleton.
> > can
> > > you add the arc++ code that will use this?
> > 
> > Since there is already a mojo ipc channel with ServiceRegistry established
> > between GpuProcessHost on browser process and GpuChildThread on GPU process.
I
> > want to register a mojo service in the GPU process (via
> > ServiceRegistry::AddService in GpuChildThread) and then use the registered
> > service in the browser process (get the service via
> > ServiceRegistry::ConnectToRemoteService). So this function is needed and is
> > called in browser process.
> 
> If it's called in the browser process, then it should be in
> content/public/browser.
> 
> > 
> > The code to utilize them is not ready yet.
> 
> Please upload one version that works. At that point, we can split it out into
> smaller patches. But right now, preliminary patches that don't have a working
> system aren't ready for review because code is in the wrong processes.

The CL of embedder call is http://crrev.com/1641353003. I'm still revising that
CL, but it already can work what I described (register service in the GPU
process and query service in the browser process).

[kcwu, 2016-02-01 13:15:05]

John, PTAL. Thanks.

[wuchengli1, 2016-02-02 06:49:52]

wuchengli@google.com changed reviewers:
+ wuchengli@google.com

[wuchengli1, 2016-02-02 06:49:52]

Hi John. Could you take another look? Thanks a lot.

[Owen Lin, 2016-02-03 07:26:09]

https://codereview.chromium.org/1646853002/diff/60001/content/public/gpu/cont...
File content/public/gpu/content_gpu_client.h (right):

https://codereview.chromium.org/1646853002/diff/60001/content/public/gpu/cont...
content/public/gpu/content_gpu_client.h:20: virtual void
RegisterMojoServices(ServiceRegistry* registry) {}
How about something like OnGpuThreadStarted(ServiceRegistery registry)? Please
also explain what's the registry.

[kcwu, 2016-02-03 14:32:55]

https://codereview.chromium.org/1646853002/diff/60001/content/public/gpu/cont...
File content/public/gpu/content_gpu_client.h (right):

https://codereview.chromium.org/1646853002/diff/60001/content/public/gpu/cont...
content/public/gpu/content_gpu_client.h:20: virtual void
RegisterMojoServices(ServiceRegistry* registry) {}
On 2016/02/03 07:26:08, Owen Lin wrote:
> How about something like OnGpuThreadStarted(ServiceRegistery registry)? Please
> also explain what's the registry.

Done.
I added more description in the comment. For method name, I'd like to keep in
order to be consistent with content/public/utility/content_utility_client.h.

[jam, 2016-02-04 01:48:50]

just one minor comment

https://codereview.chromium.org/1646853002/diff/100001/content/public/browser...
File content/public/browser/gpu_service_registry.h (right):

https://codereview.chromium.org/1646853002/diff/100001/content/public/browser...
content/public/browser/gpu_service_registry.h:15: CONTENT_EXPORT
ServiceRegistry* GetGpuServiceRegistry();
just put this as a method on GpuProcessHost, and that way we can avoid having
another file just for this

[jam, 2016-02-04 02:10:24]

lgtm (I thought gpuprocesshost was public but it's not)

[kcwu, 2016-02-04 02:11:30]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2016-02-04 02:13:02]

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

[commit-bot: I haz the power, 2016-02-04 07:45:24]

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

[commit-bot: I haz the power, 2016-02-04 07:45:25]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kcwu, 2016-02-04 08:00:39]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2016-02-04 08:03:11]

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

[commit-bot: I haz the power, 2016-02-04 12:47:16]

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

[commit-bot: I haz the power, 2016-02-04 12:47:17]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kcwu, 2016-02-05 14:33:11]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2016-02-05 14:33:38]

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

[commit-bot: I haz the power, 2016-02-05 19:14:20]

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

[commit-bot: I haz the power, 2016-02-05 19:14:21]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kcwu, 2016-02-14 04:14:00]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2016-02-14 04:14:20]

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

[commit-bot: I haz the power, 2016-02-14 08:05:31]

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

[commit-bot: I haz the power, 2016-02-14 08:05:32]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kcwu, 2016-02-14 09:59:53]

On 2016/02/14 08:05:32, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

Hi @jam, could you give me a hand?
The commit-bot failed consistently on the same tests several times today and one
week ago.
But from the log of commit-bot, I feel the error is not relevant to my change.
Is anything wrong in my code?

[kcwu, 2016-02-16 06:53:12]

The CQ bit was checked by kcwu@chromium.org

[commit-bot: I haz the power, 2016-02-16 06:53:31]

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

[commit-bot: I haz the power, 2016-02-16 10:45:51]

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

[commit-bot: I haz the power, 2016-02-16 10:45:52]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kcwu, 2016-02-18 17:47:17]

The CQ bit was checked by kcwu@chromium.org

[kcwu, 2016-02-18 17:47:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1646853002/#ps140001
(title: "fix crash in tests")

[commit-bot: I haz the power, 2016-02-18 17:48:48]

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

[commit-bot: I haz the power, 2016-02-18 19:12:53]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-18 19:13:44]

Description was changed from

==========
Add embedder api for participating in gpu logic

With new ContentGpuClient hook and GetGpuServiceRegistry, embedder can
register service in GPU process and query service registry in browser
process.

BUG=b/25057601
==========

to

==========
Add embedder api for participating in gpu logic

With new ContentGpuClient hook and GetGpuServiceRegistry, embedder can
register service in GPU process and query service registry in browser
process.

BUG=b/25057601

Committed: https://crrev.com/08377adbc7200d4a7e0e9821cfea0809ce61cd82
Cr-Commit-Position: refs/heads/master@{#376217}
==========

[commit-bot: I haz the power, 2016-02-18 19:13:46]

Patchset 8 (id:??) landed as
https://crrev.com/08377adbc7200d4a7e0e9821cfea0809ce61cd82
Cr-Commit-Position: refs/heads/master@{#376217}