base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam@chromium.org

Committed: https://crrev.com/07171a4c0cda43aa639f8b1b6ec28b46754e8ea5
Cr-Commit-Position: refs/heads/master@{#415359}

[Sidney San Martín, 2016-08-24 19:55:55]

Description was changed from

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
==========

to

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
==========

[Sidney San Martín, 2016-08-24 19:55:55]

sdy@chromium.org changed reviewers:
+ dglazkov+blink@chromium.org, mark@chromium.org

[Sidney San Martín, 2016-08-24 20:07:12]

mark@: Could you check for Mac-y-ness?
dglazkov@: Could you check for sanity in the Blink changes?

(Pass 1/2)

https://codereview.chromium.org/2271653006/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mac/VersionUtilMac.h (right):

https://codereview.chromium.org/2271653006/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mac/VersionUtilMac.h:16: PLATFORM_EXPORT int
MacOSXMinorVersion();
dglazkov@: These functions also exist in base/mac/mac_util.h, and the question
came up of whether it would be OK to just refer to them. I found this doc which
suggests no:
http://www.chromium.org/blink/directory-dependency-in-blink
…but just wanted to let you know in case that's out of date.

[Mark Mentovai, 2016-08-25 13:58:56]

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode140
base/mac/mac_util.h:140: static constexpr bool MinVersionIsGreater() {
DeploymentTargetIsGreater() or MinRuntimeVersionIsGreater()?

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode144
base/mac/mac_util.h:144: }
//  namespace

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode163
base/mac/mac_util.h:163: // Until we switch to an SDK that knows about 10.12.
I don’t think that the SDK we use for official builds even knows about 10.11.

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode172
base/mac/mac_util.h:172: inline bool IsOSLaterThanSierra_DontCallThis() {
Probaby should rename this IsLaterThanOS10_12_DontCallThis()

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.mm#newcod...
base/mac/mac_util.mm:453: int internal::MacOSXMinorVersion() {
Can we actually write it this way now? I thought we needed to wrap it in
namespace internal.

[Sidney San Martín, 2016-08-26 00:00:01]

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode140
base/mac/mac_util.h:140: static constexpr bool MinVersionIsGreater() {
On 2016/08/25 13:58:56, Mark Mentovai wrote:
> DeploymentTargetIsGreater() or MinRuntimeVersionIsGreater()?

I like DeploymentTargetIsGreater(), even though it's not called that on the
command line. Done.

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode144
base/mac/mac_util.h:144: }
On 2016/08/25 13:58:56, Mark Mentovai wrote:
> //  namespace

Done.

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode163
base/mac/mac_util.h:163: // Until we switch to an SDK that knows about 10.12.
On 2016/08/25 13:58:56, Mark Mentovai wrote:
> I don’t think that the SDK we use for official builds even knows about 10.11.

Done, let me know if I overlooked a more obvious way of doing it.

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.h#newcode172
base/mac/mac_util.h:172: inline bool IsOSLaterThanSierra_DontCallThis() {
On 2016/08/25 13:58:56, Mark Mentovai wrote:
> Probaby should rename this IsLaterThanOS10_12_DontCallThis()

Done, good catch :).

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

https://codereview.chromium.org/2271653006/diff/1/base/mac/mac_util.mm#newcod...
base/mac/mac_util.mm:453: int internal::MacOSXMinorVersion() {
On 2016/08/25 13:58:56, Mark Mentovai wrote:
> Can we actually write it this way now? I thought we needed to wrap it in
> namespace internal.

It's fine C++-wise, and I couldn't find anything about it in our style guide. If
you think it's weird I'd be happy to change it.

[Sidney San Martín, 2016-08-26 00:00:10]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 00:01:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 00:38:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 00:38:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mark Mentovai, 2016-08-26 15:27:27]

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:131: return DeploymentTargetIsGreater() ||
MacOSXMinorVersion() >= V;
This doesn’t short-circuit every case that it could.

(Hint: this looked suspicious because logically, you can’t implement both >= and
<= on the same underlying >.)

With DT = 10, OSVersion<10>::DeploymentTargetIsGreater() is constexpr false, so
OSVersion<10>::IsAtLeast() has to evaluate MacOSXMinorVersion(). But with DT =
10, OSVersion<10>::DeploymentTargetIsGreater() must be true, no need to evaluate
MacOSXMinorVersion().

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:151: #define DECLARE_IS_OS_FUNCS(V, ID)                     
                \
This defines them too, so DEFINE instead of DECLARE?

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:153: const auto IsAtLeastOS10_##V = internal::OSVersion<V,
ID>::IsAtLeast; \
We would not otherwise find it useful to implement IsAtLeastOS10_9 at all, but
not defining it is harder than defining it with the macros laid out as they are
here.

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:163: DECLARE_IS_OS_FUNCS(11, 0)
Since you need to do this #if thing, maybe you can simplify the logic above by
getting rid of the 0 special case and writing 101100 here.

Alternatively, fudge the whole thing by getting rid of the ID template parameter
and basing it all on V in the template:

V >= 10 ? 100000 + V * 100 : 1000 + V * 10;

and then having some static_asserts to check that (if defined) these macros are
really 1090, 101000, 101100.

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.mm#ne...
base/mac/mac_util.mm:453: int internal::MacOSXMinorVersion() {
I suppose it’s fine, I just can’t recall ever seeing this in our codebase.

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util_unitt...
File base/mac/mac_util_unittest.mm (right):

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util_unitt...
base/mac/mac_util_unittest.mm:148: if (minor == 9) {
Since you’re now defining the (pointless) IsAtLeastOS10_9, it should at least be
tested. And I guess IsAtMostOS10_12, too.

[Sidney San Martín, 2016-08-28 21:24:07]

Patchset #4 (id:60001) has been deleted

[Sidney San Martín, 2016-08-28 21:24:19]

Patchset #4 (id:80001) has been deleted

[Sidney San Martín, 2016-08-28 21:24:31]

Patchset #4 (id:100001) has been deleted

[Sidney San Martín, 2016-08-28 21:27:18]

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:131: return DeploymentTargetIsGreater() ||
MacOSXMinorVersion() >= V;
On 2016/08/26 15:27:26, Mark Mentovai wrote:
> This doesn’t short-circuit every case that it could.
> 
> (Hint: this looked suspicious because logically, you can’t implement both >=
and
> <= on the same underlying >.)
> 
> With DT = 10, OSVersion<10>::DeploymentTargetIsGreater() is constexpr false,
so
> OSVersion<10>::IsAtLeast() has to evaluate MacOSXMinorVersion(). But with DT =
> 10, OSVersion<10>::DeploymentTargetIsGreater() must be true, no need to
evaluate
> MacOSXMinorVersion().

Good catch, thanks. Let me know if this new patch is easier to reason about!

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:151: #define DECLARE_IS_OS_FUNCS(V, ID)                     
                \
On 2016/08/26 15:27:26, Mark Mentovai wrote:
> This defines them too, so DEFINE instead of DECLARE?

Done.

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:153: const auto IsAtLeastOS10_##V = internal::OSVersion<V,
ID>::IsAtLeast; \
On 2016/08/26 15:27:26, Mark Mentovai wrote:
> We would not otherwise find it useful to implement IsAtLeastOS10_9 at all, but
> not defining it is harder than defining it with the macros laid out as they
are
> here.

As discussed, I'll try to ditch the 10.9 versions completely.

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.h#new...
base/mac/mac_util.h:163: DECLARE_IS_OS_FUNCS(11, 0)
On 2016/08/26 15:27:26, Mark Mentovai wrote:
> Since you need to do this #if thing, maybe you can simplify the logic above by
> getting rid of the 0 special case and writing 101100 here.
> 
> Alternatively, fudge the whole thing by getting rid of the ID template
parameter
> and basing it all on V in the template:
> 
> V >= 10 ? 100000 + V * 100 : 1000 + V * 10;
> 
> and then having some static_asserts to check that (if defined) these macros
are
> really 1090, 101000, 101100.

Done. The static_asserts and surrounding #ifdef guards almost double the length,
so I'd still be a fan of moving them to a test.

An example of this is base/template_util_unittest.cc, which is just a bunch of
static_asserts for base/template_util.h. The static_asserts here should only
fail if Apple changes their constant scheme again, or retroactively changes the
existing constants, which seems unlikely enough to not care about catching in a
normal build.

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util.mm#ne...
base/mac/mac_util.mm:453: int internal::MacOSXMinorVersion() {
On 2016/08/26 15:27:26, Mark Mentovai wrote:
> I suppose it’s fine, I just can’t recall ever seeing this in our codebase.

Good point. Changed.

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util_unitt...
File base/mac/mac_util_unittest.mm (right):

https://codereview.chromium.org/2271653006/diff/40001/base/mac/mac_util_unitt...
base/mac/mac_util_unittest.mm:148: if (minor == 9) {
On 2016/08/26 15:27:26, Mark Mentovai wrote:
> Since you’re now defining the (pointless) IsAtLeastOS10_9, it should at least
be
> tested. And I guess IsAtMostOS10_12, too.

Done. (But hopefully I can get rid of *OS10_9().)

[Sidney San Martín, 2016-08-28 21:34:46]

The CQ bit was checked by sdy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-28 21:34:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-29 00:56:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 00:56:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Mark Mentovai, 2016-08-29 17:48:42]

LGTM

https://codereview.chromium.org/2271653006/diff/120001/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2271653006/diff/120001/base/mac/mac_util.h#ne...
base/mac/mac_util.h:126: #define DEFINE_IS_OS_FUNCS(V) _DEFINE_IS_OS_FUNCS(V,
OS_X_VERSION_ID(V))
I would put this after the #define _DEFINE_IS_OS_FUNCS, because
_DEFINE_IS_OS_FUNCS is meaningless to a top-to-bottom reader at this point. And
probably move the OS_X_VERSION_ID #define with DEFINE_IS_OS_FUNCS, since that’s
the point of first use.

https://codereview.chromium.org/2271653006/diff/120001/base/mac/mac_util.h#ne...
base/mac/mac_util.h:155: #ifdef MAC_OS_X_VERSION_10_10
You shouldn’t need this #ifdef because I think we guarantee the 10.10 SDK now.

[Sidney San Martín, 2016-08-29 18:34:52]

sdy@chromium.org changed reviewers:
+ jam@chromium.org

[Sidney San Martín, 2016-08-29 18:34:54]

Adding jam@ for the big rename.

https://codereview.chromium.org/2271653006/diff/120001/base/mac/mac_util.h
File base/mac/mac_util.h (right):

https://codereview.chromium.org/2271653006/diff/120001/base/mac/mac_util.h#ne...
base/mac/mac_util.h:126: #define DEFINE_IS_OS_FUNCS(V) _DEFINE_IS_OS_FUNCS(V,
OS_X_VERSION_ID(V))
On 2016/08/29 17:48:42, Mark Mentovai wrote:
> I would put this after the #define _DEFINE_IS_OS_FUNCS, because
> _DEFINE_IS_OS_FUNCS is meaningless to a top-to-bottom reader at this point.
And
> probably move the OS_X_VERSION_ID #define with DEFINE_IS_OS_FUNCS, since
that’s
> the point of first use.

Done.

https://codereview.chromium.org/2271653006/diff/120001/base/mac/mac_util.h#ne...
base/mac/mac_util.h:155: #ifdef MAC_OS_X_VERSION_10_10
On 2016/08/29 17:48:42, Mark Mentovai wrote:
> You shouldn’t need this #ifdef because I think we guarantee the 10.10 SDK now.

Done.

[dglazkov, 2016-08-29 21:02:40]

dglazkov@chromium.org changed reviewers:
+ haraken@chromium.org

[dglazkov, 2016-08-29 21:03:01]

+haraken to look over the Blink bits.

[haraken, 2016-08-29 21:05:37]

WebKit LGTM

[Sidney San Martín, 2016-08-29 21:43:06]

sdy@chromium.org changed required reviewers:
+ jam@chromium.org

[jam, 2016-08-30 16:34:08]

On 2016/08/29 18:34:54, Sidney San Martín wrote:
> Adding jam@ for the big rename.

per
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...,
"For widespread or refactoring changes that are mechanical or global in nature,
it is acceptable to TBR the OWNERS required for submission, as long as your
changes have had the appropriate level of review or general approval.  (For
example, if touching hundreds of files across the codebase, a thread on the
chromium-dev mailing list may be better than getting individual approval from
dozens of different OWNERS.)"

[Sidney San Martín, 2016-08-30 16:50:10]

Description was changed from

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
==========

to

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam
==========

[Sidney San Martín, 2016-08-30 16:50:10]

sdy@chromium.org changed reviewers:
- jam@chromium.org

[Sidney San Martín, 2016-08-30 16:50:10]

sdy@chromium.org changed required reviewers:
- jam@chromium.org

[Sidney San Martín, 2016-08-30 16:50:32]

Description was changed from

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam
==========

to

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam@chromium.org
==========

[Sidney San Martín, 2016-08-30 16:50:43]

The CQ bit was checked by sdy@chromium.org

[Sidney San Martín, 2016-08-30 16:50:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org
Link to the patchset: https://codereview.chromium.org/2271653006/#ps140001
(title: "Nits")

[commit-bot: I haz the power, 2016-08-30 16:51:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 18:22:19]

Description was changed from

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam@chromium.org
==========

to

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam@chromium.org
==========

[commit-bot: I haz the power, 2016-08-30 18:22:22]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-08-30 18:24:33]

Description was changed from

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam@chromium.org
==========

to

==========
base::mac::IsOSSierra() -> base::mac::IsOS10_12(), etc.

BUG=636093
TBR=jam@chromium.org

Committed: https://crrev.com/07171a4c0cda43aa639f8b1b6ec28b46754e8ea5
Cr-Commit-Position: refs/heads/master@{#415359}
==========

[commit-bot: I haz the power, 2016-08-30 18:24:34]

Patchset 5 (id:??) landed as
https://crrev.com/07171a4c0cda43aa639f8b1b6ec28b46754e8ea5
Cr-Commit-Position: refs/heads/master@{#415359}