Suppress ET_MOUSE_MOVE when the mouse hasn't moved on Windows.

Suppress ET_MOUSE_MOVE when the mouse hasn't moved on Windows.

There are a series of bugs around getting a mousemove after a click event.
Windows seems to post these events to the system for all browsers but it
appears that FireFox and Edge both disgard mouse move events in these
cases. This introduces a side effect of when a mouse leaves a window via
Alt-Tab and is re-entered we don't generate the mouseenter right away but
that exact same bug occurs on Edge and FireFox as well.

BUG=161464, 390326
Review-Url: https://codereview.chromium.org/2637403012
Cr-Commit-Position: refs/heads/master@{#446810}
Committed: https://chromium.googlesource.com/chromium/src/+/a05a1702da44842fed11d76a18a1452c06421473

[dtapuska, 2017-01-20 15:17:08]

dtapuska@chromium.org changed reviewers:
+ ananta@chromium.org

[dtapuska, 2017-01-20 15:17:08]

dtapuska@chromium.org changed reviewers:
+ ananta@chromium.org

[dtapuska, 2017-01-20 15:17:08]

ananta@ WDYT?

[dtapuska, 2017-01-20 15:17:09]

ananta@ WDYT?

[dtapuska, 2017-01-20 15:19:38]

On 2017/01/20 15:17:09, dtapuska wrote:
> ananta@ WDYT?

FWIW; here is Mozilla's similar code
https://hg.mozilla.org/mozilla-central/file/1196bf3032e1/widget/windows/nsWin...

[ananta, 2017-01-20 22:12:50]

Windows posts the WM_MOUSEMOVE message to allow applications a chance to set the
cursor. If this bug truly needs fixing, then a better option might be to do
something like in the RenderWidgetHostViewAura class?

[dtapuska, 2017-01-20 22:17:58]

On 2017/01/20 22:12:50, ananta wrote:
> Windows posts the WM_MOUSEMOVE message to allow applications a chance to set
the
> cursor. If this bug truly needs fixing, then a better option might be to do
> something like in the RenderWidgetHostViewAura class?

That is fine if you want to do it that way. Although the only OS that does this
is Windows.

But I can add a filter in the host_view_aura...

Do you suggest a pre target event filter like:
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

[ananta, 2017-01-20 22:45:05]

On 2017/01/20 22:17:58, dtapuska wrote:
> On 2017/01/20 22:12:50, ananta wrote:
> > Windows posts the WM_MOUSEMOVE message to allow applications a chance to set
> the
> > cursor. If this bug truly needs fixing, then a better option might be to do
> > something like in the RenderWidgetHostViewAura class?
> 
> That is fine if you want to do it that way. Although the only OS that does
this
> is Windows.
> 
> But I can add a filter in the host_view_aura...
> 
> Do you suggest a pre target event filter like:
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...

Sounds good

[dtapuska, 2017-01-23 20:42:55]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 20:43:28]

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

[dtapuska, 2017-01-23 20:43:41]

Description was changed from

==========
Suppress WM_MOUSEMOVE when the mouse hasn't moved.

There are a series of bugs around getting a mousemove after a click event.
Windows seems to post these events to the system for all browsers but it
appears that FireFox and Edge both disgard mouse move events in these
cases. This introduces a side effect of when a mouse leaves a window via
Alt-Tab and is re-entered we don't generate the mouseenter right away but
that exact same bug occurs on Edge and FireFox as well.

BUG=161464, 390326
==========

to

==========
Suppress ET_MOUSE_MOVE when the mouse hasn't moved on Windows.

There are a series of bugs around getting a mousemove after a click event.
Windows seems to post these events to the system for all browsers but it
appears that FireFox and Edge both disgard mouse move events in these
cases. This introduces a side effect of when a mouse leaves a window via
Alt-Tab and is re-entered we don't generate the mouseenter right away but
that exact same bug occurs on Edge and FireFox as well.

BUG=161464, 390326
==========

[dtapuska, 2017-01-23 20:43:57]

On 2017/01/20 22:45:05, ananta wrote:
> On 2017/01/20 22:17:58, dtapuska wrote:
> > On 2017/01/20 22:12:50, ananta wrote:
> > > Windows posts the WM_MOUSEMOVE message to allow applications a chance to
set
> > the
> > > cursor. If this bug truly needs fixing, then a better option might be to
do
> > > something like in the RenderWidgetHostViewAura class?
> > 
> > That is fine if you want to do it that way. Although the only OS that does
> this
> > is Windows.
> > 
> > But I can add a filter in the host_view_aura...
> > 
> > Do you suggest a pre target event filter like:
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> 
> Sounds good

anantha@ please take another look.

[commit-bot: I haz the power, 2017-01-23 23:48:31]

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

[commit-bot: I haz the power, 2017-01-23 23:48:32]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-01-24 21:41:24]

On 2017/01/23 20:43:57, dtapuska wrote:
> On 2017/01/20 22:45:05, ananta wrote:
> > On 2017/01/20 22:17:58, dtapuska wrote:
> > > On 2017/01/20 22:12:50, ananta wrote:
> > > > Windows posts the WM_MOUSEMOVE message to allow applications a chance to
> set
> > > the
> > > > cursor. If this bug truly needs fixing, then a better option might be to
> do
> > > > something like in the RenderWidgetHostViewAura class?
> > > 
> > > That is fine if you want to do it that way. Although the only OS that does
> > this
> > > is Windows.
> > > 
> > > But I can add a filter in the host_view_aura...
> > > 
> > > Do you suggest a pre target event filter like:
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> > 
> > Sounds good
> 

ananta@ ping.

[ananta, 2017-01-24 23:58:15]

On 2017/01/24 21:41:24, dtapuska wrote:
> On 2017/01/23 20:43:57, dtapuska wrote:
> > On 2017/01/20 22:45:05, ananta wrote:
> > > On 2017/01/20 22:17:58, dtapuska wrote:
> > > > On 2017/01/20 22:12:50, ananta wrote:
> > > > > Windows posts the WM_MOUSEMOVE message to allow applications a chance
to
> > set
> > > > the
> > > > > cursor. If this bug truly needs fixing, then a better option might be
to
> > do
> > > > > something like in the RenderWidgetHostViewAura class?
> > > > 
> > > > That is fine if you want to do it that way. Although the only OS that
does
> > > this
> > > > is Windows.
> > > > 
> > > > But I can add a filter in the host_view_aura...
> > > > 
> > > > Do you suggest a pre target event filter like:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> > > 
> > > Sounds good
> > 
> 
> ananta@ ping.

The approach seems reasonable. It would be good to write test programs to see
whether we can prevent the mouse messages
from being sent by the OS when the WM_POINTER messages are handled.

Additionally what is the plan for touch?

[dtapuska, 2017-01-26 18:51:50]

On 2017/01/24 23:58:15, ananta wrote:
> On 2017/01/24 21:41:24, dtapuska wrote:
> > On 2017/01/23 20:43:57, dtapuska wrote:
> > > On 2017/01/20 22:45:05, ananta wrote:
> > > > On 2017/01/20 22:17:58, dtapuska wrote:
> > > > > On 2017/01/20 22:12:50, ananta wrote:
> > > > > > Windows posts the WM_MOUSEMOVE message to allow applications a
chance
> to
> > > set
> > > > > the
> > > > > > cursor. If this bug truly needs fixing, then a better option might
be
> to
> > > do
> > > > > > something like in the RenderWidgetHostViewAura class?
> > > > > 
> > > > > That is fine if you want to do it that way. Although the only OS that
> does
> > > > this
> > > > > is Windows.
> > > > > 
> > > > > But I can add a filter in the host_view_aura...
> > > > > 
> > > > > Do you suggest a pre target event filter like:
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> > > > 
> > > > Sounds good
> > > 
> > 
> > ananta@ ping.
> 
> The approach seems reasonable. It would be good to write test programs to see
> whether we can prevent the mouse messages
> from being sent by the OS when the WM_POINTER messages are handled.
> 
> Additionally what is the plan for touch?

We can prevent the mouse messages if we indicate SetMsgHandled(TRUE) if you
EnableMouseInPointer(true). And touch doesn't generate these weird WM_MOUSEMOVE
events so I don't see how it is relevant.

Anyways this change is needed to fix the WM_MOUSEMOVE against Windows 7 are you
ok with this? Should I get an owner to review it?

[ananta, 2017-01-26 19:54:45]

On 2017/01/26 18:51:50, dtapuska wrote:
> On 2017/01/24 23:58:15, ananta wrote:
> > On 2017/01/24 21:41:24, dtapuska wrote:
> > > On 2017/01/23 20:43:57, dtapuska wrote:
> > > > On 2017/01/20 22:45:05, ananta wrote:
> > > > > On 2017/01/20 22:17:58, dtapuska wrote:
> > > > > > On 2017/01/20 22:12:50, ananta wrote:
> > > > > > > Windows posts the WM_MOUSEMOVE message to allow applications a
> chance
> > to
> > > > set
> > > > > > the
> > > > > > > cursor. If this bug truly needs fixing, then a better option might
> be
> > to
> > > > do
> > > > > > > something like in the RenderWidgetHostViewAura class?
> > > > > > 
> > > > > > That is fine if you want to do it that way. Although the only OS
that
> > does
> > > > > this
> > > > > > is Windows.
> > > > > > 
> > > > > > But I can add a filter in the host_view_aura...
> > > > > > 
> > > > > > Do you suggest a pre target event filter like:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid...
> > > > > 
> > > > > Sounds good
> > > > 
> > > 
> > > ananta@ ping.
> > 
> > The approach seems reasonable. It would be good to write test programs to
see
> > whether we can prevent the mouse messages
> > from being sent by the OS when the WM_POINTER messages are handled.
> > 
> > Additionally what is the plan for touch?
> 
> We can prevent the mouse messages if we indicate SetMsgHandled(TRUE) if you
> EnableMouseInPointer(true). And touch doesn't generate these weird
WM_MOUSEMOVE
> events so I don't see how it is relevant.
> 
> Anyways this change is needed to fix the WM_MOUSEMOVE against Windows 7 are
you
> ok with this? Should I get an owner to review it?

Ignore the touch comment. I mixed up this and lanwei's code review :(

[dtapuska, 2017-01-26 20:59:09]

dtapuska@chromium.org changed reviewers:
+ sadrul@chromium.org

[dtapuska, 2017-01-26 20:59:09]

sadrul@ PTAL

[sadrul, 2017-01-27 00:22:03]

https://codereview.chromium.org/2637403012/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2637403012/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:225:
aura::Env::GetInstance()->AddPreTargetHandler(this);
This should install the event-handler on the RWHVA's |window_|.

... although, why not remember the last location in RWHVA::OnMouseEvent without
installing any pre-target handlers at all?

[dtapuska, 2017-01-27 16:50:53]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 16:51:16]

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

[dtapuska, 2017-01-27 16:52:12]

On 2017/01/27 00:22:03, sadrul wrote:
>
https://codereview.chromium.org/2637403012/diff/20001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/2637403012/diff/20001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_view_aura.cc:225:
> aura::Env::GetInstance()->AddPreTargetHandler(this);
> This should install the event-handler on the RWHVA's |window_|.
> 
> ... although, why not remember the last location in RWHVA::OnMouseEvent
without
> installing any pre-target handlers at all?

How is this? I initially started this change in the HWND handler and then
ananta@ asked to move it to RWHVA and I asked if the PreTarget approach was
desired or not..

[commit-bot: I haz the power, 2017-01-27 18:30:08]

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

[commit-bot: I haz the power, 2017-01-27 18:30:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sadrul, 2017-01-27 21:15:06]

On 2017/01/27 16:52:12, dtapuska wrote:
> On 2017/01/27 00:22:03, sadrul wrote:
> >
>
https://codereview.chromium.org/2637403012/diff/20001/content/browser/rendere...
> > File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> > 
> >
>
https://codereview.chromium.org/2637403012/diff/20001/content/browser/rendere...
> > content/browser/renderer_host/render_widget_host_view_aura.cc:225:
> > aura::Env::GetInstance()->AddPreTargetHandler(this);
> > This should install the event-handler on the RWHVA's |window_|.
> > 
> > ... although, why not remember the last location in RWHVA::OnMouseEvent
> without
> > installing any pre-target handlers at all?
> 
> How is this? I initially started this change in the HWND handler and then
> ananta@ asked to move it to RWHVA and I asked if the PreTarget approach was
> desired or not..

This looks much more reasonable, yep. Thanks!

The try failures look relevant, btw. lgtm with those fixed.

[dtapuska, 2017-01-27 21:38:23]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2017-01-27 21:38:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2637403012/#ps60001
(title: "Fix test")

[commit-bot: I haz the power, 2017-01-27 21:39:17]

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, 2017-01-27 22:57:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485553103624740,
"parent_rev": "c4ac85eec321202ca4ecd56f0364dd49d9fb8236", "commit_rev":
"a05a1702da44842fed11d76a18a1452c06421473"}

[commit-bot: I haz the power, 2017-01-27 22:57:57]

Description was changed from

==========
Suppress ET_MOUSE_MOVE when the mouse hasn't moved on Windows.

There are a series of bugs around getting a mousemove after a click event.
Windows seems to post these events to the system for all browsers but it
appears that FireFox and Edge both disgard mouse move events in these
cases. This introduces a side effect of when a mouse leaves a window via
Alt-Tab and is re-entered we don't generate the mouseenter right away but
that exact same bug occurs on Edge and FireFox as well.

BUG=161464, 390326
==========

to

==========
Suppress ET_MOUSE_MOVE when the mouse hasn't moved on Windows.

There are a series of bugs around getting a mousemove after a click event.
Windows seems to post these events to the system for all browsers but it
appears that FireFox and Edge both disgard mouse move events in these
cases. This introduces a side effect of when a mouse leaves a window via
Alt-Tab and is re-entered we don't generate the mouseenter right away but
that exact same bug occurs on Edge and FireFox as well.

BUG=161464, 390326

Review-Url: https://codereview.chromium.org/2637403012
Cr-Commit-Position: refs/heads/master@{#446810}
Committed:
https://chromium.googlesource.com/chromium/src/+/a05a1702da44842fed11d76a18a1...
==========

[commit-bot: I haz the power, 2017-01-27 22:57:59]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a05a1702da44842fed11d76a18a1...