Closed Bug 506815 Opened 10 years ago Closed 4 years ago

Get rid of MouseTrailer

Categories

(Core :: Widget: Win32, defect)

x86
Windows NT
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: robarnold, Assigned: chutten, Mentored)

References

Details

(Keywords: power, Whiteboard: [good first bug][lang=c++])

Attachments

(2 files, 5 obsolete files)

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.
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. :-(
Mentor: jmathies, tabraldes, netzen
Whiteboard: [good first bug][lang=c++]
Attached patch 506815.patch (obsolete) — Splinter Review
This is my fix.
I tried ins and outs (to/from the window) with the mouse and it seems to work as usual.
Attachment #8496553 - Flags: review?(jmathies)
(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?
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.
(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
I have some spare time in these days, what will be later is questionable.
So what was MouseTrailer doing exactly, and how has TrackMouseEvent replaced it?
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.
Attachment #8496553 - Flags: review?(jmathies) → review+
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!
Attachment #8496553 - Attachment is obsolete: true
Attached patch 506815.patch (obsolete) — Splinter Review
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.
Attachment #8498038 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7c97034feb78
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1085027
So I guess we'll have to back this out until the follow up fix is stable and able to run tests.
pretty sure this is up on aurora as well, will get that once this merges.
Flags: needinfo?(jmathies)
Attached patch beta backout (obsolete) — Splinter Review
https://hg.mozilla.org/try/pushloghtml?changeset=6aed5018dbc8
Flags: needinfo?(jmathies)
Attachment #8533764 - Flags: approval-mozilla-beta?
Attachment #8533764 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee: mycoolclub → nobody
Target Milestone: mozilla35 → ---
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
Hi, 
I would also like to take this bug on for my first bug as well if it hasn't yet be taken.
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.
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
Blocks: 1107779
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.
Assignee: nobody → chutten
Attachment #8498038 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #8689033 - Flags: review?(jmathies)
Keywords: power
Attachment #8689033 - Flags: review?(jmathies) → review+
Let's see if this one'll stick.
Attachment #8689033 - Attachment is obsolete: true
Attachment #8689630 - Flags: review+
Keywords: checkin-needed
How is this patch does not cause bug 1085027 again? It does not contain a fix from bug 1085027.
Flags: needinfo?(chutten)
(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
Keywords: checkin-needed
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.
Flags: needinfo?(chutten)
Attachment #8690107 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e87a89f401d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Duplicate of this bug: 1107779
Blocks: 1228368
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?
Flags: needinfo?(chutten)
Comment on attachment 8533764 [details] [diff] [review]
beta backout

Clearing uplift flag on obsolete patch. Sorry about that!
Flags: needinfo?(chutten)
Attachment #8533764 - Flags: approval-mozilla-beta+
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/
Attachment #8690107 - Flags: feedback+
Blocks: 1232669
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.
Attachment #8690107 - Flags: approval-mozilla-beta?
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.
Attachment #8690107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
Flags: needinfo?(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/
Flags: needinfo?(chutten)
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.
Attachment #8700108 - Flags: review+
Attachment #8700108 - Flags: approval-mozilla-beta?
Comment on attachment 8700108 [details] [diff] [review]
Replace-MouseTrailer-with-TrackMouseEvent rebased for beta

Beta44+, let's hope it sticks this time.
Attachment #8700108 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1235821
Depends on: 1249664
Depends on: 1273091
Depends on: 1437941
You need to log in before you can comment on or make changes to this bug.