Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
374203004:
mac: Add metrics to record Bluetooth availability and capabilities. (Closed)
Descriptionmac: Add metrics to record Bluetooth availability and capabilities.
The new Handoff feature in OSX 10.10 only works with devices that support
Bluetooth LE. Record metrics to determine the percentage of Chrome users that
this affects.
BUG=392166
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282771
Patch Set 1 : First #
Total comments: 25
Patch Set 2 : Comments from avi, add another enum for the metric. #Patch Set 3 : Use ScopedCFTypeRef for matching_dict. #Patch Set 4 : Rebase against origin/master #
Total comments: 8
Patch Set 5 : Comments from mark. #
Total comments: 12
Patch Set 6 : Comments from isherman. #
Total comments: 12
Patch Set 7 : Comments from mark, round two. #
Total comments: 2
Patch Set 8 : Comments from isherman, round two. #
Total comments: 4
Patch Set 9 : Comments from mark, round three. #
Messages
Total messages: 28 (0 generated)
Please review.
https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:16: CFMutableDictionaryRef matching_dict = base::ScopedCFTypeRef<CFMutableDictionaryRef> https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:24: Immediately after IOServiceGetMatchingServices, put the iter into a base::mac::ScopedIOObject<io_iterator_t>. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:31: while ((device = IOIteratorNext(iter))) { base::mac::ScopedIOObject<io_object_t>. See GetMACAddressFromIterator for an example. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:34: device, &dict, kCFAllocatorDefault, kNilOptions); Put the dictionary in base::ScopedCFTypeRef<CFMutableDictionaryRef>. See RemovableStorageProvider::PopulateDeviceList as an example. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:41: NSDictionary* objc_dict = static_cast<NSDictionary*>(dict); base::mac::CFCast https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:42: NSNumber* lmp_version = [objc_dict objectForKey:@"LMPVersion"]; Use ObjCCast here and drop the class check on the next line. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:52: version = MAX(version, [lmp_version intValue]); std::max https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:65: int result = GetBluetoothLMPVersion(); If this value will never change, you might as well make it static.
https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:40: // Check the properties for an LMP version. Is there some documentation about LMP versions, and how that relates to BTLE? Can you add a link?
https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. No (c), 2014. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:41: NSDictionary* objc_dict = static_cast<NSDictionary*>(dict); On 2014/07/08 23:32:04, Avi (OOO July 11-27) wrote: > base::mac::CFCast Sorry, CFToNSCast.
avi: PTAL Such utility. I had no idea that Chromium had so many helper functions/objects around Core Foundation. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2014/07/08 23:38:20, Avi (OOO July 11-27) wrote: > No (c), 2014. Done. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:16: CFMutableDictionaryRef matching_dict = On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > base::ScopedCFTypeRef<CFMutableDictionaryRef> That won't work, since IOServiceGetMatchingServices takes ownership of matching_dict, and ScopedCFTypeRef doesn't have "Pass" functionality. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:24: On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > Immediately after IOServiceGetMatchingServices, put the iter into a > base::mac::ScopedIOObject<io_iterator_t>. Done. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:31: while ((device = IOIteratorNext(iter))) { On 2014/07/08 23:32:04, Avi (OOO July 11-27) wrote: > base::mac::ScopedIOObject<io_object_t>. See GetMACAddressFromIterator for an > example. I copied the example, but used io_service_t as the template parameter https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:34: device, &dict, kCFAllocatorDefault, kNilOptions); On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > Put the dictionary in base::ScopedCFTypeRef<CFMutableDictionaryRef>. See > RemovableStorageProvider::PopulateDeviceList as an example. Done. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:40: // Check the properties for an LMP version. On 2014/07/08 23:32:46, Avi (OOO July 11-27) wrote: > Is there some documentation about LMP versions, and how that relates to BTLE? > Can you add a link? Added the relevant links. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:41: NSDictionary* objc_dict = static_cast<NSDictionary*>(dict); On 2014/07/08 23:38:19, Avi (OOO July 11-27) wrote: > On 2014/07/08 23:32:04, Avi (OOO July 11-27) wrote: > > base::mac::CFCast > > Sorry, CFToNSCast. Done. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:42: NSNumber* lmp_version = [objc_dict objectForKey:@"LMPVersion"]; On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > Use ObjCCast here and drop the class check on the next line. Done. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:52: version = MAX(version, [lmp_version intValue]); On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > std::max Done. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:65: int result = GetBluetoothLMPVersion(); On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > If this value will never change, you might as well make it static. Done.
> Such utility. So helpers. Much objects. Wow. https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:16: CFMutableDictionaryRef matching_dict = On 2014/07/09 00:18:57, erikchen1 wrote: > On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > > base::ScopedCFTypeRef<CFMutableDictionaryRef> > > That won't work, since IOServiceGetMatchingServices takes ownership of > matching_dict, and ScopedCFTypeRef doesn't have "Pass" functionality. On ScopedCFTypeRef, .Pass() is spelled .release() :) See https://code.google.com/p/chromium/codesearch#chromium/src/rlz/mac/lib/machin... for an example.
https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:16: CFMutableDictionaryRef matching_dict = Actually, release() is a lot dumber than Pass(), because Pass() ensures that it ends up in a scoped_ptr. So you have to use release, but it's good enough for here.
avi: PTAL https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:16: CFMutableDictionaryRef matching_dict = On 2014/07/09 01:54:05, Avi (OOO July 11-27) wrote: > On 2014/07/09 00:18:57, erikchen1 wrote: > > On 2014/07/08 23:32:03, Avi (OOO July 11-27) wrote: > > > base::ScopedCFTypeRef<CFMutableDictionaryRef> > > > > That won't work, since IOServiceGetMatchingServices takes ownership of > > matching_dict, and ScopedCFTypeRef doesn't have "Pass" functionality. > > On ScopedCFTypeRef, .Pass() is spelled .release() :) See > https://code.google.com/p/chromium/codesearch#chromium/src/rlz/mac/lib/machin... > for an example. Right you are. I made the incorrect assumption that release() would call CFRelease somewhere in its implementation. Done.
LGTM! https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/40001/chrome/browser/mac/hardw... chrome/browser/mac/hardware_utility.mm:16: CFMutableDictionaryRef matching_dict = Yeah, it's a non-awesome name conflict.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/374203004/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26321) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/30764)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26330)
mark: Looking for owner review of chrome/browser/mac/* isherman: Looking for owner review of chrome/browser/metrics/* and tools/metrics/histograms/*
https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... File chrome/browser/mac/hardware_utility.h (right): https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.h:12: BLUETOOTH_NOT_AVAILABLE = 0, The = 0 is fine here, but just let the compiler count out the rest of the constants for you. https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.mm:16: // Returns -1 if no Bluetooth driver is present. Use constants for these. https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.mm:18: int GetBluetoothLMPVersion() { Put this in an unnamed namespace. https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.mm:48: // Version 4.0+. As I understand it, Bluetooth 4.0 allows devices to implement either original Bluetooth, Bluetooth LE, or both. Simply checking for LMPVersion 6 doesn’t tell you whether LE is supported.
mark: PTAL https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... File chrome/browser/mac/hardware_utility.h (right): https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.h:12: BLUETOOTH_NOT_AVAILABLE = 0, On 2014/07/09 17:53:00, Mark Mentovai wrote: > The = 0 is fine here, but just let the compiler count out the rest of the > constants for you. The Google C++ style guide allows for both styles, and I prefer this one here since the actual numerical values matter (there are matching values in histograms.xml). This makes it less likely that someone will accidentally change the order or insert a new enum value at the wrong location. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Enumerator_Names https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... File chrome/browser/mac/hardware_utility.mm (right): https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.mm:16: // Returns -1 if no Bluetooth driver is present. On 2014/07/09 17:53:00, Mark Mentovai wrote: > Use constants for these. Done. https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.mm:18: int GetBluetoothLMPVersion() { On 2014/07/09 17:53:00, Mark Mentovai wrote: > Put this in an unnamed namespace. Done. https://codereview.chromium.org/374203004/diff/120001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.mm:48: // Version 4.0+. On 2014/07/09 17:53:00, Mark Mentovai wrote: > As I understand it, Bluetooth 4.0 allows devices to implement either original > Bluetooth, Bluetooth LE, or both. Simply checking for LMPVersion 6 doesn’t tell > you whether LE is supported. Hm. Right you are. I spent some time looking into this to determine how Apple determines whether BT LE is supported. My best guess is that they're using the same logic as me. See https://code.google.com/p/chromium/issues/detail?id=392166 for a more detailed report. If you have other suggestions, I'm happy to hear them out. Otherwise I'm inclined to use the current logic.
https://codereview.chromium.org/374203004/diff/140001/chrome/browser/mac/hard... File chrome/browser/mac/hardware_utility.h (right): https://codereview.chromium.org/374203004/diff/140001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.h:22: } // namespace hardware_utility nit: foo_utility and foo_util classes have a tendency to, over time, become dumping grounds for all sorts of unrelated functionality. WDYT of giving this a more focused name? https://codereview.chromium.org/374203004/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/374203004/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:104: hardware_utility::BLUETOOTH_AVAILABILITY_COUNT); I might be mistaken, but I don't think that the bluetooth system is threadsafe. Are you sure that it's ok to query bluetooth state from the blocking pool's thread? https://codereview.chromium.org/374203004/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:104: hardware_utility::BLUETOOTH_AVAILABILITY_COUNT); By the way, note that there is a Bluetooth section in the system_profile.proto message definition. I think that's currently only filled out on ChromeOS, and the histogram is probably more convenient to work with, but still... FYI. https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18929: + The availability and capabilities of the Bluetooth driver on OSX devices. Please document when this is logged, i.e. at startup. Also, note that there is some bias to logging a metric at startup: Users who relaunch Chrome more frequently contribute more votes. Is that an acceptable source of bias for this metric? https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35485: + <int value="2" label="Bluetooth available with LE."/> nit: Since the labels are displayed in the left column of a tabular layout on the dashboard, shorter names are often easier to read. Hence, I'd suggest dropping the common "Bluetooth " prefix. https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35488: + availability."/> nit: I'd put this value first, so that it doesn't end up in the middle if you ever decide to expand this enum.
isherman: PTAL mark: PTAL https://codereview.chromium.org/374203004/diff/140001/chrome/browser/mac/hard... File chrome/browser/mac/hardware_utility.h (right): https://codereview.chromium.org/374203004/diff/140001/chrome/browser/mac/hard... chrome/browser/mac/hardware_utility.h:22: } // namespace hardware_utility On 2014/07/09 21:19:33, Ilya Sherman wrote: > nit: foo_utility and foo_util classes have a tendency to, over time, become > dumping grounds for all sorts of unrelated functionality. WDYT of giving this a > more focused name? I renamed the class to 'bluetooth_utility' https://codereview.chromium.org/374203004/diff/140001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/374203004/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:104: hardware_utility::BLUETOOTH_AVAILABILITY_COUNT); On 2014/07/09 21:19:33, Ilya Sherman wrote: > I might be mistaken, but I don't think that the bluetooth system is threadsafe. > Are you sure that it's ok to query bluetooth state from the blocking pool's > thread? We're not touching the bluetooth system, we're touching IOKit, which should be fine. https://codereview.chromium.org/374203004/diff/140001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:104: hardware_utility::BLUETOOTH_AVAILABILITY_COUNT); On 2014/07/09 21:19:33, Ilya Sherman wrote: > By the way, note that there is a Bluetooth section in the system_profile.proto > message definition. I think that's currently only filled out on ChromeOS, and > the histogram is probably more convenient to work with, but still... FYI. As per offline discussion, I am keeping this a histogram. https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:18929: + The availability and capabilities of the Bluetooth driver on OSX devices. On 2014/07/09 21:19:33, Ilya Sherman wrote: > Please document when this is logged, i.e. at startup. Also, note that there is > some bias to logging a metric at startup: Users who relaunch Chrome more > frequently contribute more votes. Is that an acceptable source of bias for this > metric? Adding documentation. As per offline discussion, this is an acceptable source of bias. https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35485: + <int value="2" label="Bluetooth available with LE."/> On 2014/07/09 21:19:33, Ilya Sherman wrote: > nit: Since the labels are displayed in the left column of a tabular layout on > the dashboard, shorter names are often easier to read. Hence, I'd suggest > dropping the common "Bluetooth " prefix. Done. https://codereview.chromium.org/374203004/diff/140001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35488: + availability."/> On 2014/07/09 21:19:33, Ilya Sherman wrote: > nit: I'd put this value first, so that it doesn't end up in the middle if you > ever decide to expand this enum. Done.
https://codereview.chromium.org/374203004/diff/200001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/374203004/diff/200001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:25: kBluetoothFeatureLESupportedController = (1 << 6L), Can you put this down with the other new Bluetooth stuff from the 10.7 SDK? https://codereview.chromium.org/374203004/diff/200001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:162: @interface IOBluetoothHostController (LionSDK) here. https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... File chrome/browser/mac/bluetooth_utility.mm (right): https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:17: const char* kBluetoothServiceKey = "IOBluetoothHCIController"; These constants don’t really need to exist outside of the GetBluetoothAvailability function. All of these constants are single-use. It’s fine if you want to have constants for them, but put them as close to their points of use as possible. https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:71: return BLUETOOTH_AVAILABLE_WITH_LE; For histogramming purposes, I think you want to break this out into its own bucket of “10.6 and probably LE-capable.” https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:76: if (!data || [data length] != kSupportedFeaturesSize) Don’t check the size, there’s evidence in the header that the length can be longer than 8 bytes, where it says “Byte 8 of the support features data structure (extended)”. The header talking about interpreting this byte-by-byte and the fact that it’s presented as NSData are both indications that you don’t actually treat it as an unsigned long long (as your comment says), it’s just a byte array. https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:80: // We want the fifth byte, which is at index 3. Something doesn’t add up here, but it’s probably because you’re mixing 0-based and 1-based counting. The header says that the bit you’re looking for is in 0-based byte 4. (Actually, this indicates that you’d want bytes[4], not bytes[3]…)
mark: PTAL https://codereview.chromium.org/374203004/diff/200001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/374203004/diff/200001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:25: kBluetoothFeatureLESupportedController = (1 << 6L), On 2014/07/11 18:05:04, Mark Mentovai wrote: > Can you put this down with the other new Bluetooth stuff from the 10.7 SDK? Done. https://codereview.chromium.org/374203004/diff/200001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:162: @interface IOBluetoothHostController (LionSDK) On 2014/07/11 18:05:04, Mark Mentovai wrote: > here. Done. https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... File chrome/browser/mac/bluetooth_utility.mm (right): https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:17: const char* kBluetoothServiceKey = "IOBluetoothHCIController"; On 2014/07/11 18:05:04, Mark Mentovai wrote: > These constants don’t really need to exist outside of the > GetBluetoothAvailability function. All of these constants are single-use. It’s > fine if you want to have constants for them, but put them as close to their > points of use as possible. Done. https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:71: return BLUETOOTH_AVAILABLE_WITH_LE; On 2014/07/11 18:05:04, Mark Mentovai wrote: > For histogramming purposes, I think you want to break this out into its own > bucket of “10.6 and probably LE-capable.” Done. https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:76: if (!data || [data length] != kSupportedFeaturesSize) On 2014/07/11 18:05:04, Mark Mentovai wrote: > Don’t check the size, there’s evidence in the header that the length can be > longer than 8 bytes, where it says “Byte 8 of the support features data > structure (extended)”. > > The header talking about interpreting this byte-by-byte and the fact that it’s > presented as NSData are both indications that you don’t actually treat it as an > unsigned long long (as your comment says), it’s just a byte array. Right, fixed. https://codereview.chromium.org/374203004/diff/200001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:80: // We want the fifth byte, which is at index 3. On 2014/07/11 18:05:04, Mark Mentovai wrote: > Something doesn’t add up here, but it’s probably because you’re mixing 0-based > and 1-based counting. The header says that the bit you’re looking for is in > 0-based byte 4. (Actually, this indicates that you’d want bytes[4], not > bytes[3]…) The index is interpreted in reverse order (or so we can hope).
Histograms LGTM, thanks. https://codereview.chromium.org/374203004/diff/240001/chrome/browser/mac/blue... File chrome/browser/mac/bluetooth_utility.h (right): https://codereview.chromium.org/374203004/diff/240001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.h:17: // On OSX 10.6, if the LMP version supports LE, there is no further nit: Probably worth spelling out the acronym "LMP", so that somebody trying to learn about the relevant concepts has a better chance of finding hits.
https://codereview.chromium.org/374203004/diff/240001/chrome/browser/mac/blue... File chrome/browser/mac/bluetooth_utility.h (right): https://codereview.chromium.org/374203004/diff/240001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.h:17: // On OSX 10.6, if the LMP version supports LE, there is no further On 2014/07/11 19:03:51, Ilya Sherman wrote: > nit: Probably worth spelling out the acronym "LMP", so that somebody trying to > learn about the relevant concepts has a better chance of finding hits. Done.
LGTM with these changes. https://codereview.chromium.org/374203004/diff/260001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/374203004/diff/260001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:159: enum { This does not belong in an @interface. https://codereview.chromium.org/374203004/diff/260001/chrome/browser/mac/blue... File chrome/browser/mac/bluetooth_utility.mm (right): https://codereview.chromium.org/374203004/diff/260001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:58: le_unknown = true; You can return BLUETOOTH_AVAILABLE_LE_UNKNOWN right here, and get rid of the le_unknown variable. The OS isn’t going to change from 10.6 if you make another pass through the loop.
https://codereview.chromium.org/374203004/diff/260001/base/mac/sdk_forward_de... File base/mac/sdk_forward_declarations.h (right): https://codereview.chromium.org/374203004/diff/260001/base/mac/sdk_forward_de... base/mac/sdk_forward_declarations.h:159: enum { On 2014/07/11 22:26:29, Mark Mentovai wrote: > This does not belong in an @interface. Done. https://codereview.chromium.org/374203004/diff/260001/chrome/browser/mac/blue... File chrome/browser/mac/bluetooth_utility.mm (right): https://codereview.chromium.org/374203004/diff/260001/chrome/browser/mac/blue... chrome/browser/mac/bluetooth_utility.mm:58: le_unknown = true; On 2014/07/11 22:26:29, Mark Mentovai wrote: > You can return BLUETOOTH_AVAILABLE_LE_UNKNOWN right here, and get rid of the > le_unknown variable. The OS isn’t going to change from 10.6 if you make another > pass through the loop. Done.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikchen@chromium.org/374203004/300001
Message was sent while issue was closed.
Change committed as 282771 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
