Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10919185:
Implement RemovableDeviceNotifications for Mac (Closed)
|
Created:
8 years, 3 months ago by sail Modified:
8 years, 3 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement RemovableDeviceNotifications for Mac
This CL adds a class to monitor disks being added, removed or changed. When ever such events happen notifications are posted through base::SystemMonitor.
BUG=110400, 110823
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157002
Patch Set 1 #Patch Set 2 : fix threading #
Total comments: 36
Patch Set 3 : add tests and address review comments #Patch Set 4 : fix device id #
Total comments: 6
Patch Set 5 : only check DCIM for removable #Patch Set 6 : address review comments #
Total comments: 2
Patch Set 7 : address review comments #
Total comments: 11
Patch Set 8 : address review comments #Patch Set 9 : address review comments #Patch Set 10 : rebase #Patch Set 11 : fix test #Patch Set 12 : address review comments #Messages
Total messages: 43 (0 generated)
Hi Steve, here's a first shot at the Mac implementation. I'm still missing tests but I wanted to send this out early to make sure you're ok with this approach. Also, it looks like the Linux implementation might have threading issues. For example, it calls IsMediaDevice() then posts a notification. If it's on a file thread then the receivers of the notifications will crash. Otherwise IsMediaDevice() will DCHECK. Thanks, Sailesh
Looks reasonable, but this is the first time I've see the DA api, so I'm not sure my judgement means much. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:16: // thread safe and must only be used from the file thread. Looks like only the static function isn't threadsafe. Maybe move the comment to there. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:40: } nit: gets a bit shorter with early outs: if (!is_removable) return fixed_mass; if (has_dcim) return removable_dcim return removable_no_dcim https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:58: info.bsd_name_ = base::SysCFStringRefToUTF8(bsd_name); I'd do the null checking here instead of changing the base funciton i think https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:25: void UpdateDisk(const DiskInfoMac& info, bool should_remove); nit: enums are more readable than bools https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:134: info.type() == MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM); the other file mentioned FIXED_MASS_FOO. that's not needed here?
On 2012/09/10 06:35:51, sail wrote: > Hi Steve, here's a first shot at the Mac implementation. I'm still missing tests > but I wanted to send this out early to make sure you're ok with this approach. Thanks for tackling this! > Also, it looks like the Linux implementation might have threading issues. For > example, it calls IsMediaDevice() then posts a notification. If it's on a file > thread then the receivers of the notifications will crash. Otherwise > IsMediaDevice() will DCHECK. The Linux implementation lives solely on the File thread. System monitor uses ObserverListThreadSafe, so it can receive notifications on any thread. I think the only constraint we need to keep in mind is that notifications of device arrival and departure have to come from the same thread for removable_storage_map_... I guess we should document what thread GetAttachedRemovableStorage() can be called on and then for consistency make that the UI thread across all platforms. Thanks for pointing this out. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:35: MediaStorageUtil::Type type_; You don't really need to store type separate from device_id. Though I guess if you plan to use it a lot it might be more efficient. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:84: unique_id = info.bsd_name_; This is ok for non-removable devices, but not for removable ones. Later down in the pipeline we store the unique id (as part of the device id) in preferences so that when that device arrives again, maybe after a restart of Chrome, we can associate it with the previous instance. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:24: It's not part of the System Monitor interface, but we'll also want to add something like the Linux implementation's "GetDeviceInfoForPath(const FilePath& path, base::SystemMonitor::RemovableStorageInfo* device_info)" in order to implement MediaStorageUtil::GetDeviceInforFromPath(). That should be easy given the infrastructure you've added. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:37: std::map<std::string, DiskInfoMac> disk_info_map_; nit: document what's in the map, i.e. what is the string https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:37: std::map<std::string, DiskInfoMac> disk_info_map_; Does this track all devices on the system, or just removable ones? https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:14: void GetDiskInfoAndUpdateOnFileThead( nit: Thead -> Thread https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:49: DASessionScheduleWithRunLoop( I assume this means you have to register and receive these events on the UI thread? https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:52: DARegisterDiskAppearedCallback( Does this send notifications for already attached devices? https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:81: if (ShouldPostNotificationForDisk(it->second)) { DCHECK(should_remove) ? https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:87: if (should_remove) { looks like line 80 and 87 could be merged? https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:134: info.type() == MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM); On 2012/09/10 07:02:34, Nico wrote: > the other file mentioned FIXED_MASS_FOO. that's not needed here? SystemMonitor only notifies about removable storage, so it shouldn't be needed in this predicate.
https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:16: // thread safe and must only be used from the file thread. On 2012/09/10 07:02:34, Nico wrote: > Looks like only the static function isn't threadsafe. Maybe move the comment to > there. Done. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.h:35: MediaStorageUtil::Type type_; On 2012/09/10 18:45:03, vandebo wrote: > You don't really need to store type separate from device_id. Though I guess if > you plan to use it a lot it might be more efficient. Yea, it seemed easier to store it than breaking appart device id. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:40: } On 2012/09/10 07:02:34, Nico wrote: > nit: gets a bit shorter with early outs: > > if (!is_removable) > return fixed_mass; > if (has_dcim) > return removable_dcim > return removable_no_dcim Done. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:58: info.bsd_name_ = base::SysCFStringRefToUTF8(bsd_name); On 2012/09/10 07:02:34, Nico wrote: > I'd do the null checking here instead of changing the base funciton i think Unless you feel strongly about this I'd prefer to add NULL checks in the base/* functions. I find it's much easier to read. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:84: unique_id = info.bsd_name_; On 2012/09/10 18:45:03, vandebo wrote: > This is ok for non-removable devices, but not for removable ones. Later down in > the pipeline we store the unique id (as part of the device id) in preferences so > that when that device arrives again, maybe after a restart of Chrome, we can > associate it with the previous instance. Unfortunately I couldn't find a better solution. Most devices I tried connecting to my Mac didn't provide a UUID. Here's the output from 3 devices that I tried: http://pastebin.com/pHJ7DWdw http://pastebin.com/8udVaTkf http://pastebin.com/qAssj9Xu https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:25: void UpdateDisk(const DiskInfoMac& info, bool should_remove); On 2012/09/10 07:02:34, Nico wrote: > nit: enums are more readable than bools Done. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:37: std::map<std::string, DiskInfoMac> disk_info_map_; On 2012/09/10 18:45:03, vandebo wrote: > nit: document what's in the map, i.e. what is the string Done. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:37: std::map<std::string, DiskInfoMac> disk_info_map_; On 2012/09/10 18:45:03, vandebo wrote: > Does this track all devices on the system, or just removable ones? It tracks all mountable devices. Added a comment about that. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:14: void GetDiskInfoAndUpdateOnFileThead( On 2012/09/10 18:45:03, vandebo wrote: > nit: Thead -> Thread Done. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:49: DASessionScheduleWithRunLoop( On 2012/09/10 18:45:03, vandebo wrote: > I assume this means you have to register and receive these events on the UI > thread? I think we can register on any thread that has a CFRunLoop attached. The FILE thread doesn't have a CFRunLoop attached so I decided to just register this on the UI thread instead. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:52: DARegisterDiskAppearedCallback( On 2012/09/10 18:45:03, vandebo wrote: > Does this send notifications for already attached devices? Yep! Added comment too. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:81: if (ShouldPostNotificationForDisk(it->second)) { On 2012/09/10 18:45:03, vandebo wrote: > DCHECK(should_remove) ? I'm trying to handle the update case here. For example, if the user renames the volume we get a DiskDescriptionChanged callback. In that case I post a Detached and Attached notification. The Attached notification contains the new mount point. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:87: if (should_remove) { On 2012/09/10 18:45:03, vandebo wrote: > looks like line 80 and 87 could be merged? see above. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:134: info.type() == MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM); On 2012/09/10 07:02:34, Nico wrote: > the other file mentioned FIXED_MASS_FOO. that's not needed here? Yea, we only post notifications about removable storage. Added a comment. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:134: info.type() == MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM); On 2012/09/10 18:45:03, vandebo wrote: > On 2012/09/10 07:02:34, Nico wrote: > > the other file mentioned FIXED_MASS_FOO. that's not needed here? > > SystemMonitor only notifies about removable storage, so it shouldn't be needed > in this predicate. > I didn't understand this comment. Did you mean that I don't need the WITH_DCIM/NO_DCIM check here because base::SystemMonitor already does this check?
+avi for DiskArbitration help. Hey Avi, we'd like to unique device ID that we can persist. It seems like not all devices supply a UUID. Do you a suggestion for what I can use? Here's the output of 3 devices I tried: http://pastebin.com/pHJ7DWdw http://pastebin.com/8udVaTkf http://pastebin.com/qAssj9Xu Thanks, Sailesh
https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:84: unique_id = info.bsd_name_; On 2012/09/10 21:58:43, sail wrote: > On 2012/09/10 18:45:03, vandebo wrote: > > This is ok for non-removable devices, but not for removable ones. Later down > in > > the pipeline we store the unique id (as part of the device id) in preferences > so > > that when that device arrives again, maybe after a restart of Chrome, we can > > associate it with the previous instance. > > Unfortunately I couldn't find a better solution. Most devices I tried connecting > to my Mac didn't provide a UUID. > > Here's the output from 3 devices that I tried: > http://pastebin.com/pHJ7DWdw > http://pastebin.com/8udVaTkf > http://pastebin.com/qAssj9Xu Fair, though bsd_name isn't helpful.... The Disk Manager utility does display an id for FAT devices (USB Serial Number, or something like that) on the device, not the partition entry. But this thread: http://lists.apple.com/archives/filesystem-dev/2012/Feb/msg00009.html suggests that there's no standardized way to get that. On Linux we fall back to a combination of DADeviceVendor DADeviceModel DADeviceRevision DADeviceUnit(?) https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:134: info.type() == MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM); On 2012/09/10 21:58:43, sail wrote: > On 2012/09/10 18:45:03, vandebo wrote: > > On 2012/09/10 07:02:34, Nico wrote: > > > the other file mentioned FIXED_MASS_FOO. that's not needed here? > > > > SystemMonitor only notifies about removable storage, so it shouldn't be needed > > in this predicate. > > > > I didn't understand this comment. > > Did you mean that I don't need the WITH_DCIM/NO_DCIM check here because > base::SystemMonitor already does this check? No, I was replying to Nico's comment.
I'm not sure what else I can say past what vandebo said. Plus if your devices are primarily FAT, you're pretty stuck (like the linked email says).
This CL should be ready to go. Please take a look. Added tests. Updated device ID construction. Also changed disk_info_map_ to use the device BSD name as a key. https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:84: unique_id = info.bsd_name_; On 2012/09/10 22:53:05, vandebo wrote: > On 2012/09/10 21:58:43, sail wrote: > > On 2012/09/10 18:45:03, vandebo wrote: > > > This is ok for non-removable devices, but not for removable ones. Later > down > > in > > > the pipeline we store the unique id (as part of the device id) in > preferences > > so > > > that when that device arrives again, maybe after a restart of Chrome, we can > > > associate it with the previous instance. > > > > Unfortunately I couldn't find a better solution. Most devices I tried > connecting > > to my Mac didn't provide a UUID. > > > > Here's the output from 3 devices that I tried: > > http://pastebin.com/pHJ7DWdw > > http://pastebin.com/8udVaTkf > > http://pastebin.com/qAssj9Xu > > Fair, though bsd_name isn't helpful.... > > The Disk Manager utility does display an id for FAT devices (USB Serial Number, > or something like that) on the device, not the partition entry. But this > thread: http://lists.apple.com/archives/filesystem-dev/2012/Feb/msg00009.html > suggests that there's no standardized way to get that. > > On Linux we fall back to a combination of DADeviceVendor DADeviceModel > DADeviceRevision DADeviceUnit(?) Done. Changed to use "vendor model revision unit" as a fallback device id.
https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:24: On 2012/09/10 18:45:03, vandebo wrote: > It's not part of the System Monitor interface, but we'll also want to add > something like the Linux implementation's "GetDeviceInfoForPath(const FilePath& > path, base::SystemMonitor::RemovableStorageInfo* device_info)" in order to > implement MediaStorageUtil::GetDeviceInforFromPath(). That should be easy given > the infrastructure you've added. Would it be hard to do this? https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:88: if (unit_number) { Did you figure out what this is? I'm not sure. It's 4 in all of your examples and based on the DADevicePath string in all your examples, you plugged the device into the same slot? If it's a property of how the device is connected, then it's not useful, whereas if it's a property of the device... https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:86: if (ShouldPostNotificationForDisk(it->second)) { Should we check to see that one of the properties of info changed before signalling a remove and add? I think if the volume name changed, we're fine with that, but if the mount point changed, then we should renotify... https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:135: UPDATE_SHOULD_NOT_REMOVE); Since you're using an enum, it might make things clearer to add a third value to the enum.
https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/2001/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.h:24: On 2012/09/11 00:45:50, vandebo wrote: > On 2012/09/10 18:45:03, vandebo wrote: > > It's not part of the System Monitor interface, but we'll also want to add > > something like the Linux implementation's "GetDeviceInfoForPath(const > FilePath& > > path, base::SystemMonitor::RemovableStorageInfo* device_info)" in order to > > implement MediaStorageUtil::GetDeviceInforFromPath(). That should be easy > given > > the infrastructure you've added. > > Would it be hard to do this? Done. Oops, sorry I missed this earlier. I added the method and matching tests. https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... chrome/browser/system_monitor/disk_info_mac.mm:88: if (unit_number) { On 2012/09/11 00:45:50, vandebo wrote: > Did you figure out what this is? I'm not sure. It's 4 in all of your examples > and based on the DADevicePath string in all your examples, you plugged the > device into the same slot? If it's a property of how the device is connected, > then it's not useful, whereas if it's a property of the device... Oops, you're right. This changes when you plugin the same device into a different port. Here's the output I got by plugging in my USB disk into two ports: http://pastebin.com/m028vqgt http://pastebin.com/gu2Q7YXR I've removed this from the device ID. https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:86: if (ShouldPostNotificationForDisk(it->second)) { On 2012/09/11 00:45:50, vandebo wrote: > Should we check to see that one of the properties of info changed before > signalling a remove and add? I think if the volume name changed, we're fine > with that, but if the mount point changed, then we should renotify... Currently the DescriptionChanged event is fired for kDADiskDescriptionWatchVolumePath. Also, the volume name is actually a part of the path so we should definitely notify is that changes. https://chromiumcodereview.appspot.com/10919185/diff/7004/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:135: UPDATE_SHOULD_NOT_REMOVE); On 2012/09/11 00:45:50, vandebo wrote: > Since you're using an enum, it might make things clearer to add a third value to > the enum. Done.
LGTM https://chromiumcodereview.appspot.com/10919185/diff/6004/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/6004/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:85: if (ShouldPostNotificationForDisk(it->second)) { nit: DCHECK(update_type == UPDATE_DEVICE_REMOVED || update_type == UPDATE_DEVICE_CHANGED). Alternately you could put a comment describing what's going on. It took me a couple passes to fully understand things here.
mac code lgtm
https://chromiumcodereview.appspot.com/10919185/diff/6004/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/6004/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_mac.mm:85: if (ShouldPostNotificationForDisk(it->second)) { On 2012/09/11 17:08:17, vandebo wrote: > nit: DCHECK(update_type == UPDATE_DEVICE_REMOVED || update_type == > UPDATE_DEVICE_CHANGED). Alternately you could put a comment describing what's > going on. It took me a couple passes to fully understand things here. Done. Added comment explaining that this for removed or changed devices.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/10002
Presubmit check for 10919185-10002 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
base/mac
base
Presubmit checks took 1.5s to calculate.
+mark for base/ and base/mac OWNERS review
+thestig for chrome/ changes
https://chromiumcodereview.appspot.com/10919185/diff/10002/base/sys_string_co... File base/sys_string_conversions_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/base/sys_string_co... base/sys_string_conversions_mac.mm:166: if (!ref) This intentionally does not operate on NULL input for safe round-tripping. You should adjust the caller instead of this.
https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.h:27: const string16 display_name() const { return display_name_; } Why does this one not return a reference, while the other getters that return objects do? https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.mm:28: return name + ASCIIToUTF16(" ") + addition; Can’t you just say |name + ' ' + addition| here and avoid calling the convertor for a constant character? https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.mm:100: } // chrome // namespace chrome
On 2012/09/12 21:04:16, vandebo wrote: > +thestig for chrome/ changes I'd be happy to stamp once Mark's concerns have been addressed.
https://chromiumcodereview.appspot.com/10919185/diff/10002/base/sys_string_co... File base/sys_string_conversions_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/base/sys_string_co... base/sys_string_conversions_mac.mm:166: if (!ref) On 2012/09/12 21:05:15, Mark Mentovai wrote: > This intentionally does not operate on NULL input for safe round-tripping. You > should adjust the caller instead of this. Done. (I'm not sure I understand this. Round-tripping would imply SysUTF8ToCFString(NULL) returning NULL. But we're using references.) https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... File chrome/browser/system_monitor/disk_info_mac.h (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.h:27: const string16 display_name() const { return display_name_; } On 2012/09/12 21:12:43, Mark Mentovai wrote: > Why does this one not return a reference, while the other getters that return > objects do? Done. Oops, Fixed. https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.mm:28: return name + ASCIIToUTF16(" ") + addition; On 2012/09/12 21:12:43, Mark Mentovai wrote: > Can’t you just say |name + ' ' + addition| here and avoid calling the convertor > for a constant character? Unfortunately not since this is string16. https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.mm:100: } // chrome On 2012/09/12 21:12:43, Mark Mentovai wrote: > // namespace chrome Done.
https://chromiumcodereview.appspot.com/10919185/diff/10002/base/sys_string_co... File base/sys_string_conversions_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/base/sys_string_co... base/sys_string_conversions_mac.mm:166: if (!ref) sail wrote: > On 2012/09/12 21:05:15, Mark Mentovai wrote: > > This intentionally does not operate on NULL input for safe round-tripping. You > > should adjust the caller instead of this. > > Done. > (I'm not sure I understand this. Round-tripping would imply > SysUTF8ToCFString(NULL) returning NULL. But we're using references.) Right, and there’s no such thing as a NULL reference, so this function should crash as any other function that operates on CFStrings would upon encountering a NULL CFString. There’s already a way to represent an empty CFString, and it’s not NULL. UTF8ToCFString(CFStringToUTF8(x)) == x, but as you modified this function previously, that’s not the case for NULL. Since you can’t express a NULL object (as opposed to a pointer), the correct thing to do is crash. https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.mm:28: return name + ASCIIToUTF16(" ") + addition; On 2012/09/12 22:38:26, sail wrote: > On 2012/09/12 21:12:43, Mark Mentovai wrote: > > Can’t you just say |name + ' ' + addition| here and avoid calling the > convertor > > for a constant character? > > Unfortunately not since this is string16. So what? I’m talking about a char16, not a string16. Single quotes. ' ' (or 32, if you prefer) is a space is a space, regardless of width: char, char16, wchar_t. You can cast it to char16 if you have to or want to.
https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... File chrome/browser/system_monitor/disk_info_mac.mm (right): https://chromiumcodereview.appspot.com/10919185/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/disk_info_mac.mm:28: return name + ASCIIToUTF16(" ") + addition; On 2012/09/12 22:45:26, Mark Mentovai wrote: > On 2012/09/12 22:38:26, sail wrote: > > On 2012/09/12 21:12:43, Mark Mentovai wrote: > > > Can’t you just say |name + ' ' + addition| here and avoid calling the > > convertor > > > for a constant character? > > > > Unfortunately not since this is string16. > > So what? I’m talking about a char16, not a string16. Single quotes. ' ' (or 32, > if you prefer) is a space is a space, regardless of width: char, char16, > wchar_t. You can cast it to char16 if you have to or want to. Done.
LGTM
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/13002
Failed to apply patch for chrome/chrome_browser.gypi: While running patch -p1 --forward --force; patching file chrome/chrome_browser.gypi Hunk #1 FAILED at 1872. Hunk #2 succeeded at 1913 with fuzz 1 (offset 30 lines). 1 out of 2 hunks FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/11005
Retried try job too often for step(s) sync_integration_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/11005
Retried try job too often for step(s) interactive_ui_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/11005
Retried try job too often for step(s) sync_integration_tests
On 2012/09/14 01:43:28, I haz the power (commit-bot) wrote: > Retried try job too often for step(s) sync_integration_tests My test failed with this error: [ RUN ] RemovableDeviceNotificationsMacTest.AddRemove [25706:-1597844160:0913/184053:2101185643281:FATAL:system_monitor_mac.mm(63)] Check failed: g_system_power_io_port != 0u (0 vs. 0) [25706:-1597844160:0913/184053:2101185643281:FATAL:system_monitor_mac.mm(63)] Check failed: g_system_power_io_port != 0u (0 vs. 0) I'm looking into it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/12033
Retried try job too often for step(s) sync_integration_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/10020
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/10020
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sail@chromium.org/10919185/10020
Change committed as 157002 |
