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.

For simpler code, this patch just wraps ::MapVirtualKeyEx() with widget::NativeKey.

Similar to part.2, but this patch wraps ::MapVirtualKeyEx() for converting virtual keycode to scancode with widget::KeyboardLayout.

This patch moves nsWindow::InitKeyEvent() to widget::NativeKey::InitKeyEvent().

This patch moves nsWindow::DispatchKeyEvent() to widget::NativeKey::DispatchKeyEvent().

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.

This patch implements widget::NativeKey::DispatchKeyDownEvent(). This will be private method later.

This patch just moves nsWindow::OnChar() to widget::NativeKey::HandleCharMessage().

This patch moves the "key hell" part of nsWindow::OnKeyDown() to widget::NativeKey::DispatchKeyPressEventsWithKeyboardLayout(). This method will be a private method later.

This patch moves nsWindow::DispatchPluginEvent() to nsWindowBase::DispatchPluginEvent().

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.

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.

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.

This patch moves internal block of |if (nativeKey.IsFollowedByCharMessage()) {) | in nsWindow::OnKeyDown(). I.e., the block handles normal text input path.

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.

This makes widget::RedirectedKeyDownMessageManager in KeyboardLayout.h and KeyboardLayout.cpp. The messy code in nsWindow for this is now sorted out.

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.

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.

This patch just moves nsWindow::SynthesizeNativeKeyEvent() to widget::KeyboardLayout::SynthesizeNativeKeyEvent().

This patch makes some methods as private. Additionally, some methods which simply getter of a member such as IsDeadKey() and IsPrintableKey() are removed.

This patch moves nsFakeCharMessage in nsWindowDefs.h to widget::NativeKey::FakeCharMsg in KeyboardLayout.h because the struct is only necessary in KeyboardLayout.cpp.

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.

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?

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.

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.

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'

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.

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

https://hg.mozilla.org/mozilla-central/rev/90a6e3a943b6 https://hg.mozilla.org/mozilla-central/rev/6919e1b2926e https://hg.mozilla.org/mozilla-central/rev/41e9211d9daa https://hg.mozilla.org/mozilla-central/rev/f65ebd90ad9e https://hg.mozilla.org/mozilla-central/rev/81aadfdcd59d https://hg.mozilla.org/mozilla-central/rev/cfef49bc5691 https://hg.mozilla.org/mozilla-central/rev/7b1704ec0e50 https://hg.mozilla.org/mozilla-central/rev/269686777a14 https://hg.mozilla.org/mozilla-central/rev/60420c75c496 https://hg.mozilla.org/mozilla-central/rev/13350f2b2f04 https://hg.mozilla.org/mozilla-central/rev/658bfe5bb0ea https://hg.mozilla.org/mozilla-central/rev/d3aebac825b9 https://hg.mozilla.org/mozilla-central/rev/fef076fed8ab https://hg.mozilla.org/mozilla-central/rev/91ba04b50639 https://hg.mozilla.org/mozilla-central/rev/0ed82af5475f https://hg.mozilla.org/mozilla-central/rev/1f923c894ca4 https://hg.mozilla.org/mozilla-central/rev/0435d559b314 https://hg.mozilla.org/mozilla-central/rev/0d71ddc7a304 https://hg.mozilla.org/mozilla-central/rev/49920d7e1518 https://hg.mozilla.org/mozilla-central/rev/bfb52492b116 https://hg.mozilla.org/mozilla-central/rev/21fb386d7995 https://hg.mozilla.org/mozilla-central/rev/57461a161f93

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.