Remove base::WaitForExitCode*

Remove base::WaitForExitCode*

BUG=417532
Committed: https://crrev.com/85728971a76208ff2de87d9b5bb625dfd0bd9812
Cr-Commit-Position: refs/heads/master@{#318939}

[rvargas (doing something else), 2015-02-26 20:36:37]

Patchset #1 (id:1) has been deleted

[rvargas (doing something else), 2015-02-26 20:36:43]

Patchset #1 (id:20001) has been deleted

[rvargas (doing something else), 2015-02-26 21:44:14]

Patchset #1 (id:40001) has been deleted

[rvargas (doing something else), 2015-02-26 21:47:11]

rvargas@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-02-26 22:03:26]

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h
File base/process/kill.h (right):

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h#newc...
base/process/kill.h:142: // because the implementation is closely related to
other functions declared
Why does this argument make sense for posix but not for win?

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h#newc...
base/process/kill.h:144: // This is not meant for consumers outside of
base::Process.
Can you still keep the whole comment that was on this function describing its
functionality before?

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h#newc...
base/process/kill.h:148: static const int kNoTimeout = -1;
maybe you can scope this constant name a bit more.. kWaitForExitCodeNoTimeout or
something?

[rvargas (doing something else), 2015-02-27 01:54:18]

Thanks!

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h
File base/process/kill.h (right):

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h#newc...
base/process/kill.h:142: // because the implementation is closely related to
other functions declared
On 2015/02/26 22:03:25, danakj wrote:
> Why does this argument make sense for posix but not for win?

Because the windows implementation has nothing to do with other functions
implemented in kill_xx.cc.

Posix, on the other hand, relies on WaitForSingleNonChildProcess (although I
could move that too) and WaitpidWithTimeout (I could move that too),
!defined(OS_NACL_NONSFI) (could add that too), and it is somewhat similar to
other complex code like KillProcess() (and GetKnownDeadTerminationStatus?).

I don't want to split all that code so that it is less apparent that we have
multiple functions that do almost the same but not really.

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h#newc...
base/process/kill.h:144: // This is not meant for consumers outside of
base::Process.
On 2015/02/26 22:03:25, danakj wrote:
> Can you still keep the whole comment that was on this function describing its
> functionality before?

Some of that text is now irrelevant (the part about closing the handle). The
documentation is basically the same comment of Process::WaitForExitWithTimeout
so I thought it is better to point that out than to copy/paste a comment from
somewhere else, especially because the intention is to have a single user for
this function (not an API that is really meant to be used by anyone else). BTW,
the most up to date comment lives on process.h

What if I rename it to WaitForExitWithTimeoutImpl ? It is pretty standard not to
explain in detail what WxxImpl does. (done)

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h#newc...
base/process/kill.h:148: static const int kNoTimeout = -1;
On 2015/02/26 22:03:25, danakj wrote:
> maybe you can scope this constant name a bit more.. kWaitForExitCodeNoTimeout
or
> something?

Removed.

[danakj, 2015-02-27 18:47:02]

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h
File base/process/kill.h (right):

https://codereview.chromium.org/957183002/diff/60001/base/process/kill.h#newc...
base/process/kill.h:142: // because the implementation is closely related to
other functions declared
On 2015/02/27 01:54:18, rvargas wrote:
> On 2015/02/26 22:03:25, danakj wrote:
> > Why does this argument make sense for posix but not for win?
> 
> Because the windows implementation has nothing to do with other functions
> implemented in kill_xx.cc.
> 
> Posix, on the other hand, relies on WaitForSingleNonChildProcess (although I
> could move that too) and WaitpidWithTimeout (I could move that too),
> !defined(OS_NACL_NONSFI) (could add that too), and it is somewhat similar to
> other complex code like KillProcess() (and GetKnownDeadTerminationStatus?).

Granted the posix code is larger, with some helper functions, but I don't think
that's a reason to leave it in the wrong place.

kill_win.cc has a KillProcess() with similar code to
Process::WaitForExitWithTimeout in process_win.cc also, so this seems
inconsistent still to me.

> I don't want to split all that code so that it is less apparent that we have
> multiple functions that do almost the same but not really.

I think we should do the same thing with posix and win here. I'll be happiest
then. I lean slightly toward moving them both to process_foo because then we
don't have to add "Don't use this" type functions to header files, and maybe
comments could be added to point out similarities if that really makes sense?

[rvargas (doing something else), 2015-02-27 23:10:23]

Patchset #3 (id:100001) has been deleted

[rvargas (doing something else), 2015-02-28 00:28:45]

Patchset #3 (id:120001) has been deleted

[rvargas (doing something else), 2015-02-28 03:02:10]

Patchset #3 (id:140001) has been deleted

[rvargas (doing something else), 2015-02-28 03:02:36]

Patchset #3 (id:140001) has been deleted

[rvargas (doing something else), 2015-03-02 18:59:53]

ptal

[danakj, 2015-03-03 18:45:16]

LGTM

[rvargas (doing something else), 2015-03-03 19:10:34]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-03-03 19:11:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/957183002/160001

[commit-bot: I haz the power, 2015-03-03 20:46:29]

Committed patchset #3 (id:160001)

[commit-bot: I haz the power, 2015-03-03 20:47:28]

Patchset 3 (id:??) landed as
https://crrev.com/85728971a76208ff2de87d9b5bb625dfd0bd9812
Cr-Commit-Position: refs/heads/master@{#318939}