Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1224283006:
Update OS version functions for OS X 10.11 El Capitan. (Closed)
|
Created:
5 years, 5 months ago by Andre Modified:
5 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, rickyz+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate OS version functions for OS X 10.11 El Capitan.
BUG=45650
Committed: https://crrev.com/ff07ccad13c0059e086387947b7fed5a930679dc
Cr-Commit-Position: refs/heads/master@{#339775}
Patch Set 1 #
Total comments: 7
Patch Set 2 : For rsesek #
Total comments: 6
Patch Set 3 : For Avi #Patch Set 4 : Rebase #
Total comments: 2
Patch Set 5 : Rebase again #
Depends on Patchset: Messages
Total messages: 31 (10 generated)
andresantoso@chromium.org changed reviewers: + rsesek@chromium.org
Robert PTAL. https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... File sandbox/mac/os_compatibility.cc (left): https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... sandbox/mac/os_compatibility.cc:123: !base::mac::IsOSYosemiteOrLater()) { I'm not sure about this. !IsOSYosemiteOrLater does not seem correct here.
https://codereview.chromium.org/1224283006/diff/1/base/process/memory_mac.mm File base/process/memory_mac.mm (right): https://codereview.chromium.org/1224283006/diff/1/base/process/memory_mac.mm#... base/process/memory_mac.mm:262: base::mac::IsOSElCapitan()) { This usually has to be manually verified by Avi. https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... File sandbox/mac/os_compatibility.cc (left): https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... sandbox/mac/os_compatibility.cc:123: !base::mac::IsOSYosemiteOrLater()) { On 2015/07/15 21:33:32, Andre wrote: > I'm not sure about this. !IsOSYosemiteOrLater does not seem correct here. This is checking for >= 10.7 && < 10.10.
andresantoso@chromium.org changed reviewers: + avi@chromium.org
Avi PTAL. https://codereview.chromium.org/1224283006/diff/1/base/process/memory_mac.mm File base/process/memory_mac.mm (right): https://codereview.chromium.org/1224283006/diff/1/base/process/memory_mac.mm#... base/process/memory_mac.mm:262: base::mac::IsOSElCapitan()) { +Avi. https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... File sandbox/mac/os_compatibility.cc (left): https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... sandbox/mac/os_compatibility.cc:123: !base::mac::IsOSYosemiteOrLater()) { But why? Do we expect Yosemite and El Capitan to log "Unknown OS" below?
https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... File sandbox/mac/os_compatibility.cc (left): https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... sandbox/mac/os_compatibility.cc:123: !base::mac::IsOSYosemiteOrLater()) { On 2015/07/15 21:49:17, Andre wrote: > But why? > Do we expect Yosemite and El Capitan to log "Unknown OS" below? This component needs to be updated for 10.10+ so it's not currently used. It won't be logged.
https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... File sandbox/mac/os_compatibility.cc (left): https://codereview.chromium.org/1224283006/diff/1/sandbox/mac/os_compatibilit... sandbox/mac/os_compatibility.cc:123: !base::mac::IsOSYosemiteOrLater()) { Got it. Fixed in patchset #2.
https://codereview.chromium.org/1224283006/diff/20001/base/mac/mac_util_unitt... File base/mac/mac_util_unittest.mm (right): https://codereview.chromium.org/1224283006/diff/20001/base/mac/mac_util_unitt... base/mac/mac_util_unittest.mm:157: EXPECT_FALSE(IsOSYosemiteOrLater()); insert IsOSElCapitan[OrLater] here and below. https://codereview.chromium.org/1224283006/diff/20001/base/process/memory_mac.mm File base/process/memory_mac.mm (right): https://codereview.chromium.org/1224283006/diff/20001/base/process/memory_mac... base/process/memory_mac.mm:262: base::mac::IsOSElCapitan()) { Have you tested the offsets? (Basically, does it immediately crash if you put this in? :) ) https://codereview.chromium.org/1224283006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1224283006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51603: + <int value="23" label="FutureCat (>10.11), 8-bit (?)"/> CatSixtyFour is an obsolete histogram (https://codereview.chromium.org/1030253003), so we don't need to update it.
Looks like shrike@ beat you to it: https://codereview.chromium.org/1231803007/
https://codereview.chromium.org/1224283006/diff/20001/base/mac/mac_util_unitt... File base/mac/mac_util_unittest.mm (right): https://codereview.chromium.org/1224283006/diff/20001/base/mac/mac_util_unitt... base/mac/mac_util_unittest.mm:157: EXPECT_FALSE(IsOSYosemiteOrLater()); On 2015/07/15 23:29:01, Avi wrote: > insert IsOSElCapitan[OrLater] here and below. Done. https://codereview.chromium.org/1224283006/diff/20001/base/process/memory_mac.mm File base/process/memory_mac.mm (right): https://codereview.chromium.org/1224283006/diff/20001/base/process/memory_mac... base/process/memory_mac.mm:262: base::mac::IsOSElCapitan()) { No, this does not immediately crash running on 10.11. Running the diagnostic tool from the bug: This is Mac OS X 10.11 (15A216g), running in 64-bit mode. CFAllocatorContext offset is 136, expected for Mac OS X 10.7-10.8 https://codereview.chromium.org/1224283006/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1224283006/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:51603: + <int value="23" label="FutureCat (>10.11), 8-bit (?)"/> On 2015/07/15 23:29:01, Avi wrote: > CatSixtyFour is an obsolete histogram > (https://codereview.chromium.org/1030253003), so we don't need to update it. Done.
Patchset #4 (id:60001) has been deleted
Thanks. I have rebased this CL, see patchset 4.
lgtm
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224283006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
andresantoso@chromium.org changed reviewers: + thakis@chromium.org
thakis PTAL.
lgtm https://codereview.chromium.org/1224283006/diff/80001/base/process/memory_mac.mm File base/process/memory_mac.mm (right): https://codereview.chromium.org/1224283006/diff/80001/base/process/memory_mac... base/process/memory_mac.mm:265: reinterpret_cast<const ChromeCFAllocatorLions*>(allocator)); you checked that this hasn't changed in 10.11, yes?
The CQ bit was checked by andresantoso@chromium.org
https://codereview.chromium.org/1224283006/diff/80001/base/test/expectations/... File base/test/expectations/expectation.cc (right): https://codereview.chromium.org/1224283006/diff/80001/base/test/expectations/... base/test/expectations/expectation.cc:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. FYI this file is deleted
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224283006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by andresantoso@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1224283006/#ps100001 (title: "Rebase again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1224283006/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ff07ccad13c0059e086387947b7fed5a930679dc Cr-Commit-Position: refs/heads/master@{#339775} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
