Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10882039:
Make the Linux System Monitor implementation track all devices (Closed)
|
Created:
8 years, 4 months ago by vandebo (ex-Chrome) Modified:
8 years, 3 months ago Reviewers:
Lei Zhang CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake the Linux System Monitor implementation track all devices
Add a few new functions to support media gallery and rename based on the new names in SystemMonitor
BUG=144496
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=153980
Patch Set 1 #
Total comments: 2
Patch Set 2 : Checkpoint #Patch Set 3 : Checkpoint #Patch Set 4 : nits #
Total comments: 2
Patch Set 5 : Rebase #Patch Set 6 : Fix move status #
Total comments: 24
Patch Set 7 : Address comments #
Total comments: 36
Patch Set 8 : Address comments #
Total comments: 2
Patch Set 9 : Fix bots #Patch Set 10 : Address comments #
Total comments: 4
Patch Set 11 : Address comments #Patch Set 12 : nit #Patch Set 13 : Fix CrOS build #Messages
Total messages: 24 (0 generated)
FYI
https://chromiumcodereview.appspot.com/10882039/diff/1/chrome/browser/media_g... File chrome/browser/media_gallery/disk_mount_watcher_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/1/chrome/browser/media_g... chrome/browser/media_gallery/disk_mount_watcher_linux.cc:208: if (usb) { FWIW, you can do: const char* removable = udev_device_get_sysattr_value(device, "removable"); if (removable) { return atoi(removable) != 0; } However, |device| may be a partition on a disk, it may not have the removable attribute at all. You'll need to walk up the tree with udev_device_get_parent_with_subsystem_devtype(device, "block", "disk");
https://chromiumcodereview.appspot.com/10882039/diff/1/chrome/browser/media_g... File chrome/browser/media_gallery/disk_mount_watcher_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/1/chrome/browser/media_g... chrome/browser/media_gallery/disk_mount_watcher_linux.cc:208: if (usb) { On 2012/08/25 01:22:19, Lei Zhang wrote: > FWIW, you can do: > > const char* removable = udev_device_get_sysattr_value(device, "removable"); > if (removable) { > return atoi(removable) != 0; > } > > However, |device| may be a partition on a disk, it may not have the removable > attribute at all. You'll need to walk up the tree with > udev_device_get_parent_with_subsystem_devtype(device, "block", "disk"); Done.
https://chromiumcodereview.appspot.com/10882039/diff/10001/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/10001/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:105: std::string prefix = device_id.substr(0, prefix_length + 1); You'll need to sync past r153569 and resolve this.
https://chromiumcodereview.appspot.com/10882039/diff/10001/chrome/browser/med... File chrome/browser/media_gallery/media_storage_util.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/10001/chrome/browser/med... chrome/browser/media_gallery/media_storage_util.cc:105: std::string prefix = device_id.substr(0, prefix_length + 1); On 2012/08/27 23:17:15, Lei Zhang wrote: > You'll need to sync past r153569 and resolve this. Done.
https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:71: void ReadMtab(FilePath mtab_path, Why not just keep this as a class method? If you are going to keep it as an anonymous function, const-ref |mtab_path| and |interesting_file_systems|. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:78: mtab->clear(); probably should do this first https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:126: // On success, returns true and fill in |id| and |name|. nit: comment is out of date https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:182: *unique_id = kFSUniqueIdPrefix + uuid; I have a feeling you are doing this to make the code more readable. How about we put the inside of this if block in a helper function instead? https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:208: udev_device_get_parent_with_subsystem_devtype(device, "block", "disk"); nit: make block and disk constants or use remove below instead of kRemovableSysAttr. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:212: if (value && atoi(value) == 1) *removable = (value && atoi(value) == 1) https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:225: DCHECK(g_removable_device_notifications_linux == NULL); DCHECK(!g_removable_device_notifications_linux); https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:241: DCHECK(g_removable_device_notifications_linux != NULL); DCHECK_EQ(this, g_removable_device_notifications_linux) https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:271: FilePath mount_device = FilePath(); you don't need to init a FilePath to an empty FilePath. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... File chrome/browser/media_gallery/removable_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.h:30: // returns true and fills in |device_name| and |unique_id|. nit: variable name in comment is not consistent with the function parameter. Need to mention |removable|. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.h:49: // Use |unique_id| to find and return where the device is mounted. nit: variable name in comment is not consistent with the method parameter. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.h:105: // Adds |mount_device| as mounted on |mount_point|. If the deice is a new typo
https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:71: void ReadMtab(FilePath mtab_path, On 2012/08/28 00:05:04, Lei Zhang wrote: > Why not just keep this as a class method? The type declarations in the header are pretty confusing and take a lot of effort to understand. By moving this to an anonymous function, MountPoitnDeviceMap can be moved out of the header. > If you are going to keep it as an anonymous function, const-ref |mtab_path| and > |interesting_file_systems|. Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:78: mtab->clear(); On 2012/08/28 00:05:04, Lei Zhang wrote: > probably should do this first Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:126: // On success, returns true and fill in |id| and |name|. On 2012/08/28 00:05:04, Lei Zhang wrote: > nit: comment is out of date Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:182: *unique_id = kFSUniqueIdPrefix + uuid; On 2012/08/28 00:05:04, Lei Zhang wrote: > I have a feeling you are doing this to make the code more readable. How about we > put the inside of this if block in a helper function instead? Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:208: udev_device_get_parent_with_subsystem_devtype(device, "block", "disk"); On 2012/08/28 00:05:04, Lei Zhang wrote: > nit: make block and disk constants or use remove below instead of > kRemovableSysAttr. Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:212: if (value && atoi(value) == 1) On 2012/08/28 00:05:04, Lei Zhang wrote: > *removable = (value && atoi(value) == 1) Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:225: DCHECK(g_removable_device_notifications_linux == NULL); On 2012/08/28 00:05:04, Lei Zhang wrote: > DCHECK(!g_removable_device_notifications_linux); Done. And below. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:241: DCHECK(g_removable_device_notifications_linux != NULL); On 2012/08/28 00:05:04, Lei Zhang wrote: > DCHECK_EQ(this, g_removable_device_notifications_linux) Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.cc:271: FilePath mount_device = FilePath(); On 2012/08/28 00:05:04, Lei Zhang wrote: > you don't need to init a FilePath to an empty FilePath. Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... File chrome/browser/media_gallery/removable_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.h:30: // returns true and fills in |device_name| and |unique_id|. On 2012/08/28 00:05:04, Lei Zhang wrote: > nit: variable name in comment is not consistent with the function parameter. > Need to mention |removable|. Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.h:49: // Use |unique_id| to find and return where the device is mounted. On 2012/08/28 00:05:04, Lei Zhang wrote: > nit: variable name in comment is not consistent with the method parameter. Done. https://chromiumcodereview.appspot.com/10882039/diff/7020/chrome/browser/medi... chrome/browser/media_gallery/removable_device_notifications_linux.h:105: // Adds |mount_device| as mounted on |mount_point|. If the deice is a new On 2012/08/28 00:05:04, Lei Zhang wrote: > typo Done.
https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:216: const char* value = udev_device_get_sysattr_value(parent_device, Try this on |device| first, in case |device| is /dev/sdX. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:273: if (type == MediaStorageUtil::USB_MTP) Do you think we'll be adding more storage types in the future? Perhaps change this to a switch statement so we get an compiler error when we do add a new type? https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:298: mount_priority_map_.find(mount_device)->second.begin()->first); Isn't this just: referenced_info.begin()->first ? https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:307: for (current = path; I think you want a while loop here. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:312: if (mount_info_map_.find(current) == mount_info_map_.end()) Save the iterator so you don't have to call find() again on line 320. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:317: current.AppendRelativePath(path, mount_point); I may be understanding this wrong, but this doesn't seem to match the comment in the header. If |current| is /home and |path| is /home/foo, then we are setting |mount_point| to "foo" here, whereas the mount point is "/home". https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:386: if (mount_priority_map_.find(mount_device)->second.size() > 1) no need to call find() again? ditto below. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:412: const FilePath& mount_point = mount_priority_map_[*it].begin()->first; If there was ever a bug in the code, you may end up setting |mount_point| to an empty FilePath. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:438: if (mount_priority_map_.find(mount_device) != mount_priority_map_.end()) { You can easily save the iterator from find() and not have to use mount_priority_map_[mount_device] below. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.h:53: // for that device. If |mount_point| is not NULL, set it to the mount point Mention return an empty string on error? https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.h:105: // Adds |mount_device| as mounted on |mount_point|. If the deice is a new s/deice/device. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:95: if (device == kInvalidDevice) nit: curly brackets https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:406: // kDevice1 -> kMountPointA * When I first wrote the test, I thought we came to the conclusion that if the user mounts a device at mount point A, and then mounts it again at mount point B, then mount point B is the one we show, since the user's most recent decision is to put it there. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:588: file_util::Delete(test_path_a.Append("DCIM"), false); DCIM should be a constant. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:622: EXPECT_EQ(FilePath().value(), relative_path.value()); I think you can just compare FilePaths directly.
https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:216: const char* value = udev_device_get_sysattr_value(parent_device, On 2012/08/28 07:55:30, Lei Zhang wrote: > Try this on |device| first, in case |device| is /dev/sdX. Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:273: if (type == MediaStorageUtil::USB_MTP) On 2012/08/28 07:55:30, Lei Zhang wrote: > Do you think we'll be adding more storage types in the future? Perhaps change > this to a switch statement so we get an compiler error when we do add a new > type? Added a DCHECK https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:298: mount_priority_map_.find(mount_device)->second.begin()->first); On 2012/08/28 07:55:30, Lei Zhang wrote: > Isn't this just: referenced_info.begin()->first ? Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:307: for (current = path; On 2012/08/28 07:55:30, Lei Zhang wrote: > I think you want a while loop here. Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:312: if (mount_info_map_.find(current) == mount_info_map_.end()) On 2012/08/28 07:55:30, Lei Zhang wrote: > Save the iterator so you don't have to call find() again on line 320. Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:317: current.AppendRelativePath(path, mount_point); On 2012/08/28 07:55:30, Lei Zhang wrote: > I may be understanding this wrong, but this doesn't seem to match the comment in > the header. > > If |current| is /home and |path| is /home/foo, then we are setting |mount_point| > to "foo" here, whereas the mount point is "/home". Indeed. The caller will also want the relative path, but returning the mount point will let them compute both. Fixed. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:386: if (mount_priority_map_.find(mount_device)->second.size() > 1) On 2012/08/28 07:55:30, Lei Zhang wrote: > no need to call find() again? ditto below. Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:412: const FilePath& mount_point = mount_priority_map_[*it].begin()->first; On 2012/08/28 07:55:30, Lei Zhang wrote: > If there was ever a bug in the code, you may end up setting |mount_point| to an > empty FilePath. I can see how this might be a little fragile. Added a DCHECK to catch that if a bug is introduced in the future. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:438: if (mount_priority_map_.find(mount_device) != mount_priority_map_.end()) { On 2012/08/28 07:55:30, Lei Zhang wrote: > You can easily save the iterator from find() and not have to use > mount_priority_map_[mount_device] below. Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.h:53: // for that device. If |mount_point| is not NULL, set it to the mount point On 2012/08/28 07:55:30, Lei Zhang wrote: > Mention return an empty string on error? Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.h:105: // Adds |mount_device| as mounted on |mount_point|. If the deice is a new On 2012/08/28 07:55:30, Lei Zhang wrote: > s/deice/device. Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:95: if (device == kInvalidDevice) On 2012/08/28 07:55:30, Lei Zhang wrote: > nit: curly brackets Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:406: // kDevice1 -> kMountPointA * On 2012/08/28 07:55:30, Lei Zhang wrote: > When I first wrote the test, I thought we came to the conclusion that if the > user mounts a device at mount point A, and then mounts it again at mount point > B, then mount point B is the one we show, since the user's most recent decision > is to put it there. But if we've already notified consumers of system monitor that it's a mount point a, then we'd have to detach and reattach it at mount point b. While it's still at mount point a, there's no need to do that. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:588: file_util::Delete(test_path_a.Append("DCIM"), false); On 2012/08/28 07:55:30, Lei Zhang wrote: > DCIM should be a constant. Done. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux_unittests.cc:622: EXPECT_EQ(FilePath().value(), relative_path.value()); On 2012/08/28 07:55:30, Lei Zhang wrote: > I think you can just compare FilePaths directly. Nope, it compares the pointer values instead.
Mostly good. Though I really dislike using map::operator[] for reading for the reasons below. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:273: if (type == MediaStorageUtil::USB_MTP) On 2012/08/28 18:59:30, vandebo wrote: > On 2012/08/28 07:55:30, Lei Zhang wrote: > > Do you think we'll be adding more storage types in the future? Perhaps change > > this to a switch statement so we get an compiler error when we do add a new > > type? > > Added a DCHECK I was thinking of: switch (type) { case MediaStorageUtil::MTP_OR_PTP: return FilePath(); case MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM: case MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM: case MediaStorageUtil::FIXED_MASS_STORAGE: break; // continue } Which will cause a failure at compile time rather than maybe failing at run time. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:412: const FilePath& mount_point = mount_priority_map_[*it].begin()->first; On 2012/08/28 18:59:30, vandebo wrote: > On 2012/08/28 07:55:30, Lei Zhang wrote: > > If there was ever a bug in the code, you may end up setting |mount_point| to > an > > empty FilePath. > > I can see how this might be a little fragile. Added a DCHECK to catch that if a > bug is introduced in the future. Reading from a map using operator[] still sucks, because you're actually trying to do an insert() and you are constructing an object to insert even though the insert will likely fail. See [3] in {1} for what operator [] really does. OTOH, for writing to a map, using operator [] is only slightly inefficient. You insert a default constructed object, return the reference to it in the map, and then write the rvalue into the reference. Since we don't have map-util.h, using map::insert() is rather tedious. Which is probably why a lot of Chromium's map usage don't use insert() as you mentioned in the previous CL. {1} http://www.sgi.com/tech/stl/Map.html#3 https://chromiumcodereview.appspot.com/10882039/diff/20002/chrome/browser/med... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/20002/chrome/browser/med... chrome/browser/media_gallery/removable_device_notifications_linux.cc:313: while (mount_info_map_.find(current) == mount_info_map_.end() && while (!ContainsKey(mount_info_map_, current) && current != current.DirName())
https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:273: if (type == MediaStorageUtil::USB_MTP) On 2012/08/28 20:49:04, Lei Zhang wrote: > On 2012/08/28 18:59:30, vandebo wrote: > > On 2012/08/28 07:55:30, Lei Zhang wrote: > > > Do you think we'll be adding more storage types in the future? Perhaps > change > > > this to a switch statement so we get an compiler error when we do add a new > > > type? > > > > Added a DCHECK > > I was thinking of: > > switch (type) { > case MediaStorageUtil::MTP_OR_PTP: > return FilePath(); > case MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM: > case MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM: > case MediaStorageUtil::FIXED_MASS_STORAGE: > break; // continue > } > > Which will cause a failure at compile time rather than maybe failing at run > time. It seems a bit overkill for the protection that we're trying to provide at this point. In all likelihood, the caller won't even call this function if it has a non-mass storage device. The check for MTP is just a safety net. DCHECKS to ensure the safety net isn't outdated seem fine whereas runtime overhead in release builds seems overkill to me. https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:412: const FilePath& mount_point = mount_priority_map_[*it].begin()->first; On 2012/08/28 20:49:04, Lei Zhang wrote: > On 2012/08/28 18:59:30, vandebo wrote: > > On 2012/08/28 07:55:30, Lei Zhang wrote: > > > If there was ever a bug in the code, you may end up setting |mount_point| to > > an > > > empty FilePath. > > > > I can see how this might be a little fragile. Added a DCHECK to catch that if > a > > bug is introduced in the future. > > Reading from a map using operator[] still sucks, because you're actually trying > to do an insert() and you are constructing an object to insert even though the > insert will likely fail. See [3] in {1} for what operator [] really does. > > OTOH, for writing to a map, using operator [] is only slightly inefficient. You > insert a default constructed object, return the reference to it in the map, and > then write the rvalue into the reference. Since we don't have map-util.h, using > map::insert() is rather tedious. Which is probably why a lot of Chromium's map > usage don't use insert() as you mentioned in the previous CL. > > {1} http://www.sgi.com/tech/stl/Map.html#3 I mostly agree with what you've said. But what would you like me to do about it? I think this needs to be addressed on the whole for chrome. Either we decide that reading from a map with [] is ok. Or we add helper utilities to make things less painful. https://chromiumcodereview.appspot.com/10882039/diff/20002/chrome/browser/med... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/20002/chrome/browser/med... chrome/browser/media_gallery/removable_device_notifications_linux.cc:313: while (mount_info_map_.find(current) == mount_info_map_.end() && On 2012/08/28 20:49:04, Lei Zhang wrote: > while (!ContainsKey(mount_info_map_, current) && current != current.DirName()) Done.
https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:412: const FilePath& mount_point = mount_priority_map_[*it].begin()->first; On 2012/08/28 21:25:26, vandebo wrote: > On 2012/08/28 20:49:04, Lei Zhang wrote: > > On 2012/08/28 18:59:30, vandebo wrote: > > > On 2012/08/28 07:55:30, Lei Zhang wrote: > > > > If there was ever a bug in the code, you may end up setting |mount_point| > to > > > an > > > > empty FilePath. > > > > > > I can see how this might be a little fragile. Added a DCHECK to catch that > if > > a > > > bug is introduced in the future. > > > > Reading from a map using operator[] still sucks, because you're actually > trying > > to do an insert() and you are constructing an object to insert even though the > > insert will likely fail. See [3] in {1} for what operator [] really does. > > > > OTOH, for writing to a map, using operator [] is only slightly inefficient. > You > > insert a default constructed object, return the reference to it in the map, > and > > then write the rvalue into the reference. Since we don't have map-util.h, > using > > map::insert() is rather tedious. Which is probably why a lot of Chromium's map > > usage don't use insert() as you mentioned in the previous CL. > > > > {1} http://www.sgi.com/tech/stl/Map.html#3 > > I mostly agree with what you've said. But what would you like me to do about > it? I think this needs to be addressed on the whole for chrome. Either we > decide that reading from a map with [] is ok. Or we add helper utilities to > make things less painful. We can't fix everyone's code, but we can at least write our own code to not use operator [] when reading. I'll try to put map-util.h in base when I have a chance. MountPriorityMap::iterator prio_it = mount_priority_map_.find(*it); DCHECK_NE(prio_it, mount_priority_map_.end()); const FilePath& mount_point = prio_it->begin()->first; prio_it->begin()->second = true; MountInfoMap::const_iterator info_it = mount_info_map_.find(mount_point); DCHECK_NE(info_it, mount_info_map_.end()); DCHECK(info_it->has_dcim); base::SystemMonitor::Get()->ProcessRemovableStorageAttached( info_it->device_id ... https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... chrome/browser/media_gallery/removable_device_notifications_linux.cc:317: current != current.DirName()) { I think this fits on the previous line.
https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/28/chrome/browser/media_... chrome/browser/media_gallery/removable_device_notifications_linux.cc:412: const FilePath& mount_point = mount_priority_map_[*it].begin()->first; Changed all instances (here and AddNewMount) of operator[ lookup to find. https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... chrome/browser/media_gallery/removable_device_notifications_linux.cc:317: current != current.DirName()) { On 2012/08/28 23:22:33, Lei Zhang wrote: > I think this fits on the previous line. Nope, two characters too long.
lgtm https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... chrome/browser/media_gallery/removable_device_notifications_linux.cc:317: current != current.DirName()) { On 2012/08/29 00:20:12, vandebo wrote: > On 2012/08/28 23:22:33, Lei Zhang wrote: > > I think this fits on the previous line. > > Nope, two characters too long. But you don't need the brace anymore. ;-)
https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... File chrome/browser/media_gallery/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10882039/diff/24003/chrome/browser/med... chrome/browser/media_gallery/removable_device_notifications_linux.cc:317: current != current.DirName()) { On 2012/08/29 00:22:31, Lei Zhang wrote: > On 2012/08/29 00:20:12, vandebo wrote: > > On 2012/08/28 23:22:33, Lei Zhang wrote: > > > I think this fits on the previous line. > > > > Nope, two characters too long. > > But you don't need the brace anymore. ;-) Der... Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/10882039/28004
Try job failure for 10882039-28004 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" 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/vandebo@chromium.org/10882039/24015
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/10882039/24017
Try job failure for 10882039-24017 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vandebo@chromium.org/10882039/24017
Try job failure for 10882039-24017 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" 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/vandebo@chromium.org/10882039/24017
Try job failure for 10882039-24017 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, steps "nacl_integration, interactive_ui_tests, browser_tests, sync_integration_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... |
