Closed GoogleCodeExporter closed 9 years ago
The regular Firefox does this too, however. I imagine it's due to the unusual
widget library, and I don't think there's an easy solution.
If you have a code idea, I'll consider it, but I don't think there's an easy
fix here.
Original comment by classi...@floodgap.com
on 25 Sep 2011 at 11:57
The fix will be quit easy!
The current, IMO buggy, behaviour was introduced by two bugfixes in the
nsToolkit widget.
Here the description from nsToolkit.mm:
// Cocoa Firefox's use of custom context menus requires that we explicitly
// handle mouse events from other processes that the OS handles
// "automatically" for native context menus -- mouseMoved events so that
// right-click context menus work properly when our browser doesn't have the
// focus (bmo bug 368077), and mouseDown events so that our browser can
// dismiss a context menu when a mouseDown happens in another process (bmo
// bug 339945).
The fixes for the problems were done totally wrong because cocoa applications
DO NOT open context menus by right clicking an inactive window. You can verify
this with every Apple cocoa application of your choice. The Finder acts
differently - but it's a carbon application.
The current fix installs a CGEventTap for mouse clicks and mouse movements so
that while a XUL application is running it always receives ANY events of that
type regardless of being active or inactive, hidden or visible.
The IMO correct way to fix the above mozilla bugs is as follows:
- When receiving a right click into an inactive window the right click event
MUST be ignored but just that window be activated
So essentially the calls to "UnregisterAllProcessMouseEventHandlers()" and
"RegisterForAllProcessMouseEvents()" should be removed from "nsToolkit.mm".
This step alone already will return us our lost CPU time.
Now there needs to be figured out how to prevent the reception of a right click
event into in inactive window. That would be in the nsChildView widget, I think.
Original comment by Tobias.N...@gmail.com
on 27 Sep 2011 at 8:31
I ... guess that sounds right, but it seems rather risky to me to completely
prevent the app from getting those events since that might also interfere with
XUL controls. It also seems like if the current behaviour were a real issue,
there would be a Mozilla bug open on it.
I should note one thing; we use somewhat different code on how to decide if the
window wants the event or not. You are correct; this is in
nsChildView.mm::inactiveWindowAcceptsMouseEvent. Issue 19 causes clicks to
foreground that window. I'm agreeable to making that only trigger on left
clicks for consistency with other apps, although I think the current behaviour
is "more proper."
I'll certainly review a patch for it as long as it doesn't regress any current
behaviour.
Original comment by classi...@floodgap.com
on 27 Sep 2011 at 9:03
Also, be sure to look at ::WindowAcceptsEvent (see the comments in
-inactiveWindowAcceptsMouseEvent for why).
Original comment by classi...@floodgap.com
on 27 Sep 2011 at 9:04
... and, encore, those two functions could probably be streamlined somewhat,
now that I look at it. It might be better to do this after 8 because I have a
lot of Cocoa work that I have to do first.
Original comment by classi...@floodgap.com
on 27 Sep 2011 at 9:06
I looked at it a bit closer and saw that it uses Carbon events for mouse
movements and CGEventTaps for mouse clicks.
That Carbon event now also causes a Cocoa event (at least in 10.4). If I
understand correctly the call to "InstallEventHandler()" just makes us
receiving Cocoa events also for events caused while other processes are in the
foreground. If we wouldn't call it we would still receive events while we are
in the foreground or when we are asked to go there by a click in an inactive
window.
So in "inactiveWindowAcceptsMouseEvent()" we should return YES after bringing
us to front because of a right click, in order to prevent a context menu to be
opened; left clicks into inactive windows seem to be ignored by Gecko anyway so
we shouldn't need to ask it in that case.
Btw holding a mouse button and moving the mouse around doesn't cause extra CPU
time consumption.
I'll test that in the following two weeks, I think - I hate the fact that TFF
is wasting resources in that way. I expect the responsiveness of TFF and the
whole system to be improved by fixing that properly.
Note: Right clicks shouldn't be ignored completely but just bring the window to
the foreground.
Original comment by Tobias.N...@gmail.com
on 27 Sep 2011 at 11:13
Removing those two calls from nsToolkit.mm already seems to do the trick both
on 10.5 and on 10.4 !
Moreover the fixes for issue 19 seem to accomplish what those calls were
supposed to do - just that they work better :-) !
I'll use that for some time to see if there are any issues.
Original comment by Tobias.N...@gmail.com
on 28 Sep 2011 at 5:10
Of course they work better, I wrote them! ;)
Joking aside, if that's the only two changes you propose (making
"UnregisterAllProcessMouseEventHandlers()" and
"RegisterForAllProcessMouseEvents()" into no-ops) and those seem to work, I
would take that in 8 beta. The cocoa changes in 8 are nasty gnarly, but those
should be safe if they work for you.
Original comment by classi...@floodgap.com
on 28 Sep 2011 at 5:43
Hmm....
https://bugzilla.mozilla.org/show_bug.cgi?id=675208
Original comment by classi...@floodgap.com
on 28 Sep 2011 at 8:42
While backing out most of M675208, I think we need to spin out our own widget
library (if not now, definitely when 10.5 support ends), or we're going to be
chasing a lot of things like this. I'll file that as issue 89.
Original comment by classi...@floodgap.com
on 29 Sep 2011 at 12:37
At mozilla they are always stating that they needed that nasty workarounds
because of 10.4 . I wonder why they never thought of fixing it like you did in
issue 19...
Couldn't the fix for issue 19 be improved in that it only brings the process to
the front when it actually isn't when receiving the event? I think of making it
depend on the result of [NSApp isActive] like it is done in those workarounds
in nsToolkit.mm .
That pref "ui.use_native_popup_windows" they mention seems to be interesting.
Don't know if it does anything in firefox (might be only in XUL embedding
applications) but I added it to my config.
Original comment by Tobias.N...@gmail.com
on 29 Sep 2011 at 5:57
The changes from 675208 might not need to be backed out if we could make use of
MATrackingArea available from http://mattgemmell.com/source/ .
Although MATrackingArea uses a polling timer to generate the events I think it
would consume less CPU time than the current event handling.
Don't know if licensing would be a problem...
Original comment by Tobias.N...@gmail.com
on 29 Sep 2011 at 4:00
I looked at MATrackingArea but it doesn't seem to work the same way as
NSTrackingArea, even though it's similar. We might need to try it if there's
absolutely no other way to simulate it, but we have no guarantee it acts or
supports the same things that NSTrackingArea does.
I did finish the 675208 backout, but it's hairy because of all the other things
in issue 83. 519972 in particular was a nightmare because it undoes all of our
10.4 key handling code. I'm rechecking my work against the changesets to make
sure I got everything.
Expanding issue 19's fix like you say in comment 11 could work but I just want
to make sure there are no regressions in the backout first. I'm very nervous
about this amount of rework I had to do, and I haven't even gotten through the
first (big) changeset yet.
Original comment by classi...@floodgap.com
on 29 Sep 2011 at 4:11
I'd prefer MATrackingArea since the current code needs so much CPU time.
Actually ClickToFlash (the WebKit plugin:
https://github.com/rentzsch/clicktoflash) uses NSTrackingArea if available and
if not falls back to MATrackingArea.
I'll investigate a little next week.
Original comment by Tobias.N...@gmail.com
on 29 Sep 2011 at 5:09
I know about the ClickToFlash code and there's two separate code paths in it
for MATrackingArea vs NSTrackingArea. This is why I'm really reluctant, because
we'll be dependent on a solution that I don't know will cover all the bases
NSTrackingArea does.
I'm more comfortable with your first suggestion about no-oping those functions
rather than adopting a new, untested UI component, especially because 8 will
require so much widget rework anyway. If we hit issues we can't solve with the
current code, then we have no choice, but we have a more conservative solution
proposed already and I'd rather go with that at least in the short term.
Original comment by classi...@floodgap.com
on 29 Sep 2011 at 5:48
Of course you're right that now it's not be the adequate moment to change from
receiving every mouse movement event to some sort of tracking areas that just
give us move entered and mouse exited events when we need them.
However in order to further improve performance of TFF that might actually be
the next biggest point to improve.
Original comment by Tobias.N...@gmail.com
on 1 Oct 2011 at 6:35
I finished backporting all the widget changes and TenFourFox 8 comes up. There
are some problems with buttons I have to fix (the CoreUI changes), but I was
able to revert or convert everything else. When I'm done with that I'll put in
your no-op change and see what happens.
Original comment by classi...@floodgap.com
on 1 Oct 2011 at 2:31
Patch in. Works good. Scheduled for beta.
Original comment by classi...@floodgap.com
on 4 Oct 2011 at 1:01
I applied the patch from 675208 to my 7.0 source tree and ported it to use
NSView/NSWindow trackingRects instead of NSTrackingAreas.
Some small issues are left but it's basically working! MouseMoved events are
then only generated when TFF needs them; generally when the cursor is inside
some TFF window.
Original comment by Tobias.N...@gmail.com
on 5 Oct 2011 at 10:55
And with trackingRects the MouseMoved events cause approx. 4% less CPU usage on
this G4 1,5GHz.
Original comment by Tobias.N...@gmail.com
on 5 Oct 2011 at 10:57
We can consider that when we've dealt with issue 89, but given how much widget
reworking I've already done for 8 I really don't want another big change. We're
way behind on the beta as it is (I'm pushing for this weekend once I've gotten
the colour issue sorted).
Original comment by classi...@floodgap.com
on 6 Oct 2011 at 12:44
(disabling the nsToolkit.mm call *is* in the beta btw and does appear to work)
Original comment by classi...@floodgap.com
on 6 Oct 2011 at 12:44
Never mind. It'll take some time to tweak and tune this anyway.
Original comment by Tobias.N...@gmail.com
on 6 Oct 2011 at 6:08
Fixed the remaining issues and now am testing with trackingRects in daily use.
Original comment by Tobias.N...@gmail.com
on 6 Oct 2011 at 10:51
[deleted comment]
I'm going to close this one specifically referring to the CPU rise, since that
much seems to work with the current code. When you're ready with the tracking
rects, go ahead and open up a new one for that patch.
Original comment by classi...@floodgap.com
on 8 Oct 2011 at 1:23
trackingRects working correctly in 10.5 now - but in 10.4 there's at least one
nasty bug in the OS which causes the mouseMoved events to be sent to the wrong
NSWindow. I'm working on a workaround that redirects those events to the
correct one.
Original comment by Tobias.N...@gmail.com
on 9 Oct 2011 at 6:22
10.4 workaround now working quite well.
Original comment by Tobias.N...@gmail.com
on 9 Oct 2011 at 11:57
TrackingRects are working well - the event redirecting workaround for 10.4 as
well. But I don't like that workaround - even less than the current
implementation.
For 10.5, that is without the workaround, I'd recommend to go with
trackingRects but for 10.4 I'd prefer to leave it like it is.
Original comment by Tobias.N...@gmail.com
on 7 Nov 2011 at 10:16
For 10.5, we could simply use Mozilla's code, couldn't we? (Though I've never
tested NSTrackingArea on PPC.)
Original comment by classi...@floodgap.com
on 8 Nov 2011 at 2:46
Well surely one could - but that would be a special build, wouldn't it? With
the tiger widgets split off into a separate directory it should be easy to do
those two builds from the same source tree. That would result in a total of 7
builds as !0.5 doesn't support G3s.
But then one could as well build firefox with PPC nanojit, PCRE and altivec
enhancements.
Original comment by Tobias.N...@gmail.com
on 8 Nov 2011 at 6:59
Original issue reported on code.google.com by
Tobias.N...@gmail.com
on 25 Sep 2011 at 4:25