Bug 86236 - [chromium] No modifier flags (shift/ctrl/alt) in drag&drop events on chromium linux
: [chromium] No modifier flags (shift/ctrl/alt) in drag&drop events on chromium...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-11 11:22 PST by
Modified: 2012-05-16 13:04 PST (History)


Attachments
Patch (2.63 KB, patch)
2012-05-11 11:23 PST, Varun Jain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.28 KB, patch)
2012-05-11 18:07 PST, Varun Jain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.00 KB, patch)
2012-05-14 12:33 PST, Varun Jain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.86 KB, patch)
2012-05-14 16:14 PST, Varun Jain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (19.66 KB, patch)
2012-05-14 16:18 PST, Varun Jain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.37 KB, patch)
2012-05-15 08:47 PST, Varun Jain
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.23 KB, patch)
2012-05-15 15:00 PST, Varun Jain
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-11 11:22:05 PST
[chromium] Implement PlatformKeyboardEvent::getCurrentModifierState for non win/mac
------- Comment #1 From 2012-05-11 11:23:10 PST -------
Created an attachment (id=141455) [details]
Patch
------- Comment #2 From 2012-05-11 14:08:34 PST -------
(From update of attachment 141455 [details])
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.
------- Comment #3 From 2012-05-11 14:09:31 PST -------
@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.
------- Comment #4 From 2012-05-11 14:37:22 PST -------
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.
------- Comment #5 From 2012-05-11 15:00:23 PST -------
> 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 #6 From 2012-05-11 15:27:21 PST -------
(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.
------- Comment #7 From 2012-05-11 18:07:52 PST -------
Created an attachment (id=141537) [details]
Patch
------- Comment #8 From 2012-05-11 18:12:21 PST -------
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.
------- Comment #9 From 2012-05-11 18:12:37 PST -------
(In reply to comment #6)
> (From update of attachment 141455 [details] [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 #10 From 2012-05-11 18:14:24 PST -------
(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.
------- Comment #11 From 2012-05-11 18:23:35 PST -------
(In reply to comment #10)
> (From update of attachment 141537 [details] [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.
------- Comment #12 From 2012-05-11 18:29:20 PST -------
(In reply to comment #10)
> (From update of attachment 141537 [details] [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.
------- Comment #13 From 2012-05-11 18:42:53 PST -------
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 #14 From 2012-05-11 19:19:50 PST -------
(From update of attachment 141537 [details])
Attachment 141537 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12673448
------- Comment #15 From 2012-05-14 12:33:58 PST -------
Created an attachment (id=141768) [details]
Patch
------- Comment #16 From 2012-05-14 12:39:14 PST -------
(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.
------- Comment #17 From 2012-05-14 13:38:06 PST -------
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 #18 From 2012-05-14 13:38:33 PST -------
(From update of attachment 141768 [details])
r- for no mention of tests.
------- Comment #19 From 2012-05-14 13:58:36 PST -------
On Windows, ::GetKeyState doesn't appear to work in the sandbox.
------- Comment #20 From 2012-05-14 16:14:28 PST -------
Created an attachment (id=141803) [details]
Patch
------- Comment #21 From 2012-05-14 16:15:13 PST -------
(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.
------- Comment #22 From 2012-05-14 16:18:42 PST -------
Created an attachment (id=141805) [details]
Patch
------- Comment #23 From 2012-05-14 16:20:00 PST -------
(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?
------- Comment #24 From 2012-05-14 16:30:09 PST -------
(From update of attachment 141805 [details])
Attachment 141805 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12707002
------- Comment #25 From 2012-05-14 16:30:48 PST -------
(From update of attachment 141805 [details])
Attachment 141805 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12701199
------- Comment #26 From 2012-05-14 16:33:57 PST -------
(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.

> 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 #27 From 2012-05-14 16:35:18 PST -------
(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.
------- Comment #28 From 2012-05-15 08:47:27 PST -------
Created an attachment (id=141982) [details]
Patch
------- Comment #29 From 2012-05-15 08:47:57 PST -------
(In reply to comment #23)
> (From update of attachment 141805 [details] [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.
------- Comment #30 From 2012-05-15 08:48:16 PST -------
(In reply to comment #26)
> (From update of attachment 141805 [details] [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.
------- Comment #31 From 2012-05-15 08:48:28 PST -------
(In reply to comment #27)
> (From update of attachment 141805 [details] [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 #32 From 2012-05-15 13:28:58 PST -------
(From update of attachment 141982 [details])
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?
------- Comment #33 From 2012-05-15 15:00:24 PST -------
Created an attachment (id=142068) [details]
Patch
------- Comment #34 From 2012-05-15 15:00:43 PST -------
(From update of attachment 141982 [details])
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 #35 From 2012-05-16 13:03:53 PST -------
(From update of attachment 142068 [details])
Clearing flags on attachment: 142068

Committed r117327: <http://trac.webkit.org/changeset/117327>
------- Comment #36 From 2012-05-16 13:04:08 PST -------
All reviewed patches have been landed.  Closing bug.