Closed Bug 332579 Opened 15 years ago Closed 14 years ago

Improve Mac event handling for 1.8.1

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

1.8 Branch
PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mark, Assigned: mark)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 9 obsolete files)

I've made some changes to Mac event handling on the THREADS_20060307_BRANCH that totally eliminate WaitNextEvent, which has been the source of much suckage on the Mac.  It would be nice to take a subset of these changes on the MOZILLA_1_8_BRANCH after the threads branch is merged to the trunk and has gotten some testing exposure and we've had an opportunity to assess the risk.

In what I've done, the EventRecords returned by WaitNextEvent are instead created by a transitional application-scope Carbon event handler.  The main run loop can then become a Carbon run loop.
Attached patch WaitNextEvent-free AppShell (obsolete) — Splinter Review
There's currenty a bug with this AppShell, so I'm not asking for review yet: if you open a browser window and a preferences window, and then press command-W to close the preferences window, the browser window will also close.
Attached patch MacMessagePump cleanup (obsolete) — Splinter Review
Changing event handling allows a lot of junk to be ripped out of nsMacMessagePump.  There may be more cleanup possible.
Depends on: nsIThreadManager
No longer depends on: nsIThreadManager
Attached patch Checkpoint (obsolete) — Splinter Review
The bugs are fixed in this one, but I'm not finished with cleanup yet.
Attachment #219062 - Attachment is obsolete: true
Attachment #219063 - Attachment is obsolete: true
The cleanup gets rid of:
 - already-dead code, either #ifdefed or otherwise unreachable
 - code that is becoming dead as a result of the new event loop impl
 - code that should have been dead or #ifdefed but erroneously never was
   (some of which was last useful only to Classic or CFM users)

To exercise some code of the third class, hold down the command key while starting to resize a window by grabbing the grow box in its lower-right corner.  You'll hit some code designed for live resize in the classic OS that was never properly disabled.  It interacts poorly with the current Carbon event-based live resize code.

I think the most recent "checkpoint" is the bulk of the cleanup.  I'm going to try to do a little bit more before posting a reviewable patch.  I'd like to see these changes on the trunk too.  Even though the appshell will change with the THREADS_20060307_BRANCH landing, the new appshell will be able to take advantage of this same cleanup (with an API change or two).

Note to self for the threads branch: ReceiveNextEvent (used here) is probably better than AcquireFirstMatchingEventInQueue/RemoveEventFromQueue (used there).

After this lands, there's some additional work that can be done to reduce the number of times the transitional WNE event handler is hit.  For example, right now, we have a kEventWindowUpdate handler on each nsMacWindow, but it doesn't actually properly handle the event (nor does it inform anyone that the event was handled).  The transitional event handler gets it at the application scope and has to figure out which window should deal with it.  This amounts to busywork in the path for one of the more common event types.

Some handlers for the other common event types can probably also be easily moved into the appropriate class (usually nsMacWindow), eliminating the overhead of the transitional handler.
Attached patch v3 (obsolete) — Splinter Review
Attachment #219361 - Attachment is obsolete: true
Attached patch v3, applies to branches (obsolete) — Splinter Review
Comment on attachment 219641 [details] [diff] [review]
v3

Sorry, Josh.  This is mostly a lot of stuff that's being removed.  The interesting things are happening in nsAppShell, the diff to that file is bigger than the patched file, so it might be easier to just patch it and read it.  There's also interesting stuff happening in nsMacMessagePump: that's where the transitional event handler lives.  You might recognize it from the threads branch.
Attachment #219641 - Flags: review?(joshmoz)
Comment on attachment 219642 [details] [diff] [review]
v3, applies to branches

The branch patch is different from the trunk patch in that it removes nsWatchTask.  This was already removed on the trunk.
Attachment #219641 - Flags: review?(joshmoz) → review+
Attachment #219641 - Flags: superreview?(shaver)
Depends on: 336195, 336197
Attached patch v4 (branch) fixes 336195 (obsolete) — Splinter Review
Fixes bug 336195:

Changed the timeout in nsAppShell::GetNativeEvent to not block forever when someone's manually running the event loop.

Doesn't over-free the nsMacTSMMessagePump singleton.
Attachment #219641 - Attachment is obsolete: true
Attachment #219642 - Attachment is obsolete: true
Attachment #219641 - Flags: superreview?(shaver)
Attached patch v5 (branch) also fixes 336197 (obsolete) — Splinter Review
When I was developing this patch on the trunk, opening a browser window and a preferences window and then pressing command-W would close both windows, so I worked around it by eating more events than I really should have.  I'm not seeing the same problem on the branch, and am able to fix bug 336197 by not swallowing unprocessed events.
Attachment #220598 - Attachment is obsolete: true
Attached patch v6 (branch) fixes 336365 (obsolete) — Splinter Review
Attachment #220600 - Attachment is obsolete: true
This one whacks all regressions reported so far.

Test build:

http://jackassofalltrades.com/tmp/firefox-1503-332579v7.dmg
Attachment #220607 - Attachment is obsolete: true
Comment on attachment 220634 [details] [diff] [review]
v7 (branch) fixes 336365 for non-debug builds too

Josh, if you want, I can summarize the changes I made since v3.  It was mostly just moving things around and enabling and disabling stuff to more closely track the way things worked with the WaitNextEvent loop.
Attachment #220634 - Flags: superreview?(shaver)
Attachment #220634 - Flags: superreview?(shaver)
(In reply to comment #12)
> http://jackassofalltrades.com/tmp/firefox-1503-332579v7.dmg

Mark, would you expect this build to provide significant performance benefits over a standard build without the event handling changes?  That appears to be the case here in nearly all aspects of use, at least on my admittedly slower hardware, and so far no whammies.  Excellent work.
Absolutely.  Current "standard" builds are based on an event loop that's basically spinning when it doesn't need to be.  The key architectural change here is that the loop goes to sleep until something actually happens.  There are also side benefits in the elimination of a bunch of dead code paths that would execute frequently.  These paths were useful on OS 9 but are dead ends on OS X.

Remaining known regressions are bug 336365 comment 2.  I've fixed the second one (but the fix isn't in v7) but haven't had a chance to look at the first one yet.

(A benefit of slower hardware is that it exhibits performance improvements and detriments more readily - good for testing!)
Double-processing of certain key events was being caused by weird and ugly reentrancy problems.  I avoided the reentrancy by not handling key events while a key events is already being processed.  This is not obviously safe, but it doesn't seem to harm anything yet.

Unicode keyboard breakage after running a modal sheet was fixed by making the nsMacMessagePump a singleton.  (I'd actually like better memory management here than working with static data, but none of this stuff is COMatose.)

A universal test build with these fixes is available at:

http://jackassofalltrades.com/tmp/firefox-1503-332579v8.dmg
Attachment #220634 - Attachment is obsolete: true
Attachment #220791 - Flags: superreview?(shaver)
(In reply to comment #16)
> A universal test build with these fixes is available at:
> 
> http://jackassofalltrades.com/tmp/firefox-1503-332579v8.dmg
> 

Works like a charm under Mac OS X 10.2.8, on a dead-slow 300 MHz iBook. Big improvement. For instance, when typing in a textfield like this one, the cpu-usage used to be 30-40%, but it now dropped to 20-30%.

The only defect that I found in 10 minutes of usage, was that a cursor movement in a textfield like this one, was always skipping by 2 characters. But it was an old branch I noticed (it still had the click-and-hold context menu), so I don;'t know if this is significant.
Jo, the test builds are equivalent to Firefox 1.5.0.3 plus the patches attached to this bug.  This gives a pretty stable base for testing, although the patch itself isn't targeted at the 1.8.0 branch.  That branch still does have the click-and-hold contextual menus.

I don't see anything skipping two characters in text fields.  Do you mean navigating with the arrow keys?  If you do find a bug that occurs in the v8 test build but not in Firefox 1.5.0.3, file a new bug report and mark it as blocking this bug.  If it's a keypress bug, be sure to specify which keyboard layout you're using (in the International system preference pane).  Thanks to everyone for the testing and feedback.
(In reply to comment #18)
> I don't see anything skipping two characters in text fields.  Do you mean
> navigating with the arrow keys?  If you do find a bug that occurs in the v8
> test build but not in Firefox 1.5.0.3, file a new bug report and mark it as
> blocking this bug.  If it's a keypress bug, be sure to specify which keyboard
> layout you're using (in the International system preference pane).  Thanks to
> everyone for the testing and feedback.
> 

It was with the arrow keys, using the Dutch keyboard layout.

I'm not able to repeat it right now ; it happened after several dozen webpages, testing with Flash and QuickTime, etc ... I'll keep an eye on it if I see it again.
Comment on attachment 220791 [details] [diff] [review]
v8 (branch) fixes other 336365 regressions

Josh, can you sign off on the changes since v3?  Here's a summary of the changes:

- change nsAppShell::GetNativeEvent to not block forever (when events are being processed outside of nsAppShell::Run) (fixes parts of 336195).
- manage nsTSMMessagePump and nsMacMessagePump better, both are singletons now (fixes 336365 comment 2 part 2).
- nsMacMessagePump::WNETransitionEventHandler now returns eventNotHandledErr if an event was not handled, to allow system handlers of last resort to take care of things like command-` (fixes 336197).
- nsMacMessagePump::DispatchEvent will only dispatch when nsAppShell::Run or nsAppShell::ProcessNativeEvent is on the stack, avoiding crashes when the handlers are still installed but the app is shutting down (fixes 336195).
- install TSM AE handlers after transitional CE handlers to give them first crack at handling keystrokes.  Fixes inability to type when using Unicode keyboard layouts (336365 comment 0).
- Avoid reentrancy in nsMacMessagePump::DoKey (fixes 336365 comment 2 part 1) - there may be a better solution that can be applied here.

I've been running v8 for a couple days and so have a couple of testers - it's definitely stable enough for trunk/a2, and if there are regressions left, it'll be easier to handle them once this has all landed.
Attachment #220791 - Flags: review?(joshmoz)
(In reply to comment #20)

> I've been running v8 for a couple days and so have a couple of testers

I've been running it too (10.3.9, lots of FX extensions, doing heavy js/css web development so likely to stress it in weird ways) and v8 has been performing great.  The speed improvement is amazing, particularly when interacting with things like js drag and drop widgets.
Attachment #220791 - Flags: review?(joshmoz) → review+
Using build from comment #16, and I saw an issue that might be related to this patch.  In Gmail, I clicked on the "Download" link in order to download the attachment.  This does two this: (1) sets the focus on the "Download" link, and (2) brings up (at least in my case) a dialog asking whether to save the file or open with an app.  'Save' was already selected, so I just hit Enter, but then I saw the dialog immediately reappear.

It looks like the Enter key was being counted twice, first to select the default button in the dialog, and secondly to reselect the "Download" link.
Javier, that's probably related - these changes caused behavior like that before (bug 336365 comment 2 part 1).
Comment on attachment 220791 [details] [diff] [review]
v8 (branch) fixes other 336365 regressions

sr=shaver (curse needing to log into bugzilla!)
Attachment #220791 - Flags: superreview?(shaver) → superreview+
Please land this on the Firefox 2 branch ASAP. We are approved by drivers via email. We need to have this in by Monday morning.
will land tonight when I have a chance to tree-sit
Not landing yet - tree is closed.
This needs to land today before about 2:30pm to make a2 -- branch tree is open, I think we should go straight there rather than wait for the trunk logjam to clear.
Approved for landing through tree-closure, in Fx2a2 triage meeting.  Godspeed!
Attachment #220791 - Flags: approval-branch-1.8.1+
v8 (branch) checked in on MOZILLA_1_8_BRANCH for 1.8a2.
Keywords: fixed1.8
Blocks: 337193
Depends on: 337199
Depends on: 337277
That should be 1.8.1a2.
Keywords: fixed1.8fixed1.8.1
Depends on: 337338
Key event regressions are covered in in bug 337199 patch v1 (attachment 221541 [details] [diff] [review]).

A new test build of Firefox 1.5.0.3 + 332579v8 + 337199v1 is available here:
http://jackassofalltrades.com/tmp/firefox-1503-332579v9.dmg
Depends on: 337370
Should this be marked Resolved?
Yeah, this is fixed.  Keyboard regressions are dependencies and are being mostly handled in bug 337199.  Frontporting this patch to the trunk is bug 338153.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
I've got a new test build available, equivalent to 1.5.0.4rc3 plus 332579v8 (this patch) and further key regression fixes from 337199v4.  This build is available at:

http://jackassofalltrades.com/tmp/firefox-1504rc3-332579v8-337199v4.dmg

I've prepared this build even though the above patches are now all checked in on the 1.8.1 branch because that branch is only alpha-quality at this point, and I'd like testers who have the time to be able to bang on the event improvements in isolation on a more stable codebase.  So, testers, if you have the time, have at it!
Depends on: 337646
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.