[chromium] Implement PlatformKeyboardEvent::getCurrentModifierState for non win/mac
Created attachment 141455 [details] Patch
Comment on attachment 141455 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141455&action=review > Source/WebCore/platform/chromium/PlatformKeyboardEventChromium.cpp:45 > +bool IsKeyPressed(Display* xdisplay, KeySym keySym, char* keys) IsKeyPressed -> isKeyPressed (WebKit style). Also, WebKit prefers using the static keyword to using anonymous namespaces (I'm not sure why). > Source/WebCore/platform/chromium/PlatformKeyboardEventChromium.cpp:50 > + char interestingByte = keys[skippedBytes]; Should we have some kind of bounds check here? > Source/WebCore/platform/chromium/PlatformKeyboardEventChromium.cpp:120 > #else Should we say OS(LINUX) here rather than "else" ? > Source/WebCore/platform/chromium/PlatformKeyboardEventChromium.cpp:121 > + Display* xdisplay = XOpenDisplay(0); I'm surprised we can talk directly to the X server. I'm going to have to defer to a Linux expert.
@tony: Do you know who should look at this patch? It's talking directly to the X server, which makes me wonder if it's correct.
This might run in the browser process when constructing the WebEvent. Avi might remember how this works, he did a bunch of refactoring of input events a long long time ago. It would be easy to manually test this in Chrome. varunjain, did it work for you in Chrome (not test_shell)? Does XKeysymToKeycode make a round trip to the X display? If so, this looks really slow.
> This might run in the browser process when constructing the WebEvent. Nope. The Chromium WebKit API uses the type WebKit::WebKeyboardEvent which is defined in Source/WebKit/chromium/public/WebInputEvent.h. In Source/WebKit/chromium/src/WebInputEventConversion.h there is a subclass of PlatformKeyboardEvent called PlatformKeyboardEventBuilder which is constructed using a WebKeyboardEvent. That's how the conversion is done. But that's rather beside the point. PlatformKeyboardEventChromium exists in order to fill in a few unimplemented functions on PlatformKeyboardEvent for the Chromium port. It lives entirely in WebCore, which means that on Chromium it's sandboxed. And if you look at the existing implementations of currentCapsLockState, you'll find the comment: // FIXME: Does this even work inside the sandbox? return GetKeyState(VK_CAPITAL) & 1; That's an open question. Either it works and the comment should be removed, or no one calls currentCapsLockState. Shrug. But this is definitely going to be called in the sandbox.
Comment on attachment 141455 [details] Patch r- since this won't work in the sandbox. This might work by making it a sync IPC back to the browser process and handling it on the render thread. There's some similar code in src/content/browser/renderer_host/render_widget_host_impl.cc. The OnMessageReceived function is where we handle the IPC on the browser side-- you would add something in the TOOLKIT_GTK block. But I still think this is going to be really slow since talking to X can be slow. E.g., typing into a textarea over a remote X connection would be good to try.
Created attachment 141537 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
(In reply to comment #6) > (From update of attachment 141455 [details]) > r- since this won't work in the sandbox. > > This might work by making it a sync IPC back to the browser process and handling it on the render thread. There's some similar code in src/content/browser/renderer_host/render_widget_host_impl.cc. The OnMessageReceived function is where we handle the IPC on the browser side-- you would add something in the TOOLKIT_GTK block. But I still think this is going to be really slow since talking to X can be slow. E.g., typing into a textarea over a remote X connection would be good to try. Hi Tony... due to the performance concerns you have mentioned, I have implemented this in a different way. The idea is to keep local state of shift/ctrl/alt key in PlatformKeyboardEvent and sync the state for all event comming frmo the browser. Since the events coming from the browser already have the state, we do not need to query the XServer again. This also saves the sync IPC you were suggesting. On the down side however, it requires some change on the chromeium side as well. the chromium side changes are in https://chromiumcodereview.appspot.com/10377119/ Please have a look at both changes.
Comment on attachment 141537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141537&action=review > Source/WebCore/platform/PlatformKeyboardEvent.h:205 > +#if PLATFORM(CHROMIUM) > + static bool m_shiftKey; > + static bool m_ctrlKey; > + static bool m_altKey; > + static bool m_metaKey; > +#endif This doesn't seem right. It seems unlikely that Chromium would need to store static information in this class if other port don't need do.
(In reply to comment #10) > (From update of attachment 141537 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141537&action=review > > > Source/WebCore/platform/PlatformKeyboardEvent.h:205 > > +#if PLATFORM(CHROMIUM) > > + static bool m_shiftKey; > > + static bool m_ctrlKey; > > + static bool m_altKey; > > + static bool m_metaKey; > > +#endif > > This doesn't seem right. It seems unlikely that Chromium would need to store static information in this class if other port don't need do. Hi Adam.. this will be a global state... so I can store all these fields in PlatformKeyboardEventChromium.cpp hidden in an anonymous namespace. Would that be acceptable? Also, I am looking for comments on if this is the right way to get the state of these fields.
(In reply to comment #10) > (From update of attachment 141537 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141537&action=review > > > Source/WebCore/platform/PlatformKeyboardEvent.h:205 > > +#if PLATFORM(CHROMIUM) > > + static bool m_shiftKey; > > + static bool m_ctrlKey; > > + static bool m_altKey; > > + static bool m_metaKey; > > +#endif > > This doesn't seem right. It seems unlikely that Chromium would need to store static information in this class if other port don't need do. Also note that the getCurrentModifierState() function is not implemented on any other platform (it is implemented only in PlatformKeyboardEventGtk in which it directly gets the state from a gtk_ call, which we have already decided will not work in the chromium case). So perhaps thats why they do not have any static fields.
I'm sorry that I don't have more time to engage in this discussion, but the approach of keeping static state in WebKit does not seem correct. Other approaches that might work: 1) The embedder supplies this information when creating PlatformKeyboardEvents. If the embedder wants to have some complicated IPC/caching mechanism, that's fine, but it's not a concern of WebKit. 2) WebKit can query the embedder via the Platform API to determine the modifier state. Again, how the embedder answers this question is likely to be platform-specific, but is not a concern of WebKit. My guess is that (1) is the better approach, but I haven't studied the problem in detail, so I can't be sure.
Comment on attachment 141537 [details] Patch Attachment 141537 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12673448
Created attachment 141768 [details] Patch
(In reply to comment #13) > I'm sorry that I don't have more time to engage in this discussion, but the approach of keeping static state in WebKit does not seem correct. Other approaches that might work: > > 1) The embedder supplies this information when creating PlatformKeyboardEvents. If the embedder wants to have some complicated IPC/caching mechanism, that's fine, but it's not a concern of WebKit. > Hi Adam, I have re-done the CL taking inspiration from your option #1. The issue I was trying to fix was the lack of modifier key (shift/alt/ctrl) information in drag/drop events. In my current patch, the embedder supplies the relevant information in the Drag event so that the DragController can directly get it from the event instead of asking the embedder. PTAL > 2) WebKit can query the embedder via the Platform API to determine the modifier state. Again, how the embedder answers this question is likely to be platform-specific, but is not a concern of WebKit. > > My guess is that (1) is the better approach, but I haven't studied the problem in detail, so I can't be sure.
View in context: https://bugs.webkit.org/attachment.cgi?id=141768&action=review > Source/WebCore/ChangeLog:3 > + [chromium] Implement PlatformKeyboardEvent::getCurrentModifierState for non win/mac This isn't really accurate anymore. It would be better to mention that you're trying to fix modifiers during drag&drop. > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + * page/DragController.cpp: What happened to the tests line? You should explain why you don't have automated tests (specifically, we mock the d&d events so an automated test wouldn't work). You could also add a manual test to ManualTests. > Source/WebCore/platform/DragData.h:126 > +#if PLATFORM(CHROMIUM) > + int modifierKeyState() { return m_modifierKeyState; } > +#endif I think it would be OK to add a method to DragData for all platforms. On Chromium Linux, we would use the extra state we stored, on other platforms, it would just call PlatformKeyboardEvent::getCurrentModifierState. This would simplify DragController.cpp and keep the chromium specific code in DragDataChromium.cpp. > Source/WebCore/platform/DragData.h:155 > +#if PLATFORM(CHROMIUM) > + int m_modifierKeyState; > +#endif Would it be possible to add this to m_platformDragData instead? We could convert DragDataRef to a pointer to a struct or something.
Comment on attachment 141768 [details] Patch r- for no mention of tests.
On Windows, ::GetKeyState doesn't appear to work in the sandbox.
Created attachment 141803 [details] Patch
(In reply to comment #17) > View in context: https://bugs.webkit.org/attachment.cgi?id=141768&action=review > > > Source/WebCore/ChangeLog:3 > > + [chromium] Implement PlatformKeyboardEvent::getCurrentModifierState for non win/mac > > This isn't really accurate anymore. It would be better to mention that you're trying to fix modifiers during drag&drop. Done. > > > Source/WebCore/ChangeLog:8 > > + Reviewed by NOBODY (OOPS!). > > + > > + * page/DragController.cpp: > > What happened to the tests line? You should explain why you don't have automated tests (specifically, we mock the d&d events so an automated test wouldn't work). You could also add a manual test to ManualTests. Added manual test. > > > Source/WebCore/platform/DragData.h:126 > > +#if PLATFORM(CHROMIUM) > > + int modifierKeyState() { return m_modifierKeyState; } > > +#endif > > I think it would be OK to add a method to DragData for all platforms. On Chromium Linux, we would use the extra state we stored, on other platforms, it would just call PlatformKeyboardEvent::getCurrentModifierState. This would simplify DragController.cpp and keep the chromium specific code in DragDataChromium.cpp. Done. > > > Source/WebCore/platform/DragData.h:155 > > +#if PLATFORM(CHROMIUM) > > + int m_modifierKeyState; > > +#endif > > Would it be possible to add this to m_platformDragData instead? We could convert DragDataRef to a pointer to a struct or something. Done.
Created attachment 141805 [details] Patch
Comment on attachment 141805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141805&action=review > Source/WebCore/page/DragController.cpp:86 > + metaKey = altKey | (keyState & PlatformEvent::MetaKey); Shouldn't this be metaKey?
Comment on attachment 141805 [details] Patch Attachment 141805 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12707002
Comment on attachment 141805 [details] Patch Attachment 141805 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12701199
Comment on attachment 141805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141805&action=review r- for Daniel's comment and the broken mac/win build. > Source/WebCore/page/DragController.cpp:80 > PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey); You could maybe remove this call and implement the !CHROMIUM getModifierKeyState in DragData.cpp. It would just call this function and construct the right keyState. > Source/WebCore/platform/gtk/DragDataGtk.cpp:51 > +int DragData::getModifierKeyState() const > +{ > + return 0; You could just put this in DragData.cpp behind an #if !PLATFORM(CHROMIUM). E.g., You forgot DragDataMac.mm.
Comment on attachment 141805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141805&action=review > Source/WebCore/platform/chromium/ChromiumDataObject.h:89 > + int getModifierKeyState() const { return m_modifierKeyState; } Nit: WebKit naming normally doesn't include get. getModifierKeyState -> modifierKeyState.
Created attachment 141982 [details] Patch
(In reply to comment #23) > (From update of attachment 141805 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141805&action=review > > > Source/WebCore/page/DragController.cpp:86 > > + metaKey = altKey | (keyState & PlatformEvent::MetaKey); > > Shouldn't this be metaKey? Done.
(In reply to comment #26) > (From update of attachment 141805 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141805&action=review > > r- for Daniel's comment and the broken mac/win build. > > > Source/WebCore/page/DragController.cpp:80 > > PlatformKeyboardEvent::getCurrentModifierState(shiftKey, ctrlKey, altKey, metaKey); > > You could maybe remove this call and implement the !CHROMIUM getModifierKeyState in DragData.cpp. It would just call this function and construct the right keyState. Done. > > > Source/WebCore/platform/gtk/DragDataGtk.cpp:51 > > +int DragData::getModifierKeyState() const > > +{ > > + return 0; > > You could just put this in DragData.cpp behind an #if !PLATFORM(CHROMIUM). E.g., You forgot DragDataMac.mm. Done.
(In reply to comment #27) > (From update of attachment 141805 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141805&action=review > > > Source/WebCore/platform/chromium/ChromiumDataObject.h:89 > > + int getModifierKeyState() const { return m_modifierKeyState; } > > Nit: WebKit naming normally doesn't include get. getModifierKeyState -> modifierKeyState. Done.
Comment on attachment 141982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141982&action=review > Source/WebCore/platform/DragData.cpp:28 > +#if !PLATFORM(CHROMIUM) || (!OS(CHROMEOS) && !OS(LINUX)) I would remove this #if. It's harmless to include these files on Chromium. > Source/WebCore/platform/DragData.cpp:58 > +#if !PLATFORM(CHROMIUM) || (!OS(CHROMEOS) && !OS(LINUX)) I would make this just !PLATFORM(CHROMIUM). We can use the m_modifierKeyState on all version of Chromium, right?
Created attachment 142068 [details] Patch
Comment on attachment 141982 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141982&action=review >> Source/WebCore/platform/DragData.cpp:28 >> +#if !PLATFORM(CHROMIUM) || (!OS(CHROMEOS) && !OS(LINUX)) > > I would remove this #if. It's harmless to include these files on Chromium. Done. >> Source/WebCore/platform/DragData.cpp:58 >> +#if !PLATFORM(CHROMIUM) || (!OS(CHROMEOS) && !OS(LINUX)) > > I would make this just !PLATFORM(CHROMIUM). We can use the m_modifierKeyState on all version of Chromium, right? Done.
Comment on attachment 142068 [details] Patch Clearing flags on attachment: 142068 Committed r117327: <http://trac.webkit.org/changeset/117327>
All reviewed patches have been landed. Closing bug.