We have multiple instances of Gestalt calls to check for OS X version number. Most of these calls are not strictly correct i.e. they need to mask out the upper bits, and maintaining this code will become difficult if OS X hits version 10.10+ Part of the patch from bug 668953 has the version check in a C++ compatible header we can use to refactor the version check. We should refactor these to this single common location.

On Mac OS X 10.8 Gestalt is deprecated. If I build on 10.8 with the 10.8 SDK I get a lot of warnings in the kind: /mozilla/netwerk/protocol/http/nsHttpHandler.cpp:778:12: warning: 'Gestalt' is deprecated: first deprecated in Mac OS X 10.8 [-Wdeprecated-declarations] if ((::Gestalt(gestaltSystemVersionMajor, &majorVersion) == noErr) && ^ /Developer/SDKs/MacOSX10.8.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/Headers/Gestalt.h:123:1: note: 'Gestalt' declared here Gestalt( ^

You should file a different bug for that.

Here's a list of all our instance of Gestalt: http://mxr.mozilla.org/mozilla-central/ident?i=Gestalt&filter= Most of these are used to check the version of OSX. For now the right thing to do is probably to have everything use nsCocoaFeature for version detection: http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaFeatures.h#14 This will also mean that we can replace Gestalt since it's deprecated on 10.8.

(In reply to Benoit Girard (:BenWa) from comment #3) > This will also mean that we can replace Gestalt since it's deprecated on > 10.8. Oh, does this mean, this will fix Bug 772179 than?

I would like to take up this bug if it still requires somebody. I have done some reading here, and think I understand what is required. Should I begin working on this using this list of Gestalt instances as a reference http://mxr.mozilla.org/mozilla-central/ident?i=Gestalt&filter= ?

(In reply to Nomis101 from comment #4) > (In reply to Benoit Girard (:BenWa) from comment #3) > > This will also mean that we can replace Gestalt since it's deprecated on > > 10.8. > Oh, does this mean, this will fix Bug 772179 than? Bug 772179 was fixed already: internally, the logic to detect system version changed from using Gestalt to reading the appropriate plist in the System folder. (In reply to Alex Moss from comment #5) > I would like to take up this bug if it still requires somebody. I have done > some reading here, and think I understand what is required. > > Should I begin working on this using this list of Gestalt instances as a > reference > http://mxr.mozilla.org/mozilla-central/ident?i=Gestalt&filter= > ? As :BenWa says in comment 3, we should replace calls to Gestalt with calls to the right nsCocoaFeatures (static) methods. If necessary, it could make sense to add methods to that class in case we have to check for other runtime specific conditions.

Hello, Can I pick this bug? I really would like to help.

(In reply to gui.iscaro from comment #7) > Hello, > Can I pick this bug? I really would like to help. Yes, let me know if you need any help.

I have made progress on this issue. Few questions: 1) Is Firefox supported on OSX versions before 10.6? nsCocoaFeatures.h assumes 10.6 so the class functions will return 10.6 even if the real version is older. 2) Cocoa API will not work in 10.10 (from comment in nsCocoaFeatures.mm). Does this mean my changes will need to be updated again for 10.10 in the future? Thanks

I think it refers to calling Gestalt. At the time I wrote this it wasn't clear what would happen. Now we should read the documentation and see what the proper way of checking the OSX version in later versions of OS X. We no longer support 10.5 or older. We should return support for 10.6 unconditionally. That way if builds on newer versions of OSX fail to call Gestalt they will fallback to 10.6 behavior and not 10.5 behavior.

I've replaced the calls to Gestalt except in MacQuirks.h and AppFileLocationProvider.cpp (where I put comments/questions).

Comment on attachment 8344189 [details] [diff] [review] 676907.patch Review of attachment 8344189 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just a few minor edits. ::: gfx/cairo/cairo/src/cairo-quartz-surface.c @@ +180,5 @@ > CGContextGetAlphaPtr = dlsym(RTLD_DEFAULT, "CGContextGetAlpha"); > > CTFontDrawGlyphsPtr = dlsym(RTLD_DEFAULT, "CTFontDrawGlyphs"); > > + _cairo_quartz_osx_version = nsCocoaFeatures::OSXVersion(); Cairo is complicated to patch because it's an external project. We should spin this off to a different bug and deal with this separately. ::: gfx/gl/GLContext.cpp @@ +1044,5 @@ > // for good measure, we align renderbuffers on what we do for 2D textures > mMaxRenderbufferSize = std::min(mMaxRenderbufferSize, 4096); > mNeedsTextureSizeChecks = true; > } else if (mVendor == VendorNVIDIA) { > + if (nsCocoaFeatures::OnMountainLionOrLater()) { Please set your editor to 'tab as space' and 'tabs = 2 space'. Note that some files like this one will use 4 space indents instead of 2. Is nsCocoaFeatures included here? We shouldn't really on build unification for getting the header indirectly. ::: gfx/thebes/gfxPlatformMac.cpp @@ +21,5 @@ > #include "gfx2DGlue.h" > > #include <dlfcn.h> > > +#if defined(MOZ_WIDGET_COCOA) No need to ifdef here. @@ +338,5 @@ > gfxPlatformMac::OSXVersion() > { > if (!mOSXVersion) { > // minor version is not accurate, use gestaltSystemVersionMajor, gestaltSystemVersionMinor, gestaltSystemVersionBugFix for these > + // OSXVersion() will log errors about unsupported versions. Both comments here can be removed. ::: ipc/chromium/src/base/sys_info_mac.cc @@ +15,5 @@ > int32_t *bugfix_version) { > + // We don't need to cache anymore since nsCocoaFeatures does the caching. > + *major_version = nsCocoaFeatures::OSXVersionMajor(); > + *minor_version = nsCocoaFeatures::OSXVersionMinor(); > + *bugfix_version = nsCocoaFeatures::OSXVersionBugFix(); This is also code from an upstream project. Let's leave it for now. ::: toolkit/crashreporter/google-breakpad/src/client/mac/handler/dynamic_images.cc @@ +373,2 @@ > static SInt32 GetOSVersion() { > + return nsCocoaFeatures::OSXVersion(); google-breakpad is an external project, we should avoid patching this one too. ::: toolkit/xre/MacQuirks.h @@ +237,5 @@ > #ifdef __i386__ > ProcessSerialNumber psn; > ::GetCurrentProcess(&psn); > #else > + // Seems this code is not needed? It's a work around. Please leave it there. ::: xpcom/io/nsAppFileLocationProvider.cpp @@ +572,5 @@ > static SInt32 version = 0; > > if (!version) { > + // Should I change this call to use nsCocoaFeatures? Or should this function > + // always return true since we don't support 10.5 anymore? // 10.5 or lower is no longer supported. return true; We can leave it up to the owners of this code to remove that function.

smichaud do you know what will happen to Gestalt in the next release of OS X? We're leaving a few instances of the call in upstream project and I wonder if we need to look into fixing these soon?

Made suggested changes. In GLContext.cpp there is an existing include of nsCocoaFeatures.h.

Comment on attachment 8344236 [details] [diff] [review] 676907.patch Review of attachment 8344236 [details] [diff] [review]: ----------------------------------------------------------------- Still a few more tabs. Try to match the indent of the file. ::: gfx/thebes/gfxPlatformMac.cpp @@ +335,5 @@ > int32_t > gfxPlatformMac::OSXVersion() > { > if (!mOSXVersion) { > + mOSXVersion = nsCocoaFeatures::OSXVersion(); There's a tab here. ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +745,5 @@ > mOscpu.AssignLiteral("Intel Mac OS X"); > #endif > + SInt32 majorVersion = nsCocoaFeatures::OSXVersionMajor(); > + SInt32 minorVersion = nsCocoaFeatures::OSXVersionMinor(); > + mOscpu += nsPrintfCString(" %d.%d", majorVersion, minorVersion); tabs

Removed tabs.

Comment on attachment 8344239 [details] [diff] [review] 676907.patch Review of attachment 8344239 [details] [diff] [review]: ----------------------------------------------------------------- Ok once this is fixed I'll push it to try and we can begin landing it. We have to restore Gestalt otherwise it will break 10.7 macs that need to quirks triggered. ::: gfx/thebes/gfxPlatformMac.cpp @@ +335,5 @@ > int32_t > gfxPlatformMac::OSXVersion() > { > if (!mOSXVersion) { > + mOSXVersion = nsCocoaFeatures::OSXVersion(); Optional: Can you turn this into return nsCocoaFeatures::OSXVersion and remove mOSXVersion from gfxPlatformMac? ::: toolkit/xre/MacQuirks.h @@ +238,5 @@ > #ifdef __i386__ > ProcessSerialNumber psn; > ::GetCurrentProcess(&psn); > #else > + SInt32 major = nsCocoaFeatures::OSXVersionMajor(); Sorry this needs to remain Gestalt. This is a crazy work around where we must call Gestalt.

Reverted MacQuirks.h changes and made suggested refactoring.

Thanks! Waiting on the results: https://tbpl.mozilla.org/?tree=Try&rev=79682e1e92e8

> smichaud do you know what will happen to Gestalt in the next release of OS X? I have no idea. Apple moves in mysterious ways :-( As of bug 772179 comment #3 (2013-03-07) Apple still didn't have an official replacement for Gestalt. As long as this remains the case, I'm pretty sure Apple will never get rid of it.

(In reply to Benoit Girard (:BenWa) from comment #19) > Thanks! > > Waiting on the results: > https://tbpl.mozilla.org/?tree=Try&rev=79682e1e92e8 Any ideas why the build failed? It is complaining about ManifestParser.cpp, but I can't see what the problem is. Maybe the include guard is wrong?

Now that the 'if' statement is gone, there's a closing '}' brace that should be removed at line 1.39 here: https://hg.mozilla.org/try/diff/79682e1e92e8/xpcom/components/ManifestParser.cpp#l1.39

Would be great to fix the alignment at the same time. :-)

(In reply to Steven Michaud from comment #20) > > smichaud do you know what will happen to Gestalt in the next release of OS X? > > I have no idea. Apple moves in mysterious ways :-( > > As of bug 772179 comment #3 (2013-03-07) Apple still didn't have an official > replacement for Gestalt. As long as this remains the case, I'm pretty sure > Apple will never get rid of it. Alright well in that case merging all our uses into a single unit is preferable so doing this is a good idea.

I removed the extraneous brace. Could you rerun the test job? Thanks.

I forgot to make InitializeVersionNumbers() static in nsCocoaFeatures.h. I'm hoping this patch should finally compile.

Comment on attachment 8345052 [details] [diff] [review] 676907.patch Review of attachment 8345052 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsCocoaFeatures.mm @@ +74,5 @@ > + else { > + mOSXVersion = 0x1000 + (minor << 4); > + } > + > + NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(0); This should be NS_OBJC_END_TRY_ABORT_BLOCK_RETURN() for void return type.

(In reply to Stephen Pohl [:spohl] from comment #29) > This should be NS_OBJC_END_TRY_ABORT_BLOCK_RETURN() for void return type. Looks like NS_OBJC_END_TRY_ABORT_BLOCK is actually preferred, as it appears more frequently in the code base.

Fixed issue from Stephen's comments.

Comment on attachment 8345304 [details] [diff] [review] 676907.patch Review of attachment 8345304 [details] [diff] [review]: ----------------------------------------------------------------- I still see some trailing white space and 4 spaces instead of 2 in this patch, but I'll let BenWa speak about those things. ::: widget/cocoa/nsCocoaFeatures.mm @@ +48,5 @@ > > +/*static*/ void > +nsCocoaFeatures::InitializeVersionNumbers() > +{ > + NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN; Sorry, my comment probably wasn't clear enough. You would also want to change this to NS_OBJC_BEGIN_TRY_ABORT_BLOCK to match NS_OBJC_END_TRY_ABORT_BLOCK at the end of the function.

Changed the macro and cleaned up spacing in nsCocoaFeatures.mm

Comment on attachment 8345396 [details] [diff] [review] 676907.patch Review of attachment 8345396 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsCocoaFeatures.mm @@ +57,5 @@ > + mOSXVersionMajor = major; > + mOSXVersionMinor = minor; > + mOSXVersionBugFix = bugfix; > + > + if (major != 10) { We shouldn't reject > 10. Switch this to be < 10. Can you fix the trailing whitespace (line 53, 77) in this file and put the } and else in the same line.

Fixed condition and trailing whitespace and if/else formatting.

Finally success :)

Opps, forgot to land this last week.