Move handle dumpage to the renderer process (so that it works correctly) and factor out redundant...

Move handle dumpage to the renderer process (so that it works correctly) and move handle enumerator into common.

BUG=96488
TEST=N/A
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102004

[Cris Neckar, 2011-09-13 21:31:34]

The trybots seem to be jacked up so I will try them again later.

[Cris Neckar, 2011-09-14 21:53:41]

On 2011/09/13 21:31:34, Cris Neckar wrote:
> The trybots seem to be jacked up so I will try them again later.

New version. Now with less invasive ifdefs and more actually compiling on
non-windows platforms.

[cpu_(ooo_6.6-7.5), 2011-09-16 00:02:19]

looking... please stay on the line.

[cpu_(ooo_6.6-7.5), 2011-09-16 00:12:47]

I think to add files to content you need an owner of such.

[Cris Neckar, 2011-09-16 17:25:59]

On 2011/09/16 00:12:47, cpu wrote:
> I think to add files to content you need an owner of such.

Yeah I was going to get an owner review after you guys are satisfied with it.
Thanks :)

[jam, 2011-09-19 17:10:48]

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:567: #if
defined(OS_WIN)
nit: this really isn't needed, no?

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:889: // When no
other owners of this object, we can delete ourselves.
you don't want to duplicate this code from RenderProcessHost, since this is
fragile code and if someone changes it in RPH, they won't know about here. so
move the code in RPH to a function that you can call.

as an example, you're currently not calling all_hosts.Remove(id()), which will
lead to crashes when someone iterates through the RPHs.

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/mock_render_process_host.cc (right):

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/mock_render_process_host.cc:78: return;
nit: not needed

http://codereview.chromium.org/7888024/diff/8001/content/common/child_process...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/7888024/diff/8001/content/common/child_process...
content/common/child_process_messages.h:43:
IPC_MESSAGE_CONTROL0(ChildProcessHostMsg_DumpHandlesDone)
ChildProcessHostMsg messages are at the bottom of the file

http://codereview.chromium.org/7888024/diff/8001/content/common/handle_enumer...
File content/common/handle_enumerator.h (right):

http://codereview.chromium.org/7888024/diff/8001/content/common/handle_enumer...
content/common/handle_enumerator.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: you should "svn mv" these files so that file history is not lost

http://codereview.chromium.org/7888024/diff/8001/content/content_common.gypi
File content/content_common.gypi (right):

http://codereview.chromium.org/7888024/diff/8001/content/content_common.gypi#...
content/content_common.gypi:263: 'common/handle_enumerator.cc',
how we normally handle this (which is wrong for sandbox_policy ), is that we add
a suffix to indicate which os the file is for. so these files should be
handle_enumerator_win.cc/h, and then gyp will automatically exclude them from
non-windows builds

[Cris Neckar, 2011-09-20 01:22:15]

Thanks!

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:567: #if
defined(OS_WIN)
On 2011/09/19 17:10:48, John Abd-El-Malek wrote:
> nit: this really isn't needed, no?

Done.

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:889: // When no
other owners of this object, we can delete ourselves.
On 2011/09/19 17:10:48, John Abd-El-Malek wrote:
> you don't want to duplicate this code from RenderProcessHost, since this is
> fragile code and if someone changes it in RPH, they won't know about here. so
> move the code in RPH to a function that you can call.
> 
> as an example, you're currently not calling all_hosts.Remove(id()), which will
> lead to crashes when someone iterates through the RPHs.

Done.

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
File content/browser/renderer_host/mock_render_process_host.cc (right):

http://codereview.chromium.org/7888024/diff/8001/content/browser/renderer_hos...
content/browser/renderer_host/mock_render_process_host.cc:78: return;
On 2011/09/19 17:10:48, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/7888024/diff/8001/content/common/child_process...
File content/common/child_process_messages.h (right):

http://codereview.chromium.org/7888024/diff/8001/content/common/child_process...
content/common/child_process_messages.h:43:
IPC_MESSAGE_CONTROL0(ChildProcessHostMsg_DumpHandlesDone)
On 2011/09/19 17:10:48, John Abd-El-Malek wrote:
> ChildProcessHostMsg messages are at the bottom of the file

Done.

http://codereview.chromium.org/7888024/diff/8001/content/common/handle_enumer...
File content/common/handle_enumerator.h (right):

http://codereview.chromium.org/7888024/diff/8001/content/common/handle_enumer...
content/common/handle_enumerator.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/09/19 17:10:48, John Abd-El-Malek wrote:
> nit: you should "svn mv" these files so that file history is not lost

Done.

http://codereview.chromium.org/7888024/diff/8001/content/content_common.gypi
File content/content_common.gypi (right):

http://codereview.chromium.org/7888024/diff/8001/content/content_common.gypi#...
content/content_common.gypi:263: 'common/handle_enumerator.cc',
On 2011/09/19 17:10:48, John Abd-El-Malek wrote:
> how we normally handle this (which is wrong for sandbox_policy ), is that we
add
> a suffix to indicate which os the file is for. so these files should be
> handle_enumerator_win.cc/h, and then gyp will automatically exclude them from
> non-windows builds

Done.

[jam, 2011-09-20 02:49:48]

it's time-consuming to see how you addressed the comments when you sync+address
them in the same patchset. would it be possible to upload your changes at the
same revision as patchset 2?

[jam, 2011-09-20 02:59:05]

ok nm about rebasing, but please keep that in mind for the future. lgtm with
renaming the header as well

http://codereview.chromium.org/7888024/diff/20005/content/common/handle_enume...
File content/common/handle_enumerator.h (right):

http://codereview.chromium.org/7888024/diff/20005/content/common/handle_enume...
content/common/handle_enumerator.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
why isn't thisi handle_enumerator_win.h as well?

[Cris Neckar, 2011-09-20 17:32:59]

Sorry about the rebasing, I completely forgot I had synced. 

Thank you for the review!

On 2011/09/20 02:59:05, John Abd-El-Malek wrote:
> ok nm about rebasing, but please keep that in mind for the future. lgtm with
> renaming the header as well
> 
>
http://codereview.chromium.org/7888024/diff/20005/content/common/handle_enume...
> File content/common/handle_enumerator.h (right):
> 
>
http://codereview.chromium.org/7888024/diff/20005/content/common/handle_enume...
> content/common/handle_enumerator.h:1: // Copyright (c) 2011 The Chromium
> Authors. All rights reserved.
> why isn't thisi handle_enumerator_win.h as well?

[cpu_(ooo_6.6-7.5), 2011-09-20 18:04:17]

lgtm on the enumerator code. I defer to John for the rest.