Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
858353002:
[chromedriver] Fixed window SendKeys (single quote missing) problem (Closed)
|
Created:
5 years, 11 months ago by andrewcheng1 Modified:
5 years, 9 months ago Reviewers:
samuong Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[chromedriver] Fixed window SendKeys single quote problem - some characters set are missing
BUG=chromedriver:924
Committed: https://crrev.com/ffcd7d4a2c31fa011cca774795573902186345fe
Cr-Commit-Position: refs/heads/master@{#321177}
Patch Set 1 #Patch Set 2 : remove one debug print statement #
Total comments: 16
Patch Set 3 : based on codes review #Patch Set 4 : move #define US_KEYBOARD_LAYOUT L"00000409" to local #
Total comments: 4
Patch Set 5 : based on 02/18/2015 codes review #Patch Set 6 : minor change on variable name #
Messages
Total messages: 21 (7 generated)
andrewcheng@chromium.org changed reviewers: + samuong@chromium.orghi sam
andrewcheng@chromium.org changed reviewers: + samuong@chromium.org - samuong@chromium.orghi sam
Hi Sam, Please review Andrew
https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/chrome_launcher.cc:350: VLOG(0) << "Can not set to US keyboard layout\n"; We should also mention that some keycodes may be interpreted incorrectly. Also you don't need to put a '\n' at the end of this. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/chrome_launcher.cc:351: } You mentioned earlier that this is a per-process setting, so I think it would make more sense to do this once when ChromeDriver starts, rather than once every time we launch a new Chrome session. On Linux, we set the locale for keycode conversion near the top of main() - is there any reason not to do the same on Windows? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/chrome... https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/keycode_text_conversion_win.cc (right): https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:74: HKL * keyBoard_handles_list; Can we use a scoped_ptr here instead? If you do, you could get rid of |switch_status| too (just return true/false directly). Also don't mix camelCase and snake_case - change the upper-case 'B' to a lower-case 'b'. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:91: break; What happens if none of the HKLs match the US keyboard layout? Are we going to set the keyboard to something that is neither the existing keyboard, nor the US keyboard? https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:96: HKL layout = ::LoadKeyboardLayout((LPCTSTR)"00000409", KLF_ACTIVATE); Can you define a for "00000409" (or use an existing one, if it exists)?
> Can you define a for "00000409" (or use an existing one, if it exists)? Sorry, I meant define a constant for "00000409".
Hi Sam, Please review again. thanks Andrew https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/chrome_launcher.cc:350: VLOG(0) << "Can not set to US keyboard layout\n"; On 2015/01/23 19:36:58, samuong wrote: > We should also mention that some keycodes may be interpreted incorrectly. > Also you don't need to put a '\n' at the end of this. Done. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/chrome_launcher.cc:351: } On 2015/01/23 19:36:58, samuong wrote: > You mentioned earlier that this is a per-process setting, so I think it would > make more sense to do this once when ChromeDriver starts, rather than once every > time we launch a new Chrome session. On Linux, we set the locale for keycode > conversion near the top of main() - is there any reason not to do the same on > Windows? > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/chrome... sent you email - main() will not work (reason not clear) https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/keycode_text_conversion_win.cc (right): https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:74: HKL * keyBoard_handles_list; On 2015/01/23 19:36:58, samuong wrote: > Can we use a scoped_ptr here instead? If you do, you could get rid of > |switch_status| too (just return true/false directly). > > Also don't mix camelCase and snake_case - change the upper-case 'B' to a > lower-case 'b'. 1. my initial approach do not have switch_status, and found it has too many return statements in the codes. so I modified them to have only one return at end. with more return statements make codes easy to read 2. change the upper-case 'B' to a lower-case 'b' do you mean keyboard_handles_list? Done. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:91: break; On 2015/01/23 19:36:58, samuong wrote: > What happens if none of the HKLs match the US keyboard layout? Are we going to > set the keyboard to something that is neither the existing keyboard, nor the US > keyboard? I verified this already two weeks ago - in language and keyboard, remove all keyboards. it will still provide one - default(US key board). eg. define two keyboards - Dutch, Japanese . it will return three in the list. window will provide extra default keyboard. U.S key board will always available. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:91: break; On 2015/01/23 19:36:58, samuong wrote: > What happens if none of the HKLs match the US keyboard layout? Are we going to > set the keyboard to something that is neither the existing keyboard, nor the US > keyboard? Done. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:96: HKL layout = ::LoadKeyboardLayout((LPCTSTR)"00000409", KLF_ACTIVATE); On 2015/01/23 19:36:58, samuong wrote: > Can you define a for "00000409" (or use an existing one, if it exists)? Done. I can not find any pre-defined constant for "00000409" according to MSDN, MS seems use codes as name. So, we define our own constant ---- following from MSDN Docs --------- U.S. English has a language identifier of 0x0409, so the primary U.S. English layout is named "00000409". Variants of U.S. English layout (such as the Dvorak layout) are named "00010409", "00020409", and so on.
Almost there, just a few nits. Also, could you write a python test for this? I think you can use the win32api module (e.g. http://stackoverflow.com/questions/1420925/change-keyboard-layout-with-python) to change the keyboard layout to something non-American, and then test that ChromeDriver does the conversions properly. https://codereview.chromium.org/858353002/diff/60001/chrome/test/chromedriver... File chrome/test/chromedriver/keycode_text_conversion_win.cc (right): https://codereview.chromium.org/858353002/diff/60001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:15: #define US_KEYBOARD_LAYOUT L"00000409" We should use const variables instead of #defines, so something like: LPCTSTR kUsKeyboardLayout = TEXT("00000409"); https://codereview.chromium.org/858353002/diff/60001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:66: nit: delete blank line https://codereview.chromium.org/858353002/diff/60001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:88: // if active keyboard match US keyboard layout - 00000409 delete this comment https://codereview.chromium.org/858353002/diff/60001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:99: return true; return ::LoadKeyboardLayout(kUsKeyboardLayout, KLF_ACTIVATE) != NULL;
Hi Sam, Please review Thanks, Andrew https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/chrome_launcher.cc:350: VLOG(0) << "Can not set to US keyboard layout\n"; On 2015/01/23 19:36:58, samuong wrote: > We should also mention that some keycodes may be interpreted incorrectly. > Also you don't need to put a '\n' at the end of this. Done. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/chrome_launcher.cc:351: } On 2015/01/24 01:40:00, andrewcheng1 wrote: > On 2015/01/23 19:36:58, samuong wrote: > > You mentioned earlier that this is a per-process setting, so I think it would > > make more sense to do this once when ChromeDriver starts, rather than once > every > > time we launch a new Chrome session. On Linux, we set the locale for keycode > > conversion near the top of main() - is there any reason not to do the same on > > Windows? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/chrome... > sent you email - main() will not work (which it should) work only per thread ----------- following come from M.S document ------------ Prior to Windows 8: If the specified input locale identifier is not already loaded, the function loads and activates the input locale identifier for the current thread. also tried setting KLF_SETFORPROCESS Prior to Windows 8: This flag is valid only with KLF_ACTIVATE. Activates the specified input locale identifier for the entire process and sends the WM_INPUTLANGCHANGE message to the current thread's Focus or Active window. Typically, LoadKeyboardLayout activates an input locale identifier only for the current thread. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... File chrome/test/chromedriver/keycode_text_conversion_win.cc (right): https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:74: HKL * keyBoard_handles_list; On 2015/01/23 19:36:58, samuong wrote: > Can we use a scoped_ptr here instead? If you do, you could get rid of > |switch_status| too (just return true/false directly). > > Also don't mix camelCase and snake_case - change the upper-case 'B' to a > lower-case 'b'. Done. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:96: HKL layout = ::LoadKeyboardLayout((LPCTSTR)"00000409", KLF_ACTIVATE); On 2015/01/23 19:36:58, samuong wrote: > Can you define a for "00000409" (or use an existing one, if it exists)? Done. https://codereview.chromium.org/858353002/diff/20001/chrome/test/chromedriver... chrome/test/chromedriver/keycode_text_conversion_win.cc:96: HKL layout = ::LoadKeyboardLayout((LPCTSTR)"00000409", KLF_ACTIVATE); On 2015/01/23 19:36:58, samuong wrote: > Can you define a for "00000409" (or use an existing one, if it exists)? Done.
This fix has been tested for window 7/8 system with any keyboard setting.
The CQ bit was checked by samuong@chromium.org
The CQ bit was unchecked by samuong@chromium.org
The CQ bit was checked by samuong@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/858353002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samuong@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/858353002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ffcd7d4a2c31fa011cca774795573902186345fe Cr-Commit-Position: refs/heads/master@{#321177} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
