Get render_process_host to build on Linux by butchering a bunch of files.

Get render_process_host to build on Linux by butchering a bunch of files.

[this will need to be rebased after agl finishes his work on SyncChannel and ObjectWatcher]

[Evan Martin, 2008-12-17 06:36:22]

Here's my plan: it'll be really hard to get all this biulding at the same time. 
So this gets this one file compiling by touching a bunch of other files though
not all of them.

I can check this in without the scons change (and some other commented-out
bits), getting us closer to the goal, even though it won't really change the
build yet.  But the next person who tries to port this file will find there's
only a few more headers that need to be fixed up.

http://codereview.chromium.org/14504/diff/1/2
File chrome/browser/browser.scons (right):

http://codereview.chromium.org/14504/diff/1/2#newcode88
Line 88: 'render_process_host.cc',
won't submit this

http://codereview.chromium.org/14504/diff/1/5
File chrome/browser/render_process_host.cc (right):

http://codereview.chromium.org/14504/diff/1/5#newcode34
Line 34: // #include "chrome/browser/browser.h"
won't submit these commented-out headers.

http://codereview.chromium.org/14504/diff/1/10
File chrome/browser/visitedlink_master.h (right):

http://codereview.chromium.org/14504/diff/1/10#newcode8
Line 8: // #include <windows.h>
won't submit this

http://codereview.chromium.org/14504/diff/1/12
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/14504/diff/1/12#newcode196
Line 196: // TODO(port): fail to compile here if sizeof(long*) != 4.
we have some sort of compile-time assert, right?

it's unclear to me why we even attempt to serialize void*, but the compiler
error was most unhelpful (just pointed me at the last line of
render_process_host.cc).  i suspect it's that we typedef something (maybe
HANDLE?) to void* somewhere -- need to track that down.

[agl, 2008-12-17 16:56:05]

http://codereview.chromium.org/14504/diff/1/4
File chrome/browser/plugin_service.h (right):

http://codereview.chromium.org/14504/diff/1/4#newcode179
Line 179: return *this;
How did this ever work before?

http://codereview.chromium.org/14504/diff/1/5
File chrome/browser/render_process_host.cc (right):

http://codereview.chromium.org/14504/diff/1/5#newcode262
Line 262: std::wstring channel_id = L"set to quiet the compiler";
static const by the looks of things

http://codereview.chromium.org/14504/diff/1/6
File chrome/browser/render_process_host.h (right):

http://codereview.chromium.org/14504/diff/1/6#newcode85
Line 85: static void BadMessageTerminateProcess(uint16 msg_type,
base::ProcessHandle renderer);
Nit: 80 chars

http://codereview.chromium.org/14504/diff/1/12
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/14504/diff/1/12#newcode196
Line 196: // TODO(port): fail to compile here if sizeof(long*) != 4.
On 2008/12/17 06:36:22, Evan Martin wrote:
> we have some sort of compile-time assert, right?
> 
> it's unclear to me why we even attempt to serialize void*, but the compiler
> error was most unhelpful (just pointed me at the last line of
> render_process_host.cc).  i suspect it's that we typedef something (maybe
> HANDLE?) to void* somewhere -- need to track that down.

((void)sizeof(char[1 - 2*!(sizeof(long) == 4)]))

[jam, 2008-12-17 18:32:42]

http://codereview.chromium.org/14504/diff/1/13
File chrome/common/ipc_sync_channel.h (right):

http://codereview.chromium.org/14504/diff/1/13#newcode40
Line 40: #endif
You're going to eventually have to undo all the ifdefs that you added, since
RenderProcessHost depends on IPC::SyncChannel, and this won't function without
having a cross platform way to wait on events.  I suggest you port events +
ObjectWatcher first, then you won't need to ifdef a bunch of parts of this file
out...

[Evan Martin, 2008-12-17 18:37:06]

On 2008/12/17 18:32:42, John Abd-El-Malek wrote:
> http://codereview.chromium.org/14504/diff/1/13
> File chrome/common/ipc_sync_channel.h (right):
> 
> http://codereview.chromium.org/14504/diff/1/13#newcode40
> Line 40: #endif
> You're going to eventually have to undo all the ifdefs that you added, since
> RenderProcessHost depends on IPC::SyncChannel, and this won't function without
> having a cross platform way to wait on events.  I suggest you port events +
> ObjectWatcher first, then you won't need to ifdef a bunch of parts of this
file
> out...

Does ObjectWatcher have unit tests?

The first message I sent on this review (it's hidden now, you'll have to expand
it) explains my plan in this area, if that helps at all.  In short, it's hard to
pick what to port since everything is interdependent.

[Evan Martin, 2008-12-17 19:17:24]

I'm going to hold off on this until ObjectWatcher is ported.
Thanks for catching this, John!

[Evan Stade, 2008-12-17 19:51:07]

> http://codereview.chromium.org/14504/diff/1/4#newcode179
> Line 179: return *this;
> How did this ever work before?

the return value sets the value of the whole expression (e.g. "x = y"), so it's
not necessary if you never check the value. Although that doesn't really answer
why it still compiles with no return value, guess that's just a quirk of
operator overloading?

[Evan Martin, 2008-12-17 20:00:03]

> the return value sets the value of the whole expression (e.g. "x = y"), so
it's

Er, in what language?

[brettw, 2009-01-14 16:31:26]

http://codereview.chromium.org/14504/diff/1013/1017
File chrome/browser/render_process_host.cc (right):

http://codereview.chromium.org/14504/diff/1013/1017#newcode34
Line 34: // #include "chrome/browser/browser.h"
Remove these comments if they're not needed.

http://codereview.chromium.org/14504/diff/1013/1018
File chrome/browser/render_process_host.h (right):

http://codereview.chromium.org/14504/diff/1013/1018#newcode55
Line 55: public base::ObjectWatcher,
You seem to have lost the ::Delegate.

http://codereview.chromium.org/14504/diff/1013/1018#newcode84
Line 84: static void BadMessageTerminateProcess(uint16 msg_type,
base::ProcessHandle renderer);
80 cols.

http://codereview.chromium.org/14504/diff/1013/1021
File chrome/browser/visitedlink_master.h (right):

http://codereview.chromium.org/14504/diff/1013/1021#newcode8
Line 8: // #include <windows.h>
Remove this if it's not needed.

http://codereview.chromium.org/14504/diff/1013/1022
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/14504/diff/1013/1022#newcode212
Line 212: struct ParamTraits<void*> {
Out of curiosity, what is this used for?

[darin (slow to review), 2009-01-14 16:37:41]

http://codereview.chromium.org/14504/diff/1013/1017
File chrome/browser/render_process_host.cc (right):

http://codereview.chromium.org/14504/diff/1013/1017#newcode14
Line 14: #include <wininet.h>
i'm pretty certain that we no longer need to include wininet.h.  please remove
it.

something else probably already includes windows.h too ;-)

http://codereview.chromium.org/14504/diff/1013/1017#newcode370
Line 370: win_util::MessageBox(NULL,
how about moving this to a static helper function in this file?  that way this
code here can avoid #ifdef nastiness, and you can just make the helper function
be a no-op for now on non-OS_WIN platforms.

http://codereview.chromium.org/14504/diff/1013/1017#newcode378
Line 378: #if defined(OS_WIN)
there is some obvious refactoring needed here to isolate the sandbox code, eh?

http://codereview.chromium.org/14504/diff/1013/1017#newcode670
Line 670: // Request MAXIMUM_ALLOWED to match the access a handle
all of this could be hidden inside a base::ProcessWatcher class.

http://codereview.chromium.org/14504/diff/1013/1018
File chrome/browser/render_process_host.h (right):

http://codereview.chromium.org/14504/diff/1013/1018#newcode55
Line 55: public base::ObjectWatcher,
note: you shouldn't drop the "::Delegate" bit!

we should probably create a base::ProcessWatcher class that would be implemented
in terms of ObjectWatcher on Windows and something else on Linux and Mac.

[wtc, 2009-01-14 18:30:05]

http://codereview.chromium.org/14504/diff/1013/1017
File chrome/browser/render_process_host.cc (right):

http://codereview.chromium.org/14504/diff/1013/1017#newcode14
Line 14: #include <wininet.h>
On 2009/01/14 16:37:41, darin wrote:
> i'm pretty certain that we no longer need to include wininet.h.  please remove
> it.

I also found this <wininet.h> inclusion is unnecessary
when I removed the old WinHTTP code.  I have a local
change in my build tree that removes this <wininet.h>
inclusion.

> something else probably already includes windows.h too ;-)

I can take care of this, too.

[Evan Martin, 2009-01-16 23:09:41]

This has now been merged on top of Adam's syncchannel checkin and Ben's
render_process_host refactor.

The command-line handling stuff is still in review, but it's pretty close. 
Adding jrg for his reference.

[brettw, 2009-01-21 19:18:49]

http://codereview.chromium.org/14504/diff/1256/1034
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/14504/diff/1256/1034#newcode27
Line 27: // #include "chrome/browser/browser.h"
Be sure to remove these commented ones.

http://codereview.chromium.org/14504/diff/1256/1034#newcode27
Line 27: // #include "chrome/browser/browser.h"
Be sure to remove these commented-out ones.