Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860663005:
Implement SysInfo::HardwareModelName() on iOS. (Closed)
|
Created:
3 years, 7 months ago by Alexei Svitkine (slow) Modified:
3 years, 7 months ago CC:
chromium-reviews, danakj+watch_chromium.org, mac-reviews_chromium.org, ios-reviews_chromium.org, asvitkine+watch_chromium.org, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement SysInfo::HardwareModelName() on iOS.
This allows the hardware_class field in UMA to be populated on
iOS (with values like "iPhone9,3"), which will allow us to stop
sending those strings in the cpu_architecture field which is
what's currently being done.
BUG=370104
Review-Url: https://codereview.chromium.org/2860663005
Cr-Commit-Position: refs/heads/master@{#469499}
Committed: https://chromium.googlesource.com/chromium/src/+/666e5eec463d93e15876275e1a8a7538b7117746
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comments. #
Total comments: 4
Patch Set 3 : Address more comments. #Patch Set 4 : Enable HardwareModelName test on iOS. #
Total comments: 2
Patch Set 5 : Use hw.machine and expand unit test to check format. #
Total comments: 2
Patch Set 6 : ASSERT_EQ() #
Total comments: 2
Patch Set 7 : Use simulator check. #
Total comments: 2
Patch Set 8 : Use different simulator macro. #
Messages
Total messages: 41 (22 generated)
The CQ bit was checked by asvitkine@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.chromium.or...
Patchset #1 (id:1) has been deleted
asvitkine@chromium.org changed reviewers: + mark@chromium.org, pkl@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It’s a shame that these don’t share an implementation. https://codereview.chromium.org/2860663005/diff/20001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/20001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:109: return std::string(model, 0, len); This is weird. I don’t think that there’s a std::string constructor that accepts (const char*, size_t, size_t). This is probably converting the char* into a std::string and using the (const std::string&, size_t, size_t) constructor. Yech. You know that you have a NUL-terminated string on your hands, so DCHECK_GE(len, 1); // and if you really want to, DCHECK_EQ(model[len - 1], '\0') return std::string(model, len - 1);
https://codereview.chromium.org/2860663005/diff/20001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/20001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:109: return std::string(model, 0, len); On 2017/05/04 16:29:13, Mark Mentovai wrote: > This is weird. I don’t think that there’s a std::string constructor that accepts > (const char*, size_t, size_t). This is probably converting the char* into a > std::string and using the (const std::string&, size_t, size_t) constructor. > Yech. > > You know that you have a NUL-terminated string on your hands, so > DCHECK_GE(len, 1); // and if you really want to, DCHECK_EQ(model[len - 1], > '\0') > return std::string(model, len - 1); Good catch - done for both Mac and iOS and made CPUModelName() use the same logic as well.
LGTM https://codereview.chromium.org/2860663005/diff/40001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/40001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:101: DCHECK_EQ('\0', name[len - 1]); You don’t have to write things backwards like this anymore. You never did for DCHECK, but gtest’s ASSERT_EQ/EXPECT_EQ did have an (expected, actual) ordering. This has recently changed, and it’s just (val1, val2) everywhere now, so you can write these in whatever way you find natural. https://codereview.chromium.org/2860663005/diff/40001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:111: if (sysctlbyname("hw.model", model, &len, nullptr, 0) == 0) { Since these two functions are identical other than the first parameter to sysctlbyname, maybe you want to refactor them to both call into a common function.
https://codereview.chromium.org/2860663005/diff/40001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/40001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:101: DCHECK_EQ('\0', name[len - 1]); On 2017/05/04 16:52:45, Mark Mentovai wrote: > You don’t have to write things backwards like this anymore. > > You never did for DCHECK, but gtest’s ASSERT_EQ/EXPECT_EQ did have an (expected, > actual) ordering. This has recently changed, and it’s just (val1, val2) > everywhere now, so you can write these in whatever way you find natural. Interesting. I guess I'm very used to having the constant be to the left that I do find that way natural for these statements. Also I think JUnit which we in Chrome for Android Java code still requires the expected, actual ordering and so I like the consistency. So kept as-is. https://codereview.chromium.org/2860663005/diff/40001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:111: if (sysctlbyname("hw.model", model, &len, nullptr, 0) == 0) { On 2017/05/04 16:52:45, Mark Mentovai wrote: > Since these two functions are identical other than the first parameter to > sysctlbyname, maybe you want to refactor them to both call into a common > function. Done.
The CQ bit was checked by asvitkine@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.chromium.or...
LGTM!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkl@chromium.org changed reviewers: + pkl@chromium.org
https://codereview.chromium.org/2860663005/diff/80001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/80001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:119: return GetSysctlValue("hw.model"); I patched this CL and tested it on a real device (iPhone5S). hw.model returns some strange string "N53AP". On simulator, it returns "MacPro6,1". Based on https://gist.github.com/Tokuriku/9708472cbe1095030c8b I tried "hw.machine". That returns "iPhone6,2" on device and "x86_64" on simulator. I think "hw.machine" is the better key to use here. Is there a unit test that we can add to run only on iOS devices to be sure that the returned value is somewhat "sane"?
https://codereview.chromium.org/2860663005/diff/80001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/80001/base/sys_info_ios.mm#ne... base/sys_info_ios.mm:119: return GetSysctlValue("hw.model"); On 2017/05/04 19:50:22, pkl -pls.ping.after.24h. wrote: > I patched this CL and tested it on a real device (iPhone5S). > hw.model returns some strange string "N53AP". On simulator, it returns > "MacPro6,1". > > Based on https://gist.github.com/Tokuriku/9708472cbe1095030c8b > I tried "hw.machine". That returns "iPhone6,2" on device and "x86_64" on > simulator. > I think "hw.machine" is the better key to use here. > > Is there a unit test that we can add to run only on iOS devices to be sure that > the returned value is somewhat "sane"? Wow - that's pretty strange. Thanks for checking! I can expand the test SysInfoTest.HardwareModelName - which should run on Mac and iOS - to specifically look for the pattern "Foo,Bar". Let's see if that works and matches the pattern. Will also use hw.machine. I'll ping the CL when it's ready for another look.
The CQ bit was checked by asvitkine@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.chromium.or...
Changed to hw.machine on iOS and expanded the test to check the format - running through try bots now.
LGTM. Thank you! https://codereview.chromium.org/2860663005/diff/100001/base/sys_info_unittest.cc File base/sys_info_unittest.cc (right): https://codereview.chromium.org/2860663005/diff/100001/base/sys_info_unittest... base/sys_info_unittest.cc:108: EXPECT_EQ(2u, pieces.size()); ASSERT_EQ? Otherwise pieces[1] may crash if pieces.size() == 1.
https://codereview.chromium.org/2860663005/diff/100001/base/sys_info_unittest.cc File base/sys_info_unittest.cc (right): https://codereview.chromium.org/2860663005/diff/100001/base/sys_info_unittest... base/sys_info_unittest.cc:108: EXPECT_EQ(2u, pieces.size()); On 2017/05/04 21:03:51, pkl -pls.ping.after.24h. wrote: > ASSERT_EQ? > Otherwise pieces[1] may crash if pieces.size() == 1. Right! Done.
https://codereview.chromium.org/2860663005/diff/120001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/120001/base/sys_info_ios.mm#n... base/sys_info_ios.mm:119: // Note: This uses "hw.machine" instead of "hw.model" like the Mac code, You may want to bifurcate this so that it uses hw.machine when targeting a device, and hw.model when targeting a simulator. Otherwise, the test you added will fail for simulator builds, because there’s no comma in x86_64.
(You can use <TargetConditionals.h> TARGET_OS_SIMULATOR.)
https://codereview.chromium.org/2860663005/diff/120001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/120001/base/sys_info_ios.mm#n... base/sys_info_ios.mm:119: // Note: This uses "hw.machine" instead of "hw.model" like the Mac code, On 2017/05/04 21:11:33, Mark Mentovai wrote: > You may want to bifurcate this so that it uses hw.machine when targeting a > device, and hw.model when targeting a simulator. Otherwise, the test you added > will fail for simulator builds, because there’s no comma in x86_64. Yuck. Done. Went with returning a fake "Simulator1,1" string rather than using "hw.model".
The CQ bit was checked by asvitkine@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.chromium.or...
Still LGTM.
LGTM https://codereview.chromium.org/2860663005/diff/140001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/140001/base/sys_info_ios.mm#n... base/sys_info_ios.mm:119: #if TARGET_IPHONE_SIMULATOR Unless we’re using an old SDK that doesn’t have TARGET_OS_SIMULATOR, use the newer TARGET_OS_SIMULATOR, because TARGET_IPHONE_SIMULATOR is deprecated.
https://codereview.chromium.org/2860663005/diff/140001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://codereview.chromium.org/2860663005/diff/140001/base/sys_info_ios.mm#n... base/sys_info_ios.mm:119: #if TARGET_IPHONE_SIMULATOR On 2017/05/04 21:33:45, Mark Mentovai wrote: > Unless we’re using an old SDK that doesn’t have TARGET_OS_SIMULATOR, use the > newer TARGET_OS_SIMULATOR, because TARGET_IPHONE_SIMULATOR is deprecated. Done.
The CQ bit was unchecked by asvitkine@chromium.org
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, pkl@chromium.org Link to the patchset: https://codereview.chromium.org/2860663005/#ps160001 (title: "Update macro.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement SysInfo::HardwareModelName() on iOS. The iOS implementation matches the Mac implementation. This allows the hardware_class field in UMA to be populated on iOS (with values like "iPhone9,3"), which will allow us to stop sending those strings in the cpu_architecture field which is what's currently being done. BUG=370104 ========== to ========== Implement SysInfo::HardwareModelName() on iOS. This allows the hardware_class field in UMA to be populated on iOS (with values like "iPhone9,3"), which will allow us to stop sending those strings in the cpu_architecture field which is what's currently being done. BUG=370104 ==========
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493933901137580,
"parent_rev": "111449d5ff50dab0ea64b157d9fcca71a87c68c6", "commit_rev":
"666e5eec463d93e15876275e1a8a7538b7117746"}
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493933901137580,
"parent_rev": "111449d5ff50dab0ea64b157d9fcca71a87c68c6", "commit_rev":
"666e5eec463d93e15876275e1a8a7538b7117746"}
Message was sent while issue was closed.
Description was changed from ========== Implement SysInfo::HardwareModelName() on iOS. This allows the hardware_class field in UMA to be populated on iOS (with values like "iPhone9,3"), which will allow us to stop sending those strings in the cpu_architecture field which is what's currently being done. BUG=370104 ========== to ========== Implement SysInfo::HardwareModelName() on iOS. This allows the hardware_class field in UMA to be populated on iOS (with values like "iPhone9,3"), which will allow us to stop sending those strings in the cpu_architecture field which is what's currently being done. BUG=370104 Review-Url: https://codereview.chromium.org/2860663005 Cr-Commit-Position: refs/heads/master@{#469499} Committed: https://chromium.googlesource.com/chromium/src/+/666e5eec463d93e15876275e1a8a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/666e5eec463d93e15876275e1a8a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
