Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11363153:
[Media Gallery][Linux] Improve device media gallery names. (Closed)
|
Created:
8 years, 1 month ago by kmadhusu Modified:
8 years, 1 month ago Reviewers:
Lei Zhang CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Media Gallery][Linux] Improve device media gallery names.
<Device_storage_size> <Selected_sub_folder_gallery_name> <Device Label> <(VendorName, ModelName)>
Sample gallery names:
(1) 1GB TEST_USB
(2) 4GB DCIM - RED_USB
(3) 8KB PHOTOS - ABCD-0999 (TESTCOMPANY, A101)
Rebased against codereview.chromium.org/11366144/
BUG=159671, 158600
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168063
Patch Set 1 #Patch Set 2 : Add {Vendor, Model} along with UUID details. #
Total comments: 6
Patch Set 3 : Addressed review comments #Patch Set 4 : Removed UDISKS related code #
Total comments: 6
Patch Set 5 : Addressed review comments #
Total comments: 6
Patch Set 6 : Addressed comments. #
Total comments: 2
Patch Set 7 : Addressed review comments #
Total comments: 10
Patch Set 8 : Addressed review comments. #Patch Set 9 : '' #Patch Set 10 : Fix linux_clang compile error #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:155: // If the UDISKS_PARTITION_SIZE property is set, use it as the total size. Can we reliably get the partition size from /sys? If so, I'd prefer that we do not use UDISKS, because that relies on udisks being installed. https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:191: return (!device_label.empty() && IsStringUTF8(device_label)) ? |device_label| can't ever be empty. https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:497: device_id, device_partition_size + ASCIIToUTF16(" ") + name, How about adding a util function for this, so we can do this x-platform consistently? Otherwise, if we decide to change the format, we have to do it in several places.
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:155: // If the UDISKS_PARTITION_SIZE property is set, use it as the total size. On 2012/11/09 04:51:22, Lei Zhang wrote: > Can we reliably get the partition size from /sys? If so, I'd prefer that we do > not use UDISKS, because that relies on udisks being installed. I didn't find any documentation stating that we can rely on sysfs for device information. udev_device_get_sysattr_value returns NULL if there is no sys attribute value. If the udisks is not installed, GetUdevDevicePropertyValue is going to fail. https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:191: return (!device_label.empty() && IsStringUTF8(device_label)) ? On 2012/11/09 04:51:22, Lei Zhang wrote: > |device_label| can't ever be empty. Assuming FS UUID is available for all devices, |device_label| will be never empty. Realized that model_name can be empty. So added a check to validate the model_name value. https://codereview.chromium.org/11363153/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:497: device_id, device_partition_size + ASCIIToUTF16(" ") + name, On 2012/11/09 04:51:22, Lei Zhang wrote: > How about adding a util function for this, so we can do this x-platform > consistently? Otherwise, if we decide to change the format, we have to do it in > several places. Done.
Addressed review comments. PTAL. Thanks.
Removed UDISKS related code.
https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:172: device_label = GetUdevDevicePropertyValue(device, kFsUUID); What are the chances that a partition will not have a UUID? https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:172: device_label = GetUdevDevicePropertyValue(device, kFsUUID); Also, why do we put the UUID in the display name on Linux, but not on CrOS? https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:187: DCHECK(storage_size); It's weird to make |storage_size| required, but the other output parameters are optional.
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:172: device_label = GetUdevDevicePropertyValue(device, kFsUUID); On 2012/11/12 08:01:39, Lei Zhang wrote: > Also, why do we put the UUID in the display name on Linux, but not on CrOS? On Linux, When the user opens a device in a file dialog, they can easily map the UUID to the device. On CrOS, there is no easy way of mapping the UUID with the actual device (user has to open the terminal and navigate to the mount path to map the UUID value). https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:172: device_label = GetUdevDevicePropertyValue(device, kFsUUID); On 2012/11/12 08:01:39, Lei Zhang wrote: > What are the chances that a partition will not have a UUID? Chances are very less. Added a UMA to track this. https://codereview.chromium.org/11363153/diff/2006/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:187: DCHECK(storage_size); On 2012/11/12 08:01:39, Lei Zhang wrote: > It's weird to make |storage_size| required, but the other output parameters are > optional. Made |storage_size| as a optional param just to be consistent with other output params.
https://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_mon... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): https://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_linux.cc:153: // sysfs provides the device size value, which is the actual size in bytes // sysfs provides the device size in units of 512-byte blocks. https://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_linux.cc:165: return (!partition_size.empty() && You don't need to check if it's empty. StringToUint64() cannot possible return true for empty strings. https://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_mon... File chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc (right): https://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_mon... chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:99: for (size_t i= 0; i < arraysize(kTestDeviceData); ++i) { nit: "i ="
https://chromiumcodereview.appspot.com/11363153/diff/10002/chrome/browser/sys... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/11363153/diff/10002/chrome/browser/sys... chrome/browser/system_monitor/removable_device_notifications_linux.cc:165: total_size_in_bytes * 512 : 0; Oh, it's probably good to make sure this does not overflow an uint64, even though the chances of that happening here is extremely rare.
BTW, statvfs() works fine for me. I think the only catch is the file system needs to be mounted. With sysfs, you can read the partition size from /sys even when the file system is unmounted.
On 2012/11/13 05:04:34, Lei Zhang wrote: > BTW, statvfs() works fine for me. I think the only catch is the file system > needs to be mounted. With sysfs, you can read the partition size from /sys even > when the file system is unmounted. I am not sure why statvfs didn't work for me. Good to know that /sysfs will work for both mounted and unmounted devices. Therefore, reading the device size from /sys is better than statvfs. Thanks for providing these information.
http://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): http://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:153: // sysfs provides the device size value, which is the actual size in bytes On 2012/11/13 01:16:12, Lei Zhang wrote: > // sysfs provides the device size in units of 512-byte blocks. Done. http://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:165: return (!partition_size.empty() && On 2012/11/13 01:16:12, Lei Zhang wrote: > You don't need to check if it's empty. StringToUint64() cannot possible return > true for empty strings. Done. http://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc (right): http://codereview.chromium.org/11363153/diff/15001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux_unittest.cc:99: for (size_t i= 0; i < arraysize(kTestDeviceData); ++i) { On 2012/11/13 01:16:12, Lei Zhang wrote: > nit: "i =" Done. http://codereview.chromium.org/11363153/diff/10002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): http://codereview.chromium.org/11363153/diff/10002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_linux.cc:165: total_size_in_bytes * 512 : 0; On 2012/11/13 03:35:07, Lei Zhang wrote: > Oh, it's probably good to make sure this does not overflow an uint64, even > though the chances of that happening here is extremely rare. Added to check to make sure "total_size_in_bytes * 512" does not cross the uint64 max limit. If it exceeds, I am defaulting to uint64 max value.
http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.cc:167: return (total_size_in_bytes * 512 <= kuint64max) ? this is not the right way to detect integer overflow. total_size_in_bytes * 512 will never be greater than kuint64max. http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.cc:168: total_size_in_bytes * 512 : kuint64max; If we cannot calculate the storage size properly, then we have failed. How about return 0 on failure? http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.cc:188: UTF8ToUTF16(device_label + " ") + name : string16(); So if the device does not have a UUID, or has a UUID that's not UTF-8, you are going to return " name_foo" ? http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... File chrome/browser/system_monitor/removable_device_notifications_linux.h (right): http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.h:63: uint64 GetStorageSize(const std::string& location); nit: const
http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... File chrome/browser/system_monitor/removable_device_notifications_linux.cc (right): http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.cc:167: return (total_size_in_bytes * 512 <= kuint64max) ? On 2012/11/13 20:35:04, Lei Zhang wrote: > this is not the right way to detect integer overflow. total_size_in_bytes * 512 > will never be greater than kuint64max. Fixed. // Detection of unsigned multiplication integer over flow. // Imagine we need to multiply 'num' by 'size' if (size && num > SIZE_MAX / size) { // overflow ... } http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.cc:168: total_size_in_bytes * 512 : kuint64max; On 2012/11/13 20:35:04, Lei Zhang wrote: > If we cannot calculate the storage size properly, then we have failed. How about > return 0 on failure? Done. http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.cc:188: UTF8ToUTF16(device_label + " ") + name : string16(); On 2012/11/13 20:35:04, Lei Zhang wrote: > So if the device does not have a UUID, or has a UUID that's not UTF-8, you are > going to return " name_foo" ? good catch. Fixed. http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... File chrome/browser/system_monitor/removable_device_notifications_linux.h (right): http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.h:63: uint64 GetStorageSize(const std::string& location); On 2012/11/13 20:35:04, Lei Zhang wrote: > nit: const Done.
lgtm with nit below: http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... File chrome/browser/system_monitor/removable_device_notifications_linux.h (right): http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.h:63: uint64 GetStorageSize(const std::string& location); On 2012/11/13 21:57:27, kmadhusu wrote: > On 2012/11/13 20:35:04, Lei Zhang wrote: > > nit: const > > Done. No, I mean mark the method as const.
http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... File chrome/browser/system_monitor/removable_device_notifications_linux.h (right): http://codereview.chromium.org/11363153/diff/8007/chrome/browser/system_monit... chrome/browser/system_monitor/removable_device_notifications_linux.h:63: uint64 GetStorageSize(const std::string& location); On 2012/11/13 22:17:49, Lei Zhang wrote: > On 2012/11/13 21:57:27, kmadhusu wrote: > > On 2012/11/13 20:35:04, Lei Zhang wrote: > > > nit: const > > > > Done. > > No, I mean mark the method as const. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11363153/16001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... 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/kmadhusu@chromium.org/11363153/30001
Change committed as 168063 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
