As WM_KEYDOWN or WM_KEYUP doesn't provide WM_LCONTROL, VM_RCONTROL, ..., WebInputFactory::keyboardEvent (i.e. Source/WebKit/chromium/src/win/WebInputEventFactory.cpp) should check which key is pressed, left or right.
Created attachment 149232 [details] Patch
Comment on attachment 149232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149232&action=review > Source/WebKit/chromium/ChangeLog:10 > + As WM_KEYDOWN, WM_KEYUP, WM_SYSKEYDOWN, and WM_SYSKEYUP doesn't provide > + a virtual keycode which distinguish between left-hand and right-hand > + keys. So modified keybordEvent to see a scancode and an extended key You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam. > Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99 > +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam) Why is "param" in lParam capitalized? It appears that the convention is not to capitalize it. > Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:101 > + int keycode = static_cast<int>(wparam); Why are we casting it to int? > Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118 > + Where is the closing } ?? > Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124 > + int scancode = (lParam >> 16) & 0xFF; > + // WindowsVista or new Windows can support 0xe0 or 0xe1 to specify > + // the extended scancode. In this case, if extended key bit is set, > + // extended scancode is 0xe000 | scancode or 0xe100 | scancode. > + int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX); > + return virtualKeyCode ? virtualKeyCode : keycode; For which virtual key code is this code used? > Tools/DumpRenderTree/win/EventSender.cpp:486 > + // Keyboard Scan Code Specification: > + // http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/scancode.doc > + keyData += 0x1d << 16; // Left control's scancode Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC?
Comment on attachment 149232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149232&action=review r- mostly due to Ryosuke's comments - it's clear that a new patch and another round of review will be needed. > Source/WebKit/chromium/ChangeLog:14 > + * src/win/WebInputEventFactory.cpp: > + (WebKit::windowsKeycodeWithLocation): This code seems port neutral. Why does it have to be in chromium directory? What do other ports do? > Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123 > + // WindowsVista or new Windows can support 0xe0 or 0xe1 to specify > + // the extended scancode. In this case, if extended key bit is set, > + // extended scancode is 0xe000 | scancode or 0xe100 | scancode. > + int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX); I have a lot of difficulty understanding this comment, and how it relates to the code below.
Comment on attachment 149232 [details] Patch r- per my and ap's comments.
Created attachment 161883 [details] Patch
Comment on attachment 149232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149232&action=review Thank you for reviewing. >> Source/WebKit/chromium/ChangeLog:10 >> + keys. So modified keybordEvent to see a scancode and an extended key > > You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam. Yes, I mean wParam. I know lParam has extended key bit. However, it is not A VIRTUAL KEYCODE. We have to re-create a new virtual keycode by using the given scancode, extended key bit, and virtual keycode. So I wrote "So modified keybordEvent to see a scancode and an extended key bit in lParam to distinguish." >> Source/WebKit/chromium/ChangeLog:14 >> + (WebKit::windowsKeycodeWithLocation): > > This code seems port neutral. Why does it have to be in chromium directory? What do other ports do? I think, this code is very windows-specific. As far as I know, Linux+GTK uses XKeyEvent and it contains a key symbol which distinguishes between left- and right-hand keys (e.g. keysym 0xffe1, Shift_L or keysym 0xffe2, Shift_R). (I read chromium/src/content/browser/renderer_host/native_web_keyboard_event_gtk.cc) >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99 >> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam) > > Why is "param" in lParam capitalized? > It appears that the convention is not to capitalize it. Done. >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118 >> + > > Where is the closing } ?? I'm sorry. I have just prepared windows development environment. (So I was planning to see WebKit bot's result.) Now I can build and do tests in Windows. I tested my new patch. >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123 >> + int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX); > > I have a lot of difficulty understanding this comment, and how it relates to the code below. I rewrote the comment and moved the comment before "if ((lParam >> 16) & KF_EXTENDED)". I mean, for example, the code always returns VK_RCONTROL in the case where the given lparam's extended key bit is set and the wparam is VK_CONTROL. However, it might be better to use MapVirtualKey to recreate a new virtual keycode. I think, the conversion should be done by Windows API. >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124 >> + return virtualKeyCode ? virtualKeyCode : keycode; > > For which virtual key code is this code used? Done. >> Tools/DumpRenderTree/win/EventSender.cpp:486 >> + keyData += 0x1d << 16; // Left control's scancode > > Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC? Done.
Comment on attachment 161883 [details] Patch Attachment 161883 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13729804
Created attachment 161982 [details] Patch
Comment on attachment 161982 [details] Patch Attachment 161982 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13745377
Created attachment 162003 [details] Patch
(In reply to comment #6) > (From update of attachment 149232 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149232&action=review > > Thank you for reviewing. > > >> Source/WebKit/chromium/ChangeLog:10 > >> + keys. So modified keybordEvent to see a scancode and an extended key > > > > You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam. > > Yes, I mean wParam. I know lParam has extended key bit. However, it is not A VIRTUAL KEYCODE. We have to re-create a new virtual keycode by using the given scancode, extended key bit, and virtual keycode. So I wrote "So modified keybordEvent to see a scancode and an extended key bit in lParam to distinguish." > > >> Source/WebKit/chromium/ChangeLog:14 > >> + (WebKit::windowsKeycodeWithLocation): > > > > This code seems port neutral. Why does it have to be in chromium directory? What do other ports do? > > I think, this code is very windows-specific. As far as I know, Linux+GTK uses XKeyEvent and it contains a key symbol which distinguishes between left- and right-hand keys (e.g. keysym 0xffe1, Shift_L or keysym 0xffe2, Shift_R). > (I read chromium/src/content/browser/renderer_host/native_web_keyboard_event_gtk.cc) Sure, but can it be shared with the Apple Windows port? > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99 > >> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam) > > > > Why is "param" in lParam capitalized? > > It appears that the convention is not to capitalize it. > > Done. > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118 > >> + > > > > Where is the closing } ?? > > I'm sorry. I have just prepared windows development environment. (So I was planning to see WebKit bot's result.) Now I can build and do tests in Windows. I tested my new patch. > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123 > >> + int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX); > > > > I have a lot of difficulty understanding this comment, and how it relates to the code below. > > I rewrote the comment and moved the comment before "if ((lParam >> 16) & KF_EXTENDED)". I mean, for example, the code always returns VK_RCONTROL in the case where the given lparam's extended key bit is set and the wparam is VK_CONTROL. However, it might be better to use MapVirtualKey to recreate a new virtual keycode. I think, the conversion should be done by Windows API. > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124 > >> + return virtualKeyCode ? virtualKeyCode : keycode; > > > > For which virtual key code is this code used? > > Done. > > >> Tools/DumpRenderTree/win/EventSender.cpp:486 > >> + keyData += 0x1d << 16; // Left control's scancode > > > > Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC? > > Done.
Created attachment 162214 [details] Patch
(In reply to comment #11) > (In reply to comment #6) > > (From update of attachment 149232 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=149232&action=review > > > > Thank you for reviewing. > > > > >> Source/WebKit/chromium/ChangeLog:10 > > >> + keys. So modified keybordEvent to see a scancode and an extended key > > > > > > You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam. > > > > Yes, I mean wParam. I know lParam has extended key bit. However, it is not A VIRTUAL KEYCODE. We have to re-create a new virtual keycode by using the given scancode, extended key bit, and virtual keycode. So I wrote "So modified keybordEvent to see a scancode and an extended key bit in lParam to distinguish." > > > > >> Source/WebKit/chromium/ChangeLog:14 > > >> + (WebKit::windowsKeycodeWithLocation): > > > > > > This code seems port neutral. Why does it have to be in chromium directory? What do other ports do? > > > > I think, this code is very windows-specific. As far as I know, Linux+GTK uses XKeyEvent and it contains a key symbol which distinguishes between left- and right-hand keys (e.g. keysym 0xffe1, Shift_L or keysym 0xffe2, Shift_R). > > (I read chromium/src/content/browser/renderer_host/native_web_keyboard_event_gtk.cc) > > Sure, but can it be shared with the Apple Windows port? I see. I moved the code to WebCore/platform/win/KeyEventWin.cpp. Best regards, Takashi Sakamoto > > > > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99 > > >> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam) > > > > > > Why is "param" in lParam capitalized? > > > It appears that the convention is not to capitalize it. > > > > Done. > > > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118 > > >> + > > > > > > Where is the closing } ?? > > > > I'm sorry. I have just prepared windows development environment. (So I was planning to see WebKit bot's result.) Now I can build and do tests in Windows. I tested my new patch. > > > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123 > > >> + int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX); > > > > > > I have a lot of difficulty understanding this comment, and how it relates to the code below. > > > > I rewrote the comment and moved the comment before "if ((lParam >> 16) & KF_EXTENDED)". I mean, for example, the code always returns VK_RCONTROL in the case where the given lparam's extended key bit is set and the wparam is VK_CONTROL. However, it might be better to use MapVirtualKey to recreate a new virtual keycode. I think, the conversion should be done by Windows API. > > > > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124 > > >> + return virtualKeyCode ? virtualKeyCode : keycode; > > > > > > For which virtual key code is this code used? > > > > Done. > > > > >> Tools/DumpRenderTree/win/EventSender.cpp:486 > > >> + keyData += 0x1d << 16; // Left control's scancode > > > > > > Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC? > > > > Done.
Comment on attachment 162214 [details] Patch Attachment 162214 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13742877
Created attachment 162416 [details] Patch
Comment on attachment 162416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162416&action=review r- due to style issues. > Source/WebCore/ChangeLog:9 > + Additional information of the change such as approach, rationale. Pleas As WM_KEYDOWN, WM_KEYUP, WM_SYSKEYDOWN, and WM_SYSKEYUP doesn't > + directly provide a virtual keycode which distinguish between left-hand These two lines of changelogs are messed up. > Source/WebCore/platform/win/KeyEventWin.cpp:189 > +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lparam) It's better to give these argument variable more descriptive names like keycode. > Source/WebCore/platform/win/KeyEventWin.cpp:196 > + // 24bit: Indicates whether the key is an extended key, such as > + // the right-hand ALT and CTRL keys. This is quite obvious from the code. > Source/WebCore/platform/win/KeyEventWin.cpp:197 > + // However, if we don't need to support WindowsXP or older Windows, Nit: Windows XP.
Created attachment 166148 [details] Patch
Created attachment 166154 [details] Patch
Comment on attachment 162416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162416&action=review Thank you for reviewing. >> Source/WebCore/ChangeLog:9 >> + directly provide a virtual keycode which distinguish between left-hand > > These two lines of changelogs are messed up. Done. >> Source/WebCore/platform/win/KeyEventWin.cpp:196 >> + // the right-hand ALT and CTRL keys. > > This is quite obvious from the code. Done. >> Source/WebCore/platform/win/KeyEventWin.cpp:197 >> + // However, if we don't need to support WindowsXP or older Windows, > > Nit: Windows XP. Done.
Comment on attachment 162416 [details] Patch Cleared review? from obsolete attachment 162416 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
(In reply to comment #20) > (From update of attachment 162416 [details]) > Cleared review? from obsolete attachment 162416 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). I see. I uploaded a new patch, i.e. https://bug-89742-attachments.webkit.org/attachment.cgi?id=166154. Best regards, Takashi Sakamoto
Comment on attachment 166154 [details] Patch r=me
Committed r134855: <http://trac.webkit.org/changeset/134855>