Plumb event flags (shift/alt/ctrl modifiers) for drag/drop events to WebKit.

Plumb event flags (shift/alt/ctrl modifiers) for drag/drop events to WebKit.
WebKit side changes in https://bugs.webkit.org/show_bug.cgi?id=86236

BUG=125861
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137771

[varunjain, 2012-05-13 21:44:12]

On 2012/05/12 01:15:33, varunjain wrote:

Please hold your review. I am still discussing this design with tony and abarth

[varunjain, 2012-05-15 16:41:13]

+jam for content/*
Hi reviewers.. Webkit side changes are almost through. I think we can start
reviewing this now.

[jam, 2012-05-15 16:57:53]

lgtm

http://codereview.chromium.org/10377119/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10377119/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_view_host_impl.cc:570: key_modifiers));
nit: spacing is off?

http://codereview.chromium.org/10377119/diff/1/content/browser/web_contents/w...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

http://codereview.chromium.org/10377119/diff/1/content/browser/web_contents/w...
content/browser/web_contents/web_drag_dest_mac.mm:115: 0 /* shift/ctrl/alt flags
*/);
nit: personally, I'm not a fan of adding comments like this. if someone wants to
know what this does, they can quickly get to the header (in all files)

http://codereview.chromium.org/10377119/diff/1/content/common/drag_messages.h
File content/common/drag_messages.h (right):

http://codereview.chromium.org/10377119/diff/1/content/common/drag_messages.h...
content/common/drag_messages.h:45: int /* key modifiers */)
nit: key_modifiers. also below

[dcheng, 2012-05-15 18:29:28]

Are you going to be doing any followup work to pass the modifier keys on
Windows/Linux as well?

[varunjain, 2012-05-15 18:31:29]

On 2012/05/15 18:29:28, dcheng wrote:
> Are you going to be doing any followup work to pass the modifier keys on
> Windows/Linux as well?

win already works because webkit gets the state through a windows system call. I
can work on getting linux to work later but it will not be a priority.

[dcheng, 2012-05-15 18:40:53]

On 2012/05/15 18:31:29, varunjain wrote:
> On 2012/05/15 18:29:28, dcheng wrote:
> > Are you going to be doing any followup work to pass the modifier keys on
> > Windows/Linux as well?
> 
> win already works because webkit gets the state through a windows system call.
I
> can work on getting linux to work later but it will not be a priority.

Calls to ::GetKeyState don't work in the renderer, so the Chromium Windows
implementation in WebKit is broken and doing nothing.

[varunjain, 2012-05-15 19:22:58]

On 2012/05/15 18:40:53, dcheng wrote:
> On 2012/05/15 18:31:29, varunjain wrote:
> > On 2012/05/15 18:29:28, dcheng wrote:
> > > Are you going to be doing any followup work to pass the modifier keys on
> > > Windows/Linux as well?
> > 
> > win already works because webkit gets the state through a windows system
call.
> I
> > can work on getting linux to work later but it will not be a priority.
> 
> Calls to ::GetKeyState don't work in the renderer, so the Chromium Windows
> implementation in WebKit is broken and doing nothing.

to be more specific, ::GetKeyState does not work in sandbox. Otherwise, modifier
flags in drag/drop events are working on chrome on windows.

[dcheng, 2012-05-15 20:20:32]

On 2012/05/15 19:22:58, varunjain wrote:
> On 2012/05/15 18:40:53, dcheng wrote:
> > On 2012/05/15 18:31:29, varunjain wrote:
> > > On 2012/05/15 18:29:28, dcheng wrote:
> > > > Are you going to be doing any followup work to pass the modifier keys on
> > > > Windows/Linux as well?
> > > 
> > > win already works because webkit gets the state through a windows system
> call.
> > I
> > > can work on getting linux to work later but it will not be a priority.
> > 
> > Calls to ::GetKeyState don't work in the renderer, so the Chromium Windows
> > implementation in WebKit is broken and doing nothing.
> 
> to be more specific, ::GetKeyState does not work in sandbox. Otherwise,
modifier
> flags in drag/drop events are working on chrome on windows.

To be honest, I'm not sure if it'd work even without the sandbox:
"The key status returned from this function changes as a thread reads key
messages from its message queue."

But since the renderer isn't the foreground thread, it shouldn't get key
messages, so the key status will never be updated.

[jam, 2012-05-15 20:51:06]

On 2012/05/15 20:20:32, dcheng wrote:
> On 2012/05/15 19:22:58, varunjain wrote:
> > On 2012/05/15 18:40:53, dcheng wrote:
> > > On 2012/05/15 18:31:29, varunjain wrote:
> > > > On 2012/05/15 18:29:28, dcheng wrote:
> > > > > Are you going to be doing any followup work to pass the modifier keys
on
> > > > > Windows/Linux as well?
> > > > 
> > > > win already works because webkit gets the state through a windows system
> > call.
> > > I
> > > > can work on getting linux to work later but it will not be a priority.
> > > 
> > > Calls to ::GetKeyState don't work in the renderer, so the Chromium Windows
> > > implementation in WebKit is broken and doing nothing.
> > 
> > to be more specific, ::GetKeyState does not work in sandbox. Otherwise,
> modifier
> > flags in drag/drop events are working on chrome on windows.
> 
> To be honest, I'm not sure if it'd work even without the sandbox:
> "The key status returned from this function changes as a thread reads key
> messages from its message queue."
> 
> But since the renderer isn't the foreground thread, it shouldn't get key
> messages, so the key status will never be updated.

I wasn't aware that this doesn't work on all platforms. I agree if we do this,
it should be done for all platforms.

[varunjain, 2012-05-15 23:03:39]

Implemented for all platforms. Also changed the webkit CL accordingly.
I have not really tested changes on all platforms.. but I suppose you guys can
review it while I setup a build on each platform.

http://codereview.chromium.org/10377119/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10377119/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/render_view_host_impl.cc:570: key_modifiers));
On 2012/05/15 16:57:53, John Abd-El-Malek wrote:
> nit: spacing is off?

Done.

http://codereview.chromium.org/10377119/diff/1/content/browser/web_contents/w...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

http://codereview.chromium.org/10377119/diff/1/content/browser/web_contents/w...
content/browser/web_contents/web_drag_dest_mac.mm:115: 0 /* shift/ctrl/alt flags
*/);
On 2012/05/15 16:57:53, John Abd-El-Malek wrote:
> nit: personally, I'm not a fan of adding comments like this. if someone wants
to
> know what this does, they can quickly get to the header (in all files)

Done.

http://codereview.chromium.org/10377119/diff/1/content/common/drag_messages.h
File content/common/drag_messages.h (right):

http://codereview.chromium.org/10377119/diff/1/content/common/drag_messages.h...
content/common/drag_messages.h:45: int /* key modifiers */)
On 2012/05/15 16:57:53, John Abd-El-Malek wrote:
> nit: key_modifiers. also below

Done.

[tony, 2012-05-15 23:17:04]

LGTM.  I can test this on GTK+ for you, let me just patch in the changes.

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
File content/browser/web_contents/web_drag_dest_win.cc (right):

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
content/browser/web_contents/web_drag_dest_win.cc:48: if
(::GetKeyState(VK_SHIFT) & HIGH_BIT_MASK_SHORT)
Nit: Can you use the HIBYTE macro?

[tony, 2012-05-15 23:35:03]

The GTK+ code is incorrect.  It should be:

int GetModifierFlags(GtkWidget* widget) {
  int modifier_state = 0;
  GdkModifierType state;
  gdk_window_get_pointer(gtk_widget_get_window(widget),
                         NULL, NULL, &state);
  ...
}

where |widget| is sender that you pass in.

[dcheng, 2012-05-16 00:13:28]

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
content/browser/web_contents/web_drag_dest_mac.mm:24: - (int)GetModifierFlags {
This should just be a regular C method declaration, otherwise it's a compile
error. It probably belongs in an unnamed namespace as well.

[varunjain, 2012-05-16 01:30:22]

Hi all.. Thanks to Tony and Daniel for testing on gtk and mac. I thought I would
take forever to setup up and test on all platforms... you guys are awesome! :)

Everything works... so I can land this CL after the webkit changes are landed
and gardened.

@tony: can I get cq+ on the webkit CL now?
@sky: need OWNERS review

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
content/browser/web_contents/web_drag_dest_mac.mm:24: - (int)GetModifierFlags {
On 2012/05/16 00:13:28, dcheng wrote:
> This should just be a regular C method declaration, otherwise it's a compile
> error. It probably belongs in an unnamed namespace as well.

Done.

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
File content/browser/web_contents/web_drag_dest_win.cc (right):

http://codereview.chromium.org/10377119/diff/11001/content/browser/web_conten...
content/browser/web_contents/web_drag_dest_win.cc:48: if
(::GetKeyState(VK_SHIFT) & HIGH_BIT_MASK_SHORT)
On 2012/05/15 23:17:05, tony wrote:
> Nit: Can you use the HIBYTE macro?

HIBYTE returns the whole byte afaik while we just want to check the highest bit
here.

[sky, 2012-05-17 19:49:39]

LGTM

http://codereview.chromium.org/10377119/diff/4006/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10377119/diff/4006/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:556: const gfx::Point&
client_pt, const gfx::Point& screen_pt,
wrap each param onto its own line.

http://codereview.chromium.org/10377119/diff/4006/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:569: int key_modifiers) {
wrap each param onto its own line.

[varunjain, 2012-05-17 21:14:04]

http://codereview.chromium.org/10377119/diff/4006/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10377119/diff/4006/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:556: const gfx::Point&
client_pt, const gfx::Point& screen_pt,
On 2012/05/17 19:49:40, sky wrote:
> wrap each param onto its own line.

Done.

http://codereview.chromium.org/10377119/diff/4006/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:569: int key_modifiers) {
On 2012/05/17 19:49:40, sky wrote:
> wrap each param onto its own line.

Done.

[commit-bot: I haz the power, 2012-05-17 21:28:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/10377119/9003

[commit-bot: I haz the power, 2012-05-17 23:43:45]

Change committed as 137771