Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
277973002:
Fixes the timing to dispatch VKEY_PROCESSKEY. (Closed)
|
Created:
6 years, 7 months ago by Yuki Modified:
6 years, 7 months ago Reviewers:
Seigo Nonaka CC:
chromium-reviews, nona+watch_chromium.org, James Su, yukishiino+watch_chromium.org, penghuang+watch_chromium.org, yusukes+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixes the timing to dispatch VKEY_PROCESSKEY.
Defers the dicision to dispatch a VKEY_PROCESSKEY key event until the preedit changes or the text gets committed.
BUG=368100
TEST=Done manually.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271363
Patch Set 1 #
Total comments: 6
Patch Set 2 : Synced. #Patch Set 3 : fix: nit: early exit in MaybeFireProcessKey. #
Total comments: 4
Patch Set 4 : Addressed nona's comments. #Messages
Total messages: 11 (0 generated)
Could you review this CL?
Ping as a friendly reminder.
I'm sorry for late response. https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... ui/base/ime/input_method_auralinux.cc:56: StopFiringProcessKey(); I'm bit confused about this. What's happened in the following case? 1. Arrives normal 'A' Down key event. 2. The key is consumed by IBus im context. And send composition update event. 3. The 'A' Up key event is arrived. 4. The composition update event fired in 2. is arrived. Here, the VKEY_PROCESS_KEY is generated? And I'm happy if you add the unit test for some corner cases. https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... ui/base/ime/input_method_auralinux.cc:169: if (allowed_to_fire_vkey_process_key_) { nit: I like early exit style but up to you.
https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... ui/base/ime/input_method_auralinux.cc:56: StopFiringProcessKey(); On 2014/05/14 04:15:03, Seigo Nonaka wrote: > I'm bit confused about this. What's happened in the following case? > > 1. Arrives normal 'A' Down key event. > 2. The key is consumed by IBus im context. And send composition update event. > 3. The 'A' Up key event is arrived. > 4. The composition update event fired in 2. is arrived. > > Here, the VKEY_PROCESS_KEY is generated? Exactly speaking, since we don't allow to fire VKEY_PROCESSKEY for key up events, that specific case doesn't occur. In addition, *normal* 'A' down key event doesn't change the preedit, so we don't fire VKEY_PROCESSKEY. But yes, if an IME is enabled and step 3 is "'B' down key event arrives", you're right. There is no way to prevent such case. This code helps some cases but not all the cases. I didn't find a better way for this issue. Hopefully, people don't type such fast in most cases, or 1. keydown for 'A' 2. keydown for 'B' 3. VKEY_PROCESSKEY for 'a' 4. VKEY_PROCESSKEY for 'b' would be acceptable for most web sites. > And I'm happy if you add the unit test for some corner cases. It's a tough job. We don't want to rely on a specific IME or GTK implementation, so we don't want to use X11InputMethodContextImplGtk2. Then, we need a complex-scenario-customizable mock, but use of gmock is discouraged now a days. We can implement it for ourselves, but it seems too much this time to me. https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... ui/base/ime/input_method_auralinux.cc:169: if (allowed_to_fire_vkey_process_key_) { On 2014/05/14 04:15:03, Seigo Nonaka wrote: > nit: I like early exit style but up to you. I prefer early exit, too.
https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... ui/base/ime/input_method_auralinux.cc:56: StopFiringProcessKey(); Hmm, okay... Could you double check if the other major IMEs or components works well (in terms of comparing with old implementation or with W3C specs). And nice to leave comment if there are compatibility issues you gave up. On 2014/05/14 06:30:23, Yuki wrote: > On 2014/05/14 04:15:03, Seigo Nonaka wrote: > > I'm bit confused about this. What's happened in the following case? > > > > 1. Arrives normal 'A' Down key event. > > 2. The key is consumed by IBus im context. And send composition update event. > > 3. The 'A' Up key event is arrived. > > 4. The composition update event fired in 2. is arrived. > > > > Here, the VKEY_PROCESS_KEY is generated? > > Exactly speaking, since we don't allow to fire VKEY_PROCESSKEY for key up > events, that specific case doesn't occur. In addition, *normal* 'A' down key > event doesn't change the preedit, so we don't fire VKEY_PROCESSKEY. > > But yes, if an IME is enabled and step 3 is "'B' down key event arrives", you're > right. There is no way to prevent such case. > > This code helps some cases but not all the cases. I didn't find a better way > for this issue. > > Hopefully, people don't type such fast in most cases, or > 1. keydown for 'A' > 2. keydown for 'B' > 3. VKEY_PROCESSKEY for 'a' > 4. VKEY_PROCESSKEY for 'b' > would be acceptable for most web sites. > > > > And I'm happy if you add the unit test for some corner cases. > > It's a tough job. We don't want to rely on a specific IME or GTK > implementation, so we don't want to use X11InputMethodContextImplGtk2. Then, we > need a complex-scenario-customizable mock, but use of gmock is discouraged now a > days. We can implement it for ourselves, but it seems too much this time to me. https://codereview.chromium.org/277973002/diff/40001/ui/base/ime/input_method... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/277973002/diff/40001/ui/base/ime/input_method... ui/base/ime/input_method_auralinux.cc:63: if (event.type() == ET_KEY_PRESSED && Could you add comment why we don't allow the vkey process key for key up. And if the IME consumes key up event, the any key up event seems not be fired, right? Is it acceptable? https://codereview.chromium.org/277973002/diff/40001/ui/base/ime/input_method... ui/base/ime/input_method_auralinux.cc:146: MaybeFireProcessKey(); Please double check the key down event is fired before composition events.
https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/277973002/diff/1/ui/base/ime/input_method_aur... ui/base/ime/input_method_auralinux.cc:56: StopFiringProcessKey(); On 2014/05/14 14:01:37, Seigo Nonaka wrote: > Hmm, okay... > > Could you double check if the other major IMEs or components works well (in > terms of comparing with old implementation or with W3C specs). And nice to > leave comment if there are compatibility issues you gave up. > > > > On 2014/05/14 06:30:23, Yuki wrote: > > On 2014/05/14 04:15:03, Seigo Nonaka wrote: > > > I'm bit confused about this. What's happened in the following case? > > > > > > 1. Arrives normal 'A' Down key event. > > > 2. The key is consumed by IBus im context. And send composition update > event. > > > 3. The 'A' Up key event is arrived. > > > 4. The composition update event fired in 2. is arrived. > > > > > > Here, the VKEY_PROCESS_KEY is generated? > > > > Exactly speaking, since we don't allow to fire VKEY_PROCESSKEY for key up > > events, that specific case doesn't occur. In addition, *normal* 'A' down key > > event doesn't change the preedit, so we don't fire VKEY_PROCESSKEY. > > > > But yes, if an IME is enabled and step 3 is "'B' down key event arrives", > you're > > right. There is no way to prevent such case. > > > > This code helps some cases but not all the cases. I didn't find a better way > > for this issue. > > > > Hopefully, people don't type such fast in most cases, or > > 1. keydown for 'A' > > 2. keydown for 'B' > > 3. VKEY_PROCESSKEY for 'a' > > 4. VKEY_PROCESSKEY for 'b' > > would be acceptable for most web sites. > > > > > > > And I'm happy if you add the unit test for some corner cases. > > > > It's a tough job. We don't want to rely on a specific IME or GTK > > implementation, so we don't want to use X11InputMethodContextImplGtk2. Then, > we > > need a complex-scenario-customizable mock, but use of gmock is discouraged now > a > > days. We can implement it for ourselves, but it seems too much this time to > me. Done. I've tested this with XIM, SCIM and IBus. As we talked offline, I removed this line, and added a note in the header. https://codereview.chromium.org/277973002/diff/40001/ui/base/ime/input_method... File ui/base/ime/input_method_auralinux.cc (right): https://codereview.chromium.org/277973002/diff/40001/ui/base/ime/input_method... ui/base/ime/input_method_auralinux.cc:63: if (event.type() == ET_KEY_PRESSED && On 2014/05/14 14:01:38, Seigo Nonaka wrote: > Could you add comment why we don't allow the vkey process key for key up. > And if the IME consumes key up event, the any key up event seems not be fired, > right? Is it acceptable? Done. It's not trivial if we should fire keyup events or not. I don't address that point in this CL. I think omnibox, find-in-page, etc. don't want keyup events if IME consumed it, or do they want? Anyway, I'll work on that point later if and only if it turns out that it matters. https://codereview.chromium.org/277973002/diff/40001/ui/base/ime/input_method... ui/base/ime/input_method_auralinux.cc:146: MaybeFireProcessKey(); On 2014/05/14 14:01:38, Seigo Nonaka wrote: > Please double check the key down event is fired before composition events. Done. I've changed the order of calls to MaybeFireProcessKey().
lgtm
The CQ bit was checked by yukishiino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/277973002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Message was sent while issue was closed.
Change committed as 271363 |
