Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10824162:
add device notification to Mac (Closed)
|
Created:
8 years, 4 months ago by no longer working on chromium Modified:
8 years, 4 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Lei Zhang Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd DeviceMonitorMac to BrowserMainLoop.
DeviceMonitorMac detects device changing and forwards the notifications to the system monitor.
BUG=137799
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151689
Patch Set 1 #
Total comments: 18
Patch Set 2 : addressed Tommi's comments #
Total comments: 11
Patch Set 3 : address the comments from Wei and Lei #
Total comments: 22
Patch Set 4 : addressed Avi's comments and reduced the dupped code. #Patch Set 5 : delete the monitors before system_monitor_ in browser_main_loop_. #
Total comments: 18
Patch Set 6 : addressed avi's comments and wei's comments. #
Total comments: 21
Patch Set 7 : addressed Mark's comments #Patch Set 8 : moved the .mm to .cc, changed const struct back to stuct #
Total comments: 2
Patch Set 9 : use kIOAudioDeviceClassName to cover the bluetooth devices and changed to use std::vector #
Total comments: 15
Patch Set 10 : addressed Mark's comments. #Patch Set 11 : updated a comment. #Patch Set 12 : fixed linux bots. #
Messages
Total messages: 38 (0 generated)
Hello, This is part for the device notification, a feature we are going to deliver for M22. Wei and Tommi have shipped the linux and windows code, and here comes the mac code. I borrowed the reviewer list from Wei's CL, please let me know if I should involve others. Please help reviewing ASAP. Thanks, BR, -SX
rubberstamp lgtm with the below fixed. I'm not qualified to review the mac code unfortunately, hence the rubberstamp. https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... File content/browser/device_monitor_mac.h (right): https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.h:17: virtual ~DeviceMonitorMac(); no need for virtual https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.h:19: void NotifyAudioDeviceChanged(); private? https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... File content/browser/device_monitor_mac.mm (right): https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.mm:10: #include <CoreFoundation/CFNumber.h> shouldn't these be above the chrome includes? https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.mm:36: dictionary = (CFMutableDictionaryRef)(CFRetain(dictionary)); fix cast or is this how things are done in mm land? :) https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.mm:39: void VideoDeviceChangedCallback(void *context, io_iterator_t devices) { void* context. In order to allow this method to call the private NotifyVideoDeviceChanged method (assuming you do that), you should make this a static method in the class. https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.mm:49: void AudioDeviceChangedCallback(void *context, io_iterator_t devices) { same as above. https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.mm:62: CFRunLoopRef runloop = CFRunLoopGetCurrent (); no space before () https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.mm:67: IONotificationPortCreate (kIOMasterPortDefault); same here. please fix throughout https://chromiumcodereview.appspot.com/10824162/diff/1/content/browser/device... content/browser/device_monitor_mac.mm:154: NOTREACHED() << "Failed to register the audio IO terminated notification"; fix indent
Many thanks to Tommi for reviewing the CL. I have already addressed all the comments. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... File content/browser/device_monitor_mac.h (right): http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.h:17: virtual ~DeviceMonitorMac(); On 2012/08/03 11:24:29, tommi wrote: > no need for virtual Done. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.h:19: void NotifyAudioDeviceChanged(); On 2012/08/03 11:24:29, tommi wrote: > private? Done. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.mm:10: #include <CoreFoundation/CFNumber.h> On 2012/08/03 11:24:29, tommi wrote: > shouldn't these be above the chrome includes? done. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.mm:36: dictionary = (CFMutableDictionaryRef)(CFRetain(dictionary)); On 2012/08/03 11:24:29, tommi wrote: > fix cast or is this how things are done in mm land? :) neither static_cast nor reinterpret_cast passes the compiler, though I am not sure if it is correct. Lets wait for comments from other reviewers. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.mm:39: void VideoDeviceChangedCallback(void *context, io_iterator_t devices) { On 2012/08/03 11:24:29, tommi wrote: > void* context. > > In order to allow this method to call the private NotifyVideoDeviceChanged > method (assuming you do that), you should make this a static method in the > class. Done. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.mm:49: void AudioDeviceChangedCallback(void *context, io_iterator_t devices) { On 2012/08/03 11:24:29, tommi wrote: > same as above. Done. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.mm:62: CFRunLoopRef runloop = CFRunLoopGetCurrent (); On 2012/08/03 11:24:29, tommi wrote: > no space before () Done. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.mm:67: IONotificationPortCreate (kIOMasterPortDefault); On 2012/08/03 11:24:29, tommi wrote: > same here. please fix throughout Done. http://codereview.chromium.org/10824162/diff/1/content/browser/device_monitor... content/browser/device_monitor_mac.mm:154: NOTREACHED() << "Failed to register the audio IO terminated notification"; On 2012/08/03 11:24:29, tommi wrote: > fix indent Done.
Nice work! I will leave Mac platform specific stuff Mac experts. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... File content/browser/device_monitor_mac.h (right): http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.h:27: void NotifyVideoDeviceChanged(); It seems you can move all these 6 private functions into anonymous namespace in cc file. This class doesn't have any member anyway. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:97: void DeviceMonitorMac::RegisterAudioCallbacks(IONotificationPortRef port) { It seems this function is very similar to RegisterVideoCallbacks() except several parameters, such as interface_class_code, interface_subclass_code and AudioDeviceChangedCallback. It might be better to consolidate them into one function with those parameters as arguments. That can avoid code dup. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:147: InstanceFromContext(context)->NotifyVideoDeviceChanged(); At this point, the type is known. It seems you need only one NotifyDevicesChanged(base::SystemMonitor::DeviceType type). Then you can call InstanceFromContext(context)->NotifyDevicesChanged() with the corresponding type. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:165: base::SystemMonitor* monitor = base::SystemMonitor::Get(); no need to have intermediate variable.
Please find a mac expert to review the .mm file. http://codereview.chromium.org/10824162/diff/3002/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/10824162/diff/3002/content/content_browser.gyp... content/content_browser.gypi:279: 'browser/device_monitor_mac.mm', nit: alphabetical order
http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:131: this, if the type, instead of |this|, is used here, you can get "type" when DevicesChangedCallback is called. Then you need only one CB. This can also enable you create an array of devices and make future expansion much easier.
Adding Avi for the Mac specific code. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... File content/browser/device_monitor_mac.h (right): http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.h:27: void NotifyVideoDeviceChanged(); On 2012/08/03 13:50:08, wjia wrote: > It seems you can move all these 6 private functions into anonymous namespace in > cc file. This class doesn't have any member anyway. Thanks for the ideas. Yes, we can. But then the class becomes hard to be extended for some other uses who want to use this class for other use cases. For example, gamepad, they might need to store some members to be able to filter out the notifications. I personally prefer this way. Hope it is fine. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:97: void DeviceMonitorMac::RegisterAudioCallbacks(IONotificationPortRef port) { On 2012/08/03 13:50:08, wjia wrote: > It seems this function is very similar to RegisterVideoCallbacks() except > several parameters, such as interface_class_code, interface_subclass_code and > AudioDeviceChangedCallback. It might be better to consolidate them into one > function with those parameters as arguments. That can avoid code dup. Done. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:131: this, On 2012/08/03 15:21:05, wjia wrote: > if the type, instead of |this|, is used here, you can get "type" when > DevicesChangedCallback is called. Then you need only one CB. This can also > enable you create an array of devices and make future expansion much easier. In this case, we can't access to member functions. I don't think an array can easily cover all the cases for the future expansion. For example, for webrtc we only needs one callback for device plug/unplug, but others might want to separate them. GIven the cut is in only two work days, I hope it is fine to skip the array thing. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:147: InstanceFromContext(context)->NotifyVideoDeviceChanged(); On 2012/08/03 13:50:08, wjia wrote: > At this point, the type is known. It seems you need only one > NotifyDevicesChanged(base::SystemMonitor::DeviceType type). Then you can call > InstanceFromContext(context)->NotifyDevicesChanged() with the corresponding > type. Done. http://codereview.chromium.org/10824162/diff/3002/content/browser/device_moni... content/browser/device_monitor_mac.mm:165: base::SystemMonitor* monitor = base::SystemMonitor::Get(); On 2012/08/03 13:50:08, wjia wrote: > no need to have intermediate variable. Done.
Added thakis, and hope either of Avi or thakis can help reviewing the Mac code.
Since Avi is a content OWNER, I'll leave this to him.
http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:17: DeviceMonitorMac* InstanceFromContext(void* context) { Does this help in readability? I'd think folding it into the two callers is clearer. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:21: void AddValueToDictionary(CFMutableDictionaryRef dictionary, Ditto. If you use scoped_cftyperef, this collapses to two lines, which would make CreateMachingDictionary much easier to read. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:24: CFNumberRef number_ref = CFNumberCreate(kCFAllocatorDefault, scoped_cftyperef. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:36: CFMutableDictionaryRef CreateMachingDictionary(SInt32 interface_class_code, s/CreateMachingDictionary/CreateMatchingDictionary/ http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:55: dictionary = (CFMutableDictionaryRef)(CFRetain(dictionary)); Huh? Just do CFRetain(dictionary); Why do you feel the need to capture the return value? http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:72: io_object_t thisObject; base::mac::ScopedIOObject<io_object_t>? Also, this is a C function; use this_object. For reference, look up the use of IOIteratorNext in chrome/browser/mac/install_from_dmg.mm to see the style. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:81: CFRetain(runloop); Why the retain? http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:85: IONotificationPortCreate(kIOMasterPortDefault); Where is the matching IONotificationPortDestroy? http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:136: while ((thisObject = IOIteratorNext(devices))) { See previous note about IOIteratorNext. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:148: while ((thisObject = IOIteratorNext(devices))) { Ditto.
Since you talked about timeline for this patch, I spent less than an hour to create http://codereview.chromium.org/10827163/ which makes future expansion much easier and has very little dupped code. In such way, we have consistent handling on all platforms. I didn't change Mac specific stuff. You can combine it with Avi's comments in the new patch.
Avi and Wei, could you please take another look. I am discussing with Wei offline on if a struct works well for this case. BR, -SX http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:17: DeviceMonitorMac* InstanceFromContext(void* context) { On 2012/08/03 19:41:23, Avi wrote: > Does this help in readability? I'd think folding it into the two callers is > clearer. Done. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:21: void AddValueToDictionary(CFMutableDictionaryRef dictionary, On 2012/08/03 19:41:23, Avi wrote: > Ditto. If you use scoped_cftyperef, this collapses to two lines, which would > make CreateMachingDictionary much easier to read. Done. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:24: CFNumberRef number_ref = CFNumberCreate(kCFAllocatorDefault, On 2012/08/03 19:41:23, Avi wrote: > scoped_cftyperef. Done. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:36: CFMutableDictionaryRef CreateMachingDictionary(SInt32 interface_class_code, On 2012/08/03 19:41:23, Avi wrote: > s/CreateMachingDictionary/CreateMatchingDictionary/ Done. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:55: dictionary = (CFMutableDictionaryRef)(CFRetain(dictionary)); From the silly example code, removed. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:72: io_object_t thisObject; On 2012/08/03 19:41:23, Avi wrote: > base::mac::ScopedIOObject<io_object_t>? Also, this is a C function; use > this_object. > > For reference, look up the use of IOIteratorNext in > chrome/browser/mac/install_from_dmg.mm to see the style. Done. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:81: CFRetain(runloop); On 2012/08/03 19:41:23, Avi wrote: > Why the retain? Sorry, uncleaned debugging code. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:85: IONotificationPortCreate(kIOMasterPortDefault); On 2012/08/03 19:41:23, Avi wrote: > Where is the matching IONotificationPortDestroy? Oh, thanks for pointing out, that example code I was look at does not call IONotificationPortDestroy, so I missed it. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:136: while ((thisObject = IOIteratorNext(devices))) { On 2012/08/03 19:41:23, Avi wrote: > See previous note about IOIteratorNext. Done. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:148: while ((thisObject = IOIteratorNext(devices))) { On 2012/08/03 19:41:23, Avi wrote: > Ditto. Done.
Multiple comments that I made on a previous snapshot and that were marked as fixed were not, in fact, fixed. Please fix them. More issues, too. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:55: dictionary = (CFMutableDictionaryRef)(CFRetain(dictionary)); This is not removed in the latest snapshot. http://codereview.chromium.org/10824162/diff/8002/content/browser/device_moni... content/browser/device_monitor_mac.mm:81: CFRetain(runloop); This isn't fixed in the latest snapshot either. http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:7: #include <CoreFoundation/CFNumber.h> You don't need to include this; the include below of CF/CF.h includes it transitively. http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:46: dictionary = (CFMutableDictionaryRef)(CFRetain(dictionary)); I already commented on this. Why do you feel you need to grab the return value of CFRetain? http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:48: io_iterator_t devices_iterator = 0; The iterator itself needs to be a base::mac::ScopedIOObject, else you will leak it. Again, I point you to chrome/browser/mac/install_from_dmg.mm. http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:56: NOTREACHED() << "Failed to register the IO mached notification for type " s/mached/matched/ http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:65: for (; this_object; this_object.reset(IOIteratorNext(devices_iterator))); Do we want to do something with the objects we're pulling off here? http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:91: CFRetain(runloop); Why are you retaining this? Please address my earlier concerns. http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:142: IOObjectRelease(this_object); Wha? No, the ScopedIOObject releases. http://codereview.chromium.org/10824162/diff/16001/content/browser/device_mon... content/browser/device_monitor_mac.mm:154: IOObjectRelease(this_object); Ditto.
Avi and Wei, could you please take another look. I think I have addressed all the comments this time. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... File content/browser/device_monitor_mac.mm (right): https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:7: #include <CoreFoundation/CFNumber.h> On 2012/08/06 13:54:37, Avi wrote: > You don't need to include this; the include below of CF/CF.h includes it > transitively. Done. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:46: dictionary = (CFMutableDictionaryRef)(CFRetain(dictionary)); On 2012/08/06 13:54:37, Avi wrote: > I already commented on this. Why do you feel you need to grab the return value > of CFRetain? I should have removed it from the previous set of code. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:48: io_iterator_t devices_iterator = 0; On 2012/08/06 13:54:37, Avi wrote: > The iterator itself needs to be a base::mac::ScopedIOObject, else you will leak > it. Again, I point you to chrome/browser/mac/install_from_dmg.mm. Thanks for pointing out, this does leak. I have to say that I don't know much about IOKit, but I looked at quite some apple examples as reference, but some of them never release this iterator explicitly while some of them release the object when stopping watching. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:56: NOTREACHED() << "Failed to register the IO mached notification for type " On 2012/08/06 13:54:37, Avi wrote: > s/mached/matched/ Done. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:65: for (; this_object; this_object.reset(IOIteratorNext(devices_iterator))); On 2012/08/06 13:54:37, Avi wrote: > Do we want to do something with the objects we're pulling off here? My understanding here is simply loop through the existing devices. Do you think we should do something more? https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:91: CFRetain(runloop); On 2012/08/06 13:54:37, Avi wrote: > Why are you retaining this? Please address my earlier concerns. This should have been removed too. Sorry. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:142: IOObjectRelease(this_object); On 2012/08/06 13:54:37, Avi wrote: > Wha? No, the ScopedIOObject releases. Done. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:154: IOObjectRelease(this_object); On 2012/08/06 13:54:37, Avi wrote: > Ditto. Done.
Mac code LGTM. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... File content/browser/device_monitor_mac.mm (right): https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:65: for (; this_object; this_object.reset(IOIteratorNext(devices_iterator))); Do we need to fire a first-time changed callback? I don't know your needs. If not then we're fine here. https://chromiumcodereview.appspot.com/10824162/diff/17007/content/browser/de... File content/browser/device_monitor_mac.mm (right): https://chromiumcodereview.appspot.com/10824162/diff/17007/content/browser/de... content/browser/device_monitor_mac.mm:93: IOObjectRelease(notification_iterators_[i]); Aha, cool.
Many thanks to avi. https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... File content/browser/device_monitor_mac.mm (right): https://chromiumcodereview.appspot.com/10824162/diff/16001/content/browser/de... content/browser/device_monitor_mac.mm:65: for (; this_object; this_object.reset(IOIteratorNext(devices_iterator))); On 2012/08/06 20:29:47, Avi wrote: > Do we need to fire a first-time changed callback? I don't know your needs. If > not then we're fine here. This for loop does the same as calling a first-time changed callback like deviceChangedCallback(NULL, devices_iterator) So we don't need to call again.
http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Why is this a .mm file? You haven’t used any Objective-C. It should be a .cc file. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:17: struct { const? http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:19: const io_name_t service_type; …but this doesn’t really need to be const http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:54: kern_return_t err = IOServiceAddMatchingNotification(port, Weird indentation. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:71: for (; this_object; this_object.reset(IOIteratorNext(*notification))); Use {} to show an empty body. Also see the comment at line 136. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:77: CFRunLoopRef runloop = CFRunLoopGetCurrent(); You can save doing this until you’re ready to add the source to the run loop. Or you can just write CFRunLoopGetCurrent() in-line in the CFRunLoopAddSource call, as you did with CFRunLoopRemoveSource in the destructor. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:92: for (size_t i = 0; i < arraysize(kDeviceServices); ++i) { This looks a little freaky. Why don’t you use a std::vector<io_iterator_t> that you can push your iterators on to, and then use notification_iterators_.size() here. Then if kDeviceServices gets screwed up and out of sync with RegisterServices, you don’t crash because notification_iterators and kDeviceServices don‘t line up. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:136: for (; this_object; this_object.reset(IOIteratorNext(iterator))) { I’d write the initialization inside the body of the for, like for (base::mac::ScopedIOObject<io_service_t> object(IOIteratorNext(iterator)); object; object.reset(IOIteratorNext(iterator))) { Also on line 71.
http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:109: switch (kDeviceServices[i].device_type) { After chatting with mmentovai@, my understanding is most of devices are handled in the same way as audio and video devices. You can include interface_class_code and subclass_code in struct for kDeviceServices[]. Having switch here defies the purpose of having kDeviceServices.
http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/08/07 18:39:10, Mark Mentovai wrote: > Why is this a .mm file? You haven’t used any Objective-C. It should be a .cc > file. It is a following design from gamepad/platform_data_fetcher_mac.mm. I can change it to .cc file. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:17: struct { On 2012/08/07 18:39:10, Mark Mentovai wrote: > const? Done. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:19: const io_name_t service_type; On 2012/08/07 18:39:10, Mark Mentovai wrote: > …but this doesn’t really need to be const I think it is a c_string, isn't it better to put it const unless we use const struct? http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:54: kern_return_t err = IOServiceAddMatchingNotification(port, On 2012/08/07 18:39:10, Mark Mentovai wrote: > Weird indentation. thanks. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:71: for (; this_object; this_object.reset(IOIteratorNext(*notification))); On 2012/08/07 18:39:10, Mark Mentovai wrote: > Use {} to show an empty body. Also see the comment at line 136. Done. I replied the comment at line 136. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:77: CFRunLoopRef runloop = CFRunLoopGetCurrent(); On 2012/08/07 18:39:10, Mark Mentovai wrote: > You can save doing this until you’re ready to add the source to the run loop. Or > you can just write CFRunLoopGetCurrent() in-line in the CFRunLoopAddSource call, > as you did with CFRunLoopRemoveSource in the destructor. Done. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:92: for (size_t i = 0; i < arraysize(kDeviceServices); ++i) { On 2012/08/07 18:39:10, Mark Mentovai wrote: > This looks a little freaky. Why don’t you use a std::vector<io_iterator_t> that > you can push your iterators on to, and then use notification_iterators_.size() > here. > > Then if kDeviceServices gets screwed up and out of sync with RegisterServices, > you don’t crash because notification_iterators and kDeviceServices don‘t line > up. a scoped_array is much more suitable here because it makes the ownership of the memory much clearer. If we use a std::vector, we need to use std::vector<io_iterator_t*> in order to release the memory correctly. And this needs more code and also becomes less readable. On the other hand, it is intentional to keep the KDeviceServices in line with notification_iterators_, in order to avoid leak. But I agree it is good point that there might be cases that KDeviceServices might be out of sync with RegisterServices, so I want to keep the KDeviceServices simple and easy to extend. I have already checked the apple document, the current KDeviceServices structure is a must to be able to add new service to the notifications. Hope it is fine to keep this. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:109: switch (kDeviceServices[i].device_type) { On 2012/08/07 20:57:27, wjia wrote: > After chatting with mmentovai@, my understanding is most of devices are handled > in the same way as audio and video devices. You can include interface_class_code > and subclass_code in struct for kDeviceServices[]. Having switch here defies the > purpose of having kDeviceServices. NO. I thought I had already explained clearly about this offline.The number of matching dictionaries can be varying from 0 to multiple. If you don't believe it, please check out /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/IOKit.framework/Versions/A/Headers or the relevant apple documents. It is never good to define a pattern which limits the user cases to use one class_code and one subclass_code. A very strong supportive point is, as Mark points out his concern about the case where KDeviceServices might be out of sync with RegisterServices. If we add the interface_class_code and subclass_code to the struct, this becomes quite likely in case the new service needs more or less matching syntact, users might skip the KDeviceServices and add their own code. http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:136: for (; this_object; this_object.reset(IOIteratorNext(iterator))) { On 2012/08/07 18:39:10, Mark Mentovai wrote: > I’d write the initialization inside the body of the for, like > > for (base::mac::ScopedIOObject<io_service_t> > object(IOIteratorNext(iterator)); > object; > object.reset(IOIteratorNext(iterator))) { > > Also on line 71. :) I think I have got different opinions on this style thing. I used to do it in the way you recommended, but in some of my early CLs, I was asked to extract the declaration out of the for loop if it does not fit in one line. And their opinion is that it helps readability. I am pretty OK to change it if you have a strong opinion on this, please just let me know.
http://codereview.chromium.org/10824162/diff/4013/content/browser/device_moni... File content/browser/device_monitor_mac.cc (right): http://codereview.chromium.org/10824162/diff/4013/content/browser/device_moni... content/browser/device_monitor_mac.cc:17: struct { We can't use const struct here because we need to pass the memory of device_type to the callback, see line 127 static_cast<void*>(&kDeviceServices[i].device_type). The purposes of doing this: # Tell what kind of device has been changed. # Use one single callback to handle different services, in favor of avoiding dupped code.
http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... File content/browser/device_monitor_mac.mm (right): http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:92: for (size_t i = 0; i < arraysize(kDeviceServices); ++i) { Aside from the ability to resize a std::vector (which you could use in this case), the std::vector<io_iterator_t*> is equivalent in usage to a scoped_array<io_iterator_t>. How is the vector “less readable” or “more bad?” http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... content/browser/device_monitor_mac.mm:136: for (; this_object; this_object.reset(IOIteratorNext(iterator))) { Whoever’s opinion you got in the past on this was wrong. :) The scoping and lifetime are much clearer when the declaration is inside the loop declaration. And you can resolve the “doesn’t fit” complaint by choosing a shorter name: “object” instead of “this_object” as I’ve recommended. That’s a good idea anyway, since you don’t have a “that_object”. You can also call it “service” since you’re dealing with an io_service_t.
http://codereview.chromium.org/10824162/diff/4013/content/browser/device_moni... File content/browser/device_monitor_mac.cc (right): http://codereview.chromium.org/10824162/diff/4013/content/browser/device_moni... content/browser/device_monitor_mac.cc:17: struct { On 2012/08/08 09:23:16, xians1 wrote: > We can't use const struct here because we need to pass the memory of device_type > to the callback, see line 127 > static_cast<void*>(&kDeviceServices[i].device_type). That’s exactly the problem I was illustrating. It makes it way too easy for the callback to accidentally change this table. Why are you passing the address? The callback only needs to see the value.
On 2012/08/08 08:42:42, xians1 wrote: > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > File content/browser/device_monitor_mac.mm (right): > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:1: // Copyright (c) 2012 The Chromium > Authors. All rights reserved. > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > Why is this a .mm file? You haven’t used any Objective-C. It should be a .cc > > file. > > It is a following design from gamepad/platform_data_fetcher_mac.mm. > I can change it to .cc file. > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:17: struct { > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > const? > > Done. > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:19: const io_name_t service_type; > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > …but this doesn’t really need to be const > > I think it is a c_string, isn't it better to put it const unless we use const > struct? > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:54: kern_return_t err = > IOServiceAddMatchingNotification(port, > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > Weird indentation. > > thanks. > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:71: for (; this_object; > this_object.reset(IOIteratorNext(*notification))); > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > Use {} to show an empty body. Also see the comment at line 136. > > Done. > > I replied the comment at line 136. > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:77: CFRunLoopRef runloop = > CFRunLoopGetCurrent(); > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > You can save doing this until you’re ready to add the source to the run loop. > Or > > you can just write CFRunLoopGetCurrent() in-line in the CFRunLoopAddSource > call, > > as you did with CFRunLoopRemoveSource in the destructor. > > Done. > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:92: for (size_t i = 0; i < > arraysize(kDeviceServices); ++i) { > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > This looks a little freaky. Why don’t you use a std::vector<io_iterator_t> > that > > you can push your iterators on to, and then use notification_iterators_.size() > > here. > > > > Then if kDeviceServices gets screwed up and out of sync with RegisterServices, > > you don’t crash because notification_iterators and kDeviceServices don‘t line > > up. > > a scoped_array is much more suitable here because it makes the ownership of the > memory much clearer. > > If we use a std::vector, we need to use std::vector<io_iterator_t*> in order to > release the memory correctly. And this needs more code and also becomes less > readable. > > On the other hand, it is intentional to keep the KDeviceServices in line with > notification_iterators_, in order to avoid leak. > > But I agree it is good point that there might be cases that KDeviceServices > might be out of sync with RegisterServices, so I want to keep the > KDeviceServices simple and easy to extend. I have already checked the apple > document, the current KDeviceServices structure is a must to be able to add new > service to the notifications. > > Hope it is fine to keep this. > > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:109: switch > (kDeviceServices[i].device_type) { > On 2012/08/07 20:57:27, wjia wrote: > > After chatting with mmentovai@, my understanding is most of devices are > handled > > in the same way as audio and video devices. You can include > interface_class_code > > and subclass_code in struct for kDeviceServices[]. Having switch here defies > the > > purpose of having kDeviceServices. > > NO. > I thought I had already explained clearly about this offline.The number of > matching dictionaries can be varying from 0 to multiple. If you don't believe > it, please check out > /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/IOKit.framework/Versions/A/Headers > or the relevant apple documents. > > It is never good to define a pattern which limits the user cases to use one > class_code and one subclass_code. > A very strong supportive point is, as Mark points out his concern about the case > where KDeviceServices might be out of sync with RegisterServices. If we add the > interface_class_code and subclass_code to the struct, this becomes quite likely > in case the new service needs more or less matching syntact, users might skip > the KDeviceServices and add their own code. > Please talk to Mark. The question is: if we want to monitor change of a type of devices, could that type of devices be identified by triplet {interface_class_code, subclass_code, notification_code}? We can use an array without device specific handling on Windows (http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/system_messag...) and Linux (http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/device_monito...). > http://codereview.chromium.org/10824162/diff/17007/content/browser/device_mon... > content/browser/device_monitor_mac.mm:136: for (; this_object; > this_object.reset(IOIteratorNext(iterator))) { > On 2012/08/07 18:39:10, Mark Mentovai wrote: > > I’d write the initialization inside the body of the for, like > > > > for (base::mac::ScopedIOObject<io_service_t> > > object(IOIteratorNext(iterator)); > > object; > > object.reset(IOIteratorNext(iterator))) { > > > > Also on line 71. > > :) I think I have got different opinions on this style thing. I used to do it in > the way you recommended, but in some of my early CLs, I was asked to extract the > declaration out of the for loop if it does not fit in one line. And their > opinion is that it helps readability. > > I am pretty OK to change it if you have a strong opinion on this, please just > let me know.
Mark, Could you please take another look at the new version. The changes include: # Use kIOAudioDeviceClassName as the service class name so that it monitors the bluetooth devices. # Changed kIOMatchedNotification to kIOFirstPublishNotification, so that it is guaranteed that we will not get duplicate notification for the same action. # Changed scoped_array to std::vector # Passed |this| to the callback instead of device_type, so that we don't rely on the global variable of system monitor. # Moved the declaration of the variable inside for loop. BR, -SX
LGTM with these minor changes. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... File content/browser/device_monitor_mac.cc (right): http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:16: namespace { We also usually put a blank line after opening a namespace, before the kServices array. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:24: CFMutableDictionaryRef matching_dictionary = IOServiceMatching( This is more legible when you break the line after the =, as in CFMutableDictionaryRef matching_dictionary = IOServiceMatching(kIOUSBInterfaceClassName); http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:71: But we don’t usually begin functions (or constructors) with blank lines. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:91: // Remove the sleep notification port from the application runloop. This seems like a copy-paste job. This code has nothing to do with sleep. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:101: CFMutableDictionaryRef dictionary = IOServiceMatching( Rewrap for readability as I suggested on line 24. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:116: // Retain |arraysize(kServices) -1| additional dictionary references because I don’t understand why you only do this for arraysize(kServices) - 1. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... File content/browser/device_monitor_mac.h (right): http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.h:9: #include <vector> Chrome code usually puts a blank line between the C system header section (<IOKit/IOKitLib.h>) and the C++ system header section (<vector>).
Thanks Mark. I am going to land it soon. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... File content/browser/device_monitor_mac.cc (right): http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:16: namespace { On 2012/08/14 15:37:05, Mark Mentovai wrote: > We also usually put a blank line after opening a namespace, before the kServices > array. Done. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:24: CFMutableDictionaryRef matching_dictionary = IOServiceMatching( On 2012/08/14 15:37:05, Mark Mentovai wrote: > This is more legible when you break the line after the =, as in > > CFMutableDictionaryRef matching_dictionary = > IOServiceMatching(kIOUSBInterfaceClassName); Done. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:71: On 2012/08/14 15:37:05, Mark Mentovai wrote: > But we don’t usually begin functions (or constructors) with blank lines. Done. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:91: // Remove the sleep notification port from the application runloop. On 2012/08/14 15:37:05, Mark Mentovai wrote: > This seems like a copy-paste job. This code has nothing to do with sleep. Done. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:101: CFMutableDictionaryRef dictionary = IOServiceMatching( On 2012/08/14 15:37:05, Mark Mentovai wrote: > Rewrap for readability as I suggested on line 24. Done. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:116: // Retain |arraysize(kServices) -1| additional dictionary references because On 2012/08/14 15:37:05, Mark Mentovai wrote: > I don’t understand why you only do this for arraysize(kServices) - 1. When creating the CFMutableDictionaryRef, the ref counter has increased to 1. So calling |arraysize(kServices) - 1| times CFRetain will increase the ref counter to |arraysize(kServices)|, which is exactly the number of times we call IOServiceAddMatchingNotification. http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... File content/browser/device_monitor_mac.h (right): http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.h:9: #include <vector> On 2012/08/14 15:37:05, Mark Mentovai wrote: > Chrome code usually puts a blank line between the C system header section > (<IOKit/IOKitLib.h>) and the C++ system header section (<vector>). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10824162/18011
http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... File content/browser/device_monitor_mac.cc (right): http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... content/browser/device_monitor_mac.cc:116: // Retain |arraysize(kServices) -1| additional dictionary references because xians1 wrote: > On 2012/08/14 15:37:05, Mark Mentovai wrote: > > I don’t understand why you only do this for arraysize(kServices) - 1. > > When creating the CFMutableDictionaryRef, the ref counter has increased to 1. > So calling |arraysize(kServices) - 1| times CFRetain will increase the ref > counter to |arraysize(kServices)|, which is exactly the number of times we call > IOServiceAddMatchingNotification. OK, so the missing piece (in my mind) was that dictionary comes in with a retain count of 1, which will be consumed by a single RegisterCallbackToIOService call, and you want to retain the dictionary once for each of the other calls. Can you reword the comment slightly to make this clearer? Your e-mail explanation was good, but the comment here didn’t make the “comes in with retain count 1, and that gets used” aspect clear.
On 2012/08/14 16:02:52, Mark Mentovai wrote: > http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... > File content/browser/device_monitor_mac.cc (right): > > http://codereview.chromium.org/10824162/diff/18008/content/browser/device_mon... > content/browser/device_monitor_mac.cc:116: // Retain |arraysize(kServices) -1| > additional dictionary references because > xians1 wrote: > > On 2012/08/14 15:37:05, Mark Mentovai wrote: > > > I don’t understand why you only do this for arraysize(kServices) - 1. > > > > When creating the CFMutableDictionaryRef, the ref counter has increased to 1. > > So calling |arraysize(kServices) - 1| times CFRetain will increase the ref > > counter to |arraysize(kServices)|, which is exactly the number of times we > call > > IOServiceAddMatchingNotification. > > OK, so the missing piece (in my mind) was that dictionary comes in with a retain > count of 1, which will be consumed by a single RegisterCallbackToIOService call, > and you want to retain the dictionary once for each of the other calls. > > Can you reword the comment slightly to make this clearer? Your e-mail > explanation was good, but the comment here didn’t make the “comes in with retain > count 1, and that gets used” aspect clear. Done with updating the comment.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10824162/18013
LGTM
Try job failure for 10824162-18013 (retry) on linux_rel for steps "interactive_ui_tests, browser_tests, content_browsertests". It's a second try, previously, steps "interactive_ui_tests, browser_tests, content_browsertests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10824162/18013
Try job failure for 10824162-18013 (retry) (retry) on linux_chromeos for steps "interactive_ui_tests, browser_tests, content_browsertests". It's a second try, previously, steps "interactive_ui_tests, browser_tests, content_browsertests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10824162/18013
Try job failure for 10824162-18013 (retry) on linux_rel for steps "interactive_ui_tests, browser_tests, content_browsertests". It's a second try, previously, steps "interactive_ui_tests, browser_tests, content_browsertests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10824162/16010
Change committed as 151689 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
