Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10159004:
Extends DBusThreadManager to connect ibus-bus. (Closed)
|
Created:
8 years, 8 months ago by Seigo Nonaka Modified:
8 years, 7 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionExtends DBusThreadManager to connect ibus-bus.
Introduce IBusUtil::GetIBusAddressTest to get ibus-daemon address.
BUG=chromium-os:26334
TEST=chromes_unittest, unit_tests, dbus_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137875
Patch Set 1 #
Total comments: 19
Patch Set 2 : Remove IBusUtil #
Total comments: 2
Patch Set 3 : Rebase on http://codereview.chromium.org/10195001/ and separate initialization. #
Total comments: 10
Patch Set 4 : Support address file watcher. #Patch Set 5 : Fixed wrong comment #Patch Set 6 : Fix nits #
Total comments: 16
Patch Set 7 : Avoid double initializing #
Total comments: 18
Patch Set 8 : Check return value and call Done function on UI thread. #Patch Set 9 : Work in progress #Patch Set 10 : Work in progress #Patch Set 11 : Resolve race condition #Patch Set 12 : Fix nits #
Total comments: 30
Patch Set 13 : Work in progress #Patch Set 14 : Introduce IBusAddressWatcher class #
Total comments: 24
Patch Set 15 : Fix ibus_daemon_address update #
Total comments: 22
Patch Set 16 : Apply comments #
Total comments: 2
Patch Set 17 : Introduce InitIBusBus and remove MaybeResetIBusBus #Patch Set 18 : Fix 32bit build failure. #
Total comments: 6
Patch Set 19 : Apply comments. #
Total comments: 2
Patch Set 20 : Apply comment. #
Messages
Total messages: 30 (0 generated)
http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/dbus_thread_mana... File chromeos/dbus/dbus_thread_manager.cc (left): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/dbus_thread_mana... chromeos/dbus/dbus_thread_manager.cc:35: don't remove this. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.cc File chromeos/dbus/ibus/ibus_util.cc (right): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.c... chromeos/dbus/ibus/ibus_util.cc:33: if (!file_util::ReadFileToString(FilePath(address_file_path), Ugh, you cannot do this here. DBusThreadManager is created on UI thread, where you cannot perform file IO... http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.c... chromeos/dbus/ibus/ibus_util.cc:40: Tokenize(address_file_contents, "\r\n", &lines); Use SplitString instead? \r won't be used in the file, right? http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.c... chromeos/dbus/ibus/ibus_util.cc:48: ite != lines.end(); ++ite) { matter of taste, but for (size_t i = 0; i < lines.zie(); ++i) is more succinct, unless you are an iterator enthusiast. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.h File chromeos/dbus/ibus/ibus_util.h (right): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.h... chromeos/dbus/ibus/ibus_util.h:16: class CHROMEOS_EXPORT IBusUtil { please don't define a class if members are just static functions. use namespace instead: namespace ibus_util { } http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.h... chromeos/dbus/ibus/ibus_util.h:22: static bool GetIBusAddress(std::string* address); Instead of returning a boolean, you can return an empty string in case of an error. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... File chromeos/dbus/ibus/ibus_util_unittest.cc (right): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... chromeos/dbus/ibus/ibus_util_unittest.cc:21: void CreateTextFile(const FilePath& filepath, std::string contents) { function comment is missing. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... chromeos/dbus/ibus/ibus_util_unittest.cc:22: std::ofstream file; don't use this. please just use file_util::WriteFile() http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... chromeos/dbus/ibus/ibus_util_unittest.cc:56: { instead of having many scopes in a single test function, please define separate test function: TEST_F(IBusUtilTest, GetIBusAddressTest_Foo) { } TEST_F(IBusUtilTest, GetIBusAddressTest_Bar) { }
http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.cc File chromeos/dbus/ibus/ibus_util.cc (right): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.c... chromeos/dbus/ibus/ibus_util.cc:33: if (!file_util::ReadFileToString(FilePath(address_file_path), I found a command line flag to specify opening address in ibus-daemon. https://github.com/ibus/ibus/blob/master/bus/main.c#L64 And Chrome launches the ibus-daemon. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/in... Thus we may be able to fix the address name. I will try them. On 2012/04/20 23:18:40, satorux1 wrote: > Ugh, you cannot do this here. DBusThreadManager is created on UI thread, where > you cannot perform file IO...
http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/dbus_thread_mana... File chromeos/dbus/dbus_thread_manager.cc (left): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/dbus_thread_mana... chromeos/dbus/dbus_thread_manager.cc:35: On 2012/04/20 23:18:40, satorux1 wrote: > don't remove this. Done. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.cc File chromeos/dbus/ibus/ibus_util.cc (right): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.c... chromeos/dbus/ibus/ibus_util.cc:33: if (!file_util::ReadFileToString(FilePath(address_file_path), I made another CL for above. http://codereview.chromium.org/10195001/ This CL will be submit after the CL is approved. On 2012/04/21 05:35:14, nona1 wrote: > I found a command line flag to specify opening address in ibus-daemon. > https://github.com/ibus/ibus/blob/master/bus/main.c#L64 > And Chrome launches the ibus-daemon. > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/in... > > Thus we may be able to fix the address name. > I will try them. > > On 2012/04/20 23:18:40, satorux1 wrote: > > Ugh, you cannot do this here. DBusThreadManager is created on UI thread, where > > you cannot perform file IO... > http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.c... chromeos/dbus/ibus/ibus_util.cc:40: Tokenize(address_file_contents, "\r\n", &lines); Yes, but I removed this file. Thanks On 2012/04/20 23:18:40, satorux1 wrote: > Use SplitString instead? \r won't be used in the file, right? http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.c... chromeos/dbus/ibus/ibus_util.cc:48: ite != lines.end(); ++ite) { I have not strong policy about iterator:) I will do in next time. Thanks. On 2012/04/20 23:18:40, satorux1 wrote: > matter of taste, but for (size_t i = 0; i < lines.zie(); ++i) is more succinct, > unless you are an iterator enthusiast. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.h File chromeos/dbus/ibus/ibus_util.h (right): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.h... chromeos/dbus/ibus/ibus_util.h:16: class CHROMEOS_EXPORT IBusUtil { Okay but I removed this file. Thanks On 2012/04/20 23:18:40, satorux1 wrote: > please don't define a class if members are just static functions. > > use namespace instead: > > namespace ibus_util { > } http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util.h... chromeos/dbus/ibus/ibus_util.h:22: static bool GetIBusAddress(std::string* address); Okay but I removed this file. Thanks On 2012/04/20 23:18:40, satorux1 wrote: > Instead of returning a boolean, you can return an empty string in case of an > error. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... File chromeos/dbus/ibus/ibus_util_unittest.cc (right): http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... chromeos/dbus/ibus/ibus_util_unittest.cc:21: void CreateTextFile(const FilePath& filepath, std::string contents) { Okay but I removed this file. Thanks On 2012/04/20 23:18:40, satorux1 wrote: > function comment is missing. http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... chromeos/dbus/ibus/ibus_util_unittest.cc:22: std::ofstream file; Okay but I removed this file. Thanks On 2012/04/20 23:18:40, satorux1 wrote: > don't use this. please just use file_util::WriteFile() http://codereview.chromium.org/10159004/diff/1/chromeos/dbus/ibus/ibus_util_u... chromeos/dbus/ibus/ibus_util_unittest.cc:56: { Okay but I removed this file. Thanks On 2012/04/20 23:18:40, satorux1 wrote: > instead of having many scopes in a single test function, please define separate > test function: > > TEST_F(IBusUtilTest, GetIBusAddressTest_Foo) { > } > > TEST_F(IBusUtilTest, GetIBusAddressTest_Bar) { > }
http://codereview.chromium.org/10159004/diff/14001/chromeos/dbus/dbus_thread_... File chromeos/dbus/dbus_thread_manager.cc (right): http://codereview.chromium.org/10159004/diff/14001/chromeos/dbus/dbus_thread_... chromeos/dbus/dbus_thread_manager.cc:57: // Create the connection to the ibus bus. Is this the right place to connect to the ibus bus? Shouldn't we connect to ibus-daemon only after we launch the ibus-daemon?
http://codereview.chromium.org/10159004/diff/14001/chromeos/dbus/dbus_thread_... File chromeos/dbus/dbus_thread_manager.cc (right): http://codereview.chromium.org/10159004/diff/14001/chromeos/dbus/dbus_thread_... chromeos/dbus/dbus_thread_manager.cc:57: // Create the connection to the ibus bus. Ah yes! Done. On 2012/04/23 23:09:21, satorux1 wrote: > Is this the right place to connect to the ibus bus? > > Shouldn't we connect to ibus-daemon only after we launch the ibus-daemon?
Adds yusukes@ as a reviewer. yusukes, could you check the initial communication with ibus-daemon?, which is in Initialize logic is in chromeos/dbus/dbus_thread_manager.cc:135 and the caller of it is chrome/browser/chromeos/input_method/ibus_controller_impl.cc:929 Thank you.
http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:899: const std::string ibus_address = base::StringPrintf( nit: move this down to line 908. http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:922: // To avoid reading address file in UI thread, sets environment variable. The You can remove 922-927. See http://codereview.chromium.org/10195001/. http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:930: dbus_thread_manager->InitializeIBusBus(ibus_address); It seems that you code does not support dynamic ibus-daemon shutdown. What happens with the following scenario? 1. mozc is enabled. 2. MaybeLaunchIBusDaemon() is called. ibus-daemon is started with --address=X 3. InitializeIBusBus is called with X 4. mozc is disabled. 5. ibus-daemon is killed. 6. mozc is re-enabled. 7. MaybeLaunchIBusDaemon() is called again. ibus-daemon is started with --address=Y 8. InitializeIBusBus is called with Y, but it fails since ibus_bus_ is already initialized. Please fix the code (or remove the dynamic shutdown code if it's super hard to fix your new client). http://codereview.chromium.org/10159004/diff/24001/chromeos/dbus/dbus_thread_... File chromeos/dbus/dbus_thread_manager.cc (right): http://codereview.chromium.org/10159004/diff/24001/chromeos/dbus/dbus_thread_... chromeos/dbus/dbus_thread_manager.cc:146: ibus_bus_ = new dbus::Bus(ibus_bus_options); This line could be executed before ibus-daemon actually starts. Is it okay? Please remember that unlike other dbus services the daemon is started and shutdown dynamically, and the base::LaunchProcess() call in ibus_controller_impl.cc just asks the OS to launch the process asynchronously and returns immediately. FYI, ibus-daemon creates an IBUS_ADDRESS_FILE file when it gets ready to accept connections, and libibus connects to the daemon only after the daemon creates the file. I guess your new client also needs this kind of synchronization stuff.
Sorry for late response. http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:899: const std::string ibus_address = base::StringPrintf( On 2012/04/24 04:58:37, Yusuke Sato wrote: > nit: move this down to line 908. Done. http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:922: // To avoid reading address file in UI thread, sets environment variable. The On 2012/04/24 04:58:37, Yusuke Sato wrote: > You can remove 922-927. See http://codereview.chromium.org/10195001/. Done. http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:930: dbus_thread_manager->InitializeIBusBus(ibus_address); I think latest patch support suggested case, however I can not reproduce 5. The ibus-daemon process is alived even I turned off Mozc from input language configuration. I tried as follows: 1. Choose English UI on the initial start-up 2. Enable mozc(Click the mozc checkbox on chrome://settings/language) 3. Turn ON mozc by Alt_L + Shift_L 4. Input something (I confirmed ibus-daemon is alive here) 5. Disable Mozc (Remove check from chrome://settings/languages) (I confirmed ibus-daemon is still alive here) 6. Input something (in english) 7. ibus-daemon is still alived. There are only Japanese and English are shown in chrome://settings/languages. And the ibus-daemon is not launched after reboot after 7. Is it bug? or I do something wrong? On 2012/04/24 04:58:37, Yusuke Sato wrote: > It seems that you code does not support dynamic ibus-daemon shutdown. What > happens with the following scenario? > > 1. mozc is enabled. > 2. MaybeLaunchIBusDaemon() is called. ibus-daemon is started with --address=X > 3. InitializeIBusBus is called with X > 4. mozc is disabled. > 5. ibus-daemon is killed. > 6. mozc is re-enabled. > 7. MaybeLaunchIBusDaemon() is called again. ibus-daemon is started with > --address=Y > 8. InitializeIBusBus is called with Y, but it fails since ibus_bus_ is already > initialized. > > Please fix the code (or remove the dynamic shutdown code if it's super hard to > fix your new client). http://codereview.chromium.org/10159004/diff/24001/chromeos/dbus/dbus_thread_... File chromeos/dbus/dbus_thread_manager.cc (right): http://codereview.chromium.org/10159004/diff/24001/chromeos/dbus/dbus_thread_... chromeos/dbus/dbus_thread_manager.cc:146: ibus_bus_ = new dbus::Bus(ibus_bus_options); Thank you for your kindly notification. I introduced FilePathWatcher and it can do same things with libibus. On 2012/04/24 04:58:37, Yusuke Sato wrote: > This line could be executed before ibus-daemon actually starts. Is it okay? > > Please remember that unlike other dbus services the daemon is started and > shutdown dynamically, and the base::LaunchProcess() call in > ibus_controller_impl.cc just asks the OS to launch the process asynchronously > and returns immediately. > > FYI, ibus-daemon creates an IBUS_ADDRESS_FILE file when it gets ready to accept > connections, and libibus connects to the daemon only after the daemon creates > the file. I guess your new client also needs this kind of synchronization stuff. Done.
looks much better now but I still see some race issues. http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:930: dbus_thread_manager->InitializeIBusBus(ibus_address); Ah ok, the dynamic shutdown feature is temporarily disabled to work around crosbug 26443. See InputMethodManagerImpl::EnableInputMethods() and InputMethodManagerImpl::RemoveInputMethodExtension(). if (ContainOnlyKeyboardLayout(active_input_method_ids_)) { // Do NOT call ibus_controller_->Stop(); here to work around a crash issue // at crosbug.com/27051. // TODO(yusukes): We can safely call Stop(); here once crosbug.com/26443 // is implemented. ... The plan is to re-enable the feature once your work is done. On 2012/04/26 00:14:32, nona1 wrote: > I think latest patch support suggested case, however I can not reproduce 5. > The ibus-daemon process is alived even I turned off Mozc from input language > configuration. > I tried as follows: > > 1. Choose English UI on the initial start-up > 2. Enable mozc(Click the mozc checkbox on chrome://settings/language) > 3. Turn ON mozc by Alt_L + Shift_L > 4. Input something > (I confirmed ibus-daemon is alive here) > 5. Disable Mozc (Remove check from chrome://settings/languages) > (I confirmed ibus-daemon is still alive here) > 6. Input something (in english) > 7. ibus-daemon is still alived. > > There are only Japanese and English are shown in chrome://settings/languages. > And the ibus-daemon is not launched after reboot after 7. > > Is it bug? or I do something wrong? > > On 2012/04/24 04:58:37, Yusuke Sato wrote: > > It seems that you code does not support dynamic ibus-daemon shutdown. What > > happens with the following scenario? > > > > 1. mozc is enabled. > > 2. MaybeLaunchIBusDaemon() is called. ibus-daemon is started with --address=X > > 3. InitializeIBusBus is called with X > > 4. mozc is disabled. > > 5. ibus-daemon is killed. > > 6. mozc is re-enabled. > > 7. MaybeLaunchIBusDaemon() is called again. ibus-daemon is started with > > --address=Y > > 8. InitializeIBusBus is called with Y, but it fails since ibus_bus_ is already > > initialized. > > > > Please fix the code (or remove the dynamic shutdown code if it's super hard to > > fix your new client). > http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:391: nit: remove http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:393: std::string ibus_address_; nit: const std::string would be better. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:395: base::files::FilePathWatcher* watcher_; nit: vertical space after this line. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:405: base::files::FilePathWatcher* watcher = new base::files::FilePathWatcher(); nit: you can omit '()'. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:420: weak_ptr_factory_(this) { ALLOW_THIS_IN_INITIALIZER_LIST? http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:465: nit: remove http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:958: std::string ibus_address = base::StringPrintf( looks like this function still has a race condition. what happens if the function is called twice or more quickly? e.g. 1. login as guest. at this point, xkb:us::eng is the only ime enabled. 2. enable mozc. this function is called. 3. before LaunchIBusDaemon is called, enable mozc-jp. this function is called again. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:958: std::string ibus_address = base::StringPrintf( const
http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/24001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:930: dbus_thread_manager->InitializeIBusBus(ibus_address); s/crosbug 26443/crosbug 27051/
http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:391: On 2012/04/26 00:46:28, Yusuke Sato wrote: > nit: remove Done. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:393: std::string ibus_address_; On 2012/04/26 00:46:28, Yusuke Sato wrote: > nit: const std::string would be better. Done. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:395: base::files::FilePathWatcher* watcher_; On 2012/04/26 00:46:28, Yusuke Sato wrote: > nit: vertical space after this line. Done. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:405: base::files::FilePathWatcher* watcher = new base::files::FilePathWatcher(); On 2012/04/26 00:46:28, Yusuke Sato wrote: > nit: you can omit '()'. Done. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:420: weak_ptr_factory_(this) { On 2012/04/26 00:46:28, Yusuke Sato wrote: > ALLOW_THIS_IN_INITIALIZER_LIST? Done. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:465: On 2012/04/26 00:46:28, Yusuke Sato wrote: > nit: remove Done. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:958: std::string ibus_address = base::StringPrintf( On 2012/04/26 00:46:28, Yusuke Sato wrote: > const Done. http://codereview.chromium.org/10159004/diff/41001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:958: std::string ibus_address = base::StringPrintf( I introduce is_initializing_ibus_daemon_ to avoid double initialization. On 2012/04/26 00:46:28, Yusuke Sato wrote: > looks like this function still has a race condition. what happens if the > function is called twice or more quickly? e.g. > > 1. login as guest. at this point, xkb:us::eng is the only ime enabled. > 2. enable mozc. this function is called. > 3. before LaunchIBusDaemon is called, enable mozc-jp. this function is called > again. > >
almost there. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:383: content::BrowserThread::PostTaskAndReply( check return value? http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:389: base::Bind(&IBusControllerImpl::IBusBusInitializationDone, flipping the flag from FILE thread w/o locking looks harmful to me. please do it in the UI thread. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:397: nit: add a comment or remove this line. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:399: nit: the same as above. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:409: env->GetVar("IBUS_ADDRESS_FILE", &address_file_path); DCHECK(!address_file_path.empty()); http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:975: content::BrowserThread::PostTaskAndReply( check return value? http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1053: void IBusControllerImpl::OnIBusDaemonExit(GPid pid, please also make sure that your code works fine even if ibus-daemon crashes. I guess your code fails to restart ibus-daemon if ibus-daemon crashes before it writes an address file (although that kind of crash unlikely happens.) http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.h (right): http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.h:166: base::WeakPtrFactory<IBusControllerImpl> weak_ptr_factory_; add comment. please explain why this is necessary. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.h:173: bool is_initializing_ibus_daemon_; I don't want to have two similar boolean flags here, should_launch_daemon_ and is_initializing_ibus_daemon_. Can you consolidate them? Having one enum variable e.g. DaemonStatus or something like that would be sufficient.
Sorry for late response. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:383: content::BrowserThread::PostTaskAndReply( On 2012/04/27 02:12:13, Yusuke Sato wrote: > check return value? Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:389: base::Bind(&IBusControllerImpl::IBusBusInitializationDone, On 2012/04/27 02:12:13, Yusuke Sato wrote: > flipping the flag from FILE thread w/o locking looks harmful to me. please do it > in the UI thread. Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:397: On 2012/04/27 02:12:13, Yusuke Sato wrote: > nit: add a comment or remove this line. Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:399: On 2012/04/27 02:12:13, Yusuke Sato wrote: > nit: the same as above. Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:409: env->GetVar("IBUS_ADDRESS_FILE", &address_file_path); On 2012/04/27 02:12:13, Yusuke Sato wrote: > DCHECK(!address_file_path.empty()); Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:975: content::BrowserThread::PostTaskAndReply( On 2012/04/27 02:12:13, Yusuke Sato wrote: > check return value? Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1053: void IBusControllerImpl::OnIBusDaemonExit(GPid pid, On 2012/04/27 02:12:13, Yusuke Sato wrote: > please also make sure that your code works fine even if ibus-daemon crashes. I > guess your code fails to restart ibus-daemon if ibus-daemon crashes before it > writes an address file (although that kind of crash unlikely happens.) > Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.h (right): http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.h:166: base::WeakPtrFactory<IBusControllerImpl> weak_ptr_factory_; On 2012/04/27 02:12:13, Yusuke Sato wrote: > add comment. please explain why this is necessary. Done. http://codereview.chromium.org/10159004/diff/50001/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.h:173: bool is_initializing_ibus_daemon_; On 2012/04/27 02:12:13, Yusuke Sato wrote: > I don't want to have two similar boolean flags here, should_launch_daemon_ and > is_initializing_ibus_daemon_. Can you consolidate them? Having one enum variable > e.g. DaemonStatus or something like that would be sufficient. > Done.
https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:420: env->GetVar("IBUS_ADDRESS_FILE", &address_file_path); Calling GetVar on FILE thread looks a bit scary. I know it works right now (because only tests call SetVar as of today), but probably it's better to call it on UI therad. Add a TODO if you agree. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:424: base::files::FilePathWatcher* watcher = new base::files::FilePathWatcher; remove the previous instance here? see my comment in .h. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:431: } nit: space between 431 & 432. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:502: if (process_handle_ == base::kNullProcessHandle) { You checked |ibus_daemon_status_| in Start() but checked |process_handle_| in Stop(). This is inconsistent and confusing. I think we should remove all if (process_handle_ == base::kNullProcessHandle) and if (process_handle_ != base::kNullProcessHandle) code. This would make your CL easier to review. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:524: ibus_daemon_status_ = IBUS_DAEMON_SHUTTING_DOWN; nit: looks like this is unnecessary. see L.507. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:534: DCHECK_EQ(IBUS_DAEMON_RUNNING, ibus_daemon_status_); I think this DCHECK is harmful. Consider this case: 1. the user enables mozc, and the user uses the IME for a while. 2. for some reason, ibus-daemon crashes. the status is changed to STOP inside the exit handler. 3. the user switches the IME by Shift+Alt --> DCHECK fail. Change the DCHECK to if (ibus_daemon_status_ != IBUS_DAEMON_RUNNING) return false; https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:977: DCHECK_EQ(base::kNullProcessHandle, process_handle_); move this to LaunchIBusDaemon()? you can check ibus_daemon_status_ instead. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:980: "unix:abstract=/tmp/ibus-%0lX", base::RandUint64()); nit: It's not a file but a socket in an abstract namespace, right? If so, remove '/tmp/'. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1045: // Reset retry count nit: this looks obvious. remove https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1085: // Corresponding to ibus-daemon is re-launched during question: when exactly could this path be taken? probably I couldn't understand your code comment here. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1089: return; should we assign STOP in this case? (see L.1077) https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1098: DCHECK_NE(IBUS_DAEMON_STOP, on_exit_state); (note: still checking if this DCHECK is valid.) https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1101: // TODO(nona): Remove current file watcher if exit on initializing state. I guess it's not actually safe. please consider this case: - ibus-daemon starts. - it creates an address file. - right after that, it crashes. - OnIBusDaemonExit is called first, then MaybeResetIBusBusAndSetsInitializationDone() is called (via a message from FILE thread). - The status becomes RUNNING even though the daemon is not yet running. (not 100% sure, could be wrong but) we can fix this problem by: - add |input_method_address_| member variable to this class, and update it in LaunchIBusDaemon(). - move MaybeResetIBusBusAndSetsInitializationDone() from anon namespace to this class, and check if the address passed to the function matches |input_method_address_|. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1107: LOG(FATAL) << "Give up re-launching ibus-daemon"; I don't think we need to reboot Chromebook here because rebooting will unlikely fix the problem. LOG(ERROR) would suffice. https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... File chrome/browser/chromeos/input_method/ibus_controller_impl.h (right): https://chromiumcodereview.appspot.com/10159004/diff/65002/chrome/browser/chr... chrome/browser/chromeos/input_method/ibus_controller_impl.h:173: int ibus_launch_retry_count_; Wouldn't it be possible not to add this variable for now? For me, the variable is making it even harder to review your CL... If you're worried that too many file watcher instances could be created, see my comment in StartAddressFileWatch().
http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:420: env->GetVar("IBUS_ADDRESS_FILE", &address_file_path); On 2012/05/01 07:06:55, Yusuke Sato wrote: > Calling GetVar on FILE thread looks a bit scary. I know it works right now > (because only tests call SetVar as of today), but probably it's better to call > it on UI therad. Add a TODO if you agree. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:424: base::files::FilePathWatcher* watcher = new base::files::FilePathWatcher; On 2012/05/01 07:06:55, Yusuke Sato wrote: > remove the previous instance here? see my comment in .h. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:431: } On 2012/05/01 07:06:55, Yusuke Sato wrote: > nit: space between 431 & 432. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:502: if (process_handle_ == base::kNullProcessHandle) { On 2012/05/01 07:06:55, Yusuke Sato wrote: > You checked |ibus_daemon_status_| in Start() but checked |process_handle_| in > Stop(). This is inconsistent and confusing. > > I think we should remove all if (process_handle_ == base::kNullProcessHandle) > and if (process_handle_ != base::kNullProcessHandle) code. This would make your > CL easier to review. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:524: ibus_daemon_status_ = IBUS_DAEMON_SHUTTING_DOWN; On 2012/05/01 07:06:55, Yusuke Sato wrote: > nit: looks like this is unnecessary. see L.507. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:534: DCHECK_EQ(IBUS_DAEMON_RUNNING, ibus_daemon_status_); On 2012/05/01 07:06:55, Yusuke Sato wrote: > I think this DCHECK is harmful. Consider this case: > > 1. the user enables mozc, and the user uses the IME for a while. > 2. for some reason, ibus-daemon crashes. the status is changed to STOP inside > the exit handler. > 3. the user switches the IME by Shift+Alt --> DCHECK fail. > > Change the DCHECK to > > if (ibus_daemon_status_ != IBUS_DAEMON_RUNNING) > return false; Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:977: DCHECK_EQ(base::kNullProcessHandle, process_handle_); On 2012/05/01 07:06:55, Yusuke Sato wrote: > move this to LaunchIBusDaemon()? > you can check ibus_daemon_status_ instead. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:980: "unix:abstract=/tmp/ibus-%0lX", base::RandUint64()); On 2012/05/01 07:06:55, Yusuke Sato wrote: > nit: It's not a file but a socket in an abstract namespace, right? If so, remove > '/tmp/'. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1045: // Reset retry count Removed retry count from CL. Thanks. On 2012/05/01 07:06:55, Yusuke Sato wrote: > nit: this looks obvious. remove http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1085: // Corresponding to ibus-daemon is re-launched during I added the this condition path in code comment. On 2012/05/01 07:06:55, Yusuke Sato wrote: > question: when exactly could this path be taken? probably I couldn't understand > your code comment here. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1089: return; Thanks moved L.1077 to L.1092 On 2012/05/01 07:06:55, Yusuke Sato wrote: > should we assign STOP in this case? (see L.1077) http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1098: DCHECK_NE(IBUS_DAEMON_STOP, on_exit_state); Thanks, removed. On 2012/05/01 07:06:55, Yusuke Sato wrote: > (note: still checking if this DCHECK is valid.) http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1101: // TODO(nona): Remove current file watcher if exit on initializing state. On 2012/05/01 07:06:55, Yusuke Sato wrote: > I guess it's not actually safe. please consider this case: > > - ibus-daemon starts. > - it creates an address file. > - right after that, it crashes. > - OnIBusDaemonExit is called first, then > MaybeResetIBusBusAndSetsInitializationDone() is called (via a message from FILE > thread). > - The status becomes RUNNING even though the daemon is not yet running. > > > (not 100% sure, could be wrong but) we can fix this problem by: > - add |input_method_address_| member variable to this class, and update it in > LaunchIBusDaemon(). > - move MaybeResetIBusBusAndSetsInitializationDone() from anon namespace to this > class, and check if the address passed to the function matches > |input_method_address_|. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1107: LOG(FATAL) << "Give up re-launching ibus-daemon"; On 2012/05/01 07:06:55, Yusuke Sato wrote: > I don't think we need to reboot Chromebook here because rebooting will unlikely > fix the problem. LOG(ERROR) would suffice. Done. http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.h (right): http://codereview.chromium.org/10159004/diff/65002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.h:173: int ibus_launch_retry_count_; Okay, I removed this variable. On 2012/05/01 07:06:55, Yusuke Sato wrote: > Wouldn't it be possible not to add this variable for now? For me, the variable > is making it even harder to review your CL... > > If you're worried that too many file watcher instances could be created, see my > comment in StartAddressFileWatch().
I think Patch Set #14 is half-baked. No function updates ibus_daemon_address_ while the variable is checked IBusControllerImpl::IBusDaemonInitializationDone(). Giving up reviewing the change for now. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:417: if (instance->IsWatching()) { remove { } http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:431: if (!watcher_) nit: remove 431-432. deleting NULL is safe. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:444: bool IsWatching() { nit: const http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:450: static IBusAddressWatcher* instance = new IBusAddressWatcher; add a thread ID check like DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:458: DISALLOW_COPY_AND_ASSIGN(IBusAddressWatcher); nit: space between 457 & 458 http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:562: // TODO(nona): use ibus_daemon_state_ after implement crosbug/26334 ?? looks like you're already checking |ibus_daemon_state_| in line 563-566. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1011: return false; keep the DVLOG(1) for now. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1034: void IBusControllerImpl::LaunchIBusDaemon(const std::string& ibus_address) { update ibus_daemon_address_ in this function. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1117: const IBusDaemonStatus on_exit_state = controller->ibus_daemon_status_; nit: move this down to 1135. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1127: // 2. Called LaunchProcess (process_handle_ become new instance ) http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1142: LOG(ERROR) << "The ibus-daemon is crashed. Re-launching..."; s/is// http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.h (right): http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.h:154: std::string ibus_daemon_address_; comment
Sorry for incomplete CL. I fixed and checked the initialization works correctly. And also I checked ibus-daemon crash case both before and after writing address file. Thanks. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:417: if (instance->IsWatching()) { On 2012/05/02 06:17:06, Yusuke Sato wrote: > remove { } Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:431: if (!watcher_) On 2012/05/02 06:17:06, Yusuke Sato wrote: > nit: remove 431-432. deleting NULL is safe. Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:444: bool IsWatching() { On 2012/05/02 06:17:06, Yusuke Sato wrote: > nit: const Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:450: static IBusAddressWatcher* instance = new IBusAddressWatcher; On 2012/05/02 06:17:06, Yusuke Sato wrote: > add a thread ID check like > DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE)); Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:458: DISALLOW_COPY_AND_ASSIGN(IBusAddressWatcher); On 2012/05/02 06:17:06, Yusuke Sato wrote: > nit: space between 457 & 458 Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:562: // TODO(nona): use ibus_daemon_state_ after implement crosbug/26334 Oops this comment is obsolete. Removed. On 2012/05/02 06:17:06, Yusuke Sato wrote: > ?? > > looks like you're already checking |ibus_daemon_state_| in line 563-566. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1011: return false; On 2012/05/02 06:17:06, Yusuke Sato wrote: > keep the DVLOG(1) for now. Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1034: void IBusControllerImpl::LaunchIBusDaemon(const std::string& ibus_address) { On 2012/05/02 06:17:06, Yusuke Sato wrote: > update ibus_daemon_address_ in this function. Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1117: const IBusDaemonStatus on_exit_state = controller->ibus_daemon_status_; On 2012/05/02 06:17:06, Yusuke Sato wrote: > nit: move this down to 1135. Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1127: // 2. Called LaunchProcess (process_handle_ become new instance On 2012/05/02 06:17:06, Yusuke Sato wrote: > ) Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1142: LOG(ERROR) << "The ibus-daemon is crashed. Re-launching..."; On 2012/05/02 06:17:06, Yusuke Sato wrote: > s/is// Done. http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.h (right): http://codereview.chromium.org/10159004/diff/78002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.h:154: std::string ibus_daemon_address_; On 2012/05/02 06:17:06, Yusuke Sato wrote: > comment Done.
input_method/ LGTM, but I'd prefer not to submit the CL until M20 branch is created. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:385: virtual void OnFilePathChanged(const FilePath& file_path) { OVERRIDE http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:413: // TODO(nona): Moves reading environment variables to UI thread. move http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:456: DISALLOW_COPY_AND_ASSIGN(IBusAddressWatcher); swap 456 & 457. see the style guide. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:531: // The daemon hasn't been started yet or now shutting down. the comment looks obvious. remove? http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1018: // because if watcher start after ibus-daemon, we may miss the ibus connection starts http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1081: // Corresponding to killing ibus-daemon under initializing, so do nothing. how about: // Stop() or OnIBusDaemonExit() has already been called. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1119: // Corresponding to shutting down ibus-daemon by exit message or crashing I guess the comment is not true since Stop() resets |process_handle_|. The comment should be something like: // ibus-daemon crashed. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1123: // This condition is corresponding as follows. s/corresponding// http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1124: // 1. Called Stop (process_handle_ become null) becomes http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1125: // 2. Called LaunchProcess (process_handle_ become new instance) becomes http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1129: // TODO(nona): Shutdown ibus-bus connection. is this really necessary? releasing the connection should be done in step #1 above. you might want to move the TODO to line 1119.
Thank you for your kindly review and sorry for heavy change. I agree about skipping M20. satorux, PTAL? http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:385: virtual void OnFilePathChanged(const FilePath& file_path) { On 2012/05/03 16:59:24, Yusuke Sato wrote: > OVERRIDE Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:413: // TODO(nona): Moves reading environment variables to UI thread. On 2012/05/03 16:59:24, Yusuke Sato wrote: > move Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:456: DISALLOW_COPY_AND_ASSIGN(IBusAddressWatcher); On 2012/05/03 16:59:24, Yusuke Sato wrote: > swap 456 & 457. see the style guide. Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:531: // The daemon hasn't been started yet or now shutting down. On 2012/05/03 16:59:24, Yusuke Sato wrote: > the comment looks obvious. remove? Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1018: // because if watcher start after ibus-daemon, we may miss the ibus connection On 2012/05/03 16:59:24, Yusuke Sato wrote: > starts Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1081: // Corresponding to killing ibus-daemon under initializing, so do nothing. On 2012/05/03 16:59:24, Yusuke Sato wrote: > how about: // Stop() or OnIBusDaemonExit() has already been called. Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1119: // Corresponding to shutting down ibus-daemon by exit message or crashing On 2012/05/03 16:59:24, Yusuke Sato wrote: > I guess the comment is not true since Stop() resets |process_handle_|. > > The comment should be something like: > // ibus-daemon crashed. Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1123: // This condition is corresponding as follows. On 2012/05/03 16:59:24, Yusuke Sato wrote: > s/corresponding// Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1124: // 1. Called Stop (process_handle_ become null) On 2012/05/03 16:59:24, Yusuke Sato wrote: > becomes Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1125: // 2. Called LaunchProcess (process_handle_ become new instance) On 2012/05/03 16:59:24, Yusuke Sato wrote: > becomes Done. http://codereview.chromium.org/10159004/diff/85002/chrome/browser/chromeos/in... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1129: // TODO(nona): Shutdown ibus-bus connection. Yes! moved. Thanks. On 2012/05/03 16:59:24, Yusuke Sato wrote: > is this really necessary? releasing the connection should be done in step #1 > above. you might want to move the TODO to line 1119.
satorux: ping? M20 branch point is gone, so I think it's time to try this patch. Could you review dbus_thread_manager*? Thank you.
http://codereview.chromium.org/10159004/diff/80003/chromeos/dbus/dbus_thread_... File chromeos/dbus/dbus_thread_manager.h (right): http://codereview.chromium.org/10159004/diff/80003/chromeos/dbus/dbus_thread_... chromeos/dbus/dbus_thread_manager.h:88: // same as before, this function doesn't do anything. does nothing.
http://codereview.chromium.org/10159004/diff/80003/chromeos/dbus/dbus_thread_... File chromeos/dbus/dbus_thread_manager.h (right): http://codereview.chromium.org/10159004/diff/80003/chromeos/dbus/dbus_thread_... chromeos/dbus/dbus_thread_manager.h:88: // same as before, this function doesn't do anything. Thunks but changed the function. On 2012/05/11 23:10:06, satorux1 wrote: > does nothing.
Please remove [Input Method Replacement: Phase 01] from the patch description. This is noisy. http://codereview.chromium.org/10159004/diff/103002/chrome/browser/chromeos/i... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): http://codereview.chromium.org/10159004/diff/103002/chrome/browser/chromeos/i... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1014: base::RandInt(0, 0x7FFFFFFF)); This looks bad... Please use std::numeric_limits<int>::max()
http://codereview.chromium.org/10159004/diff/103002/chromeos/dbus/dbus_thread... File chromeos/dbus/dbus_thread_manager.cc (right): http://codereview.chromium.org/10159004/diff/103002/chromeos/dbus/dbus_thread... chromeos/dbus/dbus_thread_manager.cc:132: ibus_bus_->ShutdownOnDBusThreadAndBlock(); This looks unsafe. ibus_bus_ is optional, right? Please do: if (ibus_bus_.get()) ibus_bus_->ShutdownOnDBusThreadAndBlock(); http://codereview.chromium.org/10159004/diff/103002/chromeos/dbus/dbus_thread... chromeos/dbus/dbus_thread_manager.cc:149: VLOG(1) << "Connected to ibus-daemon.(address:" << ibus_address << ")"; nit: VLOG(1) << "Connected to ibus-daemon:" << ibus_address;
https://chromiumcodereview.appspot.com/10159004/diff/103002/chrome/browser/ch... File chrome/browser/chromeos/input_method/ibus_controller_impl.cc (right): https://chromiumcodereview.appspot.com/10159004/diff/103002/chrome/browser/ch... chrome/browser/chromeos/input_method/ibus_controller_impl.cc:1014: base::RandInt(0, 0x7FFFFFFF)); On 2012/05/18 00:47:05, satorux1 wrote: > This looks bad... > > Please use std::numeric_limits<int>::max() Done. https://chromiumcodereview.appspot.com/10159004/diff/103002/chromeos/dbus/dbu... File chromeos/dbus/dbus_thread_manager.cc (right): https://chromiumcodereview.appspot.com/10159004/diff/103002/chromeos/dbus/dbu... chromeos/dbus/dbus_thread_manager.cc:132: ibus_bus_->ShutdownOnDBusThreadAndBlock(); Yes, thanks. On 2012/05/18 00:50:50, satorux1 wrote: > This looks unsafe. ibus_bus_ is optional, right? > > Please do: > > if (ibus_bus_.get()) > ibus_bus_->ShutdownOnDBusThreadAndBlock(); https://chromiumcodereview.appspot.com/10159004/diff/103002/chromeos/dbus/dbu... chromeos/dbus/dbus_thread_manager.cc:149: VLOG(1) << "Connected to ibus-daemon.(address:" << ibus_address << ")"; On 2012/05/18 00:50:50, satorux1 wrote: > nit: > > VLOG(1) << "Connected to ibus-daemon:" << ibus_address; Done.
/dbus/ stuff LGTM with a nit: http://codereview.chromium.org/10159004/diff/93004/chromeos/dbus/dbus_thread_... File chromeos/dbus/dbus_thread_manager.cc (right): http://codereview.chromium.org/10159004/diff/93004/chromeos/dbus/dbus_thread_... chromeos/dbus/dbus_thread_manager.cc:150: VLOG(1) << "Connected to ibus-daemon:" << ibus_address; "Connected to ibus-daemon: "
Thank you! I will commit after try bot check. https://chromiumcodereview.appspot.com/10159004/diff/93004/chromeos/dbus/dbus... File chromeos/dbus/dbus_thread_manager.cc (right): https://chromiumcodereview.appspot.com/10159004/diff/93004/chromeos/dbus/dbus... chromeos/dbus/dbus_thread_manager.cc:150: VLOG(1) << "Connected to ibus-daemon:" << ibus_address; On 2012/05/18 01:05:37, satorux1 wrote: > "Connected to ibus-daemon: " Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nona@chromium.org/10159004/96014
Change committed as 137875 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
