Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6714044:
Remove g_spawn_async. Use base/process_utils.h instead. (Closed)
|
Created:
9 years, 9 months ago by Yusuke Sato Modified:
9 years, 7 months ago CC:
chromium-reviews, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove g_spawn_async. Use base/process_utils.h instead.
BUG=chromium-os:12718
TEST=manual, trybot
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78968
Patch Set 1 #
Total comments: 8
Patch Set 2 : review fixes #Messages
Total messages: 8 (0 generated)
Let's just use base/process_utils.h APIs to avoid the problem in glib...
On 2011/03/21 09:03:54, Yusuke Sato wrote: > Let's just use base/process_utils.h APIs to avoid the problem in glib... When I initially tried using process utils, I had some issues with ibus's subprocesses continuing to run as zombies. Could you verify that they shut down correctly with this patch?
http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... File chrome/browser/chromeos/cros/input_method_library.cc (right): http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:621: g_child_watch_add(base::GetProcId(handle), Perhaps we should add a version of this function to process utils, or add a TODO to switch if one is written? I think it was the lack of this function that initially caused me to use glib's function instead. http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:665: if (base::GetProcId(library->ibus_daemon_process_handle_) == pid) { Should we check that we have a handle before calling this, or is it safe with a null handle? http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:691: base::CloseProcessHandle(ibus_daemon_process_handle_); Shouldn't this already be called in OnImeShutdown, or do we need to call it twice?
http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... File chrome/browser/chromeos/cros/input_method_library.cc (right): http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:621: g_child_watch_add(base::GetProcId(handle), On 2011/03/21 10:43:52, Zachary Kuznia wrote: > Perhaps we should add a version of this function to process utils, or add a TODO > to switch if one is written? I think it was the lack of this function that > initially caused me to use glib's function instead. I agree. Let's add a TODO here.
I don't see any zombies so far probably because g_child_watch_add is used with process_utils. On 2011/03/21 10:37:30, Zachary Kuznia wrote: > On 2011/03/21 09:03:54, Yusuke Sato wrote: > > Let's just use base/process_utils.h APIs to avoid the problem in glib... > > When I initially tried using process utils, I had some issues with ibus's > subprocesses continuing to run as zombies. Could you verify that they shut down > correctly with this patch?
Thanks for the review. Please take another look. http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... File chrome/browser/chromeos/cros/input_method_library.cc (right): http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:621: g_child_watch_add(base::GetProcId(handle), Sure, added TODO. On 2011/03/21 10:43:52, Zachary Kuznia wrote: > Perhaps we should add a version of this function to process utils, or add a TODO > to switch if one is written? I think it was the lack of this function that > initially caused me to use glib's function instead. http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:621: g_child_watch_add(base::GetProcId(handle), On 2011/03/21 11:52:11, satorux1 wrote: > On 2011/03/21 10:43:52, Zachary Kuznia wrote: > > Perhaps we should add a version of this function to process utils, or add a > TODO > > to switch if one is written? I think it was the lack of this function that > > initially caused me to use glib's function instead. > > I agree. Let's add a TODO here. Done. http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:665: if (base::GetProcId(library->ibus_daemon_process_handle_) == pid) { Added the check. (right now, calling GetProcId(kNullProcessHandle) is safe, but just in case.) On 2011/03/21 10:43:52, Zachary Kuznia wrote: > Should we check that we have a handle before calling this, or is it safe with a > null handle? http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... chrome/browser/chromeos/cros/input_method_library.cc:691: base::CloseProcessHandle(ibus_daemon_process_handle_); Let me remove all CloseProcessHandle calls since base/process_utils.h explicitly says that the function is NOP on Linux. On 2011/03/21 10:43:52, Zachary Kuznia wrote: > Shouldn't this already be called in OnImeShutdown, or do we need to call it > twice?
LGTM On 2011/03/21 12:08:17, Yusuke Sato wrote: > Thanks for the review. Please take another look. > > http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... > File chrome/browser/chromeos/cros/input_method_library.cc (right): > > http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... > chrome/browser/chromeos/cros/input_method_library.cc:621: > g_child_watch_add(base::GetProcId(handle), > Sure, added TODO. > > On 2011/03/21 10:43:52, Zachary Kuznia wrote: > > Perhaps we should add a version of this function to process utils, or add a > TODO > > to switch if one is written? I think it was the lack of this function that > > initially caused me to use glib's function instead. > > http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... > chrome/browser/chromeos/cros/input_method_library.cc:621: > g_child_watch_add(base::GetProcId(handle), > On 2011/03/21 11:52:11, satorux1 wrote: > > On 2011/03/21 10:43:52, Zachary Kuznia wrote: > > > Perhaps we should add a version of this function to process utils, or add a > > TODO > > > to switch if one is written? I think it was the lack of this function that > > > initially caused me to use glib's function instead. > > > > I agree. Let's add a TODO here. > > Done. > > http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... > chrome/browser/chromeos/cros/input_method_library.cc:665: if > (base::GetProcId(library->ibus_daemon_process_handle_) == pid) { > Added the check. > (right now, calling GetProcId(kNullProcessHandle) is safe, but just in case.) > > On 2011/03/21 10:43:52, Zachary Kuznia wrote: > > Should we check that we have a handle before calling this, or is it safe with > a > > null handle? > > http://codereview.chromium.org/6714044/diff/1/chrome/browser/chromeos/cros/in... > chrome/browser/chromeos/cros/input_method_library.cc:691: > base::CloseProcessHandle(ibus_daemon_process_handle_); > Let me remove all CloseProcessHandle calls since base/process_utils.h explicitly > says that the function is NOP on Linux. > > On 2011/03/21 10:43:52, Zachary Kuznia wrote: > > Shouldn't this already be called in OnImeShutdown, or do we need to call it > > twice?
LGTM |
