Closed Bug 855975 Opened 7 years ago Closed 7 years ago

Separate key message handlers from nsWindow to KeyboardLayout or NativeKey

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(22 files)

29.35 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.74 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.96 KB, patch
jimm
: review+
Details | Diff | Splinter Review
14.69 KB, patch
jimm
: review+
Details | Diff | Splinter Review
10.46 KB, patch
jimm
: review+
Details | Diff | Splinter Review
12.78 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.64 KB, patch
jimm
: review+
Details | Diff | Splinter Review
16.05 KB, patch
jimm
: review+
Details | Diff | Splinter Review
17.75 KB, patch
jimm
: review+
Details | Diff | Splinter Review
8.92 KB, patch
jimm
: review+
Details | Diff | Splinter Review
9.60 KB, patch
jimm
: review+
Details | Diff | Splinter Review
23.17 KB, patch
jimm
: review+
Details | Diff | Splinter Review
13.61 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.86 KB, patch
jimm
: review+
Details | Diff | Splinter Review
9.33 KB, patch
jimm
: review+
Details | Diff | Splinter Review
18.51 KB, patch
jimm
: review+
Details | Diff | Splinter Review
23.13 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.94 KB, patch
jimm
: review+
Details | Diff | Splinter Review
22.13 KB, patch
jimm
: review+
Details | Diff | Splinter Review
10.96 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.90 KB, patch
jimm
: review+
Details | Diff | Splinter Review
2.96 KB, patch
jimm
: review+
Details | Diff | Splinter Review
No description provided.
First of all, widget::KeyboardLayout should be a singleton class instead of a normal class but created only on instance. This makes widget::KeyboardLayout available on everywhere.
Attachment #749678 - Flags: review?(jmathies)
For simpler code, this patch just wraps ::MapVirtualKeyEx() with widget::NativeKey.
Attachment #749679 - Flags: review?(jmathies)
Similar to part.2, but this patch wraps ::MapVirtualKeyEx() for converting virtual keycode to scancode with widget::KeyboardLayout.
Attachment #749680 - Flags: review?(jmathies)
This patch moves nsWindow::InitKeyEvent() to widget::NativeKey::InitKeyEvent().
Attachment #749684 - Flags: review?(jmathies)
This patch moves nsWindow::DispatchKeyEvent() to widget::NativeKey::DispatchKeyEvent().
Attachment #749685 - Flags: review?(jmathies)
This patch moves nsWindow::OnKeyUp() to widget::NativeKey::HandleKeyUpMessage().

widget::NativeKey stores the native message with mMsg. All code using hwnd for handling message should use mMsg.hwnd rather than mWidget->GetWindowHandle(). mWidget should be just a widget on which DOM events are fired. So, this change allows widget::NativeKey handles messages for a window on any window.
Attachment #749689 - Flags: review?(jmathies)
This patch implements widget::NativeKey::DispatchKeyDownEvent(). This will be private method later.
Attachment #749701 - Flags: review?(jmathies)
This patch just moves nsWindow::OnChar() to widget::NativeKey::HandleCharMessage().
Attachment #749702 - Flags: review?(jmathies)
This patch moves the "key hell" part of nsWindow::OnKeyDown() to widget::NativeKey::DispatchKeyPressEventsWithKeyboardLayout().

This method will be a private method later.
Attachment #749703 - Flags: review?(jmathies)
This patch moves nsWindow::DispatchPluginEvent() to nsWindowBase::DispatchPluginEvent().
Attachment #749707 - Flags: review?(jmathies)
This patch make a method of widget::NativeKey, NeedsToHandleWithoutFollowingCharMessages().

It checks whether all following WM_CHAR or WM_SYSCHAR messages should be removed and the keydown message handler needs to dispatch keypress events without the removed messages.

For internal use of NeedsToHandleWithoutFollowingCharMessages(), this also makes IsDeadKey() and IsPrintableKey(). These methods are also useful for making the nsWindow::OnKeyDown() simpler.

All methods which are made by this patch will be private methods later.
Attachment #749708 - Flags: review?(jmathies)
The main purpose of this patch is, the internal block of
|if (nativeKey.NeedsToHandleWithoutFollowingCharMessages()) {}|
moves to an independent method.

For that, this patch also moves nsWindow::RemoveMessageAndDispatchPluginEvent() and widget::IMEHandler::IsIMEDoingKakuteiUndo() to widget::NativeKey::RemoveMessageAndDispatchPluginEvent() and widget::NativeKey::IsIMEDoingKakuteiUndo().

Additionally, while I test this patch, I realized that WinUtils::InitMSG() doesn't initialize MSG::hwnd. It's bad for this bug's refactoring. Therefore, I also change the method by this patch.
Attachment #749710 - Flags: review?(jmathies)
This patch adds mCharMsg to widget::NativeKey. It stores following WM_CHAR, WM_SYSCHAR or WM_DEADCHAR message of a WM_KEYDOWN or WM_SYSKEYDOWN message.

Then, this patch makes normal text input path of nsWindow::OnKeyDown() with making widget::NativeKey::IsFollowedByCharMessage() and widget::NativeKey::RemoveFollowingCharMessage().

These new methods will be private later.
Attachment #749716 - Flags: review?(jmathies)
This patch moves internal block of |if (nativeKey.IsFollowedByCharMessage()) {) | in nsWindow::OnKeyDown().  I.e., the block handles normal text input path.
Attachment #749717 - Flags: review?(jmathies)
By the previous patches, nsWindow::OnKeyDown() doesn't use the result of widget::NativeKey::GetCommittedCharsAndModifiers(). In other words, NativeKey instances are the only user of the value.

Therefore, NativeKey should compute the value and store it in its constructor. Then, DispatchKeyPressEvent*() don't need the aInputtingChars argument.
Attachment #749719 - Flags: review?(jmathies)
This makes widget::RedirectedKeyDownMessageManager in KeyboardLayout.h and KeyboardLayout.cpp.

The messy code in nsWindow for this is now sorted out.
Attachment #749736 - Flags: review?(jmathies)
This patch moves nsWindow::OnKeyDown() to widget::NativeKey::HandleKeyDownMessage(). For making the patch simple, following patch will change the scope of some methods of widget::NativeKey.
Attachment #749756 - Flags: review?(jmathies)
This patch merges widget::NativeKey::DispatchKeyDownEvent() into HandleKeyDownMessage() since it becomes simpler.

Note that RedirectedKeyDownMessageManager::Forget() call is removed by this patch. However, Alt+Space is never handled by HandleKeyDownMessage() with the code, therefore, the message is never redirected. So, we don't need to call it at returning at Alt+Space case.
Attachment #749763 - Flags: review?(jmathies)
This patch just moves nsWindow::SynthesizeNativeKeyEvent() to widget::KeyboardLayout::SynthesizeNativeKeyEvent().
Attachment #749764 - Flags: review?(jmathies)
This patch makes some methods as private. Additionally, some methods which simply getter of a member such as IsDeadKey() and IsPrintableKey() are removed.
Attachment #749766 - Flags: review?(jmathies)
This patch moves nsFakeCharMessage in nsWindowDefs.h to widget::NativeKey::FakeCharMsg in KeyboardLayout.h because the struct is only necessary in KeyboardLayout.cpp.
Attachment #749767 - Flags: review?(jmathies)
sModifierKeyMap in nsWindowDefs.h maps between constants of nsIWidget and native virtual keycode values. Such mapping table should be in KeyboardLayout.h since it's really necessary for key event handling.
Attachment #749769 - Flags: review?(jmathies)
Attachment #749678 - Flags: review?(jmathies) → review+
Comment on attachment 749679 [details] [diff] [review]
part.2 Wrap MapVirtualKeyEx() API for converting native key event to VK or Unichar with widget::NativeKey

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

::: widget/windows/KeyboardLayout.cpp
@@ +636,5 @@
> +NativeKey::ComputeVirtualKeyCodeFromScanCodeEx() const
> +{
> +  bool VistaOrLater =
> +    (WinUtils::GetWindowsVersion() >= WinUtils::VISTA_VERSION);
> +  NS_ENSURE_TRUE(!mIsExtended || VistaOrLater, 0);

Can you add a comment explaining why we exit on vista and later here?
Attachment #749679 - Flags: review?(jmathies) → review+
Attachment #749680 - Flags: review?(jmathies) → review+
Comment on attachment 749684 [details] [diff] [review]
part.4 Move nsWindow::InitKeyEvent() to widget::NativeKey::InitKeyEvent()

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

::: widget/windows/KeyboardLayout.cpp
@@ +385,4 @@
>                       const MSG& aKeyOrCharMessage,
>                       const ModifierKeyState& aModKeyState) :
> +  mWidget(aWidget), mDOMKeyCode(0), mMessage(aKeyOrCharMessage.message),
> +  mModKeyState(aModKeyState), mVirtualKeyCode(0), mOriginalVirtualKeyCode(0)

Is there any chance we might get a null widget here? Maybe we should add a debug assert that checks.
Attachment #749684 - Flags: review?(jmathies) → review+
Comment on attachment 749685 [details] [diff] [review]
part.5 Move nsWindow::DispatchKeyEvent() to widget::NativeKey::DispatchKeyEvent()

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

::: widget/windows/KeyboardLayout.cpp
@@ +698,5 @@
>    if (!sInstance) {
>      sInstance = new KeyboardLayout();
> +    nsCOMPtr<nsIIdleServiceInternal> idleService =
> +      do_GetService("@mozilla.org/widget/idleservice;1");
> +    sIdleService = idleService.forget().get();

Please add a comment here noting this addrefs.
Attachment #749685 - Flags: review?(jmathies) → review+
Attachment #749689 - Flags: review?(jmathies) → review+
Comment on attachment 749701 [details] [diff] [review]
part.7 Implement widget::NativeKey::DispatchKeyDownEvent()

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

::: widget/windows/KeyboardLayout.h
@@ +331,5 @@
>    bool DispatchKeyEvent(nsKeyEvent& aKeyEvent,
>                          const MSG* aMsgSentToPlugin = nullptr) const;
>  
>    /**
> +   * Dspatces keydown event.  Retrns true if tte event is consumed.

nit - 'tte'
Attachment #749701 - Flags: review?(jmathies) → review+
Attachment #749702 - Flags: review?(jmathies) → review+
Attachment #749703 - Flags: review?(jmathies) → review+
Attachment #749707 - Flags: review?(jmathies) → review+
Attachment #749708 - Flags: review?(jmathies) → review+
Attachment #749710 - Flags: review?(jmathies) → review+
Attachment #749716 - Flags: review?(jmathies) → review+
Attachment #749717 - Flags: review?(jmathies) → review+
Attachment #749719 - Flags: review?(jmathies) → review+
Attachment #749736 - Flags: review?(jmathies) → review+
Attachment #749756 - Flags: review?(jmathies) → review+
Attachment #749763 - Flags: review?(jmathies) → review+
Attachment #749764 - Flags: review?(jmathies) → review+
Attachment #749767 - Flags: review?(jmathies) → review+
Attachment #749769 - Flags: review?(jmathies) → review+
Comment on attachment 749766 [details] [diff] [review]
part.20 Sort out the scope of the methods of widget::NativKey

Thanks for breaking this up so well, it made it much easier to follow along and review.
Attachment #749766 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/57461a161f93
https://hg.mozilla.org/integration/mozilla-inbound/rev/21fb386d7995
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb52492b116
https://hg.mozilla.org/integration/mozilla-inbound/rev/49920d7e1518
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d71ddc7a304
https://hg.mozilla.org/integration/mozilla-inbound/rev/0435d559b314
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f923c894ca4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed82af5475f
https://hg.mozilla.org/integration/mozilla-inbound/rev/91ba04b50639
https://hg.mozilla.org/integration/mozilla-inbound/rev/fef076fed8ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3aebac825b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/658bfe5bb0ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/13350f2b2f04
https://hg.mozilla.org/integration/mozilla-inbound/rev/60420c75c496
https://hg.mozilla.org/integration/mozilla-inbound/rev/269686777a14
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b1704ec0e50
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfef49bc5691
https://hg.mozilla.org/integration/mozilla-inbound/rev/81aadfdcd59d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65ebd90ad9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e9211d9daa
https://hg.mozilla.org/integration/mozilla-inbound/rev/6919e1b2926e
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a6e3a943b6
I pushed a fixup for a typo in include (extra space in file name):

https://hg.mozilla.org/integration/mozilla-inbound/rev/69680cd3fe39
(In reply to Jacek Caban from comment #31)
> I pushed a fixup for a typo in include (extra space in file name):
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69680cd3fe39

Thank you and sorry for my mistake.

Just for my curiosity, does it cause failing to build with MinGW or something?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 5/28 - 6/1) from comment #33)
> Thank you and sorry for my mistake.

No problem.

> Just for my curiosity, does it cause failing to build with MinGW or
> something?

Yes, I found it because it broke MinGW build. I think GCC tried to find a file that contained this space in its name.
Depends on: 907657
You need to log in before you can comment on or make changes to this bug.