base: refactor LaunchApp variants into a single function

base: refactor LaunchApp variants into a single function

base provides a bunch of process-spawning functions that only differ by one
or two arguments; worse, they're all backed by the same implementation anyway.
Unify all the functions into one LaunchProcess() function, which takes a
struct containing a bunch of optional arguments.

For this change, I'm trying to not change the base API.  Follow up changes
can fix callers.  Because of this, I made temporary shims for all of the old
API.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91220

[Evan Martin, 2011-06-29 21:51:26]

I'm not sure who a good reviewer for this is, but Mark is probably a safe bet.

http://codereview.chromium.org/7284018
is an example of the sort of clean-up change I'd make with this cleanup in
place.

[Mark Mentovai, 2011-06-29 22:11:28]

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode203
base/process_util.h:203: //       that it doesn't leak!
And if NULL, what happens? Does the launcher wait for it, or does it leak
regardless? Not clear based on the comment.

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode224
base/process_util.h:224: // If non-NULL, set/unset environment variables.
What does it mean to “unset”? How would you “unset?” I thought “environ” would
just be used as a replacement environment.

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode229
base/process_util.h:229: const file_handle_mapping_vector* fds_to_remap;
Seems like this can just be a file_handle_mapping_vector, no *.

I may have a similar comment for |environ| depending on what you say.

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode237
base/process_util.h:237: LaunchOptions() : wait(false), process_handle(NULL),
Can you move the constructor up to the top as is ordinary in our code?

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode251
base/process_util.h:251: // - Before launching all FDs open in the parent
process will be marked as
launching comma all. Because the first time I read this, I thought you were
going to launch the FDs.

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode253
base/process_util.h:253: // - If the first argument on the command line is not
fully specified,
“fully specified” means what, exactly? No slashes in it? Absolute path?

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode335
base/process_util.h:335: // instead of a CommandLine.  Prefer the CommandLine
version if possible.
Doesn’t the CommandLine version F the argument order? I hate that. I hate it
enough to not encourage its use unless you already have a CommandLine in your
hands. Same comment applies to the Windows thing above.

I hit this last week. Using CommandLine, all of my “loose” arguments got moved
to the end of the vector. That’s OK for how Chromium parses arguments, but not
everything we launch (or will launch) is Chromium. It was scary enough to turn
me off of the CommandLine stuff (again).

Be careful in this change and subsequent cleanups that you don’t force anyone
who was using a vector of strings into the CommandLine path.

[Evan Martin, 2011-06-29 23:09:26]

I fixed all of your comments except for the pointer ones.

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode203
base/process_util.h:203: //       that it doesn't leak!
On 2011/06/29 22:11:28, Mark Mentovai wrote:
> And if NULL, what happens? Does the launcher wait for it, or does it leak
> regardless? Not clear based on the comment.

Copy'n'paste from the previous code.  Will clarify.

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode224
base/process_util.h:224: // If non-NULL, set/unset environment variables.
On 2011/06/29 22:11:28, Mark Mentovai wrote:
> What does it mean to “unset”? How would you “unset?” I thought “environ” would
> just be used as a replacement environment.

To be honest, I didn't fully understand it myself.  Updated with a comment
(which points down to ReplaceEnv).

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode229
base/process_util.h:229: const file_handle_mapping_vector* fds_to_remap;
On 2011/06/29 22:11:28, Mark Mentovai wrote:
> Seems like this can just be a file_handle_mapping_vector, no *.
> 
> I may have a similar comment for |environ| depending on what you say.

Hm, yeah.  When I first wrote this I was designing it such that you could
memset() the whole struct to zero to get the default values.  But now that I've
typed that out I have no idea why I thought that was desirable; clearly I've
been writing too much Go code lately.  :(

...edit: oh, there actually is a good reason -- the callers of the old LaunchApp
functions pass in a const ref to these objects, and I want to pass them along
without making a copy of them into this struct.  Do you think that's premature
optimization?

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode253
base/process_util.h:253: // - If the first argument on the command line is not
fully specified,
On 2011/06/29 22:11:28, Mark Mentovai wrote:
> “fully specified” means what, exactly? No slashes in it? Absolute path?

Copy'n'paste from the previous code.  Will clarify.

http://codereview.chromium.org/7283019/diff/1004/base/process_util.h#newcode335
base/process_util.h:335: // instead of a CommandLine.  Prefer the CommandLine
version if possible.
On 2011/06/29 22:11:28, Mark Mentovai wrote:
> Doesn’t the CommandLine version F the argument order? I hate that. I hate it
> enough to not encourage its use unless you already have a CommandLine in your
> hands. Same comment applies to the Windows thing above.
> 
> I hit this last week. Using CommandLine, all of my “loose” arguments got moved
> to the end of the vector. That’s OK for how Chromium parses arguments, but not
> everything we launch (or will launch) is Chromium. It was scary enough to turn
> me off of the CommandLine stuff (again).
> 
> Be careful in this change and subsequent cleanups that you don’t force anyone
> who was using a vector of strings into the CommandLine path.

I think CommandLine() is only useful for launching Chrome itself, since it also
contains our own assumptions about how arguments (particularly switches) will be
parsed.  I will update the comment.

[Mark Mentovai, 2011-06-30 01:04:04]

I do like this change. LGTM with this stuff all fixed.

http://codereview.chromium.org/7283019/diff/6001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/7283019/diff/6001/base/process_util.h#newcode237
base/process_util.h:237: // See documentation of AlterEnvironment().
OK. I don’t like this interface because it provides no way to set an environment
variable to “present but empty,” but that’s an existing problem, and them’s the
breaks.

http://codereview.chromium.org/7283019/diff/6001/base/process_util.h#newcode238
base/process_util.h:238: const environment_vector* environ;
It’s OK to keep the pointers, but maybe add a comment saying that this struct
doesn’t “own” either of them to make the memory model a tiny bit more obvious.

http://codereview.chromium.org/7283019/diff/6001/base/process_util.h#newcode257
base/process_util.h:257: // - If the first argument on the command line is not
fully specified,
You said you’d clarify this, but it’s still ambiguous.

http://codereview.chromium.org/7283019/diff/6001/base/process_util.h#newcode286
base/process_util.h:286: BASE_API bool LaunchProcess(const string16& cmdline,
This is platform-specific code, isn’t it tolerable (and maybe even more correct)
to call this a wstring instead of a string16?

[brettw, 2011-06-30 05:36:25]

http://codereview.chromium.org/7283019/diff/6001/base/process_util.h
File base/process_util.h (right):

http://codereview.chromium.org/7283019/diff/6001/base/process_util.h#newcode286
base/process_util.h:286: BASE_API bool LaunchProcess(const string16& cmdline,
I prefer string16. I think we should be trying to get 0 wstring in the codebase.
Otherwise, you'll always have some mismatch when calling from
platform-independent code to Windows code. Althoug string16 == wstring, I think
we should avoid making this assumption unless necessary.

[Evan Martin, 2011-06-30 16:52:32]

Please see "delta from patch set 6" for the comment updates.  I'll run through
the trybots now.

[Mark Mentovai, 2011-06-30 16:54:40]

LGTM