Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2391633004:
Remove deprecated Gestalt methods. (Closed)
DescriptionRemove deprecated Gestalt methods.
Gestalt has been deprecated since macOS 10.8, and it's always been overkill for
finding the macOS version anyways. uname works fine.
BUG=webrtc:6027
Committed: https://crrev.com/e606a172d495600378baaacbfe84521679453153
Cr-Commit-Position: refs/heads/master@{#14589}
Patch Set 1 #
Total comments: 8
Patch Set 2 : comments from sergeyu. #
Total comments: 2
Patch Set 3 : Comments from sergeyu. #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== Remove deprecated Gestalt methods. Gestalt has been deprecated since macOS 10.8, and it's always been overkill for finding the macOS version anyways. uname works fine. BUG=6027 ========== to ========== Remove deprecated Gestalt methods. Gestalt has been deprecated since macOS 10.8, and it's always been overkill for finding the macOS version anyways. uname works fine. BUG=webrtc:6027 ==========
erikchen@chromium.org changed reviewers: + henrika@chromium.org
henrika: Please review.
henrika@chromium.org changed reviewers: + kthelgason@webrtc.org - henrika@chromium.org
Removing myself as reviewer.
erikchen@chromium.org changed reviewers: + magjed@webrtc.org
kthelgason, magjed: Please review.
erikchen@chromium.org changed reviewers: + sergeyu@chromium.org
+sergeyu for OWNER
Rather that parsing the version from the kernel version, we should use the officially supported API. There is NSOperatingSystemInfo returned by NSProcessInfo that has exactly what we need here. https://codereview.webrtc.org/2391633004/diff/1/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.webrtc.org/2391633004/diff/1/webrtc/base/macutils.cc#newco... webrtc/base/macutils.cc:84: // The market version of macOS is always 4 lower than the internal version. This should not be relied upon. Though there has been a (relatively) straightforward mapping from kernel version to macOS release version in the past, there is no reason that should continue to be the case.
https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc#new... webrtc/base/macutils.cc:84: // The market version of macOS is always 4 lower than the internal version. On 2016/10/05 18:23:26, kthelgason wrote: > This should not be relied upon. Though there has been a (relatively) > straightforward mapping from kernel version to macOS release version in the > past, there is no reason that should continue to be the case. -[NSProcessInfo operatingSystemVersion] isn't available until macOS 10.10. This logic is pretty much the same as what we do in Chrome: https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?q=mac_util.mm&sq=pa... Including the "minus four". https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?q=mac_util.mm&sq=pa...
https://codereview.webrtc.org/2391633004/diff/1/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.webrtc.org/2391633004/diff/1/webrtc/base/macutils.cc#newco... webrtc/base/macutils.cc:84: // The market version of macOS is always 4 lower than the internal version. On 2016/10/05 20:16:35, erikchen wrote: > On 2016/10/05 18:23:26, kthelgason wrote: > > This should not be relied upon. Though there has been a (relatively) > > straightforward mapping from kernel version to macOS release version in the > > past, there is no reason that should continue to be the case. > > -[NSProcessInfo operatingSystemVersion] isn't available until macOS 10.10. This > logic is pretty much the same as what we do in Chrome: > https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?q=mac_util.mm&sq=pa... > > Including the "minus four". > https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?q=mac_util.mm&sq=pa... Maybe add CHECK(atoi(uname_info.release) >= 6) here to match the logic that we have in chrome https://codereview.webrtc.org/2391633004/diff/1/webrtc/base/macutils.cc#newco... webrtc/base/macutils.cc:88: const char* dot = strchr(uname_info.release, &delimiter); strchr() takes char as the second argument, but you pass a pointer here.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc#new... webrtc/base/macutils.cc:84: // The market version of macOS is always 4 lower than the internal version. On 2016/10/05 23:04:45, Sergey Ulanov wrote: > On 2016/10/05 20:16:35, erikchen wrote: > > On 2016/10/05 18:23:26, kthelgason wrote: > > > This should not be relied upon. Though there has been a (relatively) > > > straightforward mapping from kernel version to macOS release version in the > > > past, there is no reason that should continue to be the case. > > > > -[NSProcessInfo operatingSystemVersion] isn't available until macOS 10.10. > This > > logic is pretty much the same as what we do in Chrome: > > > https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?q=mac_util.mm&sq=pa... > > > > Including the "minus four". > > > https://cs.chromium.org/chromium/src/base/mac/mac_util.mm?q=mac_util.mm&sq=pa... > > Maybe add CHECK(atoi(uname_info.release) >= 6) here to match the logic that we > have in chrome Done. https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc#new... webrtc/base/macutils.cc:88: const char* dot = strchr(uname_info.release, &delimiter); On 2016/10/05 23:04:45, Sergey Ulanov wrote: > strchr() takes char as the second argument, but you pass a pointer here. webrtc uses an inlined strchr that has a different signature: https://cs.chromium.org/chromium/src/third_party/webrtc/base/stringutils.h?q=...
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc#new... webrtc/base/macutils.cc:88: const char* dot = strchr(uname_info.release, &delimiter); On 2016/10/05 23:48:43, erikchen wrote: > On 2016/10/05 23:04:45, Sergey Ulanov wrote: > > strchr() takes char as the second argument, but you pass a pointer here. > > webrtc uses an inlined strchr that has a different signature: > https://cs.chromium.org/chromium/src/third_party/webrtc/base/stringutils.h?q=... This is a wchar_t version, which I don't think is applicable here. There is also rtc::strchr(): https://cs.chromium.org/chromium/src/third_party/webrtc/base/stringutils.h?rc... . Is that the one being called here? It expects 0-terminated second parameter, and that's not what's being passed to it. Maybe change this to ::strchr() to ensure that the right function is being called. https://codereview.chromium.org/2391633004/diff/40001/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.chromium.org/2391633004/diff/40001/webrtc/base/macutils.cc... webrtc/base/macutils.cc:86: assert(minor_version >= 6); please use RTC_CHECK()
https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.chromium.org/2391633004/diff/1/webrtc/base/macutils.cc#new... webrtc/base/macutils.cc:88: const char* dot = strchr(uname_info.release, &delimiter); On 2016/10/07 00:08:46, Sergey Ulanov wrote: > On 2016/10/05 23:48:43, erikchen wrote: > > On 2016/10/05 23:04:45, Sergey Ulanov wrote: > > > strchr() takes char as the second argument, but you pass a pointer here. > > > > webrtc uses an inlined strchr that has a different signature: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/base/stringutils.h?q=... > > This is a wchar_t version, which I don't think is applicable here. There is also > rtc::strchr(): > https://cs.chromium.org/chromium/src/third_party/webrtc/base/stringutils.h?rc... > . > Is that the one being called here? It expects 0-terminated second parameter, and > that's not what's being passed to it. > Maybe change this to ::strchr() to ensure that the right function is being > called. ah, yup, switched to ::strchr(). https://codereview.chromium.org/2391633004/diff/40001/webrtc/base/macutils.cc File webrtc/base/macutils.cc (right): https://codereview.chromium.org/2391633004/diff/40001/webrtc/base/macutils.cc... webrtc/base/macutils.cc:86: assert(minor_version >= 6); On 2016/10/07 00:08:46, Sergey Ulanov wrote: > please use RTC_CHECK() Done.
lgtm
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Remove deprecated Gestalt methods. Gestalt has been deprecated since macOS 10.8, and it's always been overkill for finding the macOS version anyways. uname works fine. BUG=webrtc:6027 ========== to ========== Remove deprecated Gestalt methods. Gestalt has been deprecated since macOS 10.8, and it's always been overkill for finding the macOS version anyways. uname works fine. BUG=webrtc:6027 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove deprecated Gestalt methods. Gestalt has been deprecated since macOS 10.8, and it's always been overkill for finding the macOS version anyways. uname works fine. BUG=webrtc:6027 ========== to ========== Remove deprecated Gestalt methods. Gestalt has been deprecated since macOS 10.8, and it's always been overkill for finding the macOS version anyways. uname works fine. BUG=webrtc:6027 Committed: https://crrev.com/e606a172d495600378baaacbfe84521679453153 Cr-Commit-Position: refs/heads/master@{#14589} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e606a172d495600378baaacbfe84521679453153 Cr-Commit-Position: refs/heads/master@{#14589} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
