Support buttons attribute for drag event (chromium side).

Support buttons attribute for drag event (chromium side).

We should pass the modifiers for mouse buttons to Blink in order to support it.

BUG=276941
Committed: https://crrev.com/a7447af876e7dd40f1e16e2fdea37f5b304f937e
Cr-Commit-Position: refs/heads/master@{#320048}

[zino, 2015-01-28 11:07:46]

jinho.bang@samsung.com changed reviewers:
+ rbyers@chromium.org

[zino, 2015-02-04 09:58:42]

Please take a look.

Thank you.

[Rick Byers, 2015-02-04 11:10:20]

rbyers@chromium.org changed reviewers:
+ dcheng@chromium.org

[Rick Byers, 2015-02-04 11:11:29]

Seems reasonable to me.  What platforms does this cover?  Eg. is more work
needed for Mac or Windows?

+dcheng DnD expert (in particular I don't have experience with the DnD parts of
event_sender).

[dcheng, 2015-02-06 19:53:45]

Two other thoughts:
1) Drags can only be initiated with the left mouse button. I think it's going to
be pretty hard/uncommon for people to hold down additional mouse buttons as they
drag. =)
2) A lot of the plumbing (e.g. DragMsg_TargetDragEnter) refers to the modifiers
as key_modifiers. We should probably rename it if we're planning on adding mouse
buttons to this bitfield.

https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right):

https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if (mask &
Button1Mask)
Don't we need to make corresponding changes for Windows and Mac as well? It's
probably best to include them in this patch if we're changing this behavior.

https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if (mask &
Button1Mask)
Don't we need to make corresponding changes for Windows and Mac as well? It's
probably best to include them in this patch if we're changing this behavior.

[zino, 2015-02-11 09:02:24]

On 2015/02/06 19:53:45, dcheng wrote:
> Two other thoughts:
> 1) Drags can only be initiated with the left mouse button. I think it's going
to
> be pretty hard/uncommon for people to hold down additional mouse buttons as
they
> drag. =)

I agree with you. @Rick, I was wondering what your opinion is.

> 2) A lot of the plumbing (e.g. DragMsg_TargetDragEnter) refers to the
modifiers
> as key_modifiers. We should probably rename it if we're planning on adding
mouse
> buttons to this bitfield.

Okay, I'll fix it.

> 
>
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc (right):
> 
>
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if (mask
&
> Button1Mask)
> Don't we need to make corresponding changes for Windows and Mac as well? It's
> probably best to include them in this patch if we're changing this behavior.
> 
>
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if (mask
&
> Button1Mask)
> Don't we need to make corresponding changes for Windows and Mac as well? It's
> probably best to include them in this patch if we're changing this behavior.

I'll check it as well. Sorry for delay.

[Rick Byers, 2015-02-11 11:12:08]

On 2015/02/11 09:02:24, zino wrote:
> On 2015/02/06 19:53:45, dcheng wrote:
> > Two other thoughts:
> > 1) Drags can only be initiated with the left mouse button. I think it's
going
> to
> > be pretty hard/uncommon for people to hold down additional mouse buttons as
> they
> > drag. =)
> 
> I agree with you. @Rick, I was wondering what your opinion is.

Yep, I agree it'll be uncommon but certainly not impossible (eg. on a classic
USB mouse it's pretty easy to hold both buttons down when dragging).  But this
property exists so we have to put _some_ value in it.  It looks like it's
relatively easy to follow the spec here and do the technically correct thing, so
I think it's worth it. 

> 
> > 2) A lot of the plumbing (e.g. DragMsg_TargetDragEnter) refers to the
> modifiers
> > as key_modifiers. We should probably rename it if we're planning on adding
> mouse
> > buttons to this bitfield.
> 
> Okay, I'll fix it.
> 
> > 
> >
>
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> > File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
(right):
> > 
> >
>
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> > ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if
(mask
> &
> > Button1Mask)
> > Don't we need to make corresponding changes for Windows and Mac as well?
It's
> > probably best to include them in this patch if we're changing this behavior.
> > 
> >
>
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> > ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if
(mask
> &
> > Button1Mask)
> > Don't we need to make corresponding changes for Windows and Mac as well?
It's
> > probably best to include them in this patch if we're changing this behavior.
> 
> I'll check it as well. Sorry for delay.

[dcheng, 2015-02-11 22:28:12]

On 2015/02/11 at 11:12:08, rbyers wrote:
> On 2015/02/11 09:02:24, zino wrote:
> > On 2015/02/06 19:53:45, dcheng wrote:
> > > Two other thoughts:
> > > 1) Drags can only be initiated with the left mouse button. I think it's
going
> > to
> > > be pretty hard/uncommon for people to hold down additional mouse buttons
as
> > they
> > > drag. =)
> > 
> > I agree with you. @Rick, I was wondering what your opinion is.
> 
> Yep, I agree it'll be uncommon but certainly not impossible (eg. on a classic
USB mouse it's pretty easy to hold both buttons down when dragging).  But this
property exists so we have to put _some_ value in it.  It looks like it's
relatively easy to follow the spec here and do the technically correct thing, so
I think it's worth it. 

Right, I'm not saying we shouldn't make this change, just commenting on the
difficulty =)

> 
> > 
> > > 2) A lot of the plumbing (e.g. DragMsg_TargetDragEnter) refers to the
> > modifiers
> > > as key_modifiers. We should probably rename it if we're planning on adding
> > mouse
> > > buttons to this bitfield.
> > 
> > Okay, I'll fix it.
> > 
> > > 
> > >
> >
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> > > File ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc
(right):
> > > 
> > >
> >
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> > > ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if
(mask
> > &
> > > Button1Mask)
> > > Don't we need to make corresponding changes for Windows and Mac as well?
It's
> > > probably best to include them in this patch if we're changing this
behavior.
> > > 
> > >
> >
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> > > ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if
(mask
> > &
> > > Button1Mask)
> > > Don't we need to make corresponding changes for Windows and Mac as well?
It's
> > > probably best to include them in this patch if we're changing this
behavior.
> > 
> > I'll check it as well. Sorry for delay.

[zino, 2015-03-05 09:20:30]

Ping owners.

Please take a look.

On 2015/02/06 19:53:45, dcheng wrote:
>
https://codereview.chromium.org/878413003/diff/20001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_drag_drop_client_aurax11.cc:104: if (mask
&
> Button1Mask)
> Don't we need to make corresponding changes for Windows and Mac as well? It's
> probably best to include them in this patch if we're changing this behavior.

Done.

[Rick Byers, 2015-03-05 14:51:08]

On 2015/03/05 09:20:30, zino wrote:
> Ping owners.
> 
> Please take a look.

event_sender.* LGTM  (I'm not an owner of the other parts here, dcheng is the
expert).

[dcheng, 2015-03-05 15:40:00]

lgtm, assuming you've tested that this works as expected manually
(unfortunately, the platform glue for DnD is rarely covered by any automated
tests)

[zino, 2015-03-09 07:13:19]

The CQ bit was checked by jinho.bang@samsung.com

[commit-bot: I haz the power, 2015-03-09 07:14:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/878413003/80001

[commit-bot: I haz the power, 2015-03-09 07:31:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-09 07:31:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zino, 2015-03-09 07:36:38]

+ @sadrul, @avi (ui/views/*, content/*)

Please take a look.

Thank you.

[zino, 2015-03-10 01:44:34]

jinho.bang@samsung.com changed reviewers:
+ avi@chromium.org, sadrul@chromium.org

[zino, 2015-03-10 01:44:47]

+ @sadrul, @avi (ui/views/*, content/*)

Please take a look.

Thank you.

[Avi (use Gerrit), 2015-03-10 03:14:07]

https://codereview.chromium.org/878413003/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/878413003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_drag_dest_mac.mm:27: int GetModifierFlags() {
Move this function into an anonymous namespace.

https://codereview.chromium.org/878413003/diff/80001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_drop_target_win.cc (right):

https://codereview.chromium.org/878413003/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_drop_target_win.cc:97: static int
ConvertKeyStateToAuraEventFlags(DWORD key_state)
Why are you putting this function in the middle of nowhere? Move it to the top
of the file and put it into an anonymous namespace.

[zino, 2015-03-10 07:40:35]

Patchset #6 (id:100001) has been deleted

[zino, 2015-03-10 08:47:23]

I addressed all your comments.

Could you review again?

Thank you.

https://codereview.chromium.org/878413003/diff/80001/content/browser/web_cont...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/878413003/diff/80001/content/browser/web_cont...
content/browser/web_contents/web_drag_dest_mac.mm:27: int GetModifierFlags() {
On 2015/03/10 03:14:07, Avi wrote:
> Move this function into an anonymous namespace.

Done.

https://codereview.chromium.org/878413003/diff/80001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_drop_target_win.cc (right):

https://codereview.chromium.org/878413003/diff/80001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_drop_target_win.cc:97: static int
ConvertKeyStateToAuraEventFlags(DWORD key_state)
On 2015/03/10 03:14:07, Avi wrote:
> Why are you putting this function in the middle of nowhere? Move it to the top
> of the file and put it into an anonymous namespace.

Done.

[Avi (use Gerrit), 2015-03-11 02:08:15]

LGTM with formatting nits fixed.

https://codereview.chromium.org/878413003/diff/120001/content/browser/web_con...
File content/browser/web_contents/web_drag_dest_mac.mm (right):

https://codereview.chromium.org/878413003/diff/120001/content/browser/web_con...
content/browser/web_contents/web_drag_dest_mac.mm:56: } // namespace
two spaces between } and //

https://codereview.chromium.org/878413003/diff/120001/ui/views/widget/desktop...
File ui/views/widget/desktop_aura/desktop_drop_target_win.cc (right):

https://codereview.chromium.org/878413003/diff/120001/ui/views/widget/desktop...
ui/views/widget/desktop_aura/desktop_drop_target_win.cc:45: } // namespace
ditto

[zino, 2015-03-11 03:58:26]

The CQ bit was checked by jinho.bang@samsung.com

[zino, 2015-03-11 03:58:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
rbyers@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/878413003/#ps140001
(title: "nit")

[commit-bot: I haz the power, 2015-03-11 03:58:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/878413003/140001

[commit-bot: I haz the power, 2015-03-11 04:57:11]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-03-11 04:58:02]

Patchset 7 (id:??) landed as
https://crrev.com/a7447af876e7dd40f1e16e2fdea37f5b304f937e
Cr-Commit-Position: refs/heads/master@{#320048}