Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10818023:
Implements iOS device util methods. (Closed)
|
Created:
8 years, 5 months ago by Chen Yu Modified:
8 years, 4 months ago CC:
chromium-reviews, erikwright (departed), brettw-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplements iOS device util methods.
Utility methods in this package provides IOS device-specific information.
BUG=NONE
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150324
Patch Set 1 : #
Total comments: 40
Patch Set 2 : Address review comments #Patch Set 3 : address more review comments #Patch Set 4 : Rename a variable #
Total comments: 6
Patch Set 5 : Fix a comment, add const to NSString* and fix a format #
Total comments: 65
Patch Set 6 : address review comments #Patch Set 7 : Address review comments #
Total comments: 8
Patch Set 8 : address review comments #Patch Set 9 : Fix an indentation #Patch Set 10 : Sync and change the way to access sha256 data #
Total comments: 4
Patch Set 11 : address review comments #Patch Set 12 : sync #
Messages
Total messages: 21 (0 generated)
Starting with stuartmorgan for initial look. Thanks!
https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_util.h File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:11: namespace device_util { Add a blank line after this. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:14: // Possible (known) values are: s/are/include/ since we know this list isn't exhaustive https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:38: std::string platform(); All of these methods need to be converted to FunctionStyleNaming, not objCMethodStyleNaming. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:40: // Returns if the application is running on a high-ram device. (>=250M). s/if/true if/ https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:44: // an iPhone 3GS, "iPad1,1" is an iPad 1, and "iPhone3,1" is an iPhone 4. Replace this whole comment with "Returns true if the device has only one core". You can move the second sentence into the implementation. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:47: // Returns the first mac address. MAC address https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:8: #import <CommonCrypto/CommonDigest.h> Reverse these two lines. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:44: BOOL result = NO; s/BOOL/bool/ https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:58: static BOOL isSingleCoreDevice; Same for these, and both should be = false; Also, all the variables in this method need to use function_variable_names. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:64: initCheck = YES; true https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:70: NSMutableString* macString = [NSMutableString string]; mac_string https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:82: int sdlAlen = dlAddr->sdl_alen; sdl_alen https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:99: uuidObject(CFUUIDCreate(kCFAllocatorDefault)); uuid_object https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:101: NSString* str = str is a terrible name; let's use uuid_string https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:103: autorelease]; Use ScopedCFTypeRef https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:104: return base::SysNSStringToUTF8(str); SysCFStringRefToUTF8 https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:109: NSString* clientId = [defaults stringForKey:kClientIdPreferenceKey]; client_id https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:124: CFUUIDBytes* bytes = static_cast<CFUUIDBytes*>(result); uuid_bytes https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:128: NSString* str = s/str/device_id/ https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:130: autorelease]; Same here; scoped, and CF-based conversion. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... File base/ios/device_util_unittest.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util_unittest.mm:21: NSString* platformStr = base::SysUTF8ToNSString(ios::device_util::platform()); s/platformStr/platform/
Fix typo (device) in CL summary Explain briefly what the overall package of utilities is (i.e., that they get device-specific information).
On 2012/07/24 13:39:48, stuartmorgan wrote: > Explain briefly what the overall package of utilities is (i.e., that they get > device-specific information). ... in the full CL description.
Thanks for the review! https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_util.h File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:11: namespace device_util { On 2012/07/24 13:38:49, stuartmorgan wrote: > Add a blank line after this. Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:14: // Possible (known) values are: On 2012/07/24 13:38:49, stuartmorgan wrote: > s/are/include/ since we know this list isn't exhaustive Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:38: std::string platform(); On 2012/07/24 13:38:49, stuartmorgan wrote: > All of these methods need to be converted to FunctionStyleNaming, not > objCMethodStyleNaming. Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:40: // Returns if the application is running on a high-ram device. (>=250M). On 2012/07/24 13:38:49, stuartmorgan wrote: > s/if/true if/ Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:44: // an iPhone 3GS, "iPad1,1" is an iPad 1, and "iPhone3,1" is an iPhone 4. On 2012/07/24 13:38:49, stuartmorgan wrote: > Replace this whole comment with "Returns true if the device has only one core". > You can move the second sentence into the implementation. Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.h:47: // Returns the first mac address. On 2012/07/24 13:38:49, stuartmorgan wrote: > MAC address Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:8: #import <CommonCrypto/CommonDigest.h> On 2012/07/24 13:38:49, stuartmorgan wrote: > Reverse these two lines. Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:44: BOOL result = NO; On 2012/07/24 13:38:49, stuartmorgan wrote: > s/BOOL/bool/ Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:58: static BOOL isSingleCoreDevice; On 2012/07/24 13:38:49, stuartmorgan wrote: > Same for these, and both should be = false; > > Also, all the variables in this method need to use function_variable_names. Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:64: initCheck = YES; On 2012/07/24 13:38:49, stuartmorgan wrote: > true Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:70: NSMutableString* macString = [NSMutableString string]; On 2012/07/24 13:38:49, stuartmorgan wrote: > mac_string Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:82: int sdlAlen = dlAddr->sdl_alen; On 2012/07/24 13:38:49, stuartmorgan wrote: > sdl_alen Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:99: uuidObject(CFUUIDCreate(kCFAllocatorDefault)); On 2012/07/24 13:38:49, stuartmorgan wrote: > uuid_object Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:101: NSString* str = On 2012/07/24 13:38:49, stuartmorgan wrote: > str is a terrible name; let's use uuid_string Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:103: autorelease]; On 2012/07/24 13:38:49, stuartmorgan wrote: > Use ScopedCFTypeRef Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:104: return base::SysNSStringToUTF8(str); On 2012/07/24 13:38:49, stuartmorgan wrote: > SysCFStringRefToUTF8 Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:109: NSString* clientId = [defaults stringForKey:kClientIdPreferenceKey]; On 2012/07/24 13:38:49, stuartmorgan wrote: > client_id Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:124: CFUUIDBytes* bytes = static_cast<CFUUIDBytes*>(result); On 2012/07/24 13:38:49, stuartmorgan wrote: > uuid_bytes Done. https://chromiumcodereview.appspot.com/10818023/diff/1006/base/ios/device_uti... base/ios/device_util.mm:130: autorelease]; On 2012/07/24 13:38:49, stuartmorgan wrote: > Same here; scoped, and CF-based conversion. Done.
LGTM with a couple last nits. https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_util.h File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... base/ios/device_util.h:13: // This methods returns the hardware version of the device the app is running s/This method returns/Returns/ https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... base/ios/device_util.mm:24: NSString* kClientIdPreferenceKey = @"ChromiumClientID"; NSString* const https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... base/ios/device_util.mm:67: [single_core_devices containsObject: Move this up to the previous line, and reduce the indentation of the next line by 4.
https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_util.h File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... base/ios/device_util.h:13: // This methods returns the hardware version of the device the app is running On 2012/07/25 11:48:59, stuartmorgan wrote: > s/This method returns/Returns/ Done. https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... base/ios/device_util.mm:24: NSString* kClientIdPreferenceKey = @"ChromiumClientID"; On 2012/07/25 11:48:59, stuartmorgan wrote: > NSString* const Done. https://chromiumcodereview.appspot.com/10818023/diff/3006/base/ios/device_uti... base/ios/device_util.mm:67: [single_core_devices containsObject: On 2012/07/25 11:48:59, stuartmorgan wrote: > Move this up to the previous line, and reduce the indentation of the next line > by 4. Done.
+Mark for OWNERs review
https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:21: // iPhone4,1 -> ??iPhone 5 There’s no such thing yet, right? “?” is a good enough description. This is not a good place for speculation, because nobody will update it in the future and it will become stale and confusing. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:28: // iPod5,1 -> ??iPod touch 5G Same. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:32: // iPad2,1 -> iPad 2G (iProd 2,1) What does iProd mean? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:34: // AppleTV2,1 -> AppleTV 2 What’s it return on a simulator? Just the Mac model name? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:46: // Returns the first MAC address. This doesn’t explain why this function needs an argument at all or what the argument should look like or do. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:47: std::string GetMacAddress(std::string ifName); ifName should not use camelCase. Also, don’t abbreviate. interface_name. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:47: std::string GetMacAddress(std::string ifName); The argument type should be const std::string&. No need to give the function its own copy of the string. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:7: #import <CommonCrypto/CommonDigest.h> #include this. It’s not Objective-C. #import is only used for Objective-C headers. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:26: const char* const kDefaultSalt = "Salt"; Why not const char kDefaultSalt[] = "Salt"; instead? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:26: const char* const kDefaultSalt = "Salt"; What is special about this that an empty string ("") would not address? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:40: result = machine.get(); Why the copy? Why not just pass result’s buffer to sysctlbyname? See WriteInto from base/string_util.h. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:52: result = (value >= 250 * 1024 * 1024); The parentheses aren’t necessary. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:59: static bool is_init_check = false; Don’t do this thing with two statics. Use the compiler’s own guard variables. For example: namespace { bool IsSingleCoreDeviceInternal() { NSArray* single_core_devices = …; return [single_core_devices containsObject:…]; } } // namespace bool IsSingleCoreDevice() { static bool result = IsSingleCoreDeviceInternal(); return result; } https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:64: NSArray* single_core_devices = You can’t use something like hw.ncpu? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:67: base::SysUTF8ToNSString(GetPlatform())]; Since GetPlatform() returns a std::string, I think this code would be simpler and smaller if you didn’t use NSArrays and a conversion to NSString. Establish a vector of single core devices (const char* const kSingleCoreDevices[] = { "iPhone2,1", "iPad1,1", …};) and then use something like |std::find(kSingleCoreDevices, kSingleCoreDevices + arraysize(kSingleCoreDevices), GetPlatform()) != kSingleCoreDevices + arraysize(kSingleCoreDevices)| to see if GetPlatform() is in the list. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:73: std::string GetMacAddress(std::string ifName) { interface_name, as before. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:76: struct ifaddrs* addrs; Don’t abbreviate. addresses. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:78: struct ifaddrs* cursor = addrs; cursor to what? Call this |address|. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:79: while (cursor != 0) { Use NULL for pointer comparisons to zero, or just write |while (cursor) {|. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:79: while (cursor != 0) { for (struct ifaddrs* address = addresses; address; address = address->ifa_next) { https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:82: const struct sockaddr_dl* dlAddr = This, too, is namedImproperly. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:83: (const struct sockaddr_dl*)cursor->ifa_addr; Don’t use (c_style)casts. Use c_plus_plus<style>(casts). https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:84: const unsigned char* base = base is a bad name. base of what? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:85: (const unsigned char*)&dlAddr->sdl_data[dlAddr->sdl_nlen]; Casting again. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:89: [mac_string appendString:@":"]; Come on, we have a whole suite of StringPrintf utilities in base/stringprintf.h. There’s no need to start messing around with NSMutableStrings when you’re targeting a std::string. Save yourself the overhead and the conversion. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:104: // Get the string representation of CFUUID object. This comment doesn’t contribute anything. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:110: std::string GetDeviceIdentifier(const char* const salt) { If you wouldn’t accept a const int argument or make a variable that’s not going to change const (as on lines 111 and 112), why would you accept a *const argument? Following the style of this file, you’d use const char*, but not const char* const. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:123: DCHECK(CC_SHA256_DIGEST_LENGTH >= 16); This is a compile-time condition. It’s not something you DCHECK (which is for runtime checks as on line 126), it’s something that you COMPILE_ASSERT. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:123: DCHECK(CC_SHA256_DIGEST_LENGTH >= 16); Why do you care? What’s special about 16? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... File base/ios/device_util_unittest.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:21: NSString* platformStr = base::SysUTF8ToNSString( platform_string https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:21: NSString* platformStr = base::SysUTF8ToNSString( Why is this an NSString* with a conversion instead of a std::string? You’re not using any special NSString* features. You’re just checking the string’s length, which std::string is more than capable of. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:30: (void)ios::device_util::IsRunningOnHighRamDevice(); Get rid of that silly cast. Nobody’s marked this function with the warn_unused_result attribute. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:34: NSString* default_id = Why NSString* and a conversion? You can test std::string objects for equality. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:41: [defaults removeObjectForKey:@"ChromiumClientID"]; Will this trash the client ID used by the real app? That wouldn’t be great. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:49: } // anonymous namespace End unnamed namespaces with } // namespace not this.
Thanks for the review! PTAL. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:21: // iPhone4,1 -> ??iPhone 5 On 2012/07/25 14:00:23, Mark Mentovai wrote: > There’s no such thing yet, right? “?” is a good enough description. This is not > a good place for speculation, because nobody will update it in the future and it > will become stale and confusing. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:28: // iPod5,1 -> ??iPod touch 5G On 2012/07/25 14:00:23, Mark Mentovai wrote: > Same. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:32: // iPad2,1 -> iPad 2G (iProd 2,1) fixed. On 2012/07/25 14:00:23, Mark Mentovai wrote: > What does iProd mean? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:34: // AppleTV2,1 -> AppleTV 2 Added On 2012/07/25 14:00:23, Mark Mentovai wrote: > What’s it return on a simulator? Just the Mac model name? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:46: // Returns the first MAC address. On 2012/07/25 14:00:23, Mark Mentovai wrote: > This doesn’t explain why this function needs an argument at all or what the > argument should look like or do. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:47: std::string GetMacAddress(std::string ifName); On 2012/07/25 14:00:23, Mark Mentovai wrote: > ifName should not use camelCase. Also, don’t abbreviate. interface_name. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.h:47: std::string GetMacAddress(std::string ifName); On 2012/07/25 14:00:23, Mark Mentovai wrote: > ifName should not use camelCase. Also, don’t abbreviate. interface_name. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:7: #import <CommonCrypto/CommonDigest.h> On 2012/07/25 14:00:23, Mark Mentovai wrote: > #include this. It’s not Objective-C. #import is only used for Objective-C > headers. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:26: const char* const kDefaultSalt = "Salt"; Nothing special actually. But "Salt" has been used and we need to continue using it to be consistent. On 2012/07/25 14:00:23, Mark Mentovai wrote: > What is special about this that an empty string ("") would not address? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:40: result = machine.get(); On 2012/07/25 14:00:23, Mark Mentovai wrote: > Why the copy? Why not just pass result’s buffer to sysctlbyname? See WriteInto > from base/string_util.h. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:52: result = (value >= 250 * 1024 * 1024); On 2012/07/25 14:00:23, Mark Mentovai wrote: > The parentheses aren’t necessary. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:59: static bool is_init_check = false; Changed not to use cache. On 2012/07/25 14:00:23, Mark Mentovai wrote: > Don’t do this thing with two statics. Use the compiler’s own guard variables. > For example: > > namespace { > > bool IsSingleCoreDeviceInternal() { > NSArray* single_core_devices = …; > return [single_core_devices containsObject:…]; > } > > } // namespace > > bool IsSingleCoreDevice() { > static bool result = IsSingleCoreDeviceInternal(); > return result; > } https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:64: NSArray* single_core_devices = Thanks for the suggestions! The hardcoded-list might be incomplete for single-core devices. I changed to use hw.physicalcpu, (and a quick check indicates it returns 1 for iPhone 4, and 2 for iPhone 4S and iPad 2; this is consistent with the hardcoded list here.). On 2012/07/25 14:00:23, Mark Mentovai wrote: > You can’t use something like hw.ncpu? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:73: std::string GetMacAddress(std::string ifName) { On 2012/07/25 14:00:23, Mark Mentovai wrote: > interface_name, as before. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:76: struct ifaddrs* addrs; On 2012/07/25 14:00:23, Mark Mentovai wrote: > Don’t abbreviate. addresses. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:78: struct ifaddrs* cursor = addrs; On 2012/07/25 14:00:23, Mark Mentovai wrote: > cursor to what? Call this |address|. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:79: while (cursor != 0) { On 2012/07/25 14:00:23, Mark Mentovai wrote: > for (struct ifaddrs* address = addresses; address; address = address->ifa_next) > { Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:79: while (cursor != 0) { On 2012/07/25 14:00:23, Mark Mentovai wrote: > Use NULL for pointer comparisons to zero, or just write |while (cursor) {|. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:82: const struct sockaddr_dl* dlAddr = On 2012/07/25 14:00:23, Mark Mentovai wrote: > This, too, is namedImproperly. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:83: (const struct sockaddr_dl*)cursor->ifa_addr; On 2012/07/25 14:00:23, Mark Mentovai wrote: > Don’t use (c_style)casts. Use c_plus_plus<style>(casts). Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:84: const unsigned char* base = On 2012/07/25 14:00:23, Mark Mentovai wrote: > base is a bad name. base of what? Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:85: (const unsigned char*)&dlAddr->sdl_data[dlAddr->sdl_nlen]; On 2012/07/25 14:00:23, Mark Mentovai wrote: > Casting again. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:89: [mac_string appendString:@":"]; On 2012/07/25 14:00:23, Mark Mentovai wrote: > Come on, we have a whole suite of StringPrintf utilities in base/stringprintf.h. > There’s no need to start messing around with NSMutableStrings when you’re > targeting a std::string. Save yourself the overhead and the conversion. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:104: // Get the string representation of CFUUID object. On 2012/07/25 14:00:23, Mark Mentovai wrote: > This comment doesn’t contribute anything. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:110: std::string GetDeviceIdentifier(const char* const salt) { On 2012/07/25 14:00:23, Mark Mentovai wrote: > If you wouldn’t accept a const int argument or make a variable that’s not going > to change const (as on lines 111 and 112), why would you accept a *const > argument? Following the style of this file, you’d use const char*, but not const > char* const. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util.mm:123: DCHECK(CC_SHA256_DIGEST_LENGTH >= 16); I removed this COMPILE_ASSERT. On 2012/07/25 14:00:23, Mark Mentovai wrote: > Why do you care? What’s special about 16? https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... File base/ios/device_util_unittest.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:21: NSString* platformStr = base::SysUTF8ToNSString( On 2012/07/25 14:00:23, Mark Mentovai wrote: > Why is this an NSString* with a conversion instead of a std::string? You’re not > using any special NSString* features. You’re just checking the string’s length, > which std::string is more than capable of. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:30: (void)ios::device_util::IsRunningOnHighRamDevice(); On 2012/07/25 14:00:23, Mark Mentovai wrote: > Get rid of that silly cast. Nobody’s marked this function with the > warn_unused_result attribute. Done. https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:49: } // anonymous namespace On 2012/07/25 14:00:23, Mark Mentovai wrote: > End unnamed namespaces with > > } // namespace > > not this. Done.
Stuart, can you take another look again as a first line?
LGTM https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... File base/ios/device_util_unittest.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/16001/base/ios/device_ut... base/ios/device_util_unittest.mm:41: [defaults removeObjectForKey:@"ChromiumClientID"]; On 2012/07/25 14:00:23, Mark Mentovai wrote: > Will this trash the client ID used by the real app? That wouldn’t be great. Nope, defaults are per-app. https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util.h:49: // Returns the MAC address of interface with name |interface_name|. s/interface/the interface/ https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util.mm:67: strcmp(interface_name.c_str(), address->ifa_name) == 0) { This should be indented one less. https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util.mm:116: DCHECK(result == hash); DCHECK_EQ (although why are we DCHECKing this? It's not clear to me why it matters) https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... File base/ios/device_util_unittest.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util_unittest.mm:14: // The tests here depend on what it is run on, so there is no much to unittest s/tests here/behavior of most of these utility functions/ s/is it/they are/ s/no/not/
https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... File base/ios/device_util.h (right): https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util.h:49: // Returns the MAC address of interface with name |interface_name|. On 2012/07/30 14:28:54, stuartmorgan wrote: > s/interface/the interface/ Done. https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util.mm:67: strcmp(interface_name.c_str(), address->ifa_name) == 0) { On 2012/07/30 14:28:54, stuartmorgan wrote: > This should be indented one less. Done. https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util.mm:116: DCHECK(result == hash); On 2012/07/30 14:28:54, stuartmorgan wrote: > DCHECK_EQ (although why are we DCHECKing this? It's not clear to me why it > matters) DCHECK removed; also changed the way to access sha256. https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... File base/ios/device_util_unittest.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/20006/base/ios/device_ut... base/ios/device_util_unittest.mm:14: // The tests here depend on what it is run on, so there is no much to unittest On 2012/07/30 14:28:54, stuartmorgan wrote: > s/tests here/behavior of most of these utility functions/ > s/is it/they are/ > s/no/not/ Done.
https://chromiumcodereview.appspot.com/10818023/diff/29002/base/ios/device_ut... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/29002/base/ios/device_ut... base/ios/device_util.mm:44: u_int64_t memory_size = 0; uint64_t, not u_int64_t. Same on line 54. https://chromiumcodereview.appspot.com/10818023/diff/29002/base/ios/device_ut... base/ios/device_util.mm:82: base::StringAppendF(&mac_string, ":"); Invoking the formatter for this is kind of unnecessary. mac_string.push_back(':') is all that you need.
https://chromiumcodereview.appspot.com/10818023/diff/29002/base/ios/device_ut... File base/ios/device_util.mm (right): https://chromiumcodereview.appspot.com/10818023/diff/29002/base/ios/device_ut... base/ios/device_util.mm:44: u_int64_t memory_size = 0; On 2012/08/01 13:59:26, Mark Mentovai wrote: > uint64_t, not u_int64_t. Same on line 54. Done. https://chromiumcodereview.appspot.com/10818023/diff/29002/base/ios/device_ut... base/ios/device_util.mm:82: base::StringAppendF(&mac_string, ":"); On 2012/08/01 13:59:26, Mark Mentovai wrote: > Invoking the formatter for this is kind of unnecessary. > mac_string.push_back(':') is all that you need. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chenyu@chromium.org/10818023/30005
Failed to apply patch for base/base.gypi: While running patch -p1 --forward --force; patching file base/base.gypi Hunk #1 FAILED at 151. 1 out of 1 hunk FAILED -- saving rejects to file base/base.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chenyu@chromium.org/10818023/21006
Change committed as 150324 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
