Closed
Bug 506815
Opened 15 years ago
Closed 8 years ago
Get rid of MouseTrailer
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: robarnold, Assigned: chutten, Mentored)
References
Details
(Keywords: power, Whiteboard: [good first bug][lang=c++])
Attachments
(2 files, 5 obsolete files)
10.00 KB,
patch
|
jimm
:
review+
bugzilla
:
feedback+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
9.99 KB,
patch
|
chutten
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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. :-(
Updated•10 years ago
|
Mentor: jmathies, tabraldes, netzen
Whiteboard: [good first bug][lang=c++]
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)
Comment 3•10 years ago
|
||
(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.
Comment 5•10 years ago
|
||
(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.
Comment 7•10 years ago
|
||
> https://tbpl.mozilla.org/?tree=Try&rev=3c55b5f30f32
nice, all green.
Comment 8•10 years ago
|
||
So what was MouseTrailer doing exactly, and how has TrackMouseEvent replaced it?
Comment 9•10 years ago
|
||
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+
Comment 10•10 years ago
|
||
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
Comment 11•10 years ago
|
||
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+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c97034feb78
Assignee: nobody → mycoolclub
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7c97034feb78
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 14•10 years ago
|
||
So I guess we'll have to back this out until the follow up fix is stable and able to run tests.
Comment 15•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d0801a6d38dc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/06f612773cc0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 16•10 years ago
|
||
pretty sure this is up on aurora as well, will get that once this merges.
Flags: needinfo?(jmathies)
Comment 17•9 years ago
|
||
https://hg.mozilla.org/try/pushloghtml?changeset=6aed5018dbc8
Flags: needinfo?(jmathies)
Attachment #8533764 -
Flags: approval-mozilla-beta?
Updated•9 years ago
|
Attachment #8533764 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•9 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-beta/rev/33e89eefaba5 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/d424db9bb5a0
Updated•9 years ago
|
Assignee: mycoolclub → nobody
Updated•9 years ago
|
Target Milestone: mozilla35 → ---
Comment 20•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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)
Comment 23•9 years ago
|
||
(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)
Assignee | ||
Comment 24•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8689033 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 25•8 years ago
|
||
Let's see if this one'll stick.
Attachment #8689033 -
Attachment is obsolete: true
Attachment #8689630 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 26•8 years ago
|
||
How is this patch does not cause bug 1085027 again? It does not contain a fix from bug 1085027.
Flags: needinfo?(chutten)
Comment 27•8 years ago
|
||
(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
Assignee | ||
Comment 28•8 years ago
|
||
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)
Assignee | ||
Comment 29•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96358715bda1
Attachment #8533764 -
Attachment is obsolete: true
Attachment #8689630 -
Attachment is obsolete: true
Attachment #8690107 -
Flags: review?(jmathies)
Updated•8 years ago
|
Attachment #8690107 -
Flags: review?(jmathies) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e87a89f401d
Keywords: checkin-needed
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e87a89f401d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 8 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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)
Assignee | ||
Comment 34•8 years ago
|
||
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 35•8 years ago
|
||
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+
Assignee | ||
Comment 36•8 years ago
|
||
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+
status-firefox44:
--- → affected
Comment 38•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9177f28fc28e
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)
Assignee | ||
Comment 40•8 years ago
|
||
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)
Assignee | ||
Comment 41•8 years ago
|
||
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+
Comment 43•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/146eb4908a1a
Comment 44•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/146eb4908a1a
status-b2g-v2.5:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•