Bug 86236 – [chromium] No modifier flags (shift/ctrl/alt) in drag&drop events on chromium linux

• Component: New Bugs
• Status: RESOLVED FIXED

[Varun Jain]

[chromium] Implement PlatformKeyboardEvent::getCurrentModifierState for non win/mac

[Varun Jain]

Created attachment 141455 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

@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.

[Tony Chang]

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.

[Avi Drissman]

> 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.

[Tony Chang]

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.

[Varun Jain]

Created attachment 141537 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Varun Jain]

(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.

[Adam Barth]

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.

[Varun Jain]

(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.

[Varun Jain]

(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.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 141537 [details]
Patch

Attachment 141537 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12673448

[Varun Jain]

Created attachment 141768 [details]
Patch

[Varun Jain]

(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.

[Tony Chang]

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.

[Tony Chang]

Comment on attachment 141768 [details]
Patch

r- for no mention of tests.

[Daniel Cheng]

On Windows, ::GetKeyState doesn't appear to work in the sandbox.

[Varun Jain]

Created attachment 141803 [details]
Patch

[Varun Jain]

(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.

[Varun Jain]

Created attachment 141805 [details]
Patch

[Daniel Cheng]

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?

[Build Bot]

Comment on attachment 141805 [details]
Patch

Attachment 141805 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12707002

[Build Bot]

Comment on attachment 141805 [details]
Patch

Attachment 141805 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12701199

[Tony Chang]

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.

[Tony Chang]

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.

[Varun Jain]

Created attachment 141982 [details]
Patch

[Varun Jain]

(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.

[Varun Jain]

(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.

[Varun Jain]

(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.

[Tony Chang]

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?

[Varun Jain]

Created attachment 142068 [details]
Patch

[Varun Jain]

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.

[WebKit Review Bot]

Comment on attachment 142068 [details]
Patch

Clearing flags on attachment: 142068

Committed r117327: <http://trac.webkit.org/changeset/117327>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.