Handle XK_Zenkaku_Hankaku, Henkan, Muhenkan, Hangul etc. in Chrome

Handle XK_Zenkaku_Hankaku, Henkan, Muhenkan, Hangul etc. in Chrome.

BUG=chromium-os:13836
TEST=manually checked with Korean/Japanese keyboards


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100680

[Zachary Kuznia, 2011-09-12 09:48:07]

LGTM

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager.cc (right):

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:1130: const size_t
index = static_cast<size_t>(event) - 2;
This line looks scary.  Please give it a reassuring comment.

[mazda, 2011-09-12 10:02:54]

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager.cc (right):

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:60: bool
trigger_on_press;
What happens if trigger_on_press is false?

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:288:
UpdateEngineSpecificHotkeys();
Is it OK that UpdateEngineSpecificHotkeys is called before the IDs in
extra_input_methods_ids_ are added to active_input_method_ids_?

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:427:
SwitchToInputMethod(event);
default case.

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:1124: break;
Are these two error case?

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:1139: if
(!input_method_ids_to_switch) {
How about moving this to the default case of the switch statement?

[Yusuke Sato, 2011-09-12 11:46:05]

mazda: PTAL

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager.cc (right):

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:60: bool
trigger_on_press;
On 2011/09/12 10:02:54, mazda wrote:
> What happens if trigger_on_press is false?

IME or layout is changed on key release. The Shift+Alt hotkey uses "trigger on
release" not to interfere with other hotkeys like Shift+Alt+Tab.

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:288:
UpdateEngineSpecificHotkeys();
On 2011/09/12 10:02:54, mazda wrote:
> Is it OK that UpdateEngineSpecificHotkeys is called before the IDs in
> extra_input_methods_ids_ are added to active_input_method_ids_?

For now it's okay since hotkey information for an IME extension is not passed to
this class. I'll improve the class so IME extensions could support hotkeys.
Added some TODOs.

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:427:
SwitchToInputMethod(event);
Since HotkeyEvent is an enum, adding default: is not necessary (and has negative
effect - it disables gcc's warning capability.)

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:1124: break;
On 2011/09/12 10:02:54, mazda wrote:
> Are these two error case?

added a comment.

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:1130: const size_t
index = static_cast<size_t>(event) - 2;
On 2011/09/12 09:48:07, Zachary Kuznia wrote:
> This line looks scary.  Please give it a reassuring comment.

Probably this was too error-prone. I've written this in a safer way (using for
loop). Thanks.

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:1139: if
(!input_method_ids_to_switch) {
On 2011/09/12 10:02:54, mazda wrote:
> How about moving this to the default case of the switch statement?

ditto. I prefer not to use default: in order to keep gcc's warning capability
enabled.

[mazda, 2011-09-12 11:57:31]

LGTM

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager.cc (right):

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:60: bool
trigger_on_press;
Thanks.
Probably it's better to add comment since the opposite of 'trigger on press' can
mean anything else like 'do not trigger on press'.

On 2011/09/12 11:46:05, Yusuke Sato wrote:
> On 2011/09/12 10:02:54, mazda wrote:
> > What happens if trigger_on_press is false?
> 
> IME or layout is changed on key release. The Shift+Alt hotkey uses "trigger on
> release" not to interfere with other hotkeys like Shift+Alt+Tab.
> 
> 
> 
> 
>

[Yusuke Sato, 2011-09-12 12:02:14]

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
File chrome/browser/chromeos/input_method/input_method_manager.cc (right):

http://codereview.chromium.org/7865019/diff/4001/chrome/browser/chromeos/inpu...
chrome/browser/chromeos/input_method/input_method_manager.cc:60: bool
trigger_on_press;
On 2011/09/12 11:57:31, mazda wrote:
> Thanks.
> Probably it's better to add comment since the opposite of 'trigger on press'
can
> mean anything else like 'do not trigger on press'.
> 
> On 2011/09/12 11:46:05, Yusuke Sato wrote:
> > On 2011/09/12 10:02:54, mazda wrote:
> > > What happens if trigger_on_press is false?
> > 
> > IME or layout is changed on key release. The Shift+Alt hotkey uses "trigger
on
> > release" not to interfere with other hotkeys like Shift+Alt+Tab.
> > 
> > 
> > 
> > 
> > 
> 

Done.

[commit-bot: I haz the power, 2011-09-12 14:01:35]

Change committed as 100680