Closed Bug 320746 Opened 14 years ago Closed 12 years ago

[mac] Use GetCurrentEventKeyModifiers() instead of GetCurrentKeyModifiers()

Categories

(Core :: Widget, defect, P2)

1.8 Branch
PowerPC
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: reed, Assigned: jaas)

References

()

Details

Attachments

(2 files, 3 obsolete files)

Spawning from bug 311466 comment #10 (from Jonas Wallden):
According to
http://developer.apple.com/documentation/Carbon/Conceptual/Carbon_Event_Manager/
Tasks/chapter_3_section_7.html GetCurrentKeyModifiers() will read modifier keys
even if the application is in the background. This means a user switching to another app during launch may inadvertently trigger safe mode. Apple recommends calling GetCurrentEventKeyModifiers() instead.
Depends on: 311466
This patch just changes GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers(). I can't test this as I do not have a Mac OS X box. From a grep of entire tree, there are other places where this might need to be done.
Assignee: nobody → reed
Status: NEW → ASSIGNED
Attachment #206267 - Flags: first-review?
Attachment #206267 - Flags: first-review? → first-review?(bugs.mano)
Making this bug for all of the tree. Patch coming up soon.
Component: XRE Startup → Widget
Flags: first-review?(bugs.mano)
Product: Toolkit → Core
Summary: [mac] Use GetCurrentEventKeyModifiers() instead of GetCurrentKeyModifiers() in toolkit/xre/nsAppRunner.cpp → [mac] Use GetCurrentEventKeyModifiers() instead of GetCurrentKeyModifiers()
QA Contact: xre.startup → general
Change GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers() everywhere except for plugin/oji/MRJCarbon/plugin/Source/MRJContext.cp.
Attachment #206267 - Attachment is obsolete: true
Attachment #206279 - Flags: review?(sfraser_bugs)
Attachment #206279 - Flags: superreview?(mark)
Comment on attachment 206279 [details] [diff] [review]
Use GetCurrentEventKeyModifiers() - v1 (trunk)

I'm not sure about this.  Calling GetCurrentEvent with no CE handler on the stack seems risky.  Calling it implicitly through GetCurrentEventKeyModifiers is no better.  We don't want to get old cached events, or worse, garbage.  This type of thing might work now because more system internals are implemented as CEs, and because we're still dependent on a bunch of Carbon ourselves, but I'm afraid that this will break in a bad way when Apple changes AppKit.

(If Apple's explicitly said something about maintaining GetCurrentEvent compatibility in Cocoa apps, lemme know.)
Attachment #206279 - Flags: superreview?(mark)
Attachment #206279 - Flags: superreview-
Attachment #206279 - Flags: review?(sfraser_bugs)
(In reply to comment #4)
> Calling it implicitly through GetCurrentEventKeyModifiers is no better.

It's not an implicit call, it's actually a separate cache that's maintained in response to other events, but I'm still concerned about missing those other events in the future.  I'll think about this some more.
Attachment #206279 - Flags: superreview?(mark)
Attachment #206279 - Flags: superreview-
Attachment #206279 - Flags: review?(sfraser_bugs)
Attachment #206267 - Attachment is obsolete: false
Attachment #206267 - Flags: superreview?(mark)
Attachment #206267 - Flags: review?(sfraser_bugs)
Attachment #206267 - Flags: superreview?(sfraser_bugs)
Attachment #206267 - Flags: superreview?(mark)
Attachment #206267 - Flags: review?(sfraser_bugs)
Attachment #206267 - Flags: review?(mark)
Attachment #206279 - Flags: superreview?(sfraser_bugs)
Attachment #206279 - Flags: superreview?(mark)
Attachment #206279 - Flags: review?(sfraser_bugs)
Attachment #206279 - Flags: review?(mark)
Mark and I discussed this:  we can probably use NSApplication's currentEvent message, to solve this problem.
I have a patch for the Camino-only portion of the tree. Should I put that in this bug, or should I open a new bug just for our fixes?

I'm not sure how the Carbon vs. Cocoa issue is going to resolve itself for the rest of the code base, which is why I haven't patched anything outside Camino.

cl
You can separate the patch out if you want to.

As for the stuff in the core, looking at Reed's patch (by the way, Reed, did you enjoy New York?), the only thing that we'd want to convert to Cocoa for sure is the stuff in widget/src/cocoa.  In those cases, there's no need to use [NSApp currentEvent], because every place GetCurrentKeyModifiers() is called, there's a perfectly good NSEvent already available.

The rest of the stuff in the core should be left Carbon, at least for now, changing GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers().  But hold off on that until you're sure it does the right thing, for example, when you command-tab to put the app in the background.
(In reply to comment #8)
> You can separate the patch out if you want to.

Coming shortly, as an attachment to this bug.

> As for the stuff in the core, looking at Reed's patch (by the way, Reed, did
> you enjoy New York?), the only thing that we'd want to convert to Cocoa for
> sure is the stuff in widget/src/cocoa.  In those cases, there's no need to use
> [NSApp currentEvent], because every place GetCurrentKeyModifiers() is called,
> there's a perfectly good NSEvent already available.
> 
> The rest of the stuff in the core should be left Carbon, at least for now,
> changing GetCurrentKeyModifiers() to GetCurrentEventKeyModifiers().  But hold
> off on that until you're sure it does the right thing, for example, when you
> command-tab to put the app in the background.

I'll let Håkan handle this, then, since he has a far better handle on what's going on in Core than I do. Assigning to him for now, and if he can't get to it, you can take care of it when you get around to it, Mark.

cl
Assignee: reed → hwaara
Status: ASSIGNED → NEW
Attached patch Camino-only patch (obsolete) — Splinter Review
Here's the Camino portion of the patch.

cl
Attachment #206267 - Attachment is obsolete: true
Attachment #206279 - Attachment is obsolete: true
Attachment #216608 - Flags: review?(mark)
Attachment #206267 - Flags: superreview?(sfraser_bugs)
Attachment #206267 - Flags: review?(mark)
Attachment #206279 - Flags: superreview?(sfraser_bugs)
Attachment #206279 - Flags: review?(mark)
Chris, please do a quick sanity check that the places you changed work as expected before this gets checked in.
Comment on attachment 216608 [details] [diff] [review]
Camino-only patch

As discussed:

When you're sticking this stuff into a BOOL, you do need the explicit |!= 0| check.  BOOLs are 8 bits wide, and the modifiers correspond to more significant bits than that, so they'll get lost and everything will evaluate to 0 when you try to copy the wider 32-bit value into the BOOL.

This change is OK because there's no BOOL involved, and if can see all 32 bits:

-      if (GetCurrentKeyModifiers() & cmdKey)
+      if ([[NSApp currentEvent] modifierFlags] & NSCommandKeyMask)
Attachment #216608 - Flags: review?(mark) → review-
BTW, the reason I marked the other two patches as obsolete was because there's been a lot of work going on in the Cocoa widget stuff lately and they're very likely bitrotted.

Håkan, if you want to bring them up to snuff once the Camino-only stuff lands, be my guest.

cl
Attachment #216608 - Attachment is obsolete: true
Attachment #216698 - Flags: review?(mark)
Comment on attachment 216698 [details] [diff] [review]
fixes Mark's comments (checked in, trunk and 1.8)

I still think alternates are generally better where we can use 'em, but this ought to work.  Death to GetCurrentKeyModifiers.
Attachment #216698 - Flags: review?(mark) → review+
Comment on attachment 216698 [details] [diff] [review]
fixes Mark's comments (checked in, trunk and 1.8)

This patch has been checked in on the trunk and 1.8 branch.
Attachment #216698 - Attachment description: fixes Mark's comments → fixes Mark's comments (checked in, trunk and 1.8)
Assignee: hwaara → general
QA Contact: general
I think we should really change those other call sites -- this is causing a pretty annoying behavior with Safe Mode, where if you are holding down option in another app Firefox will start in Safe Mode, which requires you to quit and restart to get back to regular (unsafe?) mode.

Now that we are all Cocoa widgets, is it safe to call GetCurrentEvent with [NSApplication run] on the stack? We'd need to make sure that's the case, it might not be during startup.

Perhaps we can "fake" the behavior by also checking if we are the foreground application?

Nominating for blocking because of the really annoying behavior of Safe Mode activating while Firefox is starting up in the background.
Flags: blocking1.9?
So, this exists in Fx2, right?  Doesn't sound like this should block, but re-nom if you are dead set on blocking.
Flags: blocking1.9? → blocking1.9-
The behavior of holding down option key changed in 1.9. Before it used to bring up the profile manager, from which it was easy to continue on to your task. Not a big deal if it came up accidentally.

Now the Safe Mode dialog opens when you hold down option, and it's impossible to get to a normal browsing session from that dialog without quitting the browser and relaunching.

I was going to file a separate bug for changing that behavior, but gavin pointed me at this one. Maybe we want a separate bug afterall?
Flags: blocking1.9- → blocking1.9?
OK.  We should fix this as it's a generally bad user experience.  +'ing with a P2.  Josh, can you re-prioritize accordingly?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Priority: P2 → P3
Assignee: general → joshmoz
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
I did testing, looks like we can go ahead and make all of the conversions.
Attachment #306547 - Flags: review?(smichaud)
Josh, Apple's current doc at the URL from comment #0 says the
following:

> Note that GetCurrentEventButtonState and GetCurrentEventKeyModifiers
> do not return useful values if your application is not active. If
> your application wants to determine the current mouse or keyboard
> modifier state while in the background, it must query the hardware
> using GetCurrentButtonState or GetCurrentKeyModifiers.

Is this something we need to worry about?
Not that I know of. We read correctly on startup and whenever we send events to plugins, so I think we're fine. Those are the two cases that we're changing in my patch.
What I'm worried about is pathological cases like an event being sent
to a plugin just after the browser has switched to the background.
Apple's doc implies that GetCurrentKeyModifiers() will return garbage
when called in the background.
How would that happen? If the browser is in the background and the user takes action, the action shouldn't be fed to the plugin. If the browser is not in the background with the user takes action, the flow would be like this:

- action
- event generated for browser
- app backgrounds at some point
- event processed in queue, modifier used when it was generated is used

This seems correct to me. You either generated the event while the browser was active or not, either way it is correct.
Comment on attachment 306547 [details] [diff] [review]
convert all, v1.0

OK.  As long as you're sure GetCurrentKeyModifiers() will never be
called while the browser is backgrounded, then I have no problems with
this patch.
Attachment #306547 - Flags: review?(smichaud) → review+
Attachment #306547 - Flags: superreview?(roc)
Attachment #306547 - Flags: superreview?(roc) → superreview+
Attachment #306547 - Flags: approval1.9b4?
Comment on attachment 306547 [details] [diff] [review]
convert all, v1.0

Let's do this after beta 4.
Attachment #306547 - Flags: approval1.9b4?
Attachment #306547 - Flags: approval1.9b4-
Attachment #306547 - Flags: approval1.9?
Comment on attachment 306547 [details] [diff] [review]
convert all, v1.0

This is a blocker. No approval1.9 needed.
Attachment #306547 - Flags: approval1.9?
landed on trunk, resolving, we don't need to do more on the 1.8 branch
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Target Milestone: mozilla1.9beta4 → mozilla1.9
This patch didn't fix the bug.  Holding Alt while Firefox is starting in the background (e.g. from a python script that keeps restarting Firefox) can still make Firefox go into safe mode.  I'm using Tiger.
Filed bug 507782.
You need to log in before you can comment on or make changes to this bug.