Reset drag state variables on mouse up

Reset drag state variables on mouse up

We need to reset the drag related variables
on regerdless of whether the mouse up was
preventDefaulted or not. Because after mouse up
there is no drag until the next mouse down.

BUG=308989
Committed: https://crrev.com/b28500a5befe6dd43b3ada37bee3a13d67b9f733
Cr-Commit-Position: refs/heads/master@{#403694}

[Navid Zolghadr, 2016-06-09 20:53:50]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org, mustaq@chromium.org

[dtapuska, 2016-06-09 21:18:58]

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
clearMouseDragState();
what about the code path for gesture tap that has a similar flow?

Also there are a few places that assign m_mousePressed = false; do we need to
every clear the drag state in those?

I'm wondering if we took the code path out of handleMouseReleaseEvent and always
did the clearMouseDragState whether that would be more logical?

Or should we put an AutoReset like object on the stack of the
handleMouseReleaseEvent(...) so clearMouseDragState is always cleared at the end
of its return?

[mustaq, 2016-06-10 15:44:04]

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
clearMouseDragState();
On 2016/06/09 21:18:58, dtapuska wrote:
> what about the code path for gesture tap that has a similar flow?
> 
> Also there are a few places that assign m_mousePressed = false; do we need to
> every clear the drag state in those?
> 
> I'm wondering if we took the code path out of handleMouseReleaseEvent and
always
> did the clearMouseDragState whether that would be more logical?
> 
> Or should we put an AutoReset like object on the stack of the
> handleMouseReleaseEvent(...) so clearMouseDragState is always cleared at the
end
> of its return?
> 
> 
> 

handleMousePressEvent(MouseEventWithHitTestResults) is called from this method
and handleGestureTap. Clearing the state unconditionally at both these callers
should work (so we won't need it in
handleMousePressEvent(MouseEventWithHitTestResults).

[Navid Zolghadr, 2016-06-10 19:19:55]

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
clearMouseDragState();
On 2016/06/10 15:44:04, mustaq wrote:
> On 2016/06/09 21:18:58, dtapuska wrote:
> > what about the code path for gesture tap that has a similar flow?
> > 
> > Also there are a few places that assign m_mousePressed = false; do we need
to
> > every clear the drag state in those?
> > 
> > I'm wondering if we took the code path out of handleMouseReleaseEvent and
> always
> > did the clearMouseDragState whether that would be more logical?
> > 
> > Or should we put an AutoReset like object on the stack of the
> > handleMouseReleaseEvent(...) so clearMouseDragState is always cleared at the
> end
> > of its return?
> > 
> > 
> > 
> 
> handleMousePressEvent(MouseEventWithHitTestResults) is called from this method
> and handleGestureTap. Clearing the state unconditionally at both these callers
> should work (so we won't need it in
> handleMousePressEvent(MouseEventWithHitTestResults).


I added the call as suggested to the tap flow as well.

To be honest with you I don't know about all those state variable. I need to
look deeper which was in my plan when I start refactoring the mouse code out of
EventHandler. So I don't have a good answer right now what each of these
variables mean.

Is calling the clean right from the function okay or do you like those object
cleanup methods?

[mustaq, 2016-06-10 19:30:17]

lgtm

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
clearMouseDragState();
On 2016/06/10 19:19:55, Navid Zolghadr wrote:
> On 2016/06/10 15:44:04, mustaq wrote:
> > On 2016/06/09 21:18:58, dtapuska wrote:
> > > what about the code path for gesture tap that has a similar flow?
> > > 
> > > Also there are a few places that assign m_mousePressed = false; do we need
> to
> > > every clear the drag state in those?
> > > 
> > > I'm wondering if we took the code path out of handleMouseReleaseEvent and
> > always
> > > did the clearMouseDragState whether that would be more logical?
> > > 
> > > Or should we put an AutoReset like object on the stack of the
> > > handleMouseReleaseEvent(...) so clearMouseDragState is always cleared at
the
> > end
> > > of its return?
> > > 
> > > 
> > > 
> > 
> > handleMousePressEvent(MouseEventWithHitTestResults) is called from this
method
> > and handleGestureTap. Clearing the state unconditionally at both these
callers
> > should work (so we won't need it in
> > handleMousePressEvent(MouseEventWithHitTestResults).
> 
> 
> I added the call as suggested to the tap flow as well.
> 
> To be honest with you I don't know about all those state variable. I need to
> look deeper which was in my plan when I start refactoring the mouse code out
of
> EventHandler. So I don't have a good answer right now what each of these
> variables mean.
> 
> Is calling the clean right from the function okay or do you like those object
> cleanup methods?

I am indifferent about AutoReset here, mainly for slightly better readability
w/o it. Dave's call.

[dtapuska, 2016-06-10 19:31:58]

On 2016/06/10 19:30:17, mustaq wrote:
> lgtm
> 
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
> clearMouseDragState();
> On 2016/06/10 19:19:55, Navid Zolghadr wrote:
> > On 2016/06/10 15:44:04, mustaq wrote:
> > > On 2016/06/09 21:18:58, dtapuska wrote:
> > > > what about the code path for gesture tap that has a similar flow?
> > > > 
> > > > Also there are a few places that assign m_mousePressed = false; do we
need
> > to
> > > > every clear the drag state in those?
> > > > 
> > > > I'm wondering if we took the code path out of handleMouseReleaseEvent
and
> > > always
> > > > did the clearMouseDragState whether that would be more logical?
> > > > 
> > > > Or should we put an AutoReset like object on the stack of the
> > > > handleMouseReleaseEvent(...) so clearMouseDragState is always cleared at
> the
> > > end
> > > > of its return?
> > > > 
> > > > 
> > > > 
> > > 
> > > handleMousePressEvent(MouseEventWithHitTestResults) is called from this
> method
> > > and handleGestureTap. Clearing the state unconditionally at both these
> callers
> > > should work (so we won't need it in
> > > handleMousePressEvent(MouseEventWithHitTestResults).
> > 
> > 
> > I added the call as suggested to the tap flow as well.
> > 
> > To be honest with you I don't know about all those state variable. I need to
> > look deeper which was in my plan when I start refactoring the mouse code out
> of
> > EventHandler. So I don't have a good answer right now what each of these
> > variables mean.
> > 
> > Is calling the clean right from the function okay or do you like those
object
> > cleanup methods?
> 
> I am indifferent about AutoReset here, mainly for slightly better readability
> w/o it. Dave's call.

What about the early returns; do we really want to reset the drag state if it
was actually set somehow?

[Navid Zolghadr, 2016-06-10 20:02:42]

On 2016/06/10 19:31:58, dtapuska wrote:
> On 2016/06/10 19:30:17, mustaq wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
> > clearMouseDragState();
> > On 2016/06/10 19:19:55, Navid Zolghadr wrote:
> > > On 2016/06/10 15:44:04, mustaq wrote:
> > > > On 2016/06/09 21:18:58, dtapuska wrote:
> > > > > what about the code path for gesture tap that has a similar flow?
> > > > > 
> > > > > Also there are a few places that assign m_mousePressed = false; do we
> need
> > > to
> > > > > every clear the drag state in those?
> > > > > 
> > > > > I'm wondering if we took the code path out of handleMouseReleaseEvent
> and
> > > > always
> > > > > did the clearMouseDragState whether that would be more logical?
> > > > > 
> > > > > Or should we put an AutoReset like object on the stack of the
> > > > > handleMouseReleaseEvent(...) so clearMouseDragState is always cleared
at
> > the
> > > > end
> > > > > of its return?
> > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > handleMousePressEvent(MouseEventWithHitTestResults) is called from this
> > method
> > > > and handleGestureTap. Clearing the state unconditionally at both these
> > callers
> > > > should work (so we won't need it in
> > > > handleMousePressEvent(MouseEventWithHitTestResults).
> > > 
> > > 
> > > I added the call as suggested to the tap flow as well.
> > > 
> > > To be honest with you I don't know about all those state variable. I need
to
> > > look deeper which was in my plan when I start refactoring the mouse code
out
> > of
> > > EventHandler. So I don't have a good answer right now what each of these
> > > variables mean.
> > > 
> > > Is calling the clean right from the function okay or do you like those
> object
> > > cleanup methods?
> > 
> > I am indifferent about AutoReset here, mainly for slightly better
readability
> > w/o it. Dave's call.
> 
> What about the early returns; do we really want to reset the drag state if it
> was actually set somehow?

That is true. But I'm not sure what those cases are for and whether those
returns can reset the drag state or not. I need to put some time to understand
all the mouse code here and there. I was just trying to fix the reported issue
without breaking other tests. Actually I put this reset in a few other places
and that broke some of the hit testing tests :D.

[Navid Zolghadr, 2016-06-29 15:27:43]

On 2016/06/10 20:02:42, Navid Zolghadr(OOO til Jun 29) wrote:
> On 2016/06/10 19:31:58, dtapuska wrote:
> > On 2016/06/10 19:30:17, mustaq wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
> > > clearMouseDragState();
> > > On 2016/06/10 19:19:55, Navid Zolghadr wrote:
> > > > On 2016/06/10 15:44:04, mustaq wrote:
> > > > > On 2016/06/09 21:18:58, dtapuska wrote:
> > > > > > what about the code path for gesture tap that has a similar flow?
> > > > > > 
> > > > > > Also there are a few places that assign m_mousePressed = false; do
we
> > need
> > > > to
> > > > > > every clear the drag state in those?
> > > > > > 
> > > > > > I'm wondering if we took the code path out of
handleMouseReleaseEvent
> > and
> > > > > always
> > > > > > did the clearMouseDragState whether that would be more logical?
> > > > > > 
> > > > > > Or should we put an AutoReset like object on the stack of the
> > > > > > handleMouseReleaseEvent(...) so clearMouseDragState is always
cleared
> at
> > > the
> > > > > end
> > > > > > of its return?
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > 
> > > > > handleMousePressEvent(MouseEventWithHitTestResults) is called from
this
> > > method
> > > > > and handleGestureTap. Clearing the state unconditionally at both these
> > > callers
> > > > > should work (so we won't need it in
> > > > > handleMousePressEvent(MouseEventWithHitTestResults).
> > > > 
> > > > 
> > > > I added the call as suggested to the tap flow as well.
> > > > 
> > > > To be honest with you I don't know about all those state variable. I
need
> to
> > > > look deeper which was in my plan when I start refactoring the mouse code
> out
> > > of
> > > > EventHandler. So I don't have a good answer right now what each of these
> > > > variables mean.
> > > > 
> > > > Is calling the clean right from the function okay or do you like those
> > object
> > > > cleanup methods?
> > > 
> > > I am indifferent about AutoReset here, mainly for slightly better
> readability
> > > w/o it. Dave's call.
> > 
> > What about the early returns; do we really want to reset the drag state if
it
> > was actually set somehow?
> 
> That is true. But I'm not sure what those cases are for and whether those
> returns can reset the drag state or not. I need to put some time to understand
> all the mouse code here and there. I was just trying to fix the reported issue
> without breaking other tests. Actually I put this reset in a few other places
> and that broke some of the hit testing tests :D.

Do you think this CL as is good enough to fix the issue or do you want me put
clean up this part of the code as part of this CL?

[dtapuska, 2016-06-29 15:40:13]

On 2016/06/29 15:27:43, Navid Zolghadr wrote:
> On 2016/06/10 20:02:42, Navid Zolghadr(OOO til Jun 29) wrote:
> > On 2016/06/10 19:31:58, dtapuska wrote:
> > > On 2016/06/10 19:30:17, mustaq wrote:
> > > > lgtm
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2042333006/diff/20001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/input/EventHandler.cpp:1205:
> > > > clearMouseDragState();
> > > > On 2016/06/10 19:19:55, Navid Zolghadr wrote:
> > > > > On 2016/06/10 15:44:04, mustaq wrote:
> > > > > > On 2016/06/09 21:18:58, dtapuska wrote:
> > > > > > > what about the code path for gesture tap that has a similar flow?
> > > > > > > 
> > > > > > > Also there are a few places that assign m_mousePressed = false; do
> we
> > > need
> > > > > to
> > > > > > > every clear the drag state in those?
> > > > > > > 
> > > > > > > I'm wondering if we took the code path out of
> handleMouseReleaseEvent
> > > and
> > > > > > always
> > > > > > > did the clearMouseDragState whether that would be more logical?
> > > > > > > 
> > > > > > > Or should we put an AutoReset like object on the stack of the
> > > > > > > handleMouseReleaseEvent(...) so clearMouseDragState is always
> cleared
> > at
> > > > the
> > > > > > end
> > > > > > > of its return?
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > handleMousePressEvent(MouseEventWithHitTestResults) is called from
> this
> > > > method
> > > > > > and handleGestureTap. Clearing the state unconditionally at both
these
> > > > callers
> > > > > > should work (so we won't need it in
> > > > > > handleMousePressEvent(MouseEventWithHitTestResults).
> > > > > 
> > > > > 
> > > > > I added the call as suggested to the tap flow as well.
> > > > > 
> > > > > To be honest with you I don't know about all those state variable. I
> need
> > to
> > > > > look deeper which was in my plan when I start refactoring the mouse
code
> > out
> > > > of
> > > > > EventHandler. So I don't have a good answer right now what each of
these
> > > > > variables mean.
> > > > > 
> > > > > Is calling the clean right from the function okay or do you like those
> > > object
> > > > > cleanup methods?
> > > > 
> > > > I am indifferent about AutoReset here, mainly for slightly better
> > readability
> > > > w/o it. Dave's call.
> > > 
> > > What about the early returns; do we really want to reset the drag state if
> it
> > > was actually set somehow?
> > 
> > That is true. But I'm not sure what those cases are for and whether those
> > returns can reset the drag state or not. I need to put some time to
understand
> > all the mouse code here and there. I was just trying to fix the reported
issue
> > without breaking other tests. Actually I put this reset in a few other
places
> > and that broke some of the hit testing tests :D.
> 
> Do you think this CL as is good enough to fix the issue or do you want me put
> clean up this part of the code as part of this CL?

ya that is fine. But please add comments as to what clearMouseDragState and
clearDragState actually do; as the names of the functions seem so close and we
need to indicate they occur at different times.

[Navid Zolghadr, 2016-06-30 16:13:47]

ptal

[dtapuska, 2016-06-30 16:15:02]

On 2016/06/30 16:13:47, Navid Zolghadr wrote:
> ptal

lgtm

[Navid Zolghadr, 2016-06-30 18:47:26]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-06-30 18:47:27]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/2042333006/#ps60001
(title: "Add comments")

[commit-bot: I haz the power, 2016-06-30 18:48:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 18:58:01]

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

[commit-bot: I haz the power, 2016-06-30 18:58:03]

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

[Navid Zolghadr, 2016-06-30 19:02:30]

nzolghadr@chromium.org changed reviewers:
+ bokan@chromium.org

[Navid Zolghadr, 2016-06-30 19:02:30]

bokan@chromium.org: Please review changes in 

third_party/WebKit/Source/core/input/EventHandler.*

[bokan, 2016-07-01 19:27:29]

Few nits in the test but lgtm otherwise

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html
(right):

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:4:
<body onload="runTest()">
nit: omit body tag, use window.onload or addEventListener instead.

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:25:
<script type="text/javascript">
nit: no need for `type` attribute

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:26:
var test_mousemove = async_test("Inner frame should receive mousemove even when
mouseup preventDefault was called in the parent iframe.");
nit: s/called in the parent iframe/called in the parent frame

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:27:
var state = 0;
nit: state -> mouseUpPrevented or similar and make it true/false

[Navid Zolghadr, 2016-07-04 14:58:11]

done.

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html
(right):

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:4:
<body onload="runTest()">
On 2016/07/01 19:27:29, bokan wrote:
> nit: omit body tag, use window.onload or addEventListener instead.

Done.

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:25:
<script type="text/javascript">
On 2016/07/01 19:27:29, bokan wrote:
> nit: no need for `type` attribute

Done.

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:26:
var test_mousemove = async_test("Inner frame should receive mousemove even when
mouseup preventDefault was called in the parent iframe.");
On 2016/07/01 19:27:29, bokan wrote:
> nit: s/called in the parent iframe/called in the parent frame

Done.

https://codereview.chromium.org/2042333006/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/mouse-up-preventDefault-dragstate.html:27:
var state = 0;
On 2016/07/01 19:27:29, bokan wrote:
> nit: state -> mouseUpPrevented or similar and make it true/false

Done.

[Navid Zolghadr, 2016-07-04 15:10:48]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-04 15:10:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, mustaq@chromium.org, bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2042333006/#ps80001
(title: "Applying the comments")

[commit-bot: I haz the power, 2016-07-04 15:11:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 16:46:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-04 16:46:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-04 16:49:01]

Description was changed from

==========
Reset drag state variables on mouse up

We need to reset the drag related variables
on regerdless of whether the mouse up was
preventDefaulted or not. Because after mouse up
there is no drag until the next mouse down.

BUG=308989
==========

to

==========
Reset drag state variables on mouse up

We need to reset the drag related variables
on regerdless of whether the mouse up was
preventDefaulted or not. Because after mouse up
there is no drag until the next mouse down.

BUG=308989

Committed: https://crrev.com/b28500a5befe6dd43b3ada37bee3a13d67b9f733
Cr-Commit-Position: refs/heads/master@{#403694}
==========

[commit-bot: I haz the power, 2016-07-04 16:49:02]

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