Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10704123:
Implement SysInfo for iOS (Closed)
|
Created:
8 years, 5 months ago by Chen Yu Modified:
8 years, 5 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement SysInfo for iOS
BUG=NONE
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146106
Patch Set 1 #Patch Set 2 : #
Total comments: 23
Patch Set 3 : Address review comments #
Total comments: 8
Patch Set 4 : Address review comments #Patch Set 5 : Add examples for system names #
Total comments: 3
Patch Set 6 : Remove a blank line #
Messages
Total messages: 14 (0 generated)
https://chromiumcodereview.appspot.com/10704123/diff/2001/base/base.gypi File base/base.gypi (right): https://chromiumcodereview.appspot.com/10704123/diff/2001/base/base.gypi#newc... base/base.gypi:359: 'sys_info_ios.cc', Your file uses Objective-C and is named sys_info_ios.mm, isn’t it? This error raises a presumption that your patch hasn’t been tested or even compiled. How have you tested this change? https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:8: #include <mach/mach_host.h> Instead of #including these two mach headers, just #include <mach/mach.h>. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:13: #include "base/string_split.h" You don’t seem to use this. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:21: return SysNSStringToUTF8([[UIDevice currentDevice] systemName]); I’m curious: what does this return on iPhone? iPad? iPod Touch? https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:31: void SysInfo::OperatingSystemVersionNumbers(int32 *major_version, Chrome style puts the * on the type name. I don’t know why you changed this from the _mac.cc file. You seem to put the * on the type name correctly later in this function. See http://www.chromium.org/developers/coding-style#TOC-Code-formatting . https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:33: int32 *bugfix_version) { Don’t you want a ScopedNSAutoreleasePool here too? You wanted one last time you wrote [[UIDevice currentDevice] systemVersion], and you have the same call in this function. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:34: static int32 sSystemVersionMajor = 0; 1. There is no need to initialize this to 0. That’s the default behavior for a static local variable. 2. I don’t necessarily see much benefit in making this a static local. 3. If you do want to cache the value, this isn’t a great way to do it. Implement an “uncached” function like OperatingSystemVersionNumbersInternal, and then in OperatingSystemVersionNumbers, call the internal function in an initialization to a static variable. The compiler-generated guard variable will be unambiguous, as compared to what you’ve written here, which will continue to re-fetch the version number even if a value was previously cached and the first component was 0. 4. We don’t use Hungarian notation, even for static local variables. These should just be named_normally_like_this. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:38: // iOS doesn't have Gestalt, so parse the version string. This comment only makes sense in comparison to the _mac.cc file, but that’s not necessarily a comparison anyone will be making. If this is the correct way to fetch the operating system version on iOS, this comment isn’t necessary at all. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:44: sSystemVersionMinor = [[versionInfo objectAtIndex:1] intValue]; This is only safe if length >= 2. The docs don’t make any particular guarantee about the format of -[UIDevice systemVersion]. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:45: if (length == 3) { This can be >= instead of ==. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:63: DCHECK_EQ(HOST_BASIC_INFO_COUNT, count); This should actually follow the “result != KERN_SUCCESS” check. You can make the same change in the _mac.cc file.
thanks for the review! https://chromiumcodereview.appspot.com/10704123/diff/2001/base/base.gypi File base/base.gypi (right): https://chromiumcodereview.appspot.com/10704123/diff/2001/base/base.gypi#newc... base/base.gypi:359: 'sys_info_ios.cc', Yes, sorry, this should be .mm. On 2012/07/09 18:22:47, Mark Mentovai wrote: > Your file uses Objective-C and is named sys_info_ios.mm, isn’t it? > > This error raises a presumption that your patch hasn’t been tested or even > compiled. How have you tested this change? https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:8: #include <mach/mach_host.h> On 2012/07/09 18:22:47, Mark Mentovai wrote: > Instead of #including these two mach headers, just #include <mach/mach.h>. Done. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:13: #include "base/string_split.h" On 2012/07/09 18:22:47, Mark Mentovai wrote: > You don’t seem to use this. Done. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:21: return SysNSStringToUTF8([[UIDevice currentDevice] systemName]); On 2012/07/09 18:22:47, Mark Mentovai wrote: > I’m curious: what does this return on iPhone? iPad? iPod Touch? I saw "iPhone OS" when running it on an iPhone device (I don't have other types of device to check it out). https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:31: void SysInfo::OperatingSystemVersionNumbers(int32 *major_version, On 2012/07/09 18:22:47, Mark Mentovai wrote: > Chrome style puts the * on the type name. I don’t know why you changed this from > the _mac.cc file. You seem to put the * on the type name correctly later in this > function. See > http://www.chromium.org/developers/coding-style#TOC-Code-formatting . Done. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:33: int32 *bugfix_version) { On 2012/07/09 18:22:47, Mark Mentovai wrote: > Don’t you want a ScopedNSAutoreleasePool here too? You wanted one last time you > wrote [[UIDevice currentDevice] systemVersion], and you have the same call in > this function. Done. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:34: static int32 sSystemVersionMajor = 0; Here it is changed to not to cache the information. On 2012/07/09 18:22:47, Mark Mentovai wrote: > 1. There is no need to initialize this to 0. That’s the default behavior for a > static local variable. > 2. I don’t necessarily see much benefit in making this a static local. > 3. If you do want to cache the value, this isn’t a great way to do it. Implement > an “uncached” function like OperatingSystemVersionNumbersInternal, and then in > OperatingSystemVersionNumbers, call the internal function in an initialization > to a static variable. The compiler-generated guard variable will be unambiguous, > as compared to what you’ve written here, which will continue to re-fetch the > version number even if a value was previously cached and the first component was > 0. > 4. We don’t use Hungarian notation, even for static local variables. These > should just be named_normally_like_this. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:38: // iOS doesn't have Gestalt, so parse the version string. Removed. On 2012/07/09 18:22:47, Mark Mentovai wrote: > This comment only makes sense in comparison to the _mac.cc file, but that’s not > necessarily a comparison anyone will be making. If this is the correct way to > fetch the operating system version on iOS, this comment isn’t necessary at all. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:44: sSystemVersionMinor = [[versionInfo objectAtIndex:1] intValue]; On 2012/07/09 18:22:47, Mark Mentovai wrote: > This is only safe if length >= 2. The docs don’t make any particular guarantee > about the format of -[UIDevice systemVersion]. Done. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:45: if (length == 3) { On 2012/07/09 18:22:47, Mark Mentovai wrote: > This can be >= instead of ==. Done. https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:63: DCHECK_EQ(HOST_BASIC_INFO_COUNT, count); On 2012/07/09 18:22:47, Mark Mentovai wrote: > This should actually follow the “result != KERN_SUCCESS” check. You can make the > same change in the _mac.cc file. Done.
https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.... base/sys_info_ios.mm:19: return SysNSStringToUTF8([[UIDevice currentDevice] systemName]); I think we should understand what this will return on different devices, and probably mention that in a comment here. Can you find someone with another device type to check this? https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.... base/sys_info_ios.mm:34: NSArray* versionInfo = [version componentsSeparatedByString:@"."]; Use this_style naming. https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.... base/sys_info_ios.mm:59: if (count != HOST_BASIC_INFO_COUNT) { I believe I asked you to check result before you checked count, didn’t I? https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_mac.cc File base/sys_info_mac.cc (right): https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_mac.... base/sys_info_mac.cc:49: if (count != HOST_BASIC_INFO_COUNT) { Again, I asked you to check result before you checked count. Instead, you changed the DCHECK into a live check. I think the DCHECK is fine. In fact, I prefer it. But you can’t look at count until you’ve made sure that result is KERN_SUCCESS.
https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.... base/sys_info_ios.mm:19: return SysNSStringToUTF8([[UIDevice currentDevice] systemName]); Examples added for ipad and iphone. On 2012/07/10 16:10:04, Mark Mentovai wrote: > I think we should understand what this will return on different devices, and > probably mention that in a comment here. Can you find someone with another > device type to check this? https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.... base/sys_info_ios.mm:34: NSArray* versionInfo = [version componentsSeparatedByString:@"."]; On 2012/07/10 16:10:04, Mark Mentovai wrote: > Use this_style naming. Done. https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_ios.... base/sys_info_ios.mm:59: if (count != HOST_BASIC_INFO_COUNT) { Sorry, I misread your comment; I thought you were suggesting to follow the patten of result check and was wondering. Changed. Thanks! On 2012/07/10 16:10:04, Mark Mentovai wrote: > I believe I asked you to check result before you checked count, didn’t I? https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_mac.cc File base/sys_info_mac.cc (right): https://chromiumcodereview.appspot.com/10704123/diff/12001/base/sys_info_mac.... base/sys_info_mac.cc:49: if (count != HOST_BASIC_INFO_COUNT) { On 2012/07/10 16:10:04, Mark Mentovai wrote: > Again, I asked you to check result before you checked count. Instead, you > changed the DCHECK into a live check. I think the DCHECK is fine. In fact, I > prefer it. But you can’t look at count until you’ve made sure that result is > KERN_SUCCESS. Done.
LGTM with this one change. https://chromiumcodereview.appspot.com/10704123/diff/2004/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://chromiumcodereview.appspot.com/10704123/diff/2004/base/sys_info_ios.m... base/sys_info_ios.mm:19: // Examples of returned value: 'iPhone OS' on iPad 5.1.1 Thank you! https://chromiumcodereview.appspot.com/10704123/diff/2004/base/sys_info_ios.m... base/sys_info_ios.mm:60: Remove this blank line (so that this function matches the _mac.cc file).
https://chromiumcodereview.appspot.com/10704123/diff/2004/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://chromiumcodereview.appspot.com/10704123/diff/2004/base/sys_info_ios.m... base/sys_info_ios.mm:60: On 2012/07/10 17:11:27, Mark Mentovai wrote: > Remove this blank line (so that this function matches the _mac.cc file). Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chenyu@chromium.org/10704123/17001
https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.mm File base/sys_info_ios.mm (right): https://chromiumcodereview.appspot.com/10704123/diff/2001/base/sys_info_ios.m... base/sys_info_ios.mm:31: void SysInfo::OperatingSystemVersionNumbers(int32 *major_version, On 2012/07/09 18:22:47, Mark Mentovai wrote: > Chrome style puts the * on the type name. I don’t know why you changed this from > the _mac.cc file. FWIW, the answer is that we didn't; it was wrong everywhere at the time this file was forked in our repo, and then Peter fixed it later in http://codereview.chromium.org/6816027 (but obviously not in our copy).
List of reviewers changed. stuartmorgan@chromium.org did a drive-by without LGTM'ing!
On 2012/07/11 10:43:57, I haz the power (commit-bot) wrote: > List of reviewers changed. mailto:stuartmorgan@chromium.org did a drive-by without > LGTM'ing! Blargh, sorry about that. I didn't actually mean to add myself as a reviewer. Stupid CQ :(
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chenyu@chromium.org/10704123/17001
Change committed as 146106 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
