From Apple's documentation: Smart Magnification There is a new NSEvent type for the smart zoom gesture (2-finger double tap on trackpads) along with a corresponding NSResponder method. In response to this event, you should intelligently magnify the content. NSEventTypeSmartMagnify - (void)smartMagnifyWithEvent:(NSEvent *)event; NSEventTypeSmartMagnify is limited to 64 bit only. Better yet, let NSScrollView perform the intelligence for you. In your NSScrollView document view, implement the following method to provide NSScrollView with semantic layout information of your content. /* Return the complete rect of the most appropriate content grouping at the specified location. For example, if your content is divided into three columns, return the entire rect of the column that contains the location. NSScrollView will attempt to magnify such that the smaller dimension fits inside the scroll view while remaining within the minMagnification, maxMagnification range. If your content layout is sub-divided further than one level deep (for example, two boxes that each contain multiple text boxes), then use the visibleRect parameter to determine when to provide the rect of a sub-grouping. Always return a rect for the appropriate grouping. If there is no deeper content grouping, return the rect for the deepest grouping. NSScrollView will determine when to pan, magnify in, and magnify out. Return NSZeroRect for the default behavior. */ - (NSRect)rectForSmartMagnificationAtPoint:(NSPoint)location inRect:(NSRect)visibleRect; https://developer.apple.com/library/mac/#releasenotes/Cocoa/AppKit.html#//apple_ref/doc/uid/TP30000741

Just so that people know: We don't use *any* native UI objects in Cocoa widgets (or, so far as I know, in the widgets implementations for any other platform). This is extremely unlikely to change. So we're extremely unlikely ever to use an NSScrollView object. We may be able to learn how to emulate its behavior wrt NSEventTypeSmartMagnify, presumably by reverse-engineering it. But that will probably be a lot of work -- just as the infamous Lion UI bugs have been (like bug 678392 and bug 636564).

This patch essentially undo's the patch for bug 851128, and then implements the "smartMagnifyWithEvent" NSResponder method per Apple's documentation. A link to that documentation was included in the comments beside the other link. This way is probably the best recognition we'll be able to get. It will be inherently on-par with other applications on OS X, because it will use the exact same method as the others. Additionally, any past and future versions of OS X which supported (will support) the double-tap gesture will work perfectly with this. I removed the eGestureState_TapGesture state, because it would never get used anymore, since smartMagnifyWithEvent allows us to consider the double-tap gesture as a completely atomic event (rather than considering it to be two two-finger taps). NOTE: I wasn't sure whether or not these lines were necessary in smartMagnifyWithEvent, lemme know if I should remove them: // Clear the gesture state mGestureState = eGestureState_None;

I'll review this more carefully on Monday. But here let me note that we need double-taps to work on SnowLeopard and Lion -- not just Mountain Lion.

As this gesture was not introduced on OS X until Lion (10.7) (and therefore certainly was not in any software products before Lion), I personally disagree about supporting it on Snow Leopard.

OK, then. Looks like I'll need to read up on the double-tap gesture on OS X before I can make an informed decision. Any suggestions where I should start? And does your current patch work on both Lion and Mountain Lion, or only on Mountain Lion?

I've had some trouble finding any meaningful documentation on the gesture itself as an OS X feature, but the above-linked documentation gives some information, as well as this article here: http://sanziro.com/2011/07/double-tap-to-auto-zoom-in-mac-os-x-lion.html Also, FWIW, the description implies that the smart-zoom maximizes an element on the screen. I'm not sure this is a very accurate description for most applications, which seem to just zoom in on the content in the same manner as pinch to zoom, but to a hard-coded depth. Unfortunately, we don't have access to a Lion machine, just Mountain Lion, but we'd love if others to test this. The release notes in the link above concern me that this method did not exist in Lion and therefore this patch will not work as-is.

My environment is Lion, so that I can test it. I feel something strange of your change. You try to implement a gesture event only for a system feature integration. However, the event name, MozTapGesture event, doesn't sound so. Additionally, you try to make the event work only on Lion/Mountain Lion. In other words, MozTapGesture is clearly meaning a user operation occurred, but it isn't fired on the platforms which don't support the feature. Additionally, you want the event just emulate the system feature. So, you make the event just a command from a user operation event. I think that you should NOT dispatch MozTapGesture event, you should dispatch a command event, isn't it? http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEvent.h#1664 If different result should happen between Windows and Mac OS X in the future, dispatching command event does make sense. Mac OS X widget should dispatch proper command with the event.

Doesn't this Tap event still need coordinates? Then I wouldn't use CommandEvent (which is also Gecko specific). To me using SimpleGestureEvent sounds ok.

Olli is correct, we do need the coordinates. The reasons, as I understand, that this is a gesture event are as follows: 1) It's being done with users' fingers on a trackpad specifically, it cannot be input by ordinary mouse movement or clicks, or by key presses. It is trackpad only. 2) It's not fundamentally different from magnify, swipe, or rotate, which are clearly gesture events, not command events--and couldn't even be implemented as command events 3) Windows does support MozTapGesture, but in a different way. If I'm not mistaken, on Windows, the Tap gesture is fired when two fingers tap together at the same time, only once. On Mac, it is a two-finger double-tap. These different gestures have the same effects in other applications, so, they can be considered the same gesture, but with different implementation on OS X vs Windows. We only discovered the Windows implementation of it very recently. 4) The reason we're only doing this on Lion and Mountain Lion is because this gesture did not "exist" so to speak before Lion, i.e. Apple released the recognition for the gesture as well as applications that used the gesture with Lion, not before. So users of pre-Lion OS X would not be using double-tap anyway, since none of their applications support it. 5) Finally, as mentioned by Olli, we need the coordinates.

As I expected, NSEventTypeSmartMagnify is only supported on OS X 10.8 and up. See https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSEvent_Class/Reference/Reference.html. I don't think it's acceptable for us to support double-tap gestures only on 10.8 and up. So we may end up having to use parts of the patch for bug 851128, or hacking up something else, to support double-tap gestures on Lion. As for supporting these gestures on SnowLeopard, I haven't yet done the requisite research. But that shouldn't take long. I'll have more to say on that later today.

Comment on attachment 740009 [details] [diff] [review] Proposed patch v1.0 Because this would only work on 10.8 and up.

We *may*, conceivably, decide that we *do* want to only support double-tap gestures on OS X 10.8 and up. But we shouldn't do that without at least looking for ways to make them work on Lion.

By running Safari in gdb on OS X 10.7.5, I discovered that a double-tap gets passed to Safari as a "Gesture" event via -[NSResponder magnifyWithEvent:]: Breakpoint 3, 0x0000000100269a8f in -[TabContentView magnifyWithEvent:] () (gdb) bt #0 0x0000000100269a8f in -[TabContentView magnifyWithEvent:] () #1 0x00007fff8a9d070d in -[NSObject performSelector:withObject:] () #2 0x00007fff92a1c8ba in forwardMethod () #3 0x00007fff92a17869 in -[NSWindow sendEvent:] () #4 0x00000001002b2c98 in -[Window sendEvent:] () #5 0x00000001000b1726 in -[BrowserWindow sendEvent:] () #6 0x00007fff92bf8594 in -[NSApplication _handleGestureEvent:] () #7 0x00007fff929b03a5 in -[NSApplication sendEvent:] () #8 0x00000001000510f7 in -[BrowserApplication sendEvent:] () #9 0x00007fff92946a0e in -[NSApplication run] () #10 0x00007fff92bc2eac in NSApplicationMain () #11 0x00000001002190aa in SafariMain () #12 0x0000000100000f2c in ?? () (gdb) p/a $rdi $1 = 0x10492de10 (gdb) po $1 <TabContentView: 0x10492de10> (gdb) p/a $rdx $2 = 0x102a95db0 (gdb) po $2 NSEvent: type=Gesture loc=(410.742,206.984) time=757.7 flags=0x100 win=0x104905b70 winNum=83 ctxt=0x0 subtype=16

Not surprisingly Safari works differently on OS X 10.8.3: Breakpoint 2, 0x000000010027a08b in -[TabContentView smartMagnifyWithEvent:] () (gdb) bt #0 0x000000010027a08b in -[TabContentView smartMagnifyWithEvent:] () #1 0x00007fff8513be47 in forwardMethod () #2 0x00007fff85104342 in -[NSWindow sendEvent:] () #3 0x00000001002c89a4 in -[Window sendEvent:] () #4 0x00000001000b5d2b in -[BrowserWindow sendEvent:] () #5 0x00007fff85269ef3 in -[NSApplication _handleGestureEvent:] () #6 0x00007fff850ff674 in -[NSApplication sendEvent:] () #7 0x0000000100050c35 in -[BrowserApplication sendEvent:] () #8 0x00007fff8501524a in -[NSApplication run] () #9 0x00007fff84fb9c06 in NSApplicationMain () #10 0x0000000100228554 in SafariMain () #11 0x00007fff845a67e1 in start () (gdb) po $rdi <TabContentView: 0x103a7e2f0> (gdb) po $rdx NSEvent: type=SmartMagnify loc=(504.199,237.363) time=210.6 flags=0x100 win=0x103a2f400 winNum=60 ctxt=0x0

Good info. If I had to guess, I'd say that subtype=16 might correspond to double-tap events, but I'm not sure without being able to try it out in more detail. I'm trying to obtain 10.6 and 10.7 for virtual machines from the university, I won't be able to get my hands on anything until tomorrow.

> As for supporting these gestures on SnowLeopard, I haven't yet done > the requisite research. But that shouldn't take long. I'll have > more to say on that later today. I'm satisfied Apple doesn't support the double-tap gesture on Snow Leopard. So we don't need to, either. 1) I couldn't find any double-tap setting in Snow Leopard's Trackpad control panel. 2) I tried double-tapping in Safari on Snow Leopard and nothing happened. 3) I grepped /System/Library (case-insensitive) for "doubletap" and found no matches except in the ScreenReader private framework. (By the way, the (apparently undocumented) .GlobalPreferences setting for the double-tap gesture on Lion and Mountain Lion is "com.apple.trackpad.twoFingerDoubleTapGesture".) Yes, Apple's documentation for new features is usually very skimpy -- but there are ways around that :-)

> I'd say that subtype=16 might correspond to double-tap events I agree that sounds likely. But you really do need to test. > I'm trying to obtain 10.6 and 10.7 for virtual machines from the university Glad to hear it! But don't bother with 10.6 (which is in fact a lot trickier to get running in a VM, since Apple doesn't want you to do that).

The only alternative I can think of for how 10.7 differentiates is possibly this (can't confirm without more testing): magnifyWithEvent's event argument would presumably have type NSEventTypeMagnify = 30 for pinch events. It does on 10.8, at least. However, if magnifyWithEvent's event argument has type NSEventTypeGesture = 29, that might mean double-tap, since normally magnifyWithEvent would have type NSEventTypeMagnify.

Well, in the process of attempting to install and use Lion, I've bricked my Apple trackpad and still don't have have Lion functioning, so I will not be able to test anything on Lion.

If you have to reinstall from scratch, and have a machine that's old enough to run both Lion and Mountain Lion, consider this: Create a big partition plus one or two other small partitions, each of which can be used to boot a different version of OS X. I do this on all my new machines. The "small" partitions don't need to be larger than about 40GB -- that's enough room for the OS and a build environment. If you build in (say) /usr/local/src/Mozilla in the "big" partition, you can share your builds among the different partitions by creating soft links to /usr/local/src/Mozilla in each of the "small" partitions.

If you can't create a setup (or setups) that allows you to test (and hopefully also to build) on both Lion and Mountain Lion, I'll probably have to take over this bug myself. But I'm very busy, and it may be some time (weeks or months) before I can get to it.

I tested this version of the patch out on both Lion (10.7) and Mountain Lion (10.8). I managed to get a dual-boot working. It recognizes double-taps on both Lion and Mountain Lion, despite the different NSResponder method use between the two OS's. Additionally, I tested it on the dragons page from bug 864053, and I could not detect any input lag with this patch. So, I think this patch fixes this bug and bug 851128, without causing the breakage from bug 864053 and 862417. Some background for any who may just now be tuning in: The patch for bug 851128 broke input on OS X, causing it to be very laggy from the trackpad. Ordinary mice worked normally. It has been backed out already, and from my testing, it seems backing out has fixed the input lag, as expected. In Mountain Lion, you can implement the NSResponder method "smartMagnifyWithEvent" to detect double-tap AKA smart zoom gestures. These gestures are atomic, i.e. they can be considered to happen at a fixed point in time. On neither Lion nor Mountain Lion will double taps trigger the beginGestureWithEvent / endGestureWithEvent NSResponder methods, so it is not necessary to change mGestureState. However, in Lion, double-taps trigger the magnifyWithEvent method, and smartMagnifyWithEvent is not used. However, in Lion, ordinary pinch/zoom events in magnifyWithEvent will have the event type NSEventTypeMagnify, whereas double-tap events will have the event type NSEventTypeGesture. Through some research, it has been found that the subtype is probably not meaningful; in any case, subtype is poorly documented if at all. So, for this patch, I chose to check the event type when magnifyWithEvent is called. If it is NSEventTypeGesture, the gesture was a double-tap gesture, and we should call smartMagnifyWithEvent and return, otherwise take the normal actions. Only on Lion will double-tap events with NSEventTypeGesture exist; Mountain Lion and up will use smartMagnifyWithEvent, which will always have event type NSEventTypeSmartMagnify. Steven mentioned, but I will reiterate: we do not need to support double-tap on Snow Leopard or before, because Apple did not support it, nor did any major Snow Leopard applications (e.g. Safari). See comment 16 for more information.

Comment on attachment 741032 [details] [diff] [review] Proposed Patch v 0.5 I'm going to be giving this a careful review today, and testing on both Lion and Mountain Lion. But I've already noticed a problem: The event you send using DispatchWindowEvent doesn't contain any coordinates.

(In reply to Steven Michaud from comment #23) > But I've already noticed a problem: The event you send using > DispatchWindowEvent doesn't contain any coordinates. The convertCocoaMouseEvent takes care of that. The coordinates of the click (from top-left of the content) are available as event.clientX and event.clientY, where "event" is the MozTapGesture Gecko event. I tested that and it worked as expected.

Oops, sorry. You're right. I stand corrected.

Comment on attachment 741032 [details] [diff] [review] Proposed Patch v 0.5 Exactly how much testing did you do on Lion, Brandon? I find your patch works fine on Mountain Lion, but not at all on Lion. It *should* work. But I suspect Apple is playing dirty tricks, and somehow preventing it from working in other apps than their own. Figuring out this kind of thing (by reverse engineering) is my specialty. So I'll spend a few more hours seeing what I can find. With luck, I'll be able to find a way to work around Apple's dirty tricks. I wish I didn't have to spend the time, but it seems I have no choice. Sigh. My r- is for this problem (not working on Lion). I also found a couple of other problems, for which I wouldn't normally have r-ed your patch: /* * XXX - The swipeWithEvent, beginGestureWithEvent, magnifyWithEvent, - * rotateWithEvent, and endGestureWithEvent methods are part of a - * PRIVATE interface exported by nsResponder and reverse-engineering - * was necessary to obtain the methods' prototypes. Thus, Apple may - * change the interface in the future without notice. + * smartMagnifyWithEvent, rotateWithEvent, and endGestureWithEvent methods + * are part of a PRIVATE interface exported by nsResponder and + * reverse-engineering was necessary to obtain the methods' prototypes. Thus, + * Apple may change the interface in the future without notice. * - * The prototypes were obtained from the following link: + * In OS X Mountain Lion and above, smart zoom gestures are implemented in + * smartMagnifyWithEvent. In OS X Lion, they are implemented in + * magnifyWithEvent. See inline comments for more info. + * + * The prototypes were obtained from the following links: * http://cocoadex.com/2008/02/nsevent-modifications-swipe-ro.html + * https://developer.apple.com/library/mac/#releasenotes/Cocoa/AppKit.html */ In fact these are all now documented in https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSResponder_Class/Reference/Reference.html, so we should drop most of this comment. Of course we should still say something about how magnifyWithEvent: is used for double-tap events on Lion. But we quite evidently don't have the full story yet, so we don't yet know exactly what to say. + /* + * In OS X 10.7.* (Lion), smart zoom events come through magnifyWithEvent, + * instead of smartMagnifyWithEvent. However, smart zoom events have type + * NSEventTypeGesture, whereas pinch zoom events have type + * NSEventTypeMagnify. So, use that to discriminate between the two. + * Smart zoom gestures do not call beginGestureWithEvent or + * endGestureWithEvent, so mGestureState is not changed. + */ + if ([anEvent type] == NSEventTypeGesture) { + [self smartMagnifyWithEvent: anEvent]; + return; + } This (or whatever we end up with) should be wrapped in an if block that guarantees it won't be used except on Lion. Something like: if (nsCocoaFeatures::OnLionOrLater() && !nsCocoaFeatures::OnMountainLionOrLater()) { }

Comment on attachment 741032 [details] [diff] [review] Proposed Patch v 0.5 As for the question of whether or not to use MozTapGesture, which Masayuki brought up in comment #7, I don't know what to say. I don't know enough about how Gecko (cross-platform code) uses MozTapGesture, or any of the "gesture" events. You'll need to work that out with Masayuki and Olli.

I did about 20 minutes testing on both with no problems. Please check your trackpad settings on Lion to ensure that you have smart zoom enabled. Also, we're using MozTapGesture because that's what we were told to use by Jaws and others. I can't remember where/when specifically.

> Please check your trackpad settings on Lion to ensure that you have smart zoom > enabled. I most certainly do. And it works in Safari. This afternoon I'll be digging around in Lion's innards to figure out what's going on.

I hope you weren't testing with your patch for bug 851128 still in place :-)

(In reply to Steven Michaud from comment #30) > I hope you weren't testing with your patch for bug 851128 still in place :-) Not in the slightest, I know how to use source control.

Oh and by the way, I've been testing on Lion at a very low level. I found that no double-tap events are delivered to -[ChildView magnifyWithEvent:], though they *are* delivered to Safari's -[TabContentView magnifyWithEvent:]. I also found that pinch events *are* delivered (on Lion) to -[ChildView magnifyWithEvent:], which we send to Gecko (though Gecko doesn't seem to do anything with them).

By the way, Brandon, what hardware are you testing on? I'm testing on a Retina MacBook Pro that can run either Lion or Mountain Lion. System Information calls it "Retina, mid-2012".

Turning off HiDPI mode (by setting gfx.hidpi.enabled to '-1') makes no difference on my machine: FF with your patch still doesn't receive any double-tap events at -[ChildView magnifyWithEvent:].

Hm, it's odd that it worked for me and not you. I was using a recent iMac with an Apple Magic Trackpad. Can't recall the specific version of Lion, it's probably whatever is most recent--it's whatever Apple gives when you do the software update. I can't get more detail until later today. If you'd like I can get dumps from GDB, also later today. What I did was check out mozilla-central, apply my patch that is here, and mach build. Everything worked that way.

Another question: Where did you build (on Lion or Mountain Lion)? And which SDK did you use (if any)? Even if you didn't specify any SDK, mach might have picked one. Do about:buildconfig to find out. I built on Lion with the 10.6 SDK (as I normally do since we still support 10.6).

Brandon, I've started a tryserver build with your patch. It'll be interesting to see what results you and I get from it. It should be ready in a few hours.

I've found that Brandon's patch works on Lion if I build on Lion with the 10.7 SDK. Presumably it'd also work if built on Mountain Lion with the 10.8 SDK (though I'll test that later). However, we definitely don't want to stop using the 10.6 SDK (which we currently do in all release and nightly builds) just to be able to fix this bug. I'll keep digging to see if I can find a way out of this dilemma.

I built it on Lion, and checking the build config, it read 10.6 SDK. I also just tested it again and it worked perfectly. Are you certain that you built with my patch correctly? This is the only thing left I can think of. Here's my GDB output, broken on nsChildView.mm:3214, after a double-tap gesture, in -[ChildView magnifyWithEvent:]: Breakpoint 1, -[ChildView magnifyWithEvent:] (self=0x10bbe3460, _cmd=0x7fff8c1f097f, anEvent=0x10c4bf060) at /Users/firefox/Firefox/mozilla-central/widget/cocoa/nsChildView.mm:3214 3214 if (!anEvent || !mGeckoChild) (gdb) po anEvent NSEvent: type=Gesture loc=(268.973,1055.28) time=332.2 flags=0x100 win=0x10af76d10 winNum=74 ctxt=0x0 subtype=16 Current language: auto; currently objective-c++ After I let it continue, here's what came up to the web console, I did this: window.addEventListener("MozTapGesture", function(event) { console.log("MozTapGesture, (" + event.clientX + ", " + event.clientY + ")"); }); (Need to enable dom.debug.propagate_gesture_events_through_content) Output from web console: MozTapGesture, (269, 175) Exactly as expected. This is on Lion 10.7.5.

My mistake, it may not be SDK 10.6. It's not listed in Nightly when I go to about:buildconfig.

> It's not listed in Nightly when I go to about:buildconfig. Then you didn't build with the 10.6 SDK. Try the tryserver build when it becomes available. I'm almost 100% sure those are built with the 10.6 SDK.

I would suggest splitting off a new bug to get double tap into Mountain Lion, since we know that works. After that, we can try to get Lion working. Getting something is better than nothing.

No need. I've thought of a simple fix that should work well. It needs a bit more testing. But if that works out I'll post it shortly.

Here's my tryserver build made with Brandon's current (v0.5) patch: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-57229ae57301/try-macosx64/firefox-23.0a1.en-US.mac.dmg As I expected it's built on Lion with the 10.6 SDK. Brandon, please try it out on Lion and let us know your results.

Here's an addition to your patch, Brandon, that makes it work on Lion even when built with the 10.6 SDK. The problem is that -[NSWindow sendEvent:] refuses to call [ChildView magnifyWithEvent:] with an NSEventTypeGesture event. The fix is very easy (though hacky). Note that I've made the fix only compile with the 10.6 SDK, and only run on Lion. That's exactly where we need it. And just because it *is* hacky, we don't want it running under other conditions, and conceivably causing trouble. Brandon, please incorporate this into the next version of your patch, along with my suggested changes from comment #26. Then we should (I think) be set to go. Masayuki, if you don't mind please review this partial patch. Otherwise I'll end up reviewing my own code when Brandon puts it in his patch. I've tested this in Lion and Mountain Lion on two different machines, and didn't see any trouble. I've started another tryserver build, of Brandon's v0.5 patch plus my partial patch, with some additional NSLog logging. It should be available in a few hours.

Here's the tryserver build made from my patch from comment #45 plus Brandon's v0.5 patch: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-eb7d002621eb/try-macosx64/firefox-23.0a1.en-US.mac.dmg

Comment on attachment 741541 [details] [diff] [review] Additions to Brandon's patch to get this working on Lion when built with 10.6 SDK >+- (void)sendEvent:(NSEvent *)anEvent >+{ >+ if (nsCocoaFeatures::OnLionOrLater() && >+ !nsCocoaFeatures::OnMountainLionOrLater()) { >+ if ([anEvent type] == NSEventTypeGesture) { I don't think that you need to separate these checks to 2 if statement. If you have a plan to add some other event types into this block, it's okay to keep this style. Otherwise, please union them for reducing the indentation. Otherwise, looks good to me.

> I don't think that you need to separate these checks to 2 if statement. Fair enough. > If you have a plan to add some other event types into this block ... I don't.

After some discussion with Markus Stange over email, we decided that we *should* also check the subtype when discriminating between pinch and double-tap. We can't find documentation for what the subtypes mean, but then, as he said, much of what we're doing with Cocoa isn't supported by documentation. One reason I had discounted it before was because I'd seen both 16 and 22 as subtypes for double-tap. Well, it was 0x16 == 22. So, Markus and I decided to use it and check that it's 0x16, as well as NSEventTypeGesture. This is the most restrictive, and he and I agreed that it's better to not trigger a double tap event when it should, than trigger it when it shouldn't. I'm going to update my patch with Steven's, and alter it to use Masayuki's comments (merging the if blocks), as well as to check the subtype. I'll also check the subtype in magnifyWithEvent, for extra safety (unless we think that's unnecessary). I'll test it out again on Lion and Mountain Lion. I'll try to get it built with the 10.6 SDK, but if I can't, I'll note that.

> we *should* also check the subtype when discriminating between pinch and double-tap Good idea. Apple's use of magnifyWithEvent: with NSEventTypeGesture is undocumented behavior, so the more carefully we circumscribe our use of it the better. And we don't have to worry that Apple's usage will change, since we're only doing this when running on Lion.

> I'll try to get it built with the 10.6 SDK To do this you may need to build with a mozconfig file, and not with mach. But whether or not you succeed in building with the 10.6 SDK, please test both of my tryserver builds and let us know your results.

I tried the first tryserver build, and as expected, the double-tap gesture did not work (since that build included my code, but not Steven's additions, and was 10.6 SDK). I tried the second tryserver build, and as expected, the double-tap gesture DID work. So, I think everything is going as expected. I'm gonna finish up a patch, test it on Lion and Mountain Lion, and go from there (including ensuring that it doesn't re-break input lag issues).

Combine's Steven's code that makes this work on 10.6 SDK, as well as Steven's feedback in comment #26. I tested this on Mountain Lion and it worked well, and on Lion (but NOT using 10.6 SDK), and it also worked. I also checked that it didn't re-introduce the input lag issues. I couldn't build with 10.6 SDK; my version of XCode is too recent to come with it and Apple makes it incredibly difficult to get old versions of anything. A few notes about changes I made: -As mentioned, included Steven's code, which should make it work on 10.6 SDK -Rewrote comments in a few places, per feedback -I changed the ordering of the conditions in the IF statements, both in nsChildView.mm and nsCocoaWindow.mm. Since Obj-C uses short-circuit evaluation, it's probably easiest and fastest to exclude the most events by checking their type and subtype first, before checking the current OS. If this is ugly, it's simple to change it back.

Comment on attachment 741939 [details] [diff] [review] Proposed Patch v 0.6 This looks fine to me. Before I r+ it, though, I'll test it in a build made with the 10.6 SDK.

If you're going to r+ it, wait for me to re-upload with patch headers (e.g. From, Date, Subject, etc.). If it works with 10.6 SDK, lemme know and I'll re-upload.

Comment on attachment 741939 [details] [diff] [review] Proposed Patch v 0.6 Still haven't done the testing. But after some thought I do have one nit: + if ([anEvent type] == NSEventTypeGesture && + [anEvent subtype] == 0x16 && + nsCocoaFeatures::OnLionOrLater() && + !nsCocoaFeatures::OnMountainLionOrLater()) { Wherever you do this, please check first for the OS version. As best I can tell, the use of [NSEvent subtype] is undocumented for NSEventTypeGesture events. So there's a risk that, at some point in the future, the OS will throw an NSInternalInconsistencyException when it's used. So we should first check that we're running on Lion before using it.

Comment on attachment 741939 [details] [diff] [review] Proposed Patch v 0.6 I'm ready to r+ this patch, once you've added the change I requested in comment #56. I did a 10.6 SDK build on Lion, and then tested it on Lion and Mountain Lion on two different machines. I didn't see any problems or unexpected behavior. For good measure I also tested on OS X 10.6.8, and didn't see any unexpected behavior there, either.

This version incorporates all of Steven's previous feedback (including comment #56). Also, the patch headers (From, Subject, Date, etc.) are included.

Comment on attachment 741982 [details] [diff] [review] Proposed patch v1.0 >diff --git a/widget/cocoa/nsChildView.mm b/widget/cocoa/nsChildView.mm >index e45828d..d0a13a1 100644 >--- a/widget/cocoa/nsChildView.mm >+++ b/widget/cocoa/nsChildView.mm >@@ -3204,16 +3205,34 @@ NSEvent* gLastDragMouseDownEvent = nil; > > - (void)magnifyWithEvent:(NSEvent *)anEvent > { > NS_OBJC_BEGIN_TRY_ABORT_BLOCK; > > if (!anEvent || !mGeckoChild) > return; > >+ /* >+ * In OS X 10.7.* (Lion), smart zoom events come through magnifyWithEvent, >+ * instead of smartMagnifyWithEvent. However, smart zoom events have type >+ * NSEventTypeGesture, subtype 0x16, whereas pinch zoom events have type >+ * NSEventTypeMagnify. So, use that to discriminate between the two. >+ * Smart zoom gestures do not call beginGestureWithEvent or >+ * endGestureWithEvent, so mGestureState is not changed. >+ * Documentation couldn't be found for the meaning of the subtype 0x16, but >+ * it will probably never change. See bug 863841. >+ */ >+ if (nsCocoaFeatures::OnLionOrLater() && >+ !nsCocoaFeatures::OnMountainLionOrLater() && >+ [anEvent type] == NSEventTypeGesture && >+ [anEvent subtype] == 0x16) { >+ [self smartMagnifyWithEvent: anEvent]; >+ return; >+ } Hmm, this |if| condition is duplicated in nsCocoaWindow too. Could you make a static method such as nsChildView::IsZoomGestureEvent(NSEvent* aEvent) or something? >@@ -3238,16 +3257,40 @@ NSEvent* gLastDragMouseDownEvent = nil; > mGeckoChild->DispatchWindowEvent(geckoEvent); > > // Keep track of the cumulative magnification for the final "magnify" event. > mCumulativeMagnification += deltaZ; > > NS_OBJC_END_TRY_ABORT_BLOCK; > } > >+- (void)smartMagnifyWithEvent:(NSEvent *)anEvent >+{ >+ NS_OBJC_BEGIN_TRY_ABORT_BLOCK; >+ >+ if (!anEvent || !mGeckoChild) >+ return; nit: if (!anEvent || !mGeckoChild) { return; }

> Hmm, this |if| condition is duplicated in nsCocoaWindow too. Could > you make a static method such as > nsChildView::IsZoomGestureEvent(NSEvent* aEvent) or something? I'm not sure it's worth it. It'd never be used anywhere else in the codebase (beyond where it's used now), and it'd need to called something like nsChildView::IsFakeDoubleTapGestureUsedByAppleOnLion(), since it's only needed to deal with Apple's hacky, undocumented implementation of this gesture on Lion.

The condition isn't easy to understand the meaning without the long comment. Therefore, I think that making a method is good for easy to read because its name will explain what's checked.

Fair enough.

This version incorporates Masayuki's feedback. Also, I re-requested review from Steven, because I still cannot build with the 10.6 SDK (need either Apple premium developer license or an old version of XCode, neither of which I can get, because Apple goes to great lengths to prevent you from getting old anything). I'd appreciate it if you (Steven) could test it on 10.6 SDK Lion. Thanks!

Comment on attachment 743109 [details] [diff] [review] Proposed patch v1.1 This looks fine to me. And I've tested a 10.6 SDK build on Lion -- it worked fine. By the way, I think you can get access to older versions of XCode and its various components for free, just by registering as an Apple Developer (https://developer.apple.com/register/index.action). Once you've done this, try logging into the Mac Dev Center (https://developer.apple.com/devcenter/mac/index.action).