Issue 999033003: Remove uses of KillProcess()

Issue 999033003: Remove uses of KillProcess() (Closed)

Created:
5 years, 9 months ago by rvargas (doing something else)

Modified:
5 years, 9 months ago

Reviewers:
cpu_(ooo_6.6-7.5)

CC:
chromium-reviews, tim+watch_chromium.org, extensions-reviews_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, zea+watch_chromium.org, pvalenzuela+watch_chromium.org, posciak+watch_chromium.org, samuong+watch_chromium.org, maxbogue+watch_chromium.org, jam, mcasas+watch_chromium.org, cbentzel+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, chromoting-reviews_chromium.org, mkwst+moarreviews-renderer_chromium.org, chromium-apps-reviews_chromium.org, stgao, wjia+watch_chromium.org, maniscalco+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Remove uses of KillProcess() BUG=417532 Committed: https://crrev.com/486b2f5612c0f9711d6fced550d7ed904fea4319 Cr-Commit-Position: refs/heads/master@{#321045}

Patch Set 1 : #

Total comments: 7

Created: 5 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+61 lines, -66 lines)			Patch
M	chrome/browser/extensions/api/messaging/native_process_launcher_win.cc	View	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/lifetime/application_lifetime.cc	View	2 chunks	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc	View	1 chunk	+3 lines, -3 lines	0 comments	Download
M	chrome/browser/media/test_license_server.cc	View	2 chunks	+1 line, -3 lines	0 comments	Download
M	chrome/browser/process_info_snapshot_mac_unittest.cc	View	2 chunks	+2 lines, -3 lines	0 comments	Download
M	chrome/browser/process_singleton_browsertest.cc	View	5 chunks	+5 lines, -6 lines	2 comments	Download
M	chrome/browser/process_singleton_win.cc	View	2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/browser/sync/test/integration/sync_test.cc	View	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/service/service_utility_process_host.cc	View	2 chunks	+1 line, -3 lines	0 comments	Download
M	chrome/test/base/chrome_process_util.cc	View	2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/test/chromedriver/chrome/chrome_desktop_impl.cc	View	4 chunks	+8 lines, -8 lines	0 comments	Download
M	chrome/test/chromedriver/chrome_launcher.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	cloud_print/service/win/chrome_launcher.cc	View	7 chunks	+18 lines, -12 lines	0 comments	Download
M	components/storage_monitor/storage_monitor_linux.cc	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	content/browser/browser_child_process_host_impl.cc	View	1 chunk	+1 line, -2 lines	0 comments	Download
M	content/child/child_thread_impl.cc	View	2 chunks	+2 lines, -2 lines	2 comments	Download
M	content/plugin/plugin_thread.cc	View	3 chunks	+3 lines, -4 lines	1 comment	Download
M	content/renderer/render_frame_impl.cc	View	2 chunks	+1 line, -2 lines	0 comments	Download
M	ipc/ipc_channel_posix_unittest.cc	View	2 chunks	+2 lines, -2 lines	0 comments	Download
M	net/test/spawned_test_server/local_test_server.cc	View	2 chunks	+1 line, -2 lines	2 comments	Download
M	remoting/host/setup/daemon_controller_delegate_linux.cc	View	2 chunks	+2 lines, -3 lines	0 comments	Download

Messages

Total messages: 10 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

cpu_(ooo_6.6-7.5)

lgtm https://codereview.chromium.org/999033003/diff/40001/chrome/browser/process_singleton_browsertest.cc File chrome/browser/process_singleton_browsertest.cc (right): https://codereview.chromium.org/999033003/diff/40001/chrome/browser/process_singleton_browsertest.cc#newcode182 chrome/browser/process_singleton_browsertest.cc:182: EXPECT_TRUE(process.Terminate(kExitCode, true /* wait */)); woa, Terminate is ...

5 years, 9 months ago (2015-03-17 20:36:32 UTC) #5

rvargas (doing something else)

Thanks https://codereview.chromium.org/999033003/diff/40001/chrome/browser/process_singleton_browsertest.cc File chrome/browser/process_singleton_browsertest.cc (right): https://codereview.chromium.org/999033003/diff/40001/chrome/browser/process_singleton_browsertest.cc#newcode182 chrome/browser/process_singleton_browsertest.cc:182: EXPECT_TRUE(process.Terminate(kExitCode, true /* wait */)); On 2015/03/17 20:36:31, ...

5 years, 9 months ago (2015-03-17 22:40:53 UTC) #6

Thanks

https://codereview.chromium.org/999033003/diff/40001/chrome/browser/process_s...
File chrome/browser/process_singleton_browsertest.cc (right):

https://codereview.chromium.org/999033003/diff/40001/chrome/browser/process_s...
chrome/browser/process_singleton_browsertest.cc:182:
EXPECT_TRUE(process.Terminate(kExitCode, true /* wait */));
On 2015/03/17 20:36:31, cpu wrote:
> woa, Terminate is a const method?

Yes. I added const when adding the second argument (last cl?) to enable this use
case (a couple of cases in this CL).

The alternative was to pass around pointers to Process, and that is less clear,
IMO.

On the other hand, given that a caller can always do Handle() (of a const
object) and then use that handle to kill the process (for example), it seems
fine. In other words, don't worry about the underlying effects of a method and
instead treat it as "modify my members or not" when thinking about const :)

https://codereview.chromium.org/999033003/diff/40001/content/child/child_thre...
File content/child/child_thread_impl.cc (right):

https://codereview.chromium.org/999033003/diff/40001/content/child/child_thre...
content/child/child_thread_impl.cc:718: 
On 2015/03/17 20:36:31, cpu wrote:
> ensure connected --> kills
> ensure dead --> connects ?

That sounds reasonable!.

(I don't really know what this code is doing... it is called through a
PostDelayedTask from Init... looks like if a timeout hits we just give up on
living)

(will rename to anything you want me to).

https://codereview.chromium.org/999033003/diff/40001/net/test/spawned_test_se...
File net/test/spawned_test_server/local_test_server.cc (right):

https://codereview.chromium.org/999033003/diff/40001/net/test/spawned_test_se...
net/test/spawned_test_server/local_test_server.cc:130: ret =
process_.Terminate(1, true);
On 2015/03/17 20:36:31, cpu wrote:
> seems buggy, there is a explicit timeout above so I think they want |false|
> here.

The timeout above is 0, so it's not waiting.

commit-bot: I haz the power

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

5 years, 9 months ago (2015-03-17 23:31:55 UTC) #8

commit-bot: I haz the power

5 years, 9 months ago (2015-03-18 01:37:17 UTC) #10

Message was sent while issue was closed.

Patchset 1 (id:??) landed as
https://crrev.com/486b2f5612c0f9711d6fced550d7ed904fea4319
Cr-Commit-Position: refs/heads/master@{#321045}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages