506815 - Get rid of MouseTrailer

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Rob Arnold [:robarnold]]

It lives in nsToolkit and uses a timer (and is a bad hack). There is a Windows 98 API, TrackMouseEvent, that can do what we want (I think). It's not available for CE but MouseTrailer doesn't do anything on Windows CE anyways.

Comment 1

[:Gijs (he/him)]

I suspect this is (sort of) a dupe of bug 312566, where bug 312566 comment 3 notes that this API isn't a good replacement. :-(

Comment 2

[u518207]

Attachment: 506815.patch

This is my fix.
I tried ins and outs (to/from the window) with the mouse and it seems to work as usual.

Comment 3

[Jim Mathies [:jimm]]

(In reply to mycoolclub from comment #2)
> Created attachment 8496553 [details] [diff] [review]
> 506815.patch
> 
> This is my fix.
> I tried ins and outs (to/from the window) with the mouse and it seems to
> work as usual.

Do you have try access mcc?

Comment 4

[u518207]

I just registered in bugzilla.Wasn't notified about any permissions.

I didn't mention that this attachment is built upon some other patches (share some modified source files) those patches already accepted. Not sure whether try server works with Firefox source that already includes these patches.

Comment 5

[Jim Mathies [:jimm]]

(In reply to mycoolclub from comment #4)
> I just registered in bugzilla.Wasn't notified about any permissions.

Try server allows contributors to test patches on all of our platforms and run tests. You can request access to it if you plan to become a regular contributor.

Here's a windows try push with a full test run with your patch - 

https://tbpl.mozilla.org/?tree=Try&rev=3c55b5f30f32

Comment 6

[u518207]

I have some spare time in these days, what will be later is questionable.

Comment 7

[Jim Mathies [:jimm]]

> https://tbpl.mozilla.org/?tree=Try&rev=3c55b5f30f32

nice, all green.

Comment 8

[Jim Mathies [:jimm]]

So what was MouseTrailer doing exactly, and how has TrackMouseEvent replaced it?

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8496553 [details] [diff] [review]
506815.patch

Review of attachment 8496553 [details] [diff] [review]:
-----------------------------------------------------------------

I think I answered my own questions. Looks great!

::: widget/windows/nsWindow.cpp
@@ +3138,4 @@
>      ::ReleaseCapture();
>    }
>    sIsInMouseCapture = aCapture;
> +  TrackMouseEvent(&mTrack);

nit - add a comment above this explaining what this does..

// Requests WM_MOUSELEAVE events for this window.

Comment 10

[Jim Mathies [:jimm]]

Please update your commit message to something more explanatory -

Bug 506815: MouseTrailer was removed r=jimm

this isn't very useful. How about:

Bug 506815 - Replace old MouseTrailer code with TrackMouseEvent api. r=jimm

Then mark the bug with checkin-needed.

Thanks!

Comment 11

[u518207]

Attachment: 506815.patch

I'll try to explain.
As much as I understood in a reply to every mouse event the timer procedure was requested to launch after some short period (200ms). This procedure posted WM_MOUSELEAVE message if mouse had left the window area. Now we call the TrackMouseEvent only once, where we set the capture window (in CaptureMouse method). As the result we don't need to request for Timer Procedure upon every mouse event and mouse tracking routine is now simpler and, hopefully with less overhead (depends on implementation of TrackMouseEvent).

I can't modify this bug's page so I can't add checkin-needed.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c97034feb78

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7c97034feb78

Comment 14

[Jim Mathies [:jimm]]

So I guess we'll have to back this out until the follow up fix is stable and able to run tests.

Comment 15

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d0801a6d38dc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/06f612773cc0

Comment 16

[Jim Mathies [:jimm]]

pretty sure this is up on aurora as well, will get that once this merges.

Comment 17

[Jim Mathies [:jimm]]

Attachment: beta backout

https://hg.mozilla.org/try/pushloghtml?changeset=6aed5018dbc8

Comment 18

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6aed5018dbc8

Comment 19

[Jim Mathies [:jimm]]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/33e89eefaba5
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d424db9bb5a0

Comment 20

[Waqar Arshad]

Hi, 
I would like to take this bug on for my first Fix. 
I was wondering if you could specify or point me to the right direction about what exactly i would need to do and where in the code. 

Thank You

Comment 21

[Kevin Mather [:kmather73]]

Hi, 
I would also like to take this bug on for my first bug as well if it hasn't yet be taken.

Comment 22

[Alice]

Hi there!

I would like to take this as my first bug as part of my university course please? 

I would also appreciate a few pointers in the right direction.

Comment 23

[Jim Mathies [:jimm]]

(In reply to Alice from comment #22)
> Hi there!
> 
> I would like to take this as my first bug as part of my university course
> please? 
> 
> I would also appreciate a few pointers in the right direction.

Hi,

We had a suitable fix here posted by mycoolclub, but unfortunately that patch caused a regression in tooltip handling. Another fix landed to try and address this but that failed to address the issue. You can look at the bug that blocks this one, bug 1085027, for background on the problem the patch that landed here caused.

What we would like to do is get the original patch landed without cuasing any regressions. So I would suggest reading through both bugs to gain an understanding of the timeline and what was attempted, and see if you can come up with some ideas on how to get the work here landed without followup issues.

Comment 24

[Chris H-C :chutten]

Attachment: 0001-bug-506815-Remove-MouseTrailer-r-jimm.patch

Updated mycoolclub's patch to apply against a more modern tree.

Trypush here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33f73cb89bc1

Let's see if the permaorange recurs, or if this is now safe to repush. I'd like to get this fixed to unblock 1107779.

Comment 25

[Chris H-C :chutten]

Attachment: 0001-bug-506815-Remove-MouseTrailer-r-jimm.patch

Let's see if this one'll stick.

Comment 26

[Masatoshi Kimura [:emk]]

How is this patch does not cause bug 1085027 again? It does not contain a fix from bug 1085027.

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Masatoshi Kimura [:emk] from comment #26)
> How is this patch does not cause bug 1085027 again? It does not contain a
> fix from bug 1085027.

clearing checkin-needed since i guess we need to make that sure first

Comment 28

[Chris H-C :chutten]

I had assumed that code changes elsewhere had taken care of it so the permaorange would be the only thing I had to watch for. I was wrong.

In Windows 10, Firefox without this patch:
1) Mouseover the close button in the top right and leave it there for about 1s
2) Quickly remove the mouse from the window
Note that the mouseover highlight on the close button remains for up to 200ms (this is waiting for the FindWindowUnderPoint call taking care of it)

In Windows 10, Firefox with the patch:
Same STR, but this time the highlight never leaves.

Near as I can tell from my reading of MSDN and the patch and (of all things) the Chromium source, we're not using TrackMouseEvent broadly enough. Using it just for CaptureMouse means we'll only get MOUSELEAVE after dragging operations.

So, what we want is the general form of TrackMouseEvent: On the first MOUSEMOVE over a window, TrackMouseEvent to ask for MOUSELEAVE. On MOUSELEAVE, reset state so we can recognize the next "first MOUSEMOVE".

Annnd, whaddayaknow, it works. 

Testing performed:
Resizing and dragging window
STR as above (close button hover)
Going through Ease of Access controls and trying to reproduce bug 1085027

Next, of course, is a new trypush with the new patch.

Comment 29

[Chris H-C :chutten]

Attachment: 0001-bug-506815-Replace-MouseTrailer-with-TrackMouseEvent.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=96358715bda1

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e87a89f401d

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e87a89f401d

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This is showing up in our uplift query because of the approval flag from the obsolete patch from a year ago. Can we unset that flag so this doesn't get listed as an uplift request?

Comment 34

[Chris H-C :chutten]

Comment on attachment 8533764 [details] [diff] [review]
beta backout

Clearing uplift flag on obsolete patch. Sorry about that!

Comment 35

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8690107 [details] [diff] [review]
0001-bug-506815-Replace-MouseTrailer-with-TrackMouseEvent.patch

Review of attachment 8690107 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +5078,5 @@
>  
>      case WM_MOUSEMOVE:
>      {
> +      if (!mMousePresent) {
> +        // First MOOUSEMOVE over the client area. Ask for MOUSELEAVE

Nit: s/MOOUSEMOVE/MOUSEMOVE/

Comment 36

[Chris H-C :chutten]

Comment on attachment 8690107 [details] [diff] [review]
0001-bug-506815-Replace-MouseTrailer-with-TrackMouseEvent.patch

I've filed bug 1232669 to track the comment typo.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Reduced battery life, improper reports of user active time on Windows
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96358715bda1 also note the nice decrease in the activeTicks measurement here: http://mzl.la/1Za3vBG
[Risks and why]: Minor. This has been in Nightly a while. Just barely missed the train to uplift just to aurora.
[String/UUID change made/needed]: Nope.

Comment 37

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8690107 [details] [diff] [review]
0001-bug-506815-Replace-MouseTrailer-with-TrackMouseEvent.patch

Given the user impact and the fact that this fix has been on Nightly for almost a month, seems safe to uplift to Beta44.

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9177f28fc28e

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for windows build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=689740&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/023e4f16666b

The patch didn't apply totally cleanly in the first place, but it looked like an easy enough fix that I tried to fix it up. Apparently I didn't fix it up correctly. Can we get a rebased patch for beta?

Comment 40

[Chris H-C :chutten]

This is my first time attempting to pull down beta and work with it, so we'll see how it goes.

I think the missing piece from your attempt was Bug 1223310 which is in 45 and converted that nsIntRect to LayoutDeviceIntRect (which I think was designed to reduce footgunning by making a more narrow type (like a compiler-enforced Apps Hungarian)). So my patch currently (slowly) building is identical except s/LayoutDeviceIntRect/nsIntRect/

Comment 41

[Chris H-C :chutten]

Attachment: Replace-MouseTrailer-with-TrackMouseEvent rebased for beta

Fixed the merge failure by using the then-current type for `rect`. 

Testing: built, ran, tested that window dragging and resizing worked. Tested that user activity/inactivity was reported appropriately.

(copied same approval request comment from comment 36)

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Reduced battery life, improper reports of user active time on Windows
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96358715bda1 also note the nice decrease in the activeTicks measurement here: http://mzl.la/1Za3vBG
[Risks and why]: Minor. This has been in Nightly a while. Just barely missed the train to uplift just to aurora.
[String/UUID change made/needed]: Nope.

Comment 42

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700108 [details] [diff] [review]
Replace-MouseTrailer-with-TrackMouseEvent rebased for beta

Beta44+, let's hope it sticks this time.

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/146eb4908a1a

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/146eb4908a1a