Refactor GpuProcessHost::GetForClient to GpuProcessHost::Get, which takes a GPU process kind rather…

Refactor GpuProcessHost::GetForClient to GpuProcessHost::Get, which takes a GPU process kind rather than a client ID.

There is no longer a plan to have a GPU process for each client to identify the GPU process by its kind (sandboxed or unsandboxed) rather than client ID.

I checked about:gpu still works and that the limit on the number of times the sandboxed GPU is relaunched is still limitted.

[apatrick_chromium, 2012-03-01 01:35:33]

fischman: everything
jam: content/browser owners

Thanks.

[jam, 2012-03-01 01:38:31]

lgtm

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
File content/browser/gpu/gpu_process_host.h (right):

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.h:36: kSandboxed,
nit KIND_SANDBOXED per the chromium style guide. to make it more readable, I'd
rename the enum to GpuKind and then the values would be

GPU_KIND_SANDBOXED
GPU_KIND_UNSANDBOXED

http://www.chromium.org/developers/coding-style#Naming

[Ami GONE FROM CHROMIUM, 2012-03-01 02:33:04]

I'm pretty confused by this CL, but I suspect there's a small explanation
that'll set me right.

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
File content/browser/gpu/gpu_process_host.cc (right):

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.cc:201: // this will be extended to allow
the use of multiple GPU processes.
The CL description made me think this comment was headed for /dev/null, and
along with it g_hosts_by_id.  What am I missing?

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.cc:202: for
(IDMap<GpuProcessHost>::iterator it(g_hosts_by_id.Pointer());
If the map is kept, why is this iterating through all of the values instead of
using Lookup?

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
File content/browser/gpu/gpu_process_host.h (right):

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.h:36: kSandboxed,
If you reversed the order then the old "sandboxed" bool arg value of 0 would
continue to mean "unsandboxed" in the new enum, and lower the odds of missing
changes in this CL biting us in the future.  Just sayin'.

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.h:57: // thread.  Calls Get and if a host
is returned, sends it.  Can be called from
What a strange contract; Get() returning NULL is silently ignored here?
(obvs not part of your CL; today is just the first time I'm looking at this
code)

[apatrick_chromium, 2012-03-01 02:57:38]

jam, I went with GPU_PROCESS_KIND_X instead of GPU_KIND_X to make the
distinction kind of process rather than kind of GPU.

fischman, this code is rotting and this patch is an attempt to slow down the
rot. I would like to attack it some more.

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
File content/browser/gpu/gpu_process_host.cc (right):

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.cc:201: // this will be extended to allow
the use of multiple GPU processes.
On 2012/03/01 02:33:04, Ami Fischman wrote:
> The CL description made me think this comment was headed for /dev/null, and
> along with it g_hosts_by_id.  What am I missing?

Yeah missed this comment. Thanks. The idea is, if you want just want a
GpuChannelHost, perhaps for a new GPU process if the previous onen crashed, use
Get. If you want the GpuChannelHost for an instance of a GPU process that you
have sent messages to in the past, with an indication that it has since crashed,
look it up with FromID. I believe the reason we have more than one at a time is
because the detection of GPU processes crashing is somehow asynchronous. I'd
have to check the log.

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.cc:202: for
(IDMap<GpuProcessHost>::iterator it(g_hosts_by_id.Pointer());
On 2012/03/01 02:33:04, Ami Fischman wrote:
> If the map is kept, why is this iterating through all of the values instead of
> using Lookup?

Because the lookup is not by host ID I guess. I agree this is daft.

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
File content/browser/gpu/gpu_process_host.h (right):

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.h:36: kSandboxed,
On 2012/03/01 02:33:04, Ami Fischman wrote:
> If you reversed the order then the old "sandboxed" bool arg value of 0 would
> continue to mean "unsandboxed" in the new enum, and lower the odds of missing
> changes in this CL biting us in the future.  Just sayin'.

Good idea. Done.

https://chromiumcodereview.appspot.com/9559013/diff/5001/content/browser/gpu/...
content/browser/gpu/gpu_process_host.h:57: // thread.  Calls Get and if a host
is returned, sends it.  Can be called from
On 2012/03/01 02:33:04, Ami Fischman wrote:
> What a strange contract; Get() returning NULL is silently ignored here?
> (obvs not part of your CL; today is just the first time I'm looking at this
> code)

It handles Get returning NULL by deleting the message so it doesn't leak. If Get
returns NULL it should mean the GPU process has crashed. The failure is detected
later when the GpuProcessHost is destroyed.

[Ami GONE FROM CHROMIUM, 2012-03-01 03:21:03]

LGTM

https://chromiumcodereview.appspot.com/9559013/diff/4006/content/browser/gpu/...
File content/browser/gpu/gpu_process_host.cc (right):

https://chromiumcodereview.appspot.com/9559013/diff/4006/content/browser/gpu/...
content/browser/gpu/gpu_process_host.cc:199: for
(IDMap<GpuProcessHost>::iterator it(g_hosts_by_id.Pointer());
Add a TODO to clean this up? (with a flame-thrower?)

[apatrick_chromium, 2012-03-13 21:37:20]

fischman, I got derailed by m18 things. All I did was rebase, add the cleanup
comment you asked for in gpu_process_host.cc and update to account for your
browser_gpu_channel_host_factory.cc start-to-GPU-process-early change. PTAL.

[Ami GONE FROM CHROMIUM, 2012-03-13 21:46:59]

lgtm

[commit-bot: I haz the power, 2012-03-13 21:52:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apatrick@chromium.org/9559013/16001

[commit-bot: I haz the power, 2012-03-14 01:13:24]

Try job failure for 9559013-16001 (retry) on win_rel for step "ui_tests".
It's a second try, previously, steps "ui_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...