Last Comment Bug 757688 - Refactor KeyboardLayout
: Refactor KeyboardLayout
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla16
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Jim Mathies [:jimm]
Mentors:
Depends on:
Blocks: 680830
  Show dependency treegraph
 
Reported: 2012-05-22 18:14 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-06-16 06:53 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part.1 Make KeyboardLayout::IsDeadKey() stateless (8.51 KB, patch)
2012-05-22 19:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.1 Make KeyboardLayout::IsDeadKey() stateless (8.53 KB, patch)
2012-05-22 21:57 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it (50.23 KB, patch)
2012-05-23 01:26 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.1 Make KeyboardLayout::IsDeadKey() stateless (8.41 KB, patch)
2012-05-24 01:37 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it (50.56 KB, patch)
2012-05-24 01:37 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.3 Use widget::Modifiers for the arguments of public methods of KeyboardLayout (29.50 KB, patch)
2012-05-24 01:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.1 Make KeyboardLayout::IsDeadKey() stateless (8.41 KB, patch)
2012-06-08 01:56 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it (53.52 KB, patch)
2012-06-08 02:03 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.3 Don't use VirtualKey::ShiftState for the arguments of public methods of KeyboardLayout (29.68 KB, patch)
2012-06-08 02:08 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.4 Remove GetShiftState() and move SetShiftState() to VirtualKey (11.21 KB, patch)
2012-06-08 02:16 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.5 Make KeyboardLayout stateless for non-dead keys (33.22 KB, patch)
2012-06-08 02:23 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.6 Add numpad keys to printable key table and remove KeyboardLayout::IsNumpadKey() (13.41 KB, patch)
2012-06-08 02:26 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.6 with -w (8.11 KB, patch)
2012-06-08 02:28 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
part.7 Make nsWindow for Windows possible to test dead keys (22.20 KB, patch)
2012-06-08 02:39 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review
part.8 Make sure test_keycodes.xul emulates correct key events (17.13 KB, patch)
2012-06-08 02:54 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-22 18:14:12 PDT
I'm now working on bug 680830. When I'm thinking about some complex cases of keyup events, current KeyboardLayout isn't useful because it's stateful but only stores latest keydown. So, it cannot handle if multiple keys are pressed. I think that most function can be non-stateful.
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-22 19:34:35 PDT
Created attachment 626291 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless
Comment 2 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-22 21:57:05 PDT
Created attachment 626322 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-23 01:26:43 PDT
Created attachment 626364 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-24 01:37:13 PDT
Created attachment 626728 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless
Comment 5 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-24 01:37:40 PDT
Created attachment 626729 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-05-24 01:38:07 PDT
Created attachment 626730 [details] [diff] [review]
part.3 Use widget::Modifiers for the arguments of public methods of KeyboardLayout
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 01:56:24 PDT
Created attachment 631306 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless

First of all, KeyboardLayout::OnKeyDown() stores a lot of information which include what can be computed anytime. That makes some methods stateful. We should make them stateless as far as possible.

This patch removes mLast* from KeyboardLayout. They are used for IsDeadKey() only. It doesn't make sense.
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:03:09 PDT
Created attachment 631307 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it

nsModifierKeyState is useful class for initializing modifier state and specifies the state to other methods. So, it's useful for KeyboardLayout methods. And it's really handling key states. I think that it should be defined in KeyboardLayout.h.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:08:40 PDT
Created attachment 631309 [details] [diff] [review]
part.3 Don't use VirtualKey::ShiftState for the arguments of public methods of KeyboardLayout

This removes ShiftState from nsWindow. It is defined in KeyboardLayout.h and used by arguments of KeyboardLayout methods. Instead, we can use ModifierKeyState class or Modifiers flags. This makes other callers simpler.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:16:26 PDT
Created attachment 631312 [details] [diff] [review]
part.4 Remove GetShiftState() and move SetShiftState() to VirtualKey

KeyboardLayout::OnKeyDown() should take ModifierKeyState for its argument. Then, it doesn't need to get actual modifier key state from ::GetKeyboardState() and we can get rid of KeyboardLayout::GetShiftState().

Additionally, moves and renames KeyboardLayout::SetShiftState() to VirtualKey::FillKbdState() because ShiftState is now owned and used by VirtualKey class only.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:23:27 PDT
Created attachment 631313 [details] [diff] [review]
part.5 Make KeyboardLayout stateless for non-dead keys

If KeyboardLayout::OnKeyDown() returns the characters which are inputted by the key event, KeyboardLayout::GetUniChars() won't necessary. And also KeyboardLayout::mChars, mModifiersOfChars and mNumOfChars.

Then, we can rename GetUniCharsWithShiftState() to GetUniCharsAndModifiers().

However, there is a problem. It's too ugly to use PRUnichar[5], Modifiers[5] and PRUint8 for the result.

For solving this problem, this patch makes UniCharsAndModifiers struct. This makes nsWindow::OnKeyDown() simpler.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:26:40 PDT
Created attachment 631315 [details] [diff] [review]
part.6 Add numpad keys to printable key table and remove KeyboardLayout::IsNumpadKey()

This registers the NumPad keys are printable keys. This makes nsWindow simpler. I'll post -w version of this patch.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:28:01 PDT
Created attachment 631316 [details] [diff] [review]
part.6 with -w
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:39:29 PDT
Created attachment 631320 [details] [diff] [review]
part.7 Make nsWindow for Windows possible to test dead keys

This bug and bug 680830 change a lot of code in key event handling. And I'm not a Western language user. So, I may take some mistakes for dead key handling.

This patch allows to test dead key handling code in KeyboardLayout::OnKeyDown().

KeyboardLayout::LoadLayout() resets dead key state if different keyboard layout is loaded. This patch allows delayed loading when restores original keyboard layout. I.e., tests can perform testing for two or more key inputs.

And also I found a serious bug of the API. ::UnloadKeyboardLayout() unloads the loaded keyboard layout from the system rather than the caller's thread or process. So, it unloads keyboard layouts which are already added by the users. This patch also checks whether the keyboard layout has been loaded on the system already.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 02:54:43 PDT
Created attachment 631324 [details] [diff] [review]
part.8 Make sure test_keycodes.xul emulates correct key events

If neither Ctrl key nor Alt key is pressed, the result characters of KeyboardLayout::OnKeyDown() is never used for generating keypress event.

That means that most tests in test_keycodes.xul cannot test the result of KeyboardLayout::OnKeyDown(). That really wastes.

This patch adds debug code in handler of fake char message. If the result and fake char message which is specified by tests mismatches, it aborts. So, we can see red in tinderbox.

By this change, I can see some bugs in test_keycodes.xul.

1. The test suite doesn't handle alrGr:1 case.
  1. It doesn't send Ctrl key and Alt key flags to sendNativeKeyEvent().
  2. It doesn't assume that two or more modifier keys are pressed same time. therefore, checking keydown/keyup events' ctrlKey, metaKey, altKey and shiftKey doesn't work. They are checked in isStateChangingModifierKeyEvent() by this patch. And current checking code will run only when the event is keypress event.
  3. It doesn't assume that some modifier keys are consumed by widget when the sent key event causes text input. This adds SHOULD_NOT_CAUSE_INPUT flag for it.

2. Shift + NS_NUMPAD[0-9] don't generate any characters and the key combination generates NumLock unlocked key codes. So, it doesn't make sense to test them.

3. Testing French keyboard's </> key, the results are wrong.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-08 03:00:23 PDT
I think that we can make nsWindow::OnKeyDown() simpler too. However, it should be done later. It doesn't block bug 680830 and is not so important.
Comment 17 Jim Mathies [:jimm] 2012-06-13 07:22:48 PDT
Comment on attachment 631306 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless

Review of attachment 631306 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +6296,5 @@
>      case NS_VK_WIN:
>        return noDefault;
>    }
>  
> +  PRUint8 currentShiftState = KeyboardLayout::GetShiftState(aModKeyState);

You don't appear to be using this call in part1 but I'm assuming I'll come across this in subsequent patches.
Comment 18 Jim Mathies [:jimm] 2012-06-13 07:28:12 PDT
Comment on attachment 631307 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it

Review of attachment 631307 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.cpp
@@ +123,5 @@
> +    case NS_SIMPLE_GESTURE_EVENT:
> +    case NS_MOZTOUCH_EVENT:
> +      break;
> +    default:
> +      return;

Let's assert on this so we get a warning when someone calls InitMouseEvent with an unsupported event.
Comment 19 Jim Mathies [:jimm] 2012-06-13 07:37:59 PDT
Comment on attachment 631312 [details] [diff] [review]
part.4 Remove GetShiftState() and move SetShiftState() to VirtualKey

Review of attachment 631312 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.cpp
@@ +277,5 @@
>  
> +// static
> +void
> +VirtualKey::FillKbdState(PBYTE aKbdState,
> +                         ShiftState aShiftState)

make ShiftState const?
Comment 20 Jim Mathies [:jimm] 2012-06-13 07:50:43 PDT
Comment on attachment 631313 [details] [diff] [review]
part.5 Make KeyboardLayout stateless for non-dead keys

Review of attachment 631313 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.h
@@ +118,5 @@
> +{
> +  // Dead-key + up to 4 characters
> +  PRUnichar chars[5];
> +  Modifiers modifiers[5];
> +  PRUint32  length;

I'm not sure what our convention is here, but I think I'd prefer we use normal member naming. That way there's no confusion in the cpp code which references them.
Comment 21 Jim Mathies [:jimm] 2012-06-13 08:26:25 PDT
Comment on attachment 631320 [details] [diff] [review]
part.7 Make nsWindow for Windows possible to test dead keys

Review of attachment 631320 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.h
@@ +364,5 @@
> +  /**
> +   * LoadLayout() loads the keyboard layout.  If aLoadLater is true,
> +   * it will be done when OnKeyDown() is called.
> +   */
> +  void LoadLayout(HKL aLayout, bool aLoadLater = false);

Curious, why would we want to delay the load until a key down?
Comment 22 Jim Mathies [:jimm] 2012-06-13 08:27:09 PDT
Comment on attachment 631324 [details] [diff] [review]
part.8 Make sure test_keycodes.xul emulates correct key events

Review of attachment 631324 [details] [diff] [review]:
-----------------------------------------------------------------

With all these test changes please make sure a push to try and post results here.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-13 23:03:11 PDT
(In reply to Jim Mathies [:jimm] from comment #17)
> Comment on attachment 631306 [details] [diff] [review]
> part.1 Make KeyboardLayout::IsDeadKey() stateless
> 
> Review of attachment 631306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +6296,5 @@
> >      case NS_VK_WIN:
> >        return noDefault;
> >    }
> >  
> > +  PRUint8 currentShiftState = KeyboardLayout::GetShiftState(aModKeyState);
> 
> You don't appear to be using this call in part1 but I'm assuming I'll come
> across this in subsequent patches.

It's used in next line :-)
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-13 23:42:36 PDT
(In reply to Jim Mathies [:jimm] from comment #20)
> Comment on attachment 631313 [details] [diff] [review]
> part.5 Make KeyboardLayout stateless for non-dead keys
> 
> Review of attachment 631313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.h
> @@ +118,5 @@
> > +{
> > +  // Dead-key + up to 4 characters
> > +  PRUnichar chars[5];
> > +  Modifiers modifiers[5];
> > +  PRUint32  length;
> 
> I'm not sure what our convention is here, but I think I'd prefer we use
> normal member naming. That way there's no confusion in the cpp code which
> references them.

Some public members of struct/class are not prefixed by "m". E.g., most events in nsGUIEvent.h. However, the others have "m" prefix. So, I think that either is okay. I'll add "m" for them.
Comment 25 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-13 23:46:03 PDT
(In reply to Jim Mathies [:jimm] from comment #21)
> Comment on attachment 631320 [details] [diff] [review]
> part.7 Make nsWindow for Windows possible to test dead keys
> 
> Review of attachment 631320 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.h
> @@ +364,5 @@
> > +  /**
> > +   * LoadLayout() loads the keyboard layout.  If aLoadLater is true,
> > +   * it will be done when OnKeyDown() is called.
> > +   */
> > +  void LoadLayout(HKL aLayout, bool aLoadLater = false);
> 
> Curious, why would we want to delay the load until a key down?

During the tests, LoadLayout is called twice a SynthesizeNativeKey() call. One is for changing testing keyboard layout. The other is for restoring original keyboard layout.

When we test dead keys, the restore breaks the dead key state. It's the reason why we cannot test dead keys.

By this change, the restoring is delayed to next keydown. Therefore, it's never restored actually during tests. So, the dead key state isn't broken during testing same keyboard layout.
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-06-15 02:49:55 PDT
Finally, the tests passed! Windows machines are too slow :-(
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7abf9311d4d5

Note You need to log in before you can comment on or make changes to this bug.