Don't synthesize mousemove events if the mouse cursor is not on the widget

Don't synthesize mousemove events if the mouse cursor is not on the widget

BUG=518056
TEST=browser_tests passes, the issue doesn't happened

Committed: https://crrev.com/8019d579217e1ae4cc0a6c57b24e6a058a94f1ae
Cr-Commit-Position: refs/heads/master@{#346447}

[yoshiki, 2015-08-20 07:42:20]

yoshiki@chromium.org changed reviewers:
+ oshima@chromium.org

[yoshiki, 2015-08-20 07:43:42]

Oshima-san, PTAL. Thanks.

[oshima, 2015-08-20 08:46:34]

oshima@chromium.org changed reviewers:
+ sky@chromium.org

[oshima, 2015-08-20 08:46:34]

+sky

can you add unit test?

https://codereview.chromium.org/1297373004/diff/1/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1297373004/diff/1/ui/views/widget/widget.h#ne...
ui/views/widget/widget.h:931: bool hovered_;
maybe is_mouse_on_widget_ ? I'd defer to sky though.

[yoshiki, 2015-08-20 14:59:03]

Thank you for reviewing. Added a test.

https://codereview.chromium.org/1297373004/diff/1/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1297373004/diff/1/ui/views/widget/widget.h#ne...
ui/views/widget/widget.h:931: bool hovered_;
On 2015/08/20 08:46:34, oshima (In Tokyo 17th - 21st) wrote:
> maybe is_mouse_on_widget_ ? I'd defer to sky though.

Changed the name as for now.

[sky, 2015-08-20 18:03:13]

https://codereview.chromium.org/1297373004/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1297373004/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.cc:1242: is_mouse_on_widget_ = false;
I'm worried we won't always get here. Say you click hold and drag, or we trigger
a nested message loop or other weird cases.
Here's a though, why do we cache the mouse location for
SynthesizeMouseMoveEvent. Why don't we query for the current mouse location in
SynthesizeMouseMoveEvent, that way you can detect if mouse is over widget or
not.

[oshima, 2015-08-21 01:00:15]

https://codereview.chromium.org/1297373004/diff/20001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/1297373004/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.cc:1242: is_mouse_on_widget_ = false;
On 2015/08/20 18:03:12, sky wrote:
> I'm worried we won't always get here. Say you click hold and drag, or we
trigger
> a nested message loop or other weird cases.

For drag case, the mouse location is outside of the widget, so this still should
work correctly. I think the nested message loop case won't affect this, although
I may be missing something.

> Here's a though, why do we cache the mouse location for
> SynthesizeMouseMoveEvent. Why don't we query for the current mouse location in
> SynthesizeMouseMoveEvent, that way you can detect if mouse is over widget or
> not.

This should work too, I think.

[yoshiki, 2015-08-27 15:19:55]

On 2015/08/21 01:00:15, oshima (OOO Aug 24 thru 28) wrote:
>
https://codereview.chromium.org/1297373004/diff/20001/ui/views/widget/widget.cc
> File ui/views/widget/widget.cc (right):
> 
>
https://codereview.chromium.org/1297373004/diff/20001/ui/views/widget/widget....
> ui/views/widget/widget.cc:1242: is_mouse_on_widget_ = false;
> On 2015/08/20 18:03:12, sky wrote:
> > I'm worried we won't always get here. Say you click hold and drag, or we
> trigger
> > a nested message loop or other weird cases.
> 
> For drag case, the mouse location is outside of the widget, so this still
should
> work correctly. I think the nested message loop case won't affect this,
although
> I may be missing something.

I checked and it looks working correctly even when I'm dragging.

> > Here's a though, why do we cache the mouse location for
> > SynthesizeMouseMoveEvent. Why don't we query for the current mouse location
in
> > SynthesizeMouseMoveEvent, that way you can detect if mouse is over widget or
> > not.
> 
> This should work too, I think.

I don't know the exact reason since I'm not the person which implement this.

However, I guess caching is simper and clearer than querying. Converting
coordinate may be complex and may contain a bug, especially for multi monitor
case.

[yoshiki, 2015-08-28 00:50:15]

Sky, WDYT? Anyway, the cached location is not a scope of this change. Let us
leave that for a separated patch.

[sky, 2015-08-28 02:02:56]

For the reasons I mentioned earlier I prefer to look up the value.

  -Scott

On Thu, Aug 27, 2015 at 5:50 PM,  <yoshiki@chromium.org> wrote:
> Sky, WDYT? Anyway, the cached location is not a scope of this change. Let us
> leave that for a separated patch.
>
> https://codereview.chromium.org/1297373004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yoshiki, 2015-08-28 21:44:55]

Patchset #5 (id:80001) has been deleted

[yoshiki, 2015-08-28 22:05:17]

Patchset #5 (id:100001) has been deleted

[yoshiki, 2015-08-28 22:10:43]

Patchset #5 (id:120001) has been deleted

[yoshiki, 2015-08-28 22:11:16]

Patchset #4 (id:60001) has been deleted

[yoshiki, 2015-08-28 22:15:05]

On 2015/08/28 02:02:56, sky wrote:
> For the reasons I mentioned earlier I prefer to look up the value.
> 
>   -Scott

Scott, I updated the patch set. PTAL again. Thanks

[sky, 2015-08-31 15:30:02]

LGTM

[yoshiki, 2015-08-31 19:22:21]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2015-08-31 19:23:03]

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

[commit-bot: I haz the power, 2015-08-31 20:03:55]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-08-31 20:04:33]

Patchset 4 (id:??) landed as
https://crrev.com/8019d579217e1ae4cc0a6c57b24e6a058a94f1ae
Cr-Commit-Position: refs/heads/master@{#346447}