Bug 731878 breaks SDK 10.5 build

RESOLVED FIXED in mozilla15

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: m_kato, Assigned: masayuki)

Tracking

Trunk
mozilla15
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Now, thunderbird trunk is burning due to landing bug 731878.  Is this API from 10.6?


http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1335491470.1335491837.8282.gz#err0

/builds/slave/macosx-comm-central-bloat/build/mozilla/widget/cocoa/nsChildView.mm:3963: warning: 'NSEvent' may not respond to '+pressedMouseButtons'
/builds/slave/macosx-comm-central-bloat/build/mozilla/widget/cocoa/nsChildView.mm:3963: warning: (Messages without a matching method signature
/builds/slave/macosx-comm-central-bloat/build/mozilla/widget/cocoa/nsChildView.mm:3963: warning: will be assumed to return 'id' and accept
/builds/slave/macosx-comm-central-bloat/build/mozilla/widget/cocoa/nsChildView.mm:3963: warning: '...' as arguments.)
/builds/slave/macosx-comm-central-bloat/build/mozilla/widget/cocoa/nsChildView.mm:3963: error: invalid conversion from 'objc_object*' to 'NSUInteger'
Yes. The API was introduced on 10.6.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 4/29 - 5/6) from comment #1)
> Yes. The API was introduced on 10.6.

I think you'll want to add something like this to the header then.

> #if !defined(MAC_OS_X_VERSION_10_6) || \
>     MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_6
> @interface NSEvent (SnowLeopardEventFeatures)
> + (NSUinteger)pressedMouseButtons;
> @end
> #endif

I've also been told in the past to use [foo respondsToSelector:whatever] instead of OnVersionOrLater (feature detect instead of version check), but I think we're pretty mixed in there and that wouldn't be the issue.
The Thunderbird builders are going to be upgrading to 10.7 with an 10.6 sdk real soon (like next couple of weeks all being well), so I don't mind too much if this is broken there.

Might want to consider if we're breaking developers using the 10.5 sdk though (I can't remember what has been said about sdk requirements).
Created attachment 619003 [details] [diff] [review]
Patch

zpao:

Thank you, it works fine!
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #619003 - Flags: review?(smichaud)
Comment on attachment 619003 [details] [diff] [review]
Patch

Glad it's working! We do that sort of thing in a few other places as well. And on that note, we've kept other instances of this pattern to header files so my gut tells me we should do the same here, but I haven't spent that much time in this code. Steven will correct me if I'm wrong though!
Attachment #619003 - Flags: feedback-
Attachment #619003 - Flags: review?(smichaud) → review+
I read Paul's comment (comment #5) after I did the review :-(

Your patch works fine, Masayuki.  But now I also think it might be better to put it into a header file.  For example there's an NSEvent (Undocumented) category in nsChildView.h to which a definition of +[NSEvent pressedMouseButtons] might be added, with or without the version-specific ifdefs.

It's not a big deal.  Just a matter of style.

The definition of +[NSEvent pressedMouseButtons] is (of course) needed when building on versions of the OS (or the SDK) that don't define it, so that you don't get compile errors (or warnings).  You still also need to only call this method on versions of the OS that support it.
Created attachment 619235 [details] [diff] [review]
Patch
Attachment #619003 - Attachment is obsolete: true
Attachment #619235 - Flags: review?(smichaud)
Comment on attachment 619235 [details] [diff] [review]
Patch

Looks fine to me.
Attachment #619235 - Flags: review?(smichaud) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/baa40113b2d2

Thank you. And sorry for my fault.
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/baa40113b2d2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.