Enable touch-initiated drag-drop work on Windows

Enable touch-initiated drag-drop works on Windows.

Once long press gesture is forwarded on a draggable element, it calls DoDragDrop
function which runs into its own loop and wait for mouse down and move event to
proceed, however, there is no mouse down event triggered if it is initiated by
touch. This fix is to simulate a mouse down event to drive DoDragDrop to go ahead.

BUG=229301
TEST=http://html5demos.com/drag

[Hongbo Min, 2013-04-15 11:56:36]

Ben, please review this patch.

Currently, from my test on Win8 tablet with touch support, it works well for
touch-initiated drag-drop (e.g. http://html5demos.com/drag), but still fails to
drag-out to download file like http://www.thecssninja.com/demo/gmail_dragout/.
Will submit another patch to fix drag-out case.

[dcheng, 2013-04-15 18:07:37]

https://codereview.chromium.org/14122008/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

https://codereview.chromium.org/14122008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_win.h:287: bool
has_valid_long_press_gesture() { return has_valid_long_press_gesture_; }
Nit: make this const.

https://codereview.chromium.org/14122008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_win.h:602: bool
has_valid_long_press_gesture_;
Perhaps just call this "in_long_press_gesture".

https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_drag_win.cc:451: INPUT ip;
Initialize this with = {}? The current code leaves random stuff in some of the
other fields for MOUSEINPUT. This way, you can also skip initializing ip.mi.time
explicitly below.

https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_drag_win.cc:453: ip.mi.dwFlags =
MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
Why do we use the right mouse button? How come we can't just simulate a normal
DnD by using the left mouse button?

[Hongbo Min, 2013-04-16 01:13:44]

https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_drag_win.cc:453: ip.mi.dwFlags =
MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
On 2013/04/15 18:07:37, dcheng wrote:
> Why do we use the right mouse button? How come we can't just simulate a normal
> DnD by using the left mouse button?

The reason is, if we just simulate left mouse button down, then left mouse
button up event won't get fired to exit DoDragDrop loop, and wo also don't know
when to simulate the left mouse button up event, and so DoDragDrop won't exit
forever.

[dcheng, 2013-04-16 01:27:27]

https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
content/browser/web_contents/web_contents_drag_win.cc:453: ip.mi.dwFlags =
MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
On 2013/04/16 01:13:44, Hongbo Min wrote:
> On 2013/04/15 18:07:37, dcheng wrote:
> > Why do we use the right mouse button? How come we can't just simulate a
normal
> > DnD by using the left mouse button?
> 
> The reason is, if we just simulate left mouse button down, then left mouse
> button up event won't get fired to exit DoDragDrop loop, and wo also don't
know
> when to simulate the left mouse button up event, and so DoDragDrop won't exit
> forever.

Who's triggering the right mouse button up event then? Is that something the
system is magically triggering somewhere? It seems kind of fragile as well; if
the user also presses right-click on his pointing device during a long-press
drag, won't it cause the logic to get confused?

It seems like it would be more appropriate to teach ui::DragSourceWin about
gesture-initiated drags (e.g. add a set_in_gesture_drag() setter) so
QueryContinueDrag() can use that information to determine whether or not to exit
the dragging loop.

[Hongbo Min, 2013-04-16 03:26:03]

On 2013/04/16 01:27:27, dcheng wrote:
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> File content/browser/web_contents/web_contents_drag_win.cc (right):
> 
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> content/browser/web_contents/web_contents_drag_win.cc:453: ip.mi.dwFlags =
> MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
> On 2013/04/16 01:13:44, Hongbo Min wrote:
> > On 2013/04/15 18:07:37, dcheng wrote:
> > > Why do we use the right mouse button? How come we can't just simulate a
> normal
> > > DnD by using the left mouse button?
> > 
> > The reason is, if we just simulate left mouse button down, then left mouse
> > button up event won't get fired to exit DoDragDrop loop, and wo also don't
> know
> > when to simulate the left mouse button up event, and so DoDragDrop won't
exit
> > forever.
> 
> Who's triggering the right mouse button up event then? Is that something the
> system is magically triggering somewhere? It seems kind of fragile as well; if
> the user also presses right-click on his pointing device during a long-press
> drag, won't it cause the logic to get confused?
> 
> It seems like it would be more appropriate to teach ui::DragSourceWin about
> gesture-initiated drags (e.g. add a set_in_gesture_drag() setter) so
> QueryContinueDrag() can use that information to determine whether or not to
exit
> the dragging loop.

On Windows, press and hold will trigger right mouse button behavior, once the
touch point is removed from screen, then right mouse button up event gets
triggered, and DoDragDrop loop consumes this up event to complete the dragging
operation.

The problem is, with set_in_gesture_drag(), we still don't know when to exit
DoDragDrop loop since the mouse button up event is consumed by DoDragDrop, RWHV
won't get such event if DoDragDrop loop is running, so no clue is used for
invalidating in_long_press_gesture status.

[dcheng, 2013-04-16 03:56:35]

On 2013/04/16 03:26:03, Hongbo Min wrote:
> On 2013/04/16 01:27:27, dcheng wrote:
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > File content/browser/web_contents/web_contents_drag_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > content/browser/web_contents/web_contents_drag_win.cc:453: ip.mi.dwFlags =
> > MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
> > On 2013/04/16 01:13:44, Hongbo Min wrote:
> > > On 2013/04/15 18:07:37, dcheng wrote:
> > > > Why do we use the right mouse button? How come we can't just simulate a
> > normal
> > > > DnD by using the left mouse button?
> > > 
> > > The reason is, if we just simulate left mouse button down, then left mouse
> > > button up event won't get fired to exit DoDragDrop loop, and wo also don't
> > know
> > > when to simulate the left mouse button up event, and so DoDragDrop won't
> exit
> > > forever.
> > 
> > Who's triggering the right mouse button up event then? Is that something the
> > system is magically triggering somewhere? It seems kind of fragile as well;
if
> > the user also presses right-click on his pointing device during a long-press
> > drag, won't it cause the logic to get confused?
> > 
> > It seems like it would be more appropriate to teach ui::DragSourceWin about
> > gesture-initiated drags (e.g. add a set_in_gesture_drag() setter) so
> > QueryContinueDrag() can use that information to determine whether or not to
> exit
> > the dragging loop.
> 
> On Windows, press and hold will trigger right mouse button behavior, once the
> touch point is removed from screen, then right mouse button up event gets
> triggered, and DoDragDrop loop consumes this up event to complete the dragging
> operation.
> 
> The problem is, with set_in_gesture_drag(), we still don't know when to exit
> DoDragDrop loop since the mouse button up event is consumed by DoDragDrop,
RWHV
> won't get such event if DoDragDrop loop is running, so no clue is used for
> invalidating in_long_press_gesture status.

OK, a few more questions to try to make sure I understand the situation
correctly. =)

From what you've told me (I don't have a Windows touch device to test this
myself), initiating a long gesture is equivalent to pressing the right mouse
button, and ending the gesture is equivalent to releasing it right?

In that case, why is it critical to call SendMouseEventForTouchDnD()? If you
don't call it, is DoDragDrop() confused about which mouse buttons are pressed?

[Hongbo Min, 2013-04-16 05:23:39]

On 2013/04/16 03:56:35, dcheng wrote:
> On 2013/04/16 03:26:03, Hongbo Min wrote:
> > On 2013/04/16 01:27:27, dcheng wrote:
> > >
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > > File content/browser/web_contents/web_contents_drag_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > > content/browser/web_contents/web_contents_drag_win.cc:453: ip.mi.dwFlags =
> > > MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
> > > On 2013/04/16 01:13:44, Hongbo Min wrote:
> > > > On 2013/04/15 18:07:37, dcheng wrote:
> > > > > Why do we use the right mouse button? How come we can't just simulate
a
> > > normal
> > > > > DnD by using the left mouse button?
> > > > 
> > > > The reason is, if we just simulate left mouse button down, then left
mouse
> > > > button up event won't get fired to exit DoDragDrop loop, and wo also
don't
> > > know
> > > > when to simulate the left mouse button up event, and so DoDragDrop won't
> > exit
> > > > forever.
> > > 
> > > Who's triggering the right mouse button up event then? Is that something
the
> > > system is magically triggering somewhere? It seems kind of fragile as
well;
> if
> > > the user also presses right-click on his pointing device during a
long-press
> > > drag, won't it cause the logic to get confused?
> > > 
> > > It seems like it would be more appropriate to teach ui::DragSourceWin
about
> > > gesture-initiated drags (e.g. add a set_in_gesture_drag() setter) so
> > > QueryContinueDrag() can use that information to determine whether or not
to
> > exit
> > > the dragging loop.
> > 
> > On Windows, press and hold will trigger right mouse button behavior, once
the
> > touch point is removed from screen, then right mouse button up event gets
> > triggered, and DoDragDrop loop consumes this up event to complete the
dragging
> > operation.
> > 
> > The problem is, with set_in_gesture_drag(), we still don't know when to exit
> > DoDragDrop loop since the mouse button up event is consumed by DoDragDrop,
> RWHV
> > won't get such event if DoDragDrop loop is running, so no clue is used for
> > invalidating in_long_press_gesture status.
> 
> OK, a few more questions to try to make sure I understand the situation
> correctly. =)
> 
> From what you've told me (I don't have a Windows touch device to test this
> myself), initiating a long gesture is equivalent to pressing the right mouse
> button, and ending the gesture is equivalent to releasing it right?
> 
> In that case, why is it critical to call SendMouseEventForTouchDnD()? If you
> don't call it, is DoDragDrop() confused about which mouse buttons are pressed?

OK. Let me describe what will be happened after press and hold on Windows:

1) Suppose that you press and hold on the screen, at this moment, only WM_TOUCH
message is emitted.
2) If you try to move your finger, the event sequence is:
   * WM_TOUCH
   * WM_MOUSEMOVE
   * WM_RBUTTONDOWN
   * WM_TOUCH
   * WM_MOUSEMOVE
   * WM_TOUCH
   * WM_MOUSEMOVE
   * (repeatedly until your finger is up)
3) On your finger is up, the event sequence is:
   * WM_TOUCH
   * WM_RBUTTONUP

In summary, the right mouse button down event is fired once your finger is moved
or up after long press.

However, the DoDragDrop won't get right mouse button down event although you
move your finger or up unless a simulated right mouse is fired programmatically,
like this CL does, once DoDragDrop receives such simulated mouse down event, all
events listed above can be fired one by one, therefore, once your finger is up,
the DoDragDrop exits since it got a right button up event.

The reason I guess is, DoDragDrop will ignore touch event and only mouse down
event (both left and right) can drive DoDragDrop to proceed ahead, so once
DoDragDrop loop get touch event, it just puts the event into a cached event
queue. After it gets mouse down event, all events in queue will be populated one
by one, and these touch events will be recognized as mouse event on the fly
(similar with gesture recognizer).

[Hongbo Min, 2013-04-24 06:15:30]

On 2013/04/16 05:23:39, Hongbo Min wrote:
> On 2013/04/16 03:56:35, dcheng wrote:
> > On 2013/04/16 03:26:03, Hongbo Min wrote:
> > > On 2013/04/16 01:27:27, dcheng wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > > > File content/browser/web_contents/web_contents_drag_win.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > > > content/browser/web_contents/web_contents_drag_win.cc:453: ip.mi.dwFlags
=
> > > > MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
> > > > On 2013/04/16 01:13:44, Hongbo Min wrote:
> > > > > On 2013/04/15 18:07:37, dcheng wrote:
> > > > > > Why do we use the right mouse button? How come we can't just
simulate
> a
> > > > normal
> > > > > > DnD by using the left mouse button?
> > > > > 
> > > > > The reason is, if we just simulate left mouse button down, then left
> mouse
> > > > > button up event won't get fired to exit DoDragDrop loop, and wo also
> don't
> > > > know
> > > > > when to simulate the left mouse button up event, and so DoDragDrop
won't
> > > exit
> > > > > forever.
> > > > 
> > > > Who's triggering the right mouse button up event then? Is that something
> the
> > > > system is magically triggering somewhere? It seems kind of fragile as
> well;
> > if
> > > > the user also presses right-click on his pointing device during a
> long-press
> > > > drag, won't it cause the logic to get confused?
> > > > 
> > > > It seems like it would be more appropriate to teach ui::DragSourceWin
> about
> > > > gesture-initiated drags (e.g. add a set_in_gesture_drag() setter) so
> > > > QueryContinueDrag() can use that information to determine whether or not
> to
> > > exit
> > > > the dragging loop.
> > > 
> > > On Windows, press and hold will trigger right mouse button behavior, once
> the
> > > touch point is removed from screen, then right mouse button up event gets
> > > triggered, and DoDragDrop loop consumes this up event to complete the
> dragging
> > > operation.
> > > 
> > > The problem is, with set_in_gesture_drag(), we still don't know when to
exit
> > > DoDragDrop loop since the mouse button up event is consumed by DoDragDrop,
> > RWHV
> > > won't get such event if DoDragDrop loop is running, so no clue is used for
> > > invalidating in_long_press_gesture status.
> > 
> > OK, a few more questions to try to make sure I understand the situation
> > correctly. =)
> > 
> > From what you've told me (I don't have a Windows touch device to test this
> > myself), initiating a long gesture is equivalent to pressing the right mouse
> > button, and ending the gesture is equivalent to releasing it right?
> > 
> > In that case, why is it critical to call SendMouseEventForTouchDnD()? If you
> > don't call it, is DoDragDrop() confused about which mouse buttons are
pressed?
> 
> OK. Let me describe what will be happened after press and hold on Windows:
> 
> 1) Suppose that you press and hold on the screen, at this moment, only
WM_TOUCH
> message is emitted.
> 2) If you try to move your finger, the event sequence is:
>    * WM_TOUCH
>    * WM_MOUSEMOVE
>    * WM_RBUTTONDOWN
>    * WM_TOUCH
>    * WM_MOUSEMOVE
>    * WM_TOUCH
>    * WM_MOUSEMOVE
>    * (repeatedly until your finger is up)
> 3) On your finger is up, the event sequence is:
>    * WM_TOUCH
>    * WM_RBUTTONUP
> 
> In summary, the right mouse button down event is fired once your finger is
moved
> or up after long press.
> 
> However, the DoDragDrop won't get right mouse button down event although you
> move your finger or up unless a simulated right mouse is fired
programmatically,
> like this CL does, once DoDragDrop receives such simulated mouse down event,
all
> events listed above can be fired one by one, therefore, once your finger is
up,
> the DoDragDrop exits since it got a right button up event.
> 
> The reason I guess is, DoDragDrop will ignore touch event and only mouse down
> event (both left and right) can drive DoDragDrop to proceed ahead, so once
> DoDragDrop loop get touch event, it just puts the event into a cached event
> queue. After it gets mouse down event, all events in queue will be populated
one
> by one, and these touch events will be recognized as mouse event on the fly
> (similar with gesture recognizer).

dcheng@, since this solution is to use right mouse button up event (the finger
is up after a long press) to complete DoDragDrop operation, it might be a
problem if user disable emitting right button down after a long press on Windows
through Control Panel/Touch setting.

The thing I am considering is, is there any API on Windows to query the number
of touch points on screen, something like QueryTouchPointsNum? I failed to find
it on MSDN.

If it does, we could safely simulate left button down event to drive DoDragDrop
go ahead, and at the same time, we create a background thread to query the
number of touch point at a fixed interval (e.g. 50ms), if the number is 0, then
we simulate left button up to complete DoDragDrop operation.

[dcheng, 2013-04-25 00:55:08]

On 2013/04/24 06:15:30, Hongbo Min wrote:
> On 2013/04/16 05:23:39, Hongbo Min wrote:
> > On 2013/04/16 03:56:35, dcheng wrote:
> > > On 2013/04/16 03:26:03, Hongbo Min wrote:
> > > > On 2013/04/16 01:27:27, dcheng wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > > > > File content/browser/web_contents/web_contents_drag_win.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/14122008/diff/1/content/browser/web_contents/...
> > > > > content/browser/web_contents/web_contents_drag_win.cc:453:
ip.mi.dwFlags
> =
> > > > > MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_ABSOLUTE;
> > > > > On 2013/04/16 01:13:44, Hongbo Min wrote:
> > > > > > On 2013/04/15 18:07:37, dcheng wrote:
> > > > > > > Why do we use the right mouse button? How come we can't just
> simulate
> > a
> > > > > normal
> > > > > > > DnD by using the left mouse button?
> > > > > > 
> > > > > > The reason is, if we just simulate left mouse button down, then left
> > mouse
> > > > > > button up event won't get fired to exit DoDragDrop loop, and wo also
> > don't
> > > > > know
> > > > > > when to simulate the left mouse button up event, and so DoDragDrop
> won't
> > > > exit
> > > > > > forever.
> > > > > 
> > > > > Who's triggering the right mouse button up event then? Is that
something
> > the
> > > > > system is magically triggering somewhere? It seems kind of fragile as
> > well;
> > > if
> > > > > the user also presses right-click on his pointing device during a
> > long-press
> > > > > drag, won't it cause the logic to get confused?
> > > > > 
> > > > > It seems like it would be more appropriate to teach ui::DragSourceWin
> > about
> > > > > gesture-initiated drags (e.g. add a set_in_gesture_drag() setter) so
> > > > > QueryContinueDrag() can use that information to determine whether or
not
> > to
> > > > exit
> > > > > the dragging loop.
> > > > 
> > > > On Windows, press and hold will trigger right mouse button behavior,
once
> > the
> > > > touch point is removed from screen, then right mouse button up event
gets
> > > > triggered, and DoDragDrop loop consumes this up event to complete the
> > dragging
> > > > operation.
> > > > 
> > > > The problem is, with set_in_gesture_drag(), we still don't know when to
> exit
> > > > DoDragDrop loop since the mouse button up event is consumed by
DoDragDrop,
> > > RWHV
> > > > won't get such event if DoDragDrop loop is running, so no clue is used
for
> > > > invalidating in_long_press_gesture status.
> > > 
> > > OK, a few more questions to try to make sure I understand the situation
> > > correctly. =)
> > > 
> > > From what you've told me (I don't have a Windows touch device to test this
> > > myself), initiating a long gesture is equivalent to pressing the right
mouse
> > > button, and ending the gesture is equivalent to releasing it right?
> > > 
> > > In that case, why is it critical to call SendMouseEventForTouchDnD()? If
you
> > > don't call it, is DoDragDrop() confused about which mouse buttons are
> pressed?
> > 
> > OK. Let me describe what will be happened after press and hold on Windows:
> > 
> > 1) Suppose that you press and hold on the screen, at this moment, only
> WM_TOUCH
> > message is emitted.
> > 2) If you try to move your finger, the event sequence is:
> >    * WM_TOUCH
> >    * WM_MOUSEMOVE
> >    * WM_RBUTTONDOWN
> >    * WM_TOUCH
> >    * WM_MOUSEMOVE
> >    * WM_TOUCH
> >    * WM_MOUSEMOVE
> >    * (repeatedly until your finger is up)
> > 3) On your finger is up, the event sequence is:
> >    * WM_TOUCH
> >    * WM_RBUTTONUP
> > 
> > In summary, the right mouse button down event is fired once your finger is
> moved
> > or up after long press.
> > 
> > However, the DoDragDrop won't get right mouse button down event although you
> > move your finger or up unless a simulated right mouse is fired
> programmatically,
> > like this CL does, once DoDragDrop receives such simulated mouse down event,
> all
> > events listed above can be fired one by one, therefore, once your finger is
> up,
> > the DoDragDrop exits since it got a right button up event.
> > 
> > The reason I guess is, DoDragDrop will ignore touch event and only mouse
down
> > event (both left and right) can drive DoDragDrop to proceed ahead, so once
> > DoDragDrop loop get touch event, it just puts the event into a cached event
> > queue. After it gets mouse down event, all events in queue will be populated
> one
> > by one, and these touch events will be recognized as mouse event on the fly
> > (similar with gesture recognizer).
> 
> dcheng@, since this solution is to use right mouse button up event (the finger
> is up after a long press) to complete DoDragDrop operation, it might be a
> problem if user disable emitting right button down after a long press on
Windows
> through Control Panel/Touch setting.
> 
> The thing I am considering is, is there any API on Windows to query the number
> of touch points on screen, something like QueryTouchPointsNum? I failed to
find
> it on MSDN.
> 
> If it does, we could safely simulate left button down event to drive
DoDragDrop
> go ahead, and at the same time, we create a background thread to query the
> number of touch point at a fixed interval (e.g. 50ms), if the number is 0,
then
> we simulate left button up to complete DoDragDrop operation.

FYI, I've uploaded my patch to fix the deadlock here:
https://codereview.chromium.org/13979012/
If you don't mind, could you patch it in and see if it works for you?

[Hongbo Min, 2013-04-25 01:49:11]

Sure, I'd like to try it today.

[Hongbo Min, 2013-04-25 06:15:15]

On 2013/04/25 01:49:11, Hongbo Min wrote:
> Sure, I'd like to try it today.

dcheng@, I had a try to patch it with this CL on touchable Win8 tablet.

It sounds the deadlock issue is resolved now. Cool!

But there are still some problem in drag-out to download file:
1. It is hard to trigger DoDragDrop on touch screen after long press.
2. Even a long press is trigger, and DoDragDrop gets started, the page will
scroll if I try to move the finger to drag it out.
3. After drag it out to the desktop and the finger is up, then you will find
that a popup menu is showed to ask you to select 'Copy Here/Move Here/Create
Shortcut/Cancel' item. The behavior is the same as that of dragging files using
right mouse button on desktop. I think it is caused by that we simulate right
mouse button event to drive DoDragDrop operation.

Right now, I am a bit concerned about the right mouse button event simulation
solution, it is not so perfect and seems to have a side effect. It should be
better if we can simulate left mouse button event instead but the problem is
that we don't know when to trigger left mouse button up event to finish
DoDragDrop.

Any hints? Thanks.

[Hongbo Min, 2013-05-03 05:01:09]

ping dcheng@ ... Thanks.

[dcheng, 2013-05-07 00:27:31]

On 2013/05/03 05:01:09, Hongbo Min wrote:
> ping dcheng@ ... Thanks.

Sorry for the delay in reply.
At this point, I think the best thing we can do is use Spy++ on Explorer and see
how it handles touch drag-and-drop. I won't be able to do this myself before
Wednesday, so if you have time to investigate it, that's where I would start.

[Hongbo Min, 2013-05-16 14:47:59]

dcheng@, I have just updated the patch. 

According to my test on win8 tablet, the touch-initiated drag and drop could
work well. 

Could you have a review again? Thanks.

https://codereview.chromium.org/14122008/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/render_widget_host_view_win.h (right):

https://codereview.chromium.org/14122008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_win.h:287: bool
has_valid_long_press_gesture() { return has_valid_long_press_gesture_; }
On 2013/04/15 18:07:37, dcheng wrote:
> Nit: make this const.

Done.

https://codereview.chromium.org/14122008/diff/1/content/browser/renderer_host...
content/browser/renderer_host/render_widget_host_view_win.h:602: bool
has_valid_long_press_gesture_;
On 2013/04/15 18:07:37, dcheng wrote:
> Perhaps just call this "in_long_press_gesture".

Done.

[Hongbo Min, 2013-05-20 09:36:10]

dcheng@, could you please have a look at this again? Thanks.

[dcheng, 2013-05-23 00:44:08]

On 2013/05/20 09:36:10, Hongbo Min wrote:
> dcheng@, could you please have a look at this again? Thanks.

I haven't forgotten about this. I'm aiming to get a response out to you by
tomorrow. Sorry about the delay.

[dcheng, 2013-05-24 01:10:36]

https://codereview.chromium.org/14122008/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/14122008/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_win.cc:1751: // On
Windows, a mouse up event gets triggerred once a touch point is
Nit: triggered.

https://codereview.chromium.org/14122008/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_win.cc:1753: if (message
== WM_LBUTTONUP || message == WM_RBUTTONUP)
Shouldn't this just be WM_RBUTTONUP? Or are there times when a long press
gesture is treated as a left mouse event?

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:77: if (msg->message ==
WM_RBUTTONUP || !(GetKeyState(VK_RBUTTON) & 0x8000))
Ideally I'd like something similar here--only a mouse button up event
corresponding to the type of drag should stop forwarding messages.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:177:
(web_contents_->GetRenderWidgetHostView());
Style nit: It's more typical to see the ( on the previous line. Same for line
435.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:185:
SendMouseEventForTouchDnD();
Can we move this logic into DoDragging to avoid the duplication here?

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:442:
::GetWindowRect(::GetDesktopWindow(), &screen_rect);
This seems kind of suspicious. Other code uses GetSystemMetrics(), but that only
considers the primary monitor. I'm cc'ing oshima who did a lot of work on
ui::gfx::Screen and hopefully he'll have a better answer for this question...

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:459: INPUT ip = { 0 };
Style nit: just call this 'input'

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:464: ::SendInput(1, &ip,
sizeof(INPUT));
Style nit: prefer sizeof(ip), per the style guide.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
File content/browser/web_contents/web_drag_source_win.cc (right):

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_drag_source_win.cc:98: RenderViewHost*
render_view_host = web_contents_->GetRenderViewHost();
Why not just GetRenderWidgetHostView() directly?

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_drag_source_win.cc:134: return !(key_state &
MK_LBUTTON) && !(key_state & MK_RBUTTON);
I would prefer to see the right-mouse button check more tightly bound to the
fact that it's due to a touch-initiated drag. In fact, I think we should just
move this logic into DragSourceWin in ui/base/dragdrop. Basically, DragSourceWin
would take an enum in its constructor that indicates whether it's for a touch or
mouse-initiated drag. Then QueryContinueDrag could simply use that to check.

[Hongbo Min, 2013-05-27 14:51:40]

https://codereview.chromium.org/14122008/diff/27001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

https://codereview.chromium.org/14122008/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_win.cc:1751: // On
Windows, a mouse up event gets triggerred once a touch point is
On 2013/05/24 01:10:37, dcheng wrote:
> Nit: triggered.

Done.

https://codereview.chromium.org/14122008/diff/27001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_win.cc:1753: if (message
== WM_LBUTTONUP || message == WM_RBUTTONUP)
On 2013/05/24 01:10:37, dcheng wrote:
> Shouldn't this just be WM_RBUTTONUP? Or are there times when a long press
> gesture is treated as a left mouse event?

Only WM_RBUTTONUP is taken into consideration. Done.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:77: if (msg->message ==
WM_RBUTTONUP || !(GetKeyState(VK_RBUTTON) & 0x8000))
On 2013/05/24 01:10:37, dcheng wrote:
> Ideally I'd like something similar here--only a mouse button up event
> corresponding to the type of drag should stop forwarding messages.

Add |touch_initiated_drag_drop| bool to track it.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:177:
(web_contents_->GetRenderWidgetHostView());
On 2013/05/24 01:10:37, dcheng wrote:
> Style nit: It's more typical to see the ( on the previous line. Same for line
> 435.

Done.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:185:
SendMouseEventForTouchDnD();
On 2013/05/24 01:10:37, dcheng wrote:
> Can we move this logic into DoDragging to avoid the duplication here?

Done.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:442:
::GetWindowRect(::GetDesktopWindow(), &screen_rect);
On 2013/05/24 01:10:37, dcheng wrote:
> This seems kind of suspicious. Other code uses GetSystemMetrics(), but that
only
> considers the primary monitor. I'm cc'ing oshima who did a lot of work on
> ui::gfx::Screen and hopefully he'll have a better answer for this question...

Just use GetSystemMetrics instead. Need to oshima@ further double-check.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:459: INPUT ip = { 0 };
On 2013/05/24 01:10:37, dcheng wrote:
> Style nit: just call this 'input'

Done.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:464: ::SendInput(1, &ip,
sizeof(INPUT));
On 2013/05/24 01:10:37, dcheng wrote:
> Style nit: prefer sizeof(ip), per the style guide.

Done.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
File content/browser/web_contents/web_drag_source_win.cc (right):

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_drag_source_win.cc:98: RenderViewHost*
render_view_host = web_contents_->GetRenderViewHost();
On 2013/05/24 01:10:37, dcheng wrote:
> Why not just GetRenderWidgetHostView() directly?

Done.

https://codereview.chromium.org/14122008/diff/27001/content/browser/web_conte...
content/browser/web_contents/web_drag_source_win.cc:134: return !(key_state &
MK_LBUTTON) && !(key_state & MK_RBUTTON);
On 2013/05/24 01:10:37, dcheng wrote:
> I would prefer to see the right-mouse button check more tightly bound to the
> fact that it's due to a touch-initiated drag. In fact, I think we should just
> move this logic into DragSourceWin in ui/base/dragdrop. Basically,
DragSourceWin
> would take an enum in its constructor that indicates whether it's for a touch
or
> mouse-initiated drag. Then QueryContinueDrag could simply use that to check.

Good suggestion, thanks. Done!

[dcheng, 2013-05-28 07:38:35]

+oshima for real to see what he has to say about the monitor code. I'll take a
look at the updated patch when I'm in the office tomorrow.

[oshima, 2013-05-28 20:21:39]

Any reason why you can't or shouldn't use gfx::Screen to get the display size? I
believe This api returns the size for primary display in pixels, and may not
work for external
display and/or win8/metro UI. +girard for high DPI scenario.

[dcheng, 2013-05-28 20:34:02]

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:81: if (msg->message ==
WM_RBUTTONUP)
Nit: I think you can just fold this into line 64-65 (so you don't need the extra
PostThreadMessage here)

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:183:
(web_contents_->GetRenderWidgetHostView());
Nit: ( on previous line.

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:467: int screen_height  =
::GetSystemMetrics(SM_CYSCREEN);
Nit: extra space.

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
File content/browser/web_contents/web_drag_source_win.h (right):

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_drag_source_win.h:62: void OnDragSourceEnded();
I feel like this could use a slightly better name... maybe
CancelLongPressGestureIfNeeded() since that's what it does?

https://codereview.chromium.org/14122008/diff/49001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.cc (right):

https://codereview.chromium.org/14122008/diff/49001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.cc:9: DragSourceWin::DragSourceWin()
Nit: do we need this or can we just force all locations that instantiate this
class to be explicit?

[Hongbo Min, 2013-05-29 06:08:03]

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:81: if (msg->message ==
WM_RBUTTONUP)
On 2013/05/28 20:34:02, dcheng wrote:
> Nit: I think you can just fold this into line 64-65 (so you don't need the
extra
> PostThreadMessage here)

Done.

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:183:
(web_contents_->GetRenderWidgetHostView());
On 2013/05/28 20:34:02, dcheng wrote:
> Nit: ( on previous line.

Done.

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:467: int screen_height  =
::GetSystemMetrics(SM_CYSCREEN);
On 2013/05/28 20:34:02, dcheng wrote:
> Nit: extra space.

Use gfx::Screen and gfx::Display API instead now.

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
File content/browser/web_contents/web_drag_source_win.h (right):

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_drag_source_win.h:62: void OnDragSourceEnded();
On 2013/05/28 20:34:02, dcheng wrote:
> I feel like this could use a slightly better name... maybe
> CancelLongPressGestureIfNeeded() since that's what it does?

Done.

https://codereview.chromium.org/14122008/diff/49001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.cc (right):

https://codereview.chromium.org/14122008/diff/49001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.cc:9: DragSourceWin::DragSourceWin()
On 2013/05/28 20:34:02, dcheng wrote:
> Nit: do we need this or can we just force all locations that instantiate this
> class to be explicit?

I prefer to reserve it since it is unnecessary to change other code. This patch
is something specific HTML5 drag-drop by touch.

If the usage of DragSourceWin also has the same problem as HTML5 drag-drop, we
could remove it in future.

[Hongbo Min, 2013-05-29 06:09:12]

On 2013/05/28 20:21:39, oshima wrote:
> Any reason why you can't or shouldn't use gfx::Screen to get the display size?
I
> believe This api returns the size for primary display in pixels, and may not
> work for external
> display and/or win8/metro UI. +girard for high DPI scenario.

Now I am using gfx::Screen and GetDisplayNearestPoint to get the display for
RWHV.

[oshima, 2013-05-29 14:41:01]

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:462: gfx::Display display
= screen->GetDisplayNearestPoint(last_touch_position);
It may be better to use GetDisplayNearestWindow as you have access to
native view. Please ignore if you think it's better to use touch position
instead.

[girard, 2013-05-29 18:57:51]

Regarding the impact of high dpi: The code as written should work fine once high
dpi is enabled. In the current form, touch events get scaled to "virtual
pixels", and so does the screen size. Thus the proportional sizing should be
correct.

Once we become DPI aware, both the screen size and the touch event location will
get mapped to true pixels, and again, everything should work correctly.

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:81: if (msg->message ==
WM_RBUTTONUP)
On 2013/05/28 20:34:02, dcheng wrote:
> Nit: I think you can just fold this into line 64-65 (so you don't need the
extra
> PostThreadMessage here)

Is this code even reachable given the test in line 64-65?

[dcheng, 2013-05-29 20:16:43]

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:65: msg->message ==
WM_KEYDOWN || msg->message == WM_KEYUP ||
WM_LBUTTONUP here and lines 75 should probably be tied to
!touch_initiated_drag_drop.

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:81: if
(touch_initiated_drag_drop && (msg->message == WM_RBUTTONUP ||
I think this should be (touch_initiated_drag_drop && ((msg->message == ... ||
!(GetKeyState(...))))

[Hongbo Min, 2013-05-30 01:31:49]

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:65: msg->message ==
WM_KEYDOWN || msg->message == WM_KEYUP ||
On 2013/05/29 20:16:43, dcheng wrote:
> WM_LBUTTONUP here and lines 75 should probably be tied to
> !touch_initiated_drag_drop.

Done.

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:81: if
(touch_initiated_drag_drop && (msg->message == WM_RBUTTONUP ||
On 2013/05/29 20:16:43, dcheng wrote:
> I think this should be (touch_initiated_drag_drop && ((msg->message == ... ||
> !(GetKeyState(...))))

Why do we need additional '(...)' to wrap "msg->message == .." again?

[Hongbo Min, 2013-05-30 01:36:02]

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:81: if (msg->message ==
WM_RBUTTONUP)
On 2013/05/29 18:57:52, girard wrote:
> On 2013/05/28 20:34:02, dcheng wrote:
> > Nit: I think you can just fold this into line 64-65 (so you don't need the
> extra
> > PostThreadMessage here)
> 
> Is this code even reachable given the test in line 64-65?

Fixed.

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:462: gfx::Display display
= screen->GetDisplayNearestPoint(last_touch_position);
On 2013/05/29 14:41:01, oshima wrote:
> It may be better to use GetDisplayNearestWindow as you have access to
> native view. Please ignore if you think it's better to use touch position
> instead.

Thanks for your information. I still think it should be better to use the touch
position since the native window may cross more than one display, but the point
can be tied to only one display.

[oshima, 2013-05-30 18:32:21]

On 2013/05/30 01:36:02, Hongbo Min wrote:
>
https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
> File content/browser/web_contents/web_contents_drag_win.cc (right):
> 
>
https://codereview.chromium.org/14122008/diff/49001/content/browser/web_conte...
> content/browser/web_contents/web_contents_drag_win.cc:81: if (msg->message ==
> WM_RBUTTONUP)
> On 2013/05/29 18:57:52, girard wrote:
> > On 2013/05/28 20:34:02, dcheng wrote:
> > > Nit: I think you can just fold this into line 64-65 (so you don't need the
> > extra
> > > PostThreadMessage here)
> > 
> > Is this code even reachable given the test in line 64-65?
> 
> Fixed.
> 
>
https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
> File content/browser/web_contents/web_contents_drag_win.cc (right):
> 
>
https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
> content/browser/web_contents/web_contents_drag_win.cc:462: gfx::Display
display
> = screen->GetDisplayNearestPoint(last_touch_position);
> On 2013/05/29 14:41:01, oshima wrote:
> > It may be better to use GetDisplayNearestWindow as you have access to
> > native view. Please ignore if you think it's better to use touch position
> > instead.
> 
> Thanks for your information. I still think it should be better to use the
touch
> position since the native window may cross more than one display, but the
point
> can be tied to only one display.

Ah, that's true on windows (one window never cross displays on cros). Please
disregard my comment.

[dcheng, 2013-05-31 00:57:04]

LGTM with some nits.

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/56001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:81: if
(touch_initiated_drag_drop && (msg->message == WM_RBUTTONUP ||
On 2013/05/30 01:31:50, Hongbo Min wrote:
> On 2013/05/29 20:16:43, dcheng wrote:
> > I think this should be (touch_initiated_drag_drop && ((msg->message == ...
||
> > !(GetKeyState(...))))
> 
> Why do we need additional '(...)' to wrap "msg->message == .." again?

I mismatched the parens. See my next comment =)

https://codereview.chromium.org/14122008/diff/64001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/64001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:76: if
(!touch_initiated_drag_drop && ((msg->message == WM_LBUTTONUP ||
It might be slightly clearer to do:
if (!touch_initiated_drag_drop) {
  if (msg->message == WM_LBUTTONUP || ...) {
  }
} else {
  ...
}

https://codereview.chromium.org/14122008/diff/64001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.h (right):

https://codereview.chromium.org/14122008/diff/64001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.h:55: DragDropTypes::DragEventSource
event_source_;
Nit: mark const.

[commit-bot: I haz the power, 2013-06-01 02:30:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/14122008/72001

[commit-bot: I haz the power, 2013-06-01 02:40:26]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Hongbo Min, 2013-06-01 12:14:28]

ben@, need your lgtm for this CL. Thanks.

[Hongbo Min, 2013-06-02 02:41:21]

https://codereview.chromium.org/14122008/diff/64001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/64001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:76: if
(!touch_initiated_drag_drop && ((msg->message == WM_LBUTTONUP ||
On 2013/05/31 00:57:05, dcheng wrote:
> It might be slightly clearer to do:
> if (!touch_initiated_drag_drop) {
>   if (msg->message == WM_LBUTTONUP || ...) {
>   }
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/14122008/diff/64001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.h (right):

https://codereview.chromium.org/14122008/diff/64001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.h:55: DragDropTypes::DragEventSource
event_source_;
On 2013/05/31 00:57:05, dcheng wrote:
> Nit: mark const.

Done.

[Hongbo Min, 2013-06-04 07:46:52]

ping ben@ ... Thanks

[Hongbo Min, 2013-06-04 07:46:54]

ping ben@ ... Thanks!

[Hongbo Min, 2013-06-07 01:40:53]

dcheng@, could you please help add the owners of the changed files? Thanks.

[varunjain, 2013-06-07 19:46:47]

+sky since ben seems to be busy.

[sky, 2013-06-07 21:29:50]

https://codereview.chromium.org/14122008/diff/72001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/72001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:353:
SendMouseEventForTouchDnD();
This seems like a hack. Are we doing something wrong that is making DoDragDrop
not work from a gesture?

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.cc (right):

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.cc:15: : cancel_drag_(false),
event_source_(event_source) {
when you wrap, each param on its own line.

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.h (right):

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.h:26:
DragSourceWin(DragDropTypes::DragEventSource event_source);
explicit, and nuke the other constructors.

[Hongbo Min, 2013-06-08 03:08:48]

https://codereview.chromium.org/14122008/diff/72001/content/browser/web_conte...
File content/browser/web_contents/web_contents_drag_win.cc (right):

https://codereview.chromium.org/14122008/diff/72001/content/browser/web_conte...
content/browser/web_contents/web_contents_drag_win.cc:353:
SendMouseEventForTouchDnD();
On 2013/06/07 21:29:50, sky wrote:
> This seems like a hack. Are we doing something wrong that is making DoDragDrop
> not work from a gesture?

It is almost a hack since the limitation of DoDragDrop. The DoDragDrop can not
be driven by touch event, so in order to make DoDragDrop proceed, we need to
simulate a mouse button down event and feed it into DoDragDrop loop. Otherwise,
DoDragDrop calling will be blocked forever until receiving a mouse button down
event.

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.cc (right):

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.cc:15: : cancel_drag_(false),
event_source_(event_source) {
On 2013/06/07 21:29:50, sky wrote:
> when you wrap, each param on its own line.

Done.

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
File ui/base/dragdrop/drag_source_win.h (right):

https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
ui/base/dragdrop/drag_source_win.h:26:
DragSourceWin(DragDropTypes::DragEventSource event_source);
On 2013/06/07 21:29:50, sky wrote:
> explicit, and nuke the other constructors.

Done.

[sky, 2013-06-10 14:53:02]

On Fri, Jun 7, 2013 at 8:08 PM,  <hongbo.min@intel.com> wrote:
>
>
https://codereview.chromium.org/14122008/diff/72001/content/browser/web_conte...
> File content/browser/web_contents/web_contents_drag_win.cc (right):
>
>
https://codereview.chromium.org/14122008/diff/72001/content/browser/web_conte...
> content/browser/web_contents/web_contents_drag_win.cc:353:
> SendMouseEventForTouchDnD();
> On 2013/06/07 21:29:50, sky wrote:
>>
>> This seems like a hack. Are we doing something wrong that is making
>
> DoDragDrop
>>
>> not work from a gesture?
>
>
> It is almost a hack since the limitation of DoDragDrop. The DoDragDrop
> can not be driven by touch event, so in order to make DoDragDrop
> proceed, we need to simulate a mouse button down event and feed it into
> DoDragDrop loop. Otherwise, DoDragDrop calling will be blocked forever
> until receiving a mouse button down event.

Have you tried looking at what other apps do here? I'm thinking of
using spy++ or the like. I'm skeptical this is the right solution.

  -Scott

>
>
https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
> File ui/base/dragdrop/drag_source_win.cc (right):
>
>
https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
> ui/base/dragdrop/drag_source_win.cc:15: : cancel_drag_(false),
> event_source_(event_source) {
> On 2013/06/07 21:29:50, sky wrote:
>>
>> when you wrap, each param on its own line.
>
>
> Done.
>
>
>
https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
> File ui/base/dragdrop/drag_source_win.h (right):
>
>
https://codereview.chromium.org/14122008/diff/72001/ui/base/dragdrop/drag_sou...
> ui/base/dragdrop/drag_source_win.h:26:
> DragSourceWin(DragDropTypes::DragEventSource event_source);
> On 2013/06/07 21:29:50, sky wrote:
>>
>> explicit, and nuke the other constructors.
>
>
> Done.
>
> https://codereview.chromium.org/14122008/

[Hongbo Min, 2013-06-20 13:24:59]

On 2013/06/10 14:53:02, sky wrote:
> On Fri, Jun 7, 2013 at 8:08 PM,  <mailto:hongbo.min@intel.com> wrote:
> >
> >
> > It is almost a hack since the limitation of DoDragDrop. The DoDragDrop
> > can not be driven by touch event, so in order to make DoDragDrop
> > proceed, we need to simulate a mouse button down event and feed it into
> > DoDragDrop loop. Otherwise, DoDragDrop calling will be blocked forever
> > until receiving a mouse button down event.
> 
> Have you tried looking at what other apps do here? I'm thinking of
> using spy++ or the like. I'm skeptical this is the right solution.
> 
>   -Scott
> 

Yes, I have ever use spy++ to trace other app like file explorer. The result
shows that the drag-drop by touch depends on left mouse button down state, you
can drag a selected item from one folder to another folder if you finger is down
to screen (A WM_LBUTTONDOWN message is followed by a WM_TOUCH move event
immediately after WM_TOUCH down event).

However, if touch-initiated drag-drop is enabled in chromium, if a finger is
pressing and holding on an draggable element, the render_host will forward a
long press gesture event to render process. The long press gesture is recognized
by ui::GestureRecognizer. Without gesture recognizer, the press and hold gesture
will trigger a right mouse down event followed by a touch move event. 

On receiving long press gesture event, the render process will send IPC message
to browser process to start dragging operation which calls DoDragDrop COM
function, and browser process runs into DoDragDrop loop. But at this time point,
no any mouse button down event is sent to DoDragDrop loop, so DoDragDrop hangs.

[Hongbo Min, 2013-06-20 13:43:14]

On 2013/06/20 13:24:59, Hongbo Min wrote:
> On 2013/06/10 14:53:02, sky wrote:
> > On Fri, Jun 7, 2013 at 8:08 PM,  <mailto:hongbo.min@intel.com> wrote:
> > >
> > >
> > > It is almost a hack since the limitation of DoDragDrop. The DoDragDrop
> > > can not be driven by touch event, so in order to make DoDragDrop
> > > proceed, we need to simulate a mouse button down event and feed it into
> > > DoDragDrop loop. Otherwise, DoDragDrop calling will be blocked forever
> > > until receiving a mouse button down event.
> > 
> > Have you tried looking at what other apps do here? I'm thinking of
> > using spy++ or the like. I'm skeptical this is the right solution.
> > 
> >   -Scott
> > 
> 
> Yes, I have ever use spy++ to trace other app like file explorer. The result
> shows that the drag-drop by touch depends on left mouse button down state, you
> can drag a selected item from one folder to another folder if you finger is
down
> to screen (A WM_LBUTTONDOWN message is followed by a WM_TOUCH move event
> immediately after WM_TOUCH down event).
> 
> However, if touch-initiated drag-drop is enabled in chromium, if a finger is
> pressing and holding on an draggable element, the render_host will forward a
> long press gesture event to render process. The long press gesture is
recognized
> by ui::GestureRecognizer. Without gesture recognizer, the press and hold
gesture
> will trigger a right mouse down event followed by a touch move event. 
> 
> On receiving long press gesture event, the render process will send IPC
message
> to browser process to start dragging operation which calls DoDragDrop COM
> function, and browser process runs into DoDragDrop loop. But at this time
point,
> no any mouse button down event is sent to DoDragDrop loop, so DoDragDrop
hangs.

UPDATE DESCRIPTION:

Let me describe what will be happened after press and hold on Windows:

1) Suppose that you press and hold on the screen, at this moment, only WM_TOUCH
message is emitted.
2) If you try to move your finger, the event sequence is:
   * WM_TOUCH
   * WM_MOUSEMOVE
   * WM_RBUTTONDOWN
   * WM_TOUCH
   * WM_MOUSEMOVE
   * WM_TOUCH
   * WM_MOUSEMOVE
   * (repeatedly until your finger is up)
3) On your finger is up, the event sequence is:
   * WM_TOUCH
   * WM_RBUTTONUP

In summary, the right mouse button down event is fired once your finger is moved
or up after long press.

However, the DoDragDrop won't get right mouse button down event although you
move your finger or up unless a simulated right mouse is fired programmatically,
like this CL does, once DoDragDrop receives such simulated mouse down event, all
events listed above can be fired one by one, therefore, once your finger is up,
the DoDragDrop exits since it got a right button up event.

The reason is, DoDragDrop will ignore touch event and only mouse down
event (both left and right) can drive DoDragDrop to proceed ahead, so once
DoDragDrop loop get touch event, it just puts the event into a cached event
queue. After it gets mouse down event, all events in queue will be populated one
by one, and these touch events will be recognized as mouse event on the fly
(similar with gesture recognizer).

The following sequence will explain why touch-initiated drag-drop will hang on
Windows:

1. Press and hold on a HTML element
2. Long press gesture is recognized by ui::GestureRecognizer and forwarded to
renderer process;
3. Renderer process notifies browser process to start dragging if it is on a
draggable element
4. Run into DoDragDrop to wait for mouse down event but no mouse down event gets
triggered (only left mouse move event by touch move).

[sky, 2013-06-20 17:39:13]

What happens if you spy on events in another app, say IE or FF?

On Thu, Jun 20, 2013 at 6:43 AM,  <hongbo.min@intel.com> wrote:
> On 2013/06/20 13:24:59, Hongbo Min wrote:
>>
>> On 2013/06/10 14:53:02, sky wrote:
>> > On Fri, Jun 7, 2013 at 8:08 PM,  <mailto:hongbo.min@intel.com> wrote:
>> > >
>> > >
>> > > It is almost a hack since the limitation of DoDragDrop. The DoDragDrop
>> > > can not be driven by touch event, so in order to make DoDragDrop
>> > > proceed, we need to simulate a mouse button down event and feed it
>> > > into
>> > > DoDragDrop loop. Otherwise, DoDragDrop calling will be blocked forever
>> > > until receiving a mouse button down event.
>> >
>> > Have you tried looking at what other apps do here? I'm thinking of
>> > using spy++ or the like. I'm skeptical this is the right solution.
>> >
>> >   -Scott
>> >
>
>
>> Yes, I have ever use spy++ to trace other app like file explorer. The
>> result
>> shows that the drag-drop by touch depends on left mouse button down state,
>> you
>> can drag a selected item from one folder to another folder if you finger
>> is
>
> down
>>
>> to screen (A WM_LBUTTONDOWN message is followed by a WM_TOUCH move event
>> immediately after WM_TOUCH down event).
>
>
>> However, if touch-initiated drag-drop is enabled in chromium, if a finger
>> is
>> pressing and holding on an draggable element, the render_host will forward
>> a
>> long press gesture event to render process. The long press gesture is
>
> recognized
>>
>> by ui::GestureRecognizer. Without gesture recognizer, the press and hold
>
> gesture
>>
>> will trigger a right mouse down event followed by a touch move event.
>
>
>> On receiving long press gesture event, the render process will send IPC
>
> message
>>
>> to browser process to start dragging operation which calls DoDragDrop COM
>> function, and browser process runs into DoDragDrop loop. But at this time
>
> point,
>>
>> no any mouse button down event is sent to DoDragDrop loop, so DoDragDrop
>
> hangs.
>
> UPDATE DESCRIPTION:
>
> Let me describe what will be happened after press and hold on Windows:
>
> 1) Suppose that you press and hold on the screen, at this moment, only
> WM_TOUCH
> message is emitted.
> 2) If you try to move your finger, the event sequence is:
>    * WM_TOUCH
>    * WM_MOUSEMOVE
>    * WM_RBUTTONDOWN
>    * WM_TOUCH
>    * WM_MOUSEMOVE
>    * WM_TOUCH
>    * WM_MOUSEMOVE
>    * (repeatedly until your finger is up)
> 3) On your finger is up, the event sequence is:
>    * WM_TOUCH
>    * WM_RBUTTONUP
>
> In summary, the right mouse button down event is fired once your finger is
> moved
> or up after long press.
>
> However, the DoDragDrop won't get right mouse button down event although you
> move your finger or up unless a simulated right mouse is fired
> programmatically,
> like this CL does, once DoDragDrop receives such simulated mouse down event,
> all
> events listed above can be fired one by one, therefore, once your finger is
> up,
> the DoDragDrop exits since it got a right button up event.
>
> The reason is, DoDragDrop will ignore touch event and only mouse down
> event (both left and right) can drive DoDragDrop to proceed ahead, so once
> DoDragDrop loop get touch event, it just puts the event into a cached event
> queue. After it gets mouse down event, all events in queue will be populated
> one
> by one, and these touch events will be recognized as mouse event on the fly
> (similar with gesture recognizer).
>
> The following sequence will explain why touch-initiated drag-drop will hang
> on
> Windows:
>
> 1. Press and hold on a HTML element
> 2. Long press gesture is recognized by ui::GestureRecognizer and forwarded
> to
> renderer process;
> 3. Renderer process notifies browser process to start dragging if it is on a
> draggable element
> 4. Run into DoDragDrop to wait for mouse down event but no mouse down event
> gets
> triggered (only left mouse move event by touch move).
>
>
>
>
>
>
> https://codereview.chromium.org/14122008/

[Hongbo Min, 2013-06-21 02:00:14]

On 2013/06/20 17:39:13, sky wrote:
> What happens if you spy on events in another app, say IE or FF?
> 

IE10 on Win 8 doesn't support touch-initiated HTML5 drag-drop.

FF supports in a different way from chromium, no long press gesture is needed to
drag in FF. You could drag an draggable element when a finger is moving right
after it is on the element. Press and hold gesture will trigger context menu in
FF.

[sky, 2013-06-21 15:44:49]

Aren't there other apps that support what we're trying to do?

On Thu, Jun 20, 2013 at 7:00 PM,  <hongbo.min@intel.com> wrote:
> On 2013/06/20 17:39:13, sky wrote:
>>
>> What happens if you spy on events in another app, say IE or FF?
>
>
>
> IE10 on Win 8 doesn't support touch-initiated HTML5 drag-drop.
>
> FF supports in a different way from chromium, no long press gesture is
> needed to
> drag in FF. You could drag an draggable element when a finger is moving
> right
> after it is on the element. Press and hold gesture will trigger context menu
> in
> FF.
>
>
> https://codereview.chromium.org/14122008/

[Hongbo Min, 2013-06-24 02:46:03]

On 2013/06/21 15:44:49, sky wrote:
> Aren't there other apps that support what we're trying to do?
> 
> On Thu, Jun 20, 2013 at 7:00 PM,  <mailto:hongbo.min@intel.com> wrote:
> > On 2013/06/20 17:39:13, sky wrote:
> >>
> >> What happens if you spy on events in another app, say IE or FF?
> >
> >
> >
> > IE10 on Win 8 doesn't support touch-initiated HTML5 drag-drop.
> >
> > FF supports in a different way from chromium, no long press gesture is
> > needed to
> > drag in FF. You could drag an draggable element when a finger is moving
> > right
> > after it is on the element. Press and hold gesture will trigger context menu
> > in
> > FF.
> >
> >
> > https://codereview.chromium.org/14122008/

Yes currently. I ever tried to spy the file explorer window, the result shows it
is a window which doesnt not accept touch event (WM_TOUCH) at all, so its
process of drag-drop is meaningless for us.

Actually, we do drag-drop in a different way:
1) The RWHV window is registered as a touch window by RegisterTouchWindow;
2) We have our own GestureRecognizer to recognize the long press gesture from
WM_TOUCH event, which might not be compatible with Windows event recognition
(e.g. right mouse button down from press and hold gesture).

So if we implement drag-drop using long-press gesture on Windows, this CL is the
current best solution I can know. Your thoughts?

[sky, 2013-06-24 16:13:41]

This still feels like the wrong thing to me, but I don't have any other ideas.
Maybe Varun or Girard does.

[Hongbo Min, 2013-06-24 23:32:50]

On 2013/06/24 16:13:41, sky wrote:
> This still feels like the wrong thing to me, but I don't have any other ideas.
> Maybe Varun or Girard does.

ping varunjain@, ang girard@

[girard, 2013-06-27 07:47:12]

On 2013/06/24 16:13:41, sky wrote:
> This still feels like the wrong thing to me, but I don't have any other ideas.
> Maybe Varun or Girard does.

Some background: the DoDragDrop routine does not support WM_TOUCH events, and
only responds to mouse events. The apps in question either register for touch
events, in which case they receive "simulated" mouse events. Those mouse events
enable DoDragDrop to operate. (If apps don't register for touch events, a
different system kicks in... I'll ignore that case because, well, it doesn't
apply to us.)

In our case, we trap simulated mouse events in
RenderWidgetHostViewWin::ForwardMouseEventToRenderer, and discard them. This
breaks DoDragDrop. Hongbo Min's patch corrects this by generating new mouse
events.

An alternate approach might be to stop discarding the simulated mouse events
we're already receiving. In a quick experiment, I was able to disable one test
in RenderWidgetHostViewWin::ForwardMouseEventToRenderer (line 2958)
      if (!touch_events_enabled_ || !ui::IsMouseEventFromTouch(message)) {
and touch drag and drop worked reliably.

This "fix" would also send fake mouse messages through webkit and the javascript
client, and we don't want that to happen. So we'll need to do something a little
better.

[Hongbo Min, 2013-06-27 14:49:28]

On 2013/06/27 07:47:12, girard wrote:
> On 2013/06/24 16:13:41, sky wrote:
> > This still feels like the wrong thing to me, but I don't have any other
ideas.
> > Maybe Varun or Girard does.
> 
> Some background: the DoDragDrop routine does not support WM_TOUCH events, and
> only responds to mouse events. The apps in question either register for touch
> events, in which case they receive "simulated" mouse events. Those mouse
events
> enable DoDragDrop to operate. (If apps don't register for touch events, a
> different system kicks in... I'll ignore that case because, well, it doesn't
> apply to us.)
> 
> In our case, we trap simulated mouse events in
> RenderWidgetHostViewWin::ForwardMouseEventToRenderer, and discard them. This
> breaks DoDragDrop. Hongbo Min's patch corrects this by generating new mouse
> events.
> 
> An alternate approach might be to stop discarding the simulated mouse events
> we're already receiving. In a quick experiment, I was able to disable one test
> in RenderWidgetHostViewWin::ForwardMouseEventToRenderer (line 2958)
>       if (!touch_events_enabled_ || !ui::IsMouseEventFromTouch(message)) {
> and touch drag and drop worked reliably.

Do you mean that the touch dnd could work as expected by disabling one test on
ForwardMouseEventToRenderer:2958? I am curious since the touch dnd should be
triggered by long-press gesture, but on Windows, long press gesture will trigger
right mouse down button. The fact whether touch dnd can work won't depend on
mouse event forwarding to renderer. 

For chromium, if --enable-touch-drag-drop switch is on, the touch event on
browser side will be recognized as gesture. Once long press gesture is
recognized and forwarded to renderer, the hit test will decide whether to send a
start dragging notification to browser according to the hitting element is a
draggable one.

I didn't find any event handler logic related to line 2958. Could you explain it
a bit more? 

> 
> This "fix" would also send fake mouse messages through webkit and the
javascript
> client, and we don't want that to happen. So we'll need to do something a
little
> better.

Good catch. Will update it later.

[Hongbo Min, 2013-06-27 15:04:29]

On 2013/06/27 14:49:28, Hongbo Min wrote:
> On 2013/06/27 07:47:12, girard wrote:
> > On 2013/06/24 16:13:41, sky wrote:
> > > This still feels like the wrong thing to me, but I don't have any other
> ideas.
> > > Maybe Varun or Girard does.
> > 
> > 
> > This "fix" would also send fake mouse messages through webkit and the
> javascript
> > client, and we don't want that to happen. So we'll need to do something a
> little
> > better.
> 
> Good catch. Will update it later.

After re-thinking the problem you raised, I think the webkit and JS won't get
the fake mouse message since the SendMouseEventForDnD gets called inisde
WebContentsDragWin::DoDragging, followed by DoDragDrop and then run a separate
event loop to consume this event, as a result, the RWHV doesn't have a chance to
forward the fake event at all.

[girard, 2013-06-27 16:30:11]

On 2013/06/27 15:04:29, Hongbo Min wrote:
> On 2013/06/27 14:49:28, Hongbo Min wrote:
> > On 2013/06/27 07:47:12, girard wrote:
> > > On 2013/06/24 16:13:41, sky wrote:
> > > > This still feels like the wrong thing to me, but I don't have any other
> > ideas.
> > > > Maybe Varun or Girard does.
> > > 
> > > 
> > > This "fix" would also send fake mouse messages through webkit and the
> > javascript
> > > client, and we don't want that to happen. So we'll need to do something a
> > little
> > > better.
> > 
> > Good catch. Will update it later.
> 
> After re-thinking the problem you raised, I think the webkit and JS won't get
> the fake mouse message since the SendMouseEventForDnD gets called inisde
> WebContentsDragWin::DoDragging, followed by DoDragDrop and then run a separate
> event loop to consume this event, as a result, the RWHV doesn't have a chance
to
> forward the fake event at all.

Sorry - my comment about fake mouse messages was referring to my own patch, not
yours. Yours won't cause any issues with the content window. Mine was allowing
events (that had previously been blocked/dropped) to be sent to the content
window.

[varunjain, 2013-06-27 18:58:01]

On 2013/06/27 16:30:11, girard wrote:
> On 2013/06/27 15:04:29, Hongbo Min wrote:
> > On 2013/06/27 14:49:28, Hongbo Min wrote:
> > > On 2013/06/27 07:47:12, girard wrote:
> > > > On 2013/06/24 16:13:41, sky wrote:
> > > > > This still feels like the wrong thing to me, but I don't have any
other
> > > ideas.
> > > > > Maybe Varun or Girard does.
> > > > 
> > > > 
> > > > This "fix" would also send fake mouse messages through webkit and the
> > > javascript
> > > > client, and we don't want that to happen. So we'll need to do something
a
> > > little
> > > > better.
> > > 
> > > Good catch. Will update it later.
> > 
> > After re-thinking the problem you raised, I think the webkit and JS won't
get
> > the fake mouse message since the SendMouseEventForDnD gets called inisde
> > WebContentsDragWin::DoDragging, followed by DoDragDrop and then run a
separate
> > event loop to consume this event, as a result, the RWHV doesn't have a
chance
> to
> > forward the fake event at all.
> 
> Sorry - my comment about fake mouse messages was referring to my own patch,
not
> yours. Yours won't cause any issues with the content window. Mine was allowing
> events (that had previously been blocked/dropped) to be sent to the content
> window.

FYI, I have been told that IE11 preview
(http://msdn.microsoft.com/en-us/library/ie/dn265022(v=vs.85).aspx) supports
touch drag drop initiated through long press. It might be worthwhile to use
spy++ to check how they got over this issue.

[girard, 2013-06-27 19:06:43]

On 2013/06/27 18:58:01, varunjain wrote:
> On 2013/06/27 16:30:11, girard wrote:
> > On 2013/06/27 15:04:29, Hongbo Min wrote:
> > > On 2013/06/27 14:49:28, Hongbo Min wrote:
> > > > On 2013/06/27 07:47:12, girard wrote:
> > > > > On 2013/06/24 16:13:41, sky wrote:
> > > > > > This still feels like the wrong thing to me, but I don't have any
> other
> > > > ideas.
> > > > > > Maybe Varun or Girard does.
> > > > > 
> > > > > 
> > > > > This "fix" would also send fake mouse messages through webkit and the
> > > > javascript
> > > > > client, and we don't want that to happen. So we'll need to do
something
> a
> > > > little
> > > > > better.
> > > > 
> > > > Good catch. Will update it later.
> > > 
> > > After re-thinking the problem you raised, I think the webkit and JS won't
> get
> > > the fake mouse message since the SendMouseEventForDnD gets called inisde
> > > WebContentsDragWin::DoDragging, followed by DoDragDrop and then run a
> separate
> > > event loop to consume this event, as a result, the RWHV doesn't have a
> chance
> > to
> > > forward the fake event at all.
> > 
> > Sorry - my comment about fake mouse messages was referring to my own patch,
> not
> > yours. Yours won't cause any issues with the content window. Mine was
allowing
> > events (that had previously been blocked/dropped) to be sent to the content
> > window.
> 
> FYI, I have been told that IE11 preview
> (http://msdn.microsoft.com/en-us/library/ie/dn265022%28v=vs.85%29.aspx)
supports
> touch drag drop initiated through long press. It might be worthwhile to use
> spy++ to check how they got over this issue.

IE fires pointer events through to the browser. They don't suppress mouse events
like we do. That means they can call DoDragDrop, and DoDragDrop will still
receive WM_MOUSEMOVE, WM_LBUTTONUP, etc.

[Hongbo Min, 2013-07-03 01:58:00]

On 2013/06/27 19:06:43, girard wrote:
> On 2013/06/27 18:58:01, varunjain wrote:
> > On 2013/06/27 16:30:11, girard wrote:
> > > On 2013/06/27 15:04:29, Hongbo Min wrote:
> > > > On 2013/06/27 14:49:28, Hongbo Min wrote:
> > > > > On 2013/06/27 07:47:12, girard wrote:
> > > > > > On 2013/06/24 16:13:41, sky wrote:
> > > > > > > This still feels like the wrong thing to me, but I don't have any
> > other
> > > > > ideas.
> > > > > > > Maybe Varun or Girard does.
> > > > > > 
> > > > > > 
> > > > > > This "fix" would also send fake mouse messages through webkit and
the
> > > > > javascript
> > > > > > client, and we don't want that to happen. So we'll need to do
> something
> > a
> > > > > little
> > > > > > better.
> > > > > 
> > > > > Good catch. Will update it later.
> > > > 
> > > > After re-thinking the problem you raised, I think the webkit and JS
won't
> > get
> > > > the fake mouse message since the SendMouseEventForDnD gets called inisde
> > > > WebContentsDragWin::DoDragging, followed by DoDragDrop and then run a
> > separate
> > > > event loop to consume this event, as a result, the RWHV doesn't have a
> > chance
> > > to
> > > > forward the fake event at all.
> > > 
> > > Sorry - my comment about fake mouse messages was referring to my own
patch,
> > not
> > > yours. Yours won't cause any issues with the content window. Mine was
> allowing
> > > events (that had previously been blocked/dropped) to be sent to the
content
> > > window.
> > 
> > FYI, I have been told that IE11 preview
> > (http://msdn.microsoft.com/en-us/library/ie/dn265022%2528v=vs.85%2529.aspx)
> supports
> > touch drag drop initiated through long press. It might be worthwhile to use
> > spy++ to check how they got over this issue.
> 
> IE fires pointer events through to the browser. They don't suppress mouse
events
> like we do. That means they can call DoDragDrop, and DoDragDrop will still
> receive WM_MOUSEMOVE, WM_LBUTTONUP, etc.

varunjain@, then what is the conclusion for this patch?

[varunjain, 2013-07-03 04:52:46]

On 2013/07/03 01:58:00, Hongbo Min wrote:
> On 2013/06/27 19:06:43, girard wrote:
> > On 2013/06/27 18:58:01, varunjain wrote:
> > > On 2013/06/27 16:30:11, girard wrote:
> > > > On 2013/06/27 15:04:29, Hongbo Min wrote:
> > > > > On 2013/06/27 14:49:28, Hongbo Min wrote:
> > > > > > On 2013/06/27 07:47:12, girard wrote:
> > > > > > > On 2013/06/24 16:13:41, sky wrote:
> > > > > > > > This still feels like the wrong thing to me, but I don't have
any
> > > other
> > > > > > ideas.
> > > > > > > > Maybe Varun or Girard does.
> > > > > > > 
> > > > > > > 
> > > > > > > This "fix" would also send fake mouse messages through webkit and
> the
> > > > > > javascript
> > > > > > > client, and we don't want that to happen. So we'll need to do
> > something
> > > a
> > > > > > little
> > > > > > > better.
> > > > > > 
> > > > > > Good catch. Will update it later.
> > > > > 
> > > > > After re-thinking the problem you raised, I think the webkit and JS
> won't
> > > get
> > > > > the fake mouse message since the SendMouseEventForDnD gets called
inisde
> > > > > WebContentsDragWin::DoDragging, followed by DoDragDrop and then run a
> > > separate
> > > > > event loop to consume this event, as a result, the RWHV doesn't have a
> > > chance
> > > > to
> > > > > forward the fake event at all.
> > > > 
> > > > Sorry - my comment about fake mouse messages was referring to my own
> patch,
> > > not
> > > > yours. Yours won't cause any issues with the content window. Mine was
> > allowing
> > > > events (that had previously been blocked/dropped) to be sent to the
> content
> > > > window.
> > > 
> > > FYI, I have been told that IE11 preview
> > >
(http://msdn.microsoft.com/en-us/library/ie/dn265022%252528v=vs.85%252529.aspx)
> > supports
> > > touch drag drop initiated through long press. It might be worthwhile to
use
> > > spy++ to check how they got over this issue.
> > 
> > IE fires pointer events through to the browser. They don't suppress mouse
> events
> > like we do. That means they can call DoDragDrop, and DoDragDrop will still
> > receive WM_MOUSEMOVE, WM_LBUTTONUP, etc.
> 
> varunjain@, then what is the conclusion for this patch?

Hi Hongbo, apologies for the delayed response. I have been meaning to give this
more attention the last few days but always keep getting distracted by other
things. I wanted to wait before replying till I have had a chance to test stuff
on a win machine myself.
I did get a chance to play around with touch dnd on win 7 a little bit last
week. Although it seems flaky, it does work. So I am less convinced that this is
the right fix. I will allot some time tomorrow to play with it a little more to
see when/why exactly it does not work. I should have a better response for you
after that.

[Hongbo Min, 2013-07-03 04:56:32]

On 2013/07/03 04:52:46, varunjain wrote:
> On 2013/07/03 01:58:00, Hongbo Min wrote:
> > On 2013/06/27 19:06:43, girard wrote:
> > > On 2013/06/27 18:58:01, varunjain wrote:
> > > > On 2013/06/27 16:30:11, girard wrote:
> > > > > On 2013/06/27 15:04:29, Hongbo Min wrote:
> > > > > > On 2013/06/27 14:49:28, Hongbo Min wrote:
> > > > > > > On 2013/06/27 07:47:12, girard wrote:
> > > > > > > > On 2013/06/24 16:13:41, sky wrote:
> > > > > > > > > This still feels like the wrong thing to me, but I don't have
> any
> > > > other
> > > > > > > ideas.
> > > > > > > > > Maybe Varun or Girard does.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > This "fix" would also send fake mouse messages through webkit
and
> > the
> > > > > > > javascript
> > > > > > > > client, and we don't want that to happen. So we'll need to do
> > > something
> > > > a
> > > > > > > little
> > > > > > > > better.
> > > > > > > 
> > > > > > > Good catch. Will update it later.
> > > > > > 
> > > > > > After re-thinking the problem you raised, I think the webkit and JS
> > won't
> > > > get
> > > > > > the fake mouse message since the SendMouseEventForDnD gets called
> inisde
> > > > > > WebContentsDragWin::DoDragging, followed by DoDragDrop and then run
a
> > > > separate
> > > > > > event loop to consume this event, as a result, the RWHV doesn't have
a
> > > > chance
> > > > > to
> > > > > > forward the fake event at all.
> > > > > 
> > > > > Sorry - my comment about fake mouse messages was referring to my own
> > patch,
> > > > not
> > > > > yours. Yours won't cause any issues with the content window. Mine was
> > > allowing
> > > > > events (that had previously been blocked/dropped) to be sent to the
> > content
> > > > > window.
> > > > 
> > > > FYI, I have been told that IE11 preview
> > > >
>
(http://msdn.microsoft.com/en-us/library/ie/dn265022%25252528v=vs.85%25252529....)
> > > supports
> > > > touch drag drop initiated through long press. It might be worthwhile to
> use
> > > > spy++ to check how they got over this issue.
> > > 
> > > IE fires pointer events through to the browser. They don't suppress mouse
> > events
> > > like we do. That means they can call DoDragDrop, and DoDragDrop will still
> > > receive WM_MOUSEMOVE, WM_LBUTTONUP, etc.
> > 
> > varunjain@, then what is the conclusion for this patch?
> 
> Hi Hongbo, apologies for the delayed response. I have been meaning to give
this
> more attention the last few days but always keep getting distracted by other
> things. I wanted to wait before replying till I have had a chance to test
stuff
> on a win machine myself.
> I did get a chance to play around with touch dnd on win 7 a little bit last
> week. Although it seems flaky, it does work. So I am less convinced that this
is
> the right fix. I will allot some time tomorrow to play with it a little more
to
> see when/why exactly it does not work. I should have a better response for you
> after that.

That's fine. Thanks for your update.

[varunjain, 2013-07-04 05:31:56]

On 2013/07/03 04:56:32, Hongbo Min wrote:
> On 2013/07/03 04:52:46, varunjain wrote:
> > On 2013/07/03 01:58:00, Hongbo Min wrote:
> > > On 2013/06/27 19:06:43, girard wrote:
> > > > On 2013/06/27 18:58:01, varunjain wrote:
> > > > > On 2013/06/27 16:30:11, girard wrote:
> > > > > > On 2013/06/27 15:04:29, Hongbo Min wrote:
> > > > > > > On 2013/06/27 14:49:28, Hongbo Min wrote:
> > > > > > > > On 2013/06/27 07:47:12, girard wrote:
> > > > > > > > > On 2013/06/24 16:13:41, sky wrote:
> > > > > > > > > > This still feels like the wrong thing to me, but I don't
have
> > any
> > > > > other
> > > > > > > > ideas.
> > > > > > > > > > Maybe Varun or Girard does.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This "fix" would also send fake mouse messages through webkit
> and
> > > the
> > > > > > > > javascript
> > > > > > > > > client, and we don't want that to happen. So we'll need to do
> > > > something
> > > > > a
> > > > > > > > little
> > > > > > > > > better.
> > > > > > > > 
> > > > > > > > Good catch. Will update it later.
> > > > > > > 
> > > > > > > After re-thinking the problem you raised, I think the webkit and
JS
> > > won't
> > > > > get
> > > > > > > the fake mouse message since the SendMouseEventForDnD gets called
> > inisde
> > > > > > > WebContentsDragWin::DoDragging, followed by DoDragDrop and then
run
> a
> > > > > separate
> > > > > > > event loop to consume this event, as a result, the RWHV doesn't
have
> a
> > > > > chance
> > > > > > to
> > > > > > > forward the fake event at all.
> > > > > > 
> > > > > > Sorry - my comment about fake mouse messages was referring to my own
> > > patch,
> > > > > not
> > > > > > yours. Yours won't cause any issues with the content window. Mine
was
> > > > allowing
> > > > > > events (that had previously been blocked/dropped) to be sent to the
> > > content
> > > > > > window.
> > > > > 
> > > > > FYI, I have been told that IE11 preview
> > > > >
> >
>
(http://msdn.microsoft.com/en-us/library/ie/dn265022%2525252528v=vs.85%2525252...)
> > > > supports
> > > > > touch drag drop initiated through long press. It might be worthwhile
to
> > use
> > > > > spy++ to check how they got over this issue.
> > > > 
> > > > IE fires pointer events through to the browser. They don't suppress
mouse
> > > events
> > > > like we do. That means they can call DoDragDrop, and DoDragDrop will
still
> > > > receive WM_MOUSEMOVE, WM_LBUTTONUP, etc.
> > > 
> > > varunjain@, then what is the conclusion for this patch?
> > 
> > Hi Hongbo, apologies for the delayed response. I have been meaning to give
> this
> > more attention the last few days but always keep getting distracted by other
> > things. I wanted to wait before replying till I have had a chance to test
> stuff
> > on a win machine myself.
> > I did get a chance to play around with touch dnd on win 7 a little bit last
> > week. Although it seems flaky, it does work. So I am less convinced that
this
> is
> > the right fix. I will allot some time tomorrow to play with it a little more
> to
> > see when/why exactly it does not work. I should have a better response for
you
> > after that.
> 
> That's fine. Thanks for your update.

Hi, I got a chance to look into this a bit today. After discussing with girard@
I think we are convinced that simulated mouse events are necessary for DnD to
work. So you are in the right direction, but your fix is not quite the right
way. I would like to go back to girard@'s suggestion of letting simulated mouse
events flow through in RenderWidgetHostViewWin::ForwardMouseEventToRenderer().
Obviously we do not want to send these events to the renderer. But we do want to
do other stuff that is currently not being done. I think the code should look
something like this (starting from line 2955):

  if (!touch_events_enabled_ || !ui::IsMouseEventFromTouch(message)) {
    // Send the event to the renderer before changing mouse capture, so that
    // the capturelost event arrives after mouseup.
    render_widget_host_->ForwardMouseEvent(event);
  }

  if (touch_dnd_in_progress_ || !touch_events_enabled_ ||
!ui::IsMouseEventFromTouch(message)) {  
    switch (event.type) {
      case WebInputEvent::MouseMove:
        TrackMouseLeave(true);
        break;
      case WebInputEvent::MouseLeave:
        TrackMouseLeave(false);
        break;
      case WebInputEvent::MouseDown:
        SetCapture();
        break;
      case WebInputEvent::MouseUp:
        if (GetCapture() == m_hWnd)
          ReleaseCapture();
        break;
    }
  }

This makes touch dnd work. Basically if touch dnd is in progress, we let
simulated mouse events in (without sending them to the renderer). girard@
suggested that we activate this part of the code (the switch statement block)
for simulated mouse events regardless of whether touch dnd is in progress or
not. I agree that this would keep things clean, but am hesitant in turning it on
generally since I am not familiar with what it does. But I think this is a small
detail that the reviewer(s) should decide on.

As far as I can tell, this seems like the least hacky way to get this working.
In fact I think it is not even hacky because there is no reason we should not
handle simulated mouse event (we just dont want to send it to the renderer and
confuse js). @sky: what do you think about this way.

[Hongbo Min, 2013-07-04 09:03:24]

On 2013/07/04 05:31:56, varunjain wrote:
> On 2013/07/03 04:56:32, Hongbo Min wrote:
> > On 2013/07/03 04:52:46, varunjain wrote:
> > > On 2013/07/03 01:58:00, Hongbo Min wrote:
> > > > On 2013/06/27 19:06:43, girard wrote:
> > > > > On 2013/06/27 18:58:01, varunjain wrote:
> > > > > > On 2013/06/27 16:30:11, girard wrote:
> > > > > > > On 2013/06/27 15:04:29, Hongbo Min wrote:
> > > > > > > > On 2013/06/27 14:49:28, Hongbo Min wrote:
> > > > > > > > > On 2013/06/27 07:47:12, girard wrote:
> > > > > > > > > > On 2013/06/24 16:13:41, sky wrote:
> > > > > > > > > > > This still feels like the wrong thing to me, but I don't
> have
> > > any
> > > > > > other
> > > > > > > > > ideas.
> > > > > > > > > > > Maybe Varun or Girard does.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > This "fix" would also send fake mouse messages through
webkit
> > and
> > > > the
> > > > > > > > > javascript
> > > > > > > > > > client, and we don't want that to happen. So we'll need to
do
> > > > > something
> > > > > > a
> > > > > > > > > little
> > > > > > > > > > better.
> > > > > > > > > 
> > > > > > > > > Good catch. Will update it later.
> > > > > > > > 
> > > > > > > > After re-thinking the problem you raised, I think the webkit and
> JS
> > > > won't
> > > > > > get
> > > > > > > > the fake mouse message since the SendMouseEventForDnD gets
called
> > > inisde
> > > > > > > > WebContentsDragWin::DoDragging, followed by DoDragDrop and then
> run
> > a
> > > > > > separate
> > > > > > > > event loop to consume this event, as a result, the RWHV doesn't
> have
> > a
> > > > > > chance
> > > > > > > to
> > > > > > > > forward the fake event at all.
> > > > > > > 
> > > > > > > Sorry - my comment about fake mouse messages was referring to my
own
> > > > patch,
> > > > > > not
> > > > > > > yours. Yours won't cause any issues with the content window. Mine
> was
> > > > > allowing
> > > > > > > events (that had previously been blocked/dropped) to be sent to
the
> > > > content
> > > > > > > window.
> > > > > > 
> > > > > > FYI, I have been told that IE11 preview
> > > > > >
> > >
> >
>
(http://msdn.microsoft.com/en-us/library/ie/dn265022%252525252528v=vs.85%25252...)
> > > > > supports
> > > > > > touch drag drop initiated through long press. It might be worthwhile
> to
> > > use
> > > > > > spy++ to check how they got over this issue.
> > > > > 
> > > > > IE fires pointer events through to the browser. They don't suppress
> mouse
> > > > events
> > > > > like we do. That means they can call DoDragDrop, and DoDragDrop will
> still
> > > > > receive WM_MOUSEMOVE, WM_LBUTTONUP, etc.
> > > > 
> > > > varunjain@, then what is the conclusion for this patch?
> > > 
> > > Hi Hongbo, apologies for the delayed response. I have been meaning to give
> > this
> > > more attention the last few days but always keep getting distracted by
other
> > > things. I wanted to wait before replying till I have had a chance to test
> > stuff
> > > on a win machine myself.
> > > I did get a chance to play around with touch dnd on win 7 a little bit
last
> > > week. Although it seems flaky, it does work. So I am less convinced that
> this
> > is
> > > the right fix. I will allot some time tomorrow to play with it a little
more
> > to
> > > see when/why exactly it does not work. I should have a better response for
> you
> > > after that.
> > 
> > That's fine. Thanks for your update.
> 
> Hi, I got a chance to look into this a bit today. After discussing with
girard@
> I think we are convinced that simulated mouse events are necessary for DnD to
> work. So you are in the right direction, but your fix is not quite the right
> way. I would like to go back to girard@'s suggestion of letting simulated
mouse
> events flow through in RenderWidgetHostViewWin::ForwardMouseEventToRenderer().
> Obviously we do not want to send these events to the renderer. But we do want
to
> do other stuff that is currently not being done. I think the code should look
> something like this (starting from line 2955):
> 
>   if (!touch_events_enabled_ || !ui::IsMouseEventFromTouch(message)) {
>     // Send the event to the renderer before changing mouse capture, so that
>     // the capturelost event arrives after mouseup.
>     render_widget_host_->ForwardMouseEvent(event);
>   }
> 
>   if (touch_dnd_in_progress_ || !touch_events_enabled_ ||
> !ui::IsMouseEventFromTouch(message)) {  
>     switch (event.type) {
>       case WebInputEvent::MouseMove:
>         TrackMouseLeave(true);
>         break;
>       case WebInputEvent::MouseLeave:
>         TrackMouseLeave(false);
>         break;
>       case WebInputEvent::MouseDown:
>         SetCapture();
>         break;
>       case WebInputEvent::MouseUp:
>         if (GetCapture() == m_hWnd)
>           ReleaseCapture();
>         break;
>     }
>   }
> 
> This makes touch dnd work. Basically if touch dnd is in progress, we let
> simulated mouse events in (without sending them to the renderer). girard@
> suggested that we activate this part of the code (the switch statement block)
> for simulated mouse events regardless of whether touch dnd is in progress or
> not. I agree that this would keep things clean, but am hesitant in turning it
on
> generally since I am not familiar with what it does. But I think this is a
small
> detail that the reviewer(s) should decide on.

I am quitely a bit confused now. If the touch DnD is in progress, it means that
DoDragDrop was already called, and the event loop is controlled by the blackbox
of DoDragDrop, then the RWHV window doesn't have a chance to receive such a
simulated mouse event, right?