Remove base::WaitForSingleProcess

Remove base::WaitForSingleProcess

BUG=417532
Committed: https://crrev.com/2f70a1515d5f3e6ea3525c079743bfbcb0818b98
Cr-Commit-Position: refs/heads/master@{#317690}

[rvargas (doing something else), 2015-02-18 02:35:14]

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

[rvargas (doing something else), 2015-02-18 02:35:15]

Lei, PTAL

Note that I'm implying that all POSIX callers of WaitForExit* should have been
able to use the old WaitForSingleProcess.

[Lei Zhang, 2015-02-18 02:49:45]

https://codereview.chromium.org/938453002/diff/20001/base/process/kill_win.cc
File base/process/kill_win.cc (left):

https://codereview.chromium.org/938453002/diff/20001/base/process/kill_win.cc...
base/process/kill_win.cc:229: return WaitForExitCodeWithTimeout(handle,
&exit_code, wait) && exit_code == 0;
On Windows, we used to check the exit code, but with
WaitForExitCodeWithTimeout(), we just pass it back to the caller, which may not
always check the code. Does this matter?

[Lei Zhang, 2015-02-18 03:07:43]

Otherwise looking good.

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:147: base::TimeDelta::FromSeconds(5), &exit_code);
nit: Indentation looks funny. Shouldn't it be 4 spaces from where "bool" starts?
Ditto in service_process_control_browsertest.cc.

[rvargas (doing something else), 2015-02-18 23:17:57]

rvargas@chromium.org changed reviewers:
+ kalman@chromium.org, tsepez@chromium.org, ttuttle@chromium.org

[rvargas (doing something else), 2015-02-18 23:17:58]

Thanks,

+kalman -> apps
+tsepez -> ipc
+ttuttle -> net

https://codereview.chromium.org/938453002/diff/20001/base/process/kill_win.cc
File base/process/kill_win.cc (left):

https://codereview.chromium.org/938453002/diff/20001/base/process/kill_win.cc...
base/process/kill_win.cc:229: return WaitForExitCodeWithTimeout(handle,
&exit_code, wait) && exit_code == 0;
On 2015/02/18 02:49:45, Lei Zhang wrote:
> On Windows, we used to check the exit code, but with
> WaitForExitCodeWithTimeout(), we just pass it back to the caller, which may
not
> always check the code. Does this matter?

I don't think it matters. I added EXPECT_EQ(0, exit_code) to some of the tests.

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:147: base::TimeDelta::FromSeconds(5), &exit_code);
On 2015/02/18 03:07:43, Lei Zhang (Soon to be OOO) wrote:
> nit: Indentation looks funny. Shouldn't it be 4 spaces from where "bool"
starts?
> Ditto in service_process_control_browsertest.cc.

Maybe? I actually prefer this (since it was taught to me on my readability
review) because (tldr):

SomethingLong(with_some_long_args,
              is_indented_here_and_not_under_thing

something = sequence_of_foo +
            some_more_bar

Which means that there are points of "natural" indentation. The four spaces rule
should not go beyond those points unless there is not enough space even after
moving to that extra line (so in fact should be "4 spaces from that point").

something = SomethingExtraLong(
                with_many_long_args) +
            SomethingElse()

is easier to parse (even if the whole thing has to be moved to the next line),
versus:

something =
    SomethingExtraLong(
    with_many_long_args) + <----
    SomethingElse()

"with_many_long_args" really needs more indentation, not just 4 from start, and
in one case it is just using the same principles while with the other
interpretation it just comes out of nowhere.

[not at google - send to devlin, 2015-02-18 23:19:37]

apps lgtm

[Lei Zhang, 2015-02-18 23:48:31]

lgtm

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:147: base::TimeDelta::FromSeconds(5), &exit_code);
On 2015/02/18 23:17:58, rvargas wrote:
> On 2015/02/18 03:07:43, Lei Zhang (Soon to be OOO) wrote:
> > nit: Indentation looks funny. Shouldn't it be 4 spaces from where "bool"
> starts?
> > Ditto in service_process_control_browsertest.cc.
> 
> Maybe? I actually prefer this (since it was taught to me on my readability
> review) because (tldr):
> 
> SomethingLong(with_some_long_args,
>               is_indented_here_and_not_under_thing
> 
> something = sequence_of_foo +
>             some_more_bar
> 
> Which means that there are points of "natural" indentation. The four spaces
rule
> should not go beyond those points unless there is not enough space even after
> moving to that extra line (so in fact should be "4 spaces from that point").
> 
> something = SomethingExtraLong(
>                 with_many_long_args) +
>             SomethingElse()
> 
> is easier to parse (even if the whole thing has to be moved to the next line),
> versus:
> 
> something =
>     SomethingExtraLong(
>     with_many_long_args) + <----
>     SomethingElse()
> 
> "with_many_long_args" really needs more indentation, not just 4 from start,
and
> in one case it is just using the same principles while with the other
> interpretation it just comes out of nowhere.

I haven't seen much code written this way though. See lines 81 / 90 for
instance. Maybe just clang-format it all and call it a day?

[Tom Sepez, 2015-02-18 23:49:53]

LGTM % comment.

https://codereview.chromium.org/938453002/diff/40001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/938453002/diff/40001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:145: int exit_code;
do we want to set exit_code to some impossible value and then EXPECT that
WaitForExitWithTimeout set it to something reasonable?

[rvargas (doing something else), 2015-02-19 02:00:29]

New patchsets have been uploaded after l-g-t-m from
kalman@chromium.org,thestig@chromium.org,tsepez@chromium.org

[rvargas (doing something else), 2015-02-19 02:01:04]

Thanks

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/938453002/diff/20001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:147: base::TimeDelta::FromSeconds(5), &exit_code);
On 2015/02/18 23:48:31, Lei Zhang (Soon to be OOO) wrote:
> On 2015/02/18 23:17:58, rvargas wrote:
> > On 2015/02/18 03:07:43, Lei Zhang (Soon to be OOO) wrote:
> > > nit: Indentation looks funny. Shouldn't it be 4 spaces from where "bool"
> > starts?
> > > Ditto in service_process_control_browsertest.cc.
> > 
> > Maybe? I actually prefer this (since it was taught to me on my readability
> > review) because (tldr):
> > 
> > SomethingLong(with_some_long_args,
> >               is_indented_here_and_not_under_thing
> > 
> > something = sequence_of_foo +
> >             some_more_bar
> > 
> > Which means that there are points of "natural" indentation. The four spaces
> rule
> > should not go beyond those points unless there is not enough space even
after
> > moving to that extra line (so in fact should be "4 spaces from that point").
> > 
> > something = SomethingExtraLong(
> >                 with_many_long_args) +
> >             SomethingElse()
> > 
> > is easier to parse (even if the whole thing has to be moved to the next
line),
> > versus:
> > 
> > something =
> >     SomethingExtraLong(
> >     with_many_long_args) + <----
> >     SomethingElse()
> > 
> > "with_many_long_args" really needs more indentation, not just 4 from start,
> and
> > in one case it is just using the same principles while with the other
> > interpretation it just comes out of nowhere.
> 
> I haven't seen much code written this way though. See lines 81 / 90 for
> instance. Maybe just clang-format it all and call it a day?

I cringe every time I see code like that :(. But I agree that it is a common
pattern in parts of the code. I have also seen clang-format destroy better
formatting in the name of easier-to-program rules, which also makes me sad.

clang-format seems inconsistent (buggy?) to me in that it generates:
  bool rv = kjhkd +
            client_process_.WaitForExitWithTimeout(
                base::TimeDelta::FromSeconds(5), &exit_code);

but without the first term it insist in sending the arguments beyond rv. I filed
bug 459823 about it.

I would be happy with defaulting to clang-format if the style guide were
replaced with "use whatever the tool does".

For now there's not much point in arguing about it.

https://codereview.chromium.org/938453002/diff/40001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/938453002/diff/40001/ipc/ipc_test_base.cc#new...
ipc/ipc_test_base.cc:145: int exit_code;
On 2015/02/18 23:49:53, Tom Sepez wrote:
> do we want to set exit_code to some impossible value and then EXPECT that
> WaitForExitWithTimeout set it to something reasonable?

I have no idea what an impossible (or reasonable) value would be.

I can EXPECT that to be 0 if you want me to, and see if the bots are happy with
that... I just don't see how this layer can predict what the exit code would be
or how a zero exit code (or anything) is implied by the contract of
WaitForClientShutdown()

Let me know what you want me to do.

[rvargas (doing something else), 2015-02-21 01:31:51]

ttuttle: ping

[rvargas (doing something else), 2015-02-23 22:52:53]

rvargas@chromium.org changed reviewers:
+ rtenneti@chromium.org
- ttuttle@chromium.org

[rvargas (doing something else), 2015-02-23 22:52:53]

-ttuttle
+rtenneti for net

[ramant (doing other things), 2015-02-23 23:02:44]

net/ code LGTM

[rvargas (doing something else), 2015-02-23 23:07:32]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-02-23 23:08:24]

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

[commit-bot: I haz the power, 2015-02-24 00:29:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-24 00:30:08]

Patchset 4 (id:??) landed as
https://crrev.com/2f70a1515d5f3e6ea3525c079743bfbcb0818b98
Cr-Commit-Position: refs/heads/master@{#317690}