Delete IsAtLeastOS10_9() and IsAtMostOS10_9()

Delete IsAtLeastOS10_9() and IsAtMostOS10_9()

…and refactor where it's more appropriate to use *OS10_10().

BUG=636093

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

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

[Sidney San Martín, 2016-08-28 21:50:25]

Seem reasonable?

[Sidney San Martín, 2016-08-29 01:03:38]

Description was changed from

==========
Delete IsOS10_9(), IsAtLeastOS10_9, and IsAtMostOS10_9.

BUG=
==========

to

==========
Delete IsOS10_9(), IsAtLeastOS10_9, and IsAtMostOS10_9.

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

[Mark Mentovai, 2016-08-29 17:57:56]

Yup, seems reasonable.

https://codereview.chromium.org/2288003002/diff/1/base/mac/mac_util_unittest.mm
File base/mac/mac_util_unittest.mm (left):

https://codereview.chromium.org/2288003002/diff/1/base/mac/mac_util_unittest....
base/mac/mac_util_unittest.mm:150: EXPECT_TRUE(IsAtMostOS10_9());
CL description: be consistent. If you use a () on use, use () on all three. (Or
on none. Your choice!)

https://codereview.chromium.org/2288003002/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2288003002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1117: if
(base::mac::IsAtLeastOS10_10())
I think that this kind of thing really was 10.9-specific (like not needed on
10.8 and not needed on 10.10), but since we don’t support those older OSes, I
guess it’s OK to fudge it.

https://codereview.chromium.org/2288003002/diff/1/sandbox/mac/os_compatibilit...
File sandbox/mac/os_compatibility.cc (right):

https://codereview.chromium.org/2288003002/diff/1/sandbox/mac/os_compatibilit...
sandbox/mac/os_compatibility.cc:185: return base::WrapUnique(new
OSCompatibility_10_10());
Keeping things affirmative. Good call. Stay positive!

https://codereview.chromium.org/2288003002/diff/1/ui/gl/test/gl_image_test_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/2288003002/diff/1/ui/gl/test/gl_image_test_te...
ui/gl/test/gl_image_test_template.h:228: if (!base::mac::IsAtLeastOS10_10())
Considering the very next line, you should unify this check and the one that
follows into a single !IsAtLeastOS10_11() check.

https://codereview.chromium.org/2288003002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/2288003002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:247:
paint.setColor(kMenuPopupBackgroundColor);
Looks like you flipped the “then” and “else” sides here. Drop the
not/exclamation point?

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

Description was changed from

==========
Delete IsOS10_9(), IsAtLeastOS10_9, and IsAtMostOS10_9.

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

to

==========
Delete IsOS10_9(), IsAtLeastOS10_9(), and IsAtMostOS10_9().

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

[Sidney San Martín, 2016-08-29 19:26:13]

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

[Sidney San Martín, 2016-08-29 19:26:14]

+thakis@ to cover the rest of the files.

https://codereview.chromium.org/2288003002/diff/1/base/mac/mac_util_unittest.mm
File base/mac/mac_util_unittest.mm (left):

https://codereview.chromium.org/2288003002/diff/1/base/mac/mac_util_unittest....
base/mac/mac_util_unittest.mm:150: EXPECT_TRUE(IsAtMostOS10_9());
On 2016/08/29 17:57:56, Mark Mentovai wrote:
> CL description: be consistent. If you use a () on use, use () on all three.
(Or
> on none. Your choice!)

Done. I don't think it was on purpose, not sure why I did that.

https://codereview.chromium.org/2288003002/diff/1/ui/gl/test/gl_image_test_te...
File ui/gl/test/gl_image_test_template.h (right):

https://codereview.chromium.org/2288003002/diff/1/ui/gl/test/gl_image_test_te...
ui/gl/test/gl_image_test_template.h:228: if (!base::mac::IsAtLeastOS10_10())
On 2016/08/29 17:57:56, Mark Mentovai wrote:
> Considering the very next line, you should unify this check and the one that
> follows into a single !IsAtLeastOS10_11() check.

Done.

https://codereview.chromium.org/2288003002/diff/1/ui/native_theme/native_them...
File ui/native_theme/native_theme_mac.mm (right):

https://codereview.chromium.org/2288003002/diff/1/ui/native_theme/native_them...
ui/native_theme/native_theme_mac.mm:247:
paint.setColor(kMenuPopupBackgroundColor);
On 2016/08/29 17:57:56, Mark Mentovai wrote:
> Looks like you flipped the “then” and “else” sides here. Drop the
> not/exclamation point?

Oof, thanks.

[Nico, 2016-08-29 19:44:23]

https://codereview.chromium.org/2288003002/diff/20001/base/sys_info_mac.mm
File base/sys_info_mac.mm (left):

https://codereview.chromium.org/2288003002/diff/20001/base/sys_info_mac.mm#ol...
base/sys_info_mac.mm:54: DCHECK(base::mac::IsOS10_9());
Hm, the lhs seems better to me. When we dropped old macOS versions in the past,
we grepped for (say) "10.8" and that let us find a bunch of code to delete. This
is now harder. Is there any rush to remove Is10_9?

[Sidney San Martín, 2016-08-30 14:53:33]

Description was changed from

==========
Delete IsOS10_9(), IsAtLeastOS10_9(), and IsAtMostOS10_9().

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

to

==========
Delete IsAtLeastOS10_9() and IsAtMostOS10_9()

…and refactor where it's more appropriate to use *OS10_10().

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

[Sidney San Martín, 2016-08-30 15:27:04]

https://codereview.chromium.org/2288003002/diff/20001/base/sys_info_mac.mm
File base/sys_info_mac.mm (left):

https://codereview.chromium.org/2288003002/diff/20001/base/sys_info_mac.mm#ol...
base/sys_info_mac.mm:54: DCHECK(base::mac::IsOS10_9());
On 2016/08/29 19:44:23, Nico wrote:
> Hm, the lhs seems better to me. When we dropped old macOS versions in the
past,
> we grepped for (say) "10.8" and that let us find a bunch of code to delete.
This
> is now harder. Is there any rush to remove Is10_9?

FWIW, grepping doesn't get any harder. When we drop 10.9, we'd search for:

- IsAtLeastOS10_10(): now guaranteed to be true with or without `!`, time to
delete some code.
- IsOS10_10() -> !IsAtLeastOS10_11()
- IsAtMost10_10() -> !IsAtLeastOS10_11()

…and then we could delete the *10_10() functions.

On the other hand, if it's more of a semantic meaning/readability thing —
IsOS10_9() doesn't actually mean the same thing as !IsOS10_10(), even though it
does in practice — then how do you feel about reinstating IsOS10_9() and
reverting the more-confusing changes?

[Sidney San Martín, 2016-08-30 15:27:55]

https://codereview.chromium.org/2288003002/diff/20001/base/sys_info_mac.mm
File base/sys_info_mac.mm (left):

https://codereview.chromium.org/2288003002/diff/20001/base/sys_info_mac.mm#ol...
base/sys_info_mac.mm:54: DCHECK(base::mac::IsOS10_9());
On 2016/08/30 15:27:04, Sidney San Martín wrote:
> IsOS10_9() doesn't actually mean the same thing as !IsOS10_10(), even though
it
> does in practice — then how do you feel about reinstating IsOS10_9() and
> reverting the more-confusing changes?

See latest patch set.

[Sidney San Martín, 2016-08-31 18:13:44]

Patchset #4 (id:60001) has been deleted

[Sidney San Martín, 2016-08-31 18:20:17]

[Closing this — after a rebase and a fresh look, I don't think any of this needs
to be changed.]