1021309 - Add "mac-yosemite-theme" system metric

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

We will need a new system metric for OS X 10.10.

This is an untested patch that should implement the functionality. However, I had to add a pretty hacky method to check for Yosemite in nsCocoaFeatures. I'm not aware of a better approach, but if there is, I would be happy to change it.

I'll test this myself later on my 10.10 machine.

Comment 1

[André Reinald]

(In reply to Josiah Bruner [:JosiahOne] from comment #0)
> Created attachment 8435313 [details] [diff] [review]
> Patch.

Though I'm not qualified to review the whole patch, you should
use [pInfo release] instead of [pInfo dealloc]
in nsCocoaFeatures::OnYosemiteOrLater()

Comment 2

[Markus Stange [:mstange]]

From nsCocoaFeatures.mm:
// This API will not work for OS X 10.10, see Gestalt.h.
I think this comment does not apply since we are not using Gestalt any more. GetSystemVersion parses a version string nowadays. So I think you should just be able to do this:

#define MAC_OS_X_VERSION_10_10_HEX 0x000010A0
...
  return (OSXVersion() >= MAC_OS_X_VERSION_10_10_HEX);

(and remove the misleading comment)

I can't test this at the moment because I don't have access to Yosemite yet.

Comment 3

[André Reinald]

(In reply to Markus Stange [:mstange] from comment #2)
> GetSystemVersion parses a version string nowadays. So I think you should
> just be able to do this:
> ...

Agree with you. The plist parsing was a trick I found in some sample code. But it was nowhere guaranteed to work through future versions. If it works in Yosemite, it would sure be preferable to keep using it.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8435313 [details] [diff] [review]
Patch.

> Apple's docs claim operatingSystemVersionString should not be parsed.

Which docs, where?

Does Apple now have an official way to check the OS version that doesn't involve using Gestalt?  It didn't the last time I checked.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> The plist parsing was a trick I found in some sample code. But it
> was nowhere guaranteed to work through future versions. If it works
> in Yosemite, it would sure be preferable to keep using it.

I agree with André.

Apple has botched OS X versioning almost unforgivably badly.  Anything
they say about it should be taken with a boulder of salt.

Comment 6

[Josiah Bruner [:jsbruner]]

(In reply to Steven Michaud from comment #4)
> Comment on attachment 8435313 [details] [diff] [review]
> Patch.
> 
> > Apple's docs claim operatingSystemVersionString should not be parsed.
> 
> Which docs, where?

https://developer.apple.com/library/ios/documentation/Cocoa/Reference/Foundation/Classes/NSProcessInfo_Class/Reference/Reference.html#//apple_ref/occ/instm/NSProcessInfo/operatingSystemVersionString

> 
> Does Apple now have an official way to check the OS version that doesn't
> involve using Gestalt?  It didn't the last time I checked.

10.10 introduced

- (BOOL)isOperatingSystemAtLeastVersion:(NSOperatingSystemVersion)version

and a new NSOperatingSystemVersion object, but of course, you must build with the 10.10 SDK to use that.

Comment 7

[Josiah Bruner [:jsbruner]]

Err. Excuse me. I meant the mac one of course: https://developer.apple.com/library/mac/documentation/Cocoa/Reference/Foundation/Classes/NSProcessInfo_Class/Reference/Reference.html#//apple_ref/occ/instm/NSProcessInfo/operatingSystemVersionString

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Thanks for the info, Josiah.

So we'll eventually be able to use isOperatingSystemAtLeastVersion:.  But for now let's stick to what we already have -- let's keep using GetSystemVersion() in nsCocoaFeatures.mm, presuming it still works on Yosemite.

Comment 9

[Josiah Bruner [:jsbruner]]

Attachment: Add metric

I still need to test this, as I've been extremely busy this past week, but here's the patch that should work.

Comment 10

[Tim Nguyen :ntim]

I think you should take advantage of the current -moz-os-version media query we have. The implementation is only for Windows for now. But it'd be nice if you could extend it.

Comment 11

[Josiah Bruner [:jsbruner]]

Comment on attachment 8439988 [details] [diff] [review]
Add metric

Review of attachment 8439988 [details] [diff] [review]:
-----------------------------------------------------------------

I just tested this patch, and it works properly, so requesting review.

-moz-os-version seems like a fine replacement, but I personally would rather add that in a separate bug, as we'd need to migrate all our -moz-mac-lion-theme queries as well, but wouldn't mind doing the patch for that bug.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8439988 [details] [diff] [review]
Add metric

Great, land away!

Comment 13

[Josiah Bruner [:jsbruner]]

Attachment: Patch.

Had some trailing white space.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Can you please at least run a set of OSX builds on Try for sanity's sake? Thanks :)

Comment 15

[Josiah Bruner [:jsbruner]]

Try Builds: https://tbpl.mozilla.org/?tree=Try&rev=5766157476b3

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Thanks :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf664c3735f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aaf664c3735f

Comment 18

[Robert Kaiser]

Funnily enough we added a "OS version" media query in bug 810399 but we are adding another OS-version-specific one here :)

Comment 19

[Tim Nguyen :ntim]

(In reply to Robert Kaiser (:[e-mail]) from comment #18)
> Funnily enough we added a "OS version" media query in bug 810399 but we are
> adding another OS-version-specific one here :)

You can file a new bug about it, so we can convert all our current OS versions ones to -moz-os-version :)

Comment 20

[Josiah Bruner [:jsbruner]]

(In reply to Tim Nguyen [:ntim] from comment #19)
> (In reply to Robert Kaiser (:[e-mail]) from comment #18)
> > Funnily enough we added a "OS version" media query in bug 810399 but we are
> > adding another OS-version-specific one here :)
> 
> You can file a new bug about it, so we can convert all our current OS
> versions ones to -moz-os-version :)

That's bug 1033436, which I'm assigned too, but probably won't get to for several weeks, so if someone wants to do it sooner that would be appreciated.

Comment 21

[Robert Kaiser]

(In reply to Josiah Bruner [:JosiahOne] from comment #20)
> That's bug 1033436, which I'm assigned too, but probably won't get to for
> several weeks, so if someone wants to do it sooner that would be appreciated.

Ah, awesome, I just stumbled over all this when looking for the apparently not-yet-existing solution to bug 1060941. ;-)