Remove base::WaitForExitCode*

base/process/kill.h

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file contains routines to kill processes and get the exit code and
 // termination status.
 
 #ifndef BASE_PROCESS_KILL_H_
 #define BASE_PROCESS_KILL_H_
 
@@ skipping 76 matching lines @@
 // termination status.
 //
 // Note that it is not an option to call WaitForExitCode and then
 // GetTerminationStatus as the child will be reaped when WaitForExitCode
 // returns, and this information will be lost.
 //
 BASE_EXPORT TerminationStatus GetKnownDeadTerminationStatus(
     ProcessHandle handle, int* exit_code);
 #endif  // defined(OS_POSIX)
 
-// Waits for process to exit. On POSIX systems, if the process hasn't been
-// signaled then puts the exit code in |exit_code|; otherwise it's considered
-// a failure. On Windows |exit_code| is always filled. Returns true on success,
-// and closes |handle| in any case.
-BASE_EXPORT bool WaitForExitCode(ProcessHandle handle, int* exit_code);
-
-// Waits for process to exit. If it did exit within |timeout_milliseconds|,
-// then puts the exit code in |exit_code|, and returns true.
-// In POSIX systems, if the process has been signaled then |exit_code| is set
-// to -1. Returns false on failure (the caller is then responsible for closing
-// |handle|).
-// The caller is always responsible for closing the |handle|.
-BASE_EXPORT bool WaitForExitCodeWithTimeout(ProcessHandle handle,
-                                            int* exit_code,
-                                            base::TimeDelta timeout);
-
 // Wait for all the processes based on the named executable to exit.  If filter
 // is non-null, then only processes selected by the filter are waited on.
 // Returns after all processes have exited or wait_milliseconds have expired.
 // Returns true if all the processes exited, false otherwise.
 BASE_EXPORT bool WaitForProcessesToExit(
     const FilePath::StringType& executable_name,
     base::TimeDelta wait,
     const ProcessFilter* filter);
 
 // Waits a certain amount of time (can be 0) for all the processes with a given
@@ skipping 22 matching lines @@
 // SYNCHRONIZE permissions.
 //
 BASE_EXPORT void EnsureProcessTerminated(Process process);
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
 // The nicer version of EnsureProcessTerminated() that is patient and will
 // wait for |pid| to finish and then reap it.
 BASE_EXPORT void EnsureProcessGetsReaped(ProcessId pid);
 #endif
 
+#if defined(OS_POSIX)
+namespace internal {
+// Implements Process::WaitForExitWithTimeout for POSIX. It lives in this file
+// because the implementation is closely related to other functions declared

[danakj, 2015/02/26 22:03:25]

Why does this argument make sense for posix but not for win?

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

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.

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

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 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?

+// here.
+// This is not meant for consumers outside of base::Process.

[danakj, 2015/02/26 22:03:25]

Can you still keep the whole comment that was on this function describing its
functionality before?

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

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)

+bool WaitForExitCodeWithTimeout(ProcessHandle handle,
+                                int* exit_code,
+                                TimeDelta timeout);
+static const int kNoTimeout = -1;

[danakj, 2015/02/26 22:03:25]

maybe you can scope this constant name a bit more.. kWaitForExitCodeNoTimeout or
something?

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

Removed. 

+}  // namespace internal
+#endif  // defined(OS_POSIX)
+
 }  // namespace base
 
 #endif  // BASE_PROCESS_KILL_H_