Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7120004:
Do not execute two or more setxkbmap commands in parallel. (Closed)
|
Created:
9 years, 6 months ago by Yusuke Sato Modified:
9 years, 6 months ago CC:
chromium-reviews, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDo not execute two or more setxkbmap commands in parallel.
BUG=chromium-os:15516
TEST=checked the chrome log using --vmodule=xkeyboard=1.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88104
Patch Set 1 #
Total comments: 4
Patch Set 2 : review fix, improved error handling #
Total comments: 4
Patch Set 3 : add more comments #Messages
Total messages: 9 (0 generated)
http://codereview.chromium.org/7120004/diff/1/chrome/browser/chromeos/input_m... File chrome/browser/chromeos/input_method/xkeyboard.cc (right): http://codereview.chromium.org/7120004/diff/1/chrome/browser/chromeos/input_m... chrome/browser/chromeos/input_method/xkeyboard.cc:258: if (execute_queue_.size() == 1) { Checking the size here looks a bit tricky. Maybe better to do this in ExecuteSetLayoutCommand()?
LGTM w/ tiny nit. http://codereview.chromium.org/7120004/diff/1/chrome/browser/chromeos/input_m... File chrome/browser/chromeos/input_method/xkeyboard.cc (right): http://codereview.chromium.org/7120004/diff/1/chrome/browser/chromeos/input_m... chrome/browser/chromeos/input_method/xkeyboard.cc:257: execute_queue_.push(layouts_to_set); Is layouts_to_set a collection or a single layout? If just one, please rename to layout_to_set.
Added error handling code (for base::LaunchApp failure) as well. http://codereview.chromium.org/7120004/diff/1/chrome/browser/chromeos/input_m... File chrome/browser/chromeos/input_method/xkeyboard.cc (right): http://codereview.chromium.org/7120004/diff/1/chrome/browser/chromeos/input_m... chrome/browser/chromeos/input_method/xkeyboard.cc:257: execute_queue_.push(layouts_to_set); On 2011/06/07 03:19:38, Daniel Kurtz wrote: > Is layouts_to_set a collection or a single layout? > If just one, please rename to layout_to_set. Done. http://codereview.chromium.org/7120004/diff/1/chrome/browser/chromeos/input_m... chrome/browser/chromeos/input_method/xkeyboard.cc:258: if (execute_queue_.size() == 1) { Modifed both this function and the callback. PTAL. On 2011/06/07 03:19:27, satorux1 wrote: > Checking the size here looks a bit tricky. Maybe better to do this in > ExecuteSetLayoutCommand()?
LGTM. Please add to R12 and R13
http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/xkeyboard.cc (right): http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/xkeyboard.cc:260: if (start_execution) { I thought we could move the queue management logic to MaybeExecuteSetLayoutCommand(); Something like below may be a bit cleaner. execute_queue_.push(layout_to_set); MaybeExecuteSetLayoutCommand(); void MaybeExecuteSetLayoutCommand() { // We execute the setxkbmap command one at a time. // OnSetLayoutFinish() dequeues and retries as needed. if (execute_queue_.size() != 1) return;
http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/xkeyboard.cc (right): http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/xkeyboard.cc:260: if (start_execution) { I think it will not work if SetLayout() is rapidly called more than 3 times. SetLayout(A); // queue.size() == 1, call setxkbmap SetLayout(B); // queue.size() == 2, do not call setxkbmap since the size != 1 SetLayout(C); // queue.size() == 3, do not call setxkbmap since the size != 1 OnSetLayoutFinish() for A; // queue.pop() is called first and queue.size() becomes 2, the next setxkbmap will NOT be called since the size != 1 On 2011/06/07 05:41:29, satorux1 wrote: > I thought we could move the queue management logic to > MaybeExecuteSetLayoutCommand(); > > Something like below may be a bit cleaner. > > execute_queue_.push(layout_to_set); > MaybeExecuteSetLayoutCommand(); > > > void MaybeExecuteSetLayoutCommand() { > // We execute the setxkbmap command one at a time. > // OnSetLayoutFinish() dequeues and retries as needed. > if (execute_queue_.size() != 1) > return;
LGTM http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/xkeyboard.cc (right): http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/xkeyboard.cc:260: if (start_execution) { Thank you for the explanation. The code wasn't obvious to me. Would be nice to add a comment why you are doing this here. On 2011/06/07 05:51:43, Yusuke Sato wrote: > I think it will not work if SetLayout() is rapidly called more than 3 times. > > SetLayout(A); // queue.size() == 1, call setxkbmap > SetLayout(B); // queue.size() == 2, do not call setxkbmap since the size != 1 > SetLayout(C); // queue.size() == 3, do not call setxkbmap since the size != 1 > OnSetLayoutFinish() for A; // queue.pop() is called first and queue.size() > becomes 2, the next setxkbmap will NOT be called since the size != 1 > > On 2011/06/07 05:41:29, satorux1 wrote: > > I thought we could move the queue management logic to > > MaybeExecuteSetLayoutCommand(); > > > > Something like below may be a bit cleaner. > > > > execute_queue_.push(layout_to_set); > > MaybeExecuteSetLayoutCommand(); > > > > > > void MaybeExecuteSetLayoutCommand() { > > // We execute the setxkbmap command one at a time. > > // OnSetLayoutFinish() dequeues and retries as needed. > > if (execute_queue_.size() != 1) > > return; >
submitting.. http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... File chrome/browser/chromeos/input_method/xkeyboard.cc (right): http://codereview.chromium.org/7120004/diff/1002/chrome/browser/chromeos/inpu... chrome/browser/chromeos/input_method/xkeyboard.cc:260: if (start_execution) { On 2011/06/07 05:59:13, satorux1 wrote: > Thank you for the explanation. The code wasn't obvious to me. Would be nice to > add a comment why you are doing this here. > > > On 2011/06/07 05:51:43, Yusuke Sato wrote: > > I think it will not work if SetLayout() is rapidly called more than 3 times. > > > > SetLayout(A); // queue.size() == 1, call setxkbmap > > SetLayout(B); // queue.size() == 2, do not call setxkbmap since the size != 1 > > SetLayout(C); // queue.size() == 3, do not call setxkbmap since the size != 1 > > OnSetLayoutFinish() for A; // queue.pop() is called first and queue.size() > > becomes 2, the next setxkbmap will NOT be called since the size != 1 > > > > On 2011/06/07 05:41:29, satorux1 wrote: > > > I thought we could move the queue management logic to > > > MaybeExecuteSetLayoutCommand(); > > > > > > Something like below may be a bit cleaner. > > > > > > execute_queue_.push(layout_to_set); > > > MaybeExecuteSetLayoutCommand(); > > > > > > > > > void MaybeExecuteSetLayoutCommand() { > > > // We execute the setxkbmap command one at a time. > > > // OnSetLayoutFinish() dequeues and retries as needed. > > > if (execute_queue_.size() != 1) > > > return; > > > Done. |
