Refactor KeyboardLayout

RESOLVED FIXED in mozilla16

Status

()

Core
Widget: Win32
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla16
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 6 obsolete attachments)

8.41 KB, patch
jimm
: review+
Details | Diff | Splinter Review
53.52 KB, patch
jimm
: review+
Details | Diff | Splinter Review
29.68 KB, patch
jimm
: review+
Details | Diff | Splinter Review
11.21 KB, patch
jimm
: review+
Details | Diff | Splinter Review
33.22 KB, patch
jimm
: review+
Details | Diff | Splinter Review
13.41 KB, patch
jimm
: review+
Details | Diff | Splinter Review
8.11 KB, patch
Details | Diff | Splinter Review
22.20 KB, patch
jimm
: review+
Details | Diff | Splinter Review
17.13 KB, patch
jimm
: review+
Details | Diff | Splinter Review
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.
Created attachment 626291 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless
Created attachment 626322 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless
Attachment #626291 - Attachment is obsolete: true
Created attachment 626364 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it
Created attachment 626728 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless
Attachment #626322 - Attachment is obsolete: true
Created attachment 626729 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it
Attachment #626364 - Attachment is obsolete: true
Created attachment 626730 [details] [diff] [review]
part.3 Use widget::Modifiers for the arguments of public methods of KeyboardLayout
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.
Attachment #626728 - Attachment is obsolete: true
Attachment #626729 - Attachment is obsolete: true
Attachment #626730 - Attachment is obsolete: true
Attachment #631306 - Flags: review?(jmathies)
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.
Attachment #631307 - Flags: review?(jmathies)
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.
Attachment #631309 - Flags: review?(jmathies)
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.
Attachment #631312 - Flags: review?(jmathies)
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.
Attachment #631313 - Flags: review?(jmathies)
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.
Attachment #631315 - Flags: review?(jmathies)
Created attachment 631316 [details] [diff] [review]
part.6 with -w
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.
Attachment #631320 - Flags: review?(jmathies)
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.
Attachment #631324 - Flags: review?(jmathies)
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

5 years ago
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.
Attachment #631306 - Flags: review?(jmathies) → review+

Comment 18

5 years ago
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.
Attachment #631307 - Flags: review?(jmathies) → review+

Updated

5 years ago
Attachment #631309 - Flags: review?(jmathies) → review+

Comment 19

5 years ago
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?
Attachment #631312 - Flags: review?(jmathies) → review+

Comment 20

5 years ago
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.
Attachment #631313 - Flags: review?(jmathies) → review+

Updated

5 years ago
Attachment #631315 - Flags: review?(jmathies) → review+

Comment 21

5 years ago
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

5 years ago
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.
Attachment #631324 - Flags: review?(jmathies) → review+
(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 :-)
(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.
(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.

Updated

5 years ago
Attachment #631320 - Flags: review?(jmathies) → review+
Finally, the tests passed! Windows machines are too slow :-(
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7abf9311d4d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa71abf5c39
https://hg.mozilla.org/integration/mozilla-inbound/rev/82d06f0f02d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de03e1fb9dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7af55c4536
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd9de8d2e181
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bed04319ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b8c6a545cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b46347c04b

Thank you, Jim for your review!
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/efa71abf5c39
https://hg.mozilla.org/mozilla-central/rev/82d06f0f02d0
https://hg.mozilla.org/mozilla-central/rev/3de03e1fb9dd
https://hg.mozilla.org/mozilla-central/rev/5d7af55c4536
https://hg.mozilla.org/mozilla-central/rev/fd9de8d2e181
https://hg.mozilla.org/mozilla-central/rev/d3bed04319ab
https://hg.mozilla.org/mozilla-central/rev/f5b8c6a545cf
https://hg.mozilla.org/mozilla-central/rev/c4b46347c04b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.