Closed Bug 1021309 Opened 6 years ago Closed 6 years ago

Add "mac-yosemite-theme" system metric

Categories

(Core :: CSS Parsing and Computation, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jsbruner, Assigned: jsbruner)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch. (obsolete) — Splinter Review
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.
Assignee: nobody → josiah
Status: NEW → ASSIGNED
Blocks: 1019587
(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()
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.
(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 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.
> 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.
(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.
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.
Attached patch Add metric (obsolete) — Splinter Review
I still need to test this, as I've been extremely busy this past week, but here's the patch that should work.
Attachment #8435313 - Attachment is obsolete: true
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 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.
Attachment #8439988 - Flags: review?(mstange)
Comment on attachment 8439988 [details] [diff] [review]
Add metric

Great, land away!
Attachment #8439988 - Flags: review?(mstange) → review+
Attached patch Patch.Splinter Review
Had some trailing white space.
Attachment #8439988 - Attachment is obsolete: true
Attachment #8440934 - Flags: review+
Can you please at least run a set of OSX builds on Try for sanity's sake? Thanks :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aaf664c3735f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Funnily enough we added a "OS version" media query in bug 810399 but we are adding another OS-version-specific one here :)
(In reply to Robert Kaiser (:kairo@mozilla.com) 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 :)
(In reply to Tim Nguyen [:ntim] from comment #19)
> (In reply to Robert Kaiser (:kairo@mozilla.com) 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.
(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. ;-)
You need to log in before you can comment on or make changes to this bug.