[Media Galleries] Populate volume metadata in ChromeOS/Linux

chrome/browser/storage_monitor/removable_device_notifications_linux.cc

Index: chrome/browser/storage_monitor/removable_device_notifications_linux.cc
diff --git a/chrome/browser/storage_monitor/removable_device_notifications_linux.cc b/chrome/browser/storage_monitor/removable_device_notifications_linux.cc
index db6ef69c67d59aeb18271830f24244a9dd3a3ac4..68e763e0614a8619acac1535eff5e19ba403e277 100644
--- a/chrome/browser/storage_monitor/removable_device_notifications_linux.cc
+++ b/chrome/browser/storage_monitor/removable_device_notifications_linux.cc
@@ -135,8 +135,20 @@ uint64 GetDeviceStorageSize(const base::FilePath& device_path,
 
 // Constructs the device name from the device properties. If the device details
 // are unavailable, returns an empty string.
-string16 GetDeviceName(struct udev_device* device) {
+string16 GetDeviceName(struct udev_device* device,
+                       string16* out_volume_label,
+                       string16* out_vendor_name,
+                       string16* out_model_name) {
   std::string device_label = GetUdevDevicePropertyValue(device, kLabel);
+  std::string vendor_name = GetUdevDevicePropertyValue(device, kVendor);
+  std::string model_name = GetUdevDevicePropertyValue(device, kModel);
+  if (out_volume_label)
+    *out_volume_label = UTF8ToUTF16(device_label);
+  if (out_vendor_name)
+    *out_vendor_name = UTF8ToUTF16(vendor_name);
+  if (out_model_name)
+    *out_model_name = UTF8ToUTF16(model_name);
+
   if (!device_label.empty() && IsStringUTF8(device_label))
     return UTF8ToUTF16(device_label);
 
@@ -146,9 +158,7 @@ string16 GetDeviceName(struct udev_device* device) {
       "RemovableDeviceNotificationsLinux.device_file_system_uuid_available",
       !device_label.empty());
 
-  const string16 name = GetFullProductName(
-      GetUdevDevicePropertyValue(device, kVendor),
-      GetUdevDevicePropertyValue(device, kModel));
+  const string16 name = GetFullProductName(vendor_name, model_name);
 
   const string16 device_label_utf16 =
       (!device_label.empty() && IsStringUTF8(device_label)) ?
@@ -162,10 +172,14 @@ string16 GetDeviceName(struct udev_device* device) {
 // On success, returns true and fill in |unique_id|, |name|, |removable| and
 // |partition_size_in_bytes|.
 void GetDeviceInfo(const base::FilePath& device_path,
-                   std::string* unique_id,
+                   const base::FilePath& mount_point,
+                   std::string* device_id,
                    string16* name,
                    bool* removable,
-                   uint64* partition_size_in_bytes) {
+                   uint64* partition_size_in_bytes,
+                   string16* out_volume_label,
+                   string16* out_vendor_name,
+                   string16* out_model_name) {
   DCHECK(!device_path.empty());
   ScopedUdevObject udev_obj(udev_new());
   if (!udev_obj.get()) {
@@ -196,26 +210,43 @@ void GetDeviceInfo(const base::FilePath& device_path,
     return;
   }
 
+  string16 volume_label;
+  string16 vendor_name;
+  string16 model_name;
+  string16 device_name = GetDeviceName(device, &volume_label,
+                                       &vendor_name, &model_name);
   if (name)
-    *name = GetDeviceName(device);
-
-  if (unique_id)
-    *unique_id = MakeDeviceUniqueId(device);
-
+    *name = device_name;
+
+  std::string unique_id = MakeDeviceUniqueId(device);
+  MediaStorageUtil::RecordDeviceInfoHistogram(true, unique_id, device_name);
+
+  const char* value = udev_device_get_sysattr_value(device,
+                                                    kRemovableSysAttr);
+  if (!value) {
+    // |parent_device| is owned by |device| and does not need to be cleaned
+    // up.
+    struct udev_device* parent_device =
+        udev_device_get_parent_with_subsystem_devtype(device,
+                                                      kBlockSubsystemKey,
+                                                      kDiskDeviceTypeKey);
+    value = udev_device_get_sysattr_value(parent_device, kRemovableSysAttr);
+  }
+  bool is_removable = (value && atoi(value) == 1);
   if (removable) {
-    const char* value = udev_device_get_sysattr_value(device,
-                                                      kRemovableSysAttr);
-    if (!value) {
-      // |parent_device| is owned by |device| and does not need to be cleaned
-      // up.
-      struct udev_device* parent_device =
-          udev_device_get_parent_with_subsystem_devtype(device,
-                                                        kBlockSubsystemKey,
-                                                        kDiskDeviceTypeKey);
-      value = udev_device_get_sysattr_value(parent_device, kRemovableSysAttr);
-    }
-    *removable = (value && atoi(value) == 1);
+    *removable = is_removable;
+  }
+
+  bool has_dcim = IsMediaDevice(mount_point.value());

[vandebo (ex-Chrome), 2013/03/01 22:21:48]

This block should all be in if(device_id) {

[Greg Billock, 2013/03/05 19:20:55]

Done.

+  MediaStorageUtil::Type type = MediaStorageUtil::FIXED_MASS_STORAGE;
+  if (is_removable) {
+    if (has_dcim)
+      type = MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM;
+    else
+      type = MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM;
   }
+  if (device_id)
+    *device_id = MediaStorageUtil::MakeDeviceId(type, unique_id);
 
   if (partition_size_in_bytes)
     *partition_size_in_bytes = GetDeviceStorageSize(device_path, device);
@@ -268,12 +299,8 @@ bool RemovableDeviceNotificationsLinux::GetStorageInfoForPath(
   MountMap::const_iterator mount_info = mount_info_map_.find(current);
   if (mount_info == mount_info_map_.end())
     return false;
-
-  if (device_info) {
-    device_info->device_id = mount_info->second.device_id;
-    device_info->name = mount_info->second.device_name;
-    device_info->location = current.value();
-  }
+  if (device_info)
+    *device_info = mount_info->second;
   return true;
 }
 
@@ -282,7 +309,7 @@ uint64 RemovableDeviceNotificationsLinux::GetStorageSize(
   MountMap::const_iterator mount_info = mount_info_map_.find(
       base::FilePath(location));
   return (mount_info != mount_info_map_.end()) ?
-      mount_info->second.partition_size_in_bytes : 0;
+      mount_info->second.total_size_in_bytes : 0;
 }
 
 void RemovableDeviceNotificationsLinux::OnFilePathChanged(
@@ -302,8 +329,17 @@ void RemovableDeviceNotificationsLinux::OnFilePathChanged(
   UpdateMtab();
 }
 
-RemovableDeviceNotificationsLinux::MountPointInfo::MountPointInfo()
-    : partition_size_in_bytes(0) {
+base::FilePath RemovableDeviceNotificationsLinux::GetDeviceForMountPoint(

[vandebo (ex-Chrome), 2013/03/01 22:21:48]

Not a fan of this.  Yes all the information is here, but it's complicated to get
at.  How about changing MountPointInfo to:

struct MountPointInfo {
  StorageInfo storage_info;
  base::FilePath mount_device;
};

[Greg Billock, 2013/03/05 19:20:55]

I think it's complicated because of the priority map, but I don't have a
suggested simplification for that -- tracking multiple mount points is just
complex.

One thing to like about this is that the two data structures are now storing
disjoint information -- I think even better would be to rekey mount_info_map_,
but I didn't do that here because it's kind of hard to follow.

[vandebo (ex-Chrome), 2013/03/06 01:34:54]

So the way it was before was storing some redundant information, but that made
it fast to access.  At the least, this seems like an unrelated change that
should go into its own CL.

+    const base::FilePath& mount_point) {
+  for (MountPriorityMap::const_iterator iter = mount_priority_map_.begin();
+       iter != mount_priority_map_.end(); ++iter) {
+    for (ReferencedMountPoint::const_iterator itermp = iter->second.begin();
+         itermp != iter->second.end(); ++itermp) {
+      if (itermp->first == mount_point)
+        return iter->first;
+    }
+  }
+  return base::FilePath();
 }
 
 void RemovableDeviceNotificationsLinux::InitOnFileThread() {
@@ -339,7 +375,7 @@ void RemovableDeviceNotificationsLinux::UpdateMtab() {
   for (MountMap::const_iterator old_iter = mount_info_map_.begin();
        old_iter != mount_info_map_.end(); ++old_iter) {
     const base::FilePath& mount_point = old_iter->first;
-    const base::FilePath& mount_device = old_iter->second.mount_device;
+    const base::FilePath& mount_device = GetDeviceForMountPoint(mount_point);
     MountPointDeviceMap::iterator new_iter = new_mtab.find(mount_point);
     // |mount_point| not in |new_mtab| or |mount_device| is no longer mounted at
     // |mount_point|.
@@ -386,11 +422,10 @@ void RemovableDeviceNotificationsLinux::UpdateMtab() {
     const base::FilePath& mount_point = first_mount_point_info->first;
     first_mount_point_info->second = true;
 
-    const MountPointInfo& mount_info =
+    const StorageInfo& mount_info =
         mount_info_map_.find(mount_point)->second;
     DCHECK(MediaStorageUtil::IsRemovableDevice(mount_info.device_id));
-    receiver()->ProcessAttach(StorageInfo(
-        mount_info.device_id, mount_info.device_name, mount_point.value()));
+    receiver()->ProcessAttach(mount_info);
   }
 
   // Check new mtab entries against existing ones.
@@ -400,7 +435,7 @@ void RemovableDeviceNotificationsLinux::UpdateMtab() {
     const base::FilePath& mount_device = new_iter->second;
     MountMap::iterator old_iter = mount_info_map_.find(mount_point);
     if (old_iter == mount_info_map_.end() ||
-        old_iter->second.mount_device != mount_device) {
+        GetDeviceForMountPoint(old_iter->first) != mount_device) {
       // New mount point found or an existing mount point found with a new
       // device.
       AddNewMount(mount_device, mount_point);
@@ -420,36 +455,29 @@ void RemovableDeviceNotificationsLinux::AddNewMount(
     return;
   }
 
-  std::string unique_id;
+  std::string device_id;
   string16 name;
   bool removable;
   uint64 partition_size_in_bytes;
-  get_device_info_func_(mount_device, &unique_id, &name, &removable,
-                        &partition_size_in_bytes);
+  string16 volume_label;
+  string16 vendor_name;
+  string16 model_name;
+  get_device_info_func_(mount_device, mount_point, &device_id, &name,
+                        &removable, &partition_size_in_bytes,
+                        &volume_label, &vendor_name, &model_name);
 
   // Keep track of device info details to see how often we get invalid values.
-  MediaStorageUtil::RecordDeviceInfoHistogram(true, unique_id, name);
-  if (unique_id.empty() || name.empty())
+  if (device_id.empty() || name.empty())
     return;
 
-  bool has_dcim = IsMediaDevice(mount_point.value());
-  MediaStorageUtil::Type type;
-  if (removable) {
-    if (has_dcim) {
-      type = MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM;
-    } else {
-      type = MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM;
-    }
-  } else {
-    type = MediaStorageUtil::FIXED_MASS_STORAGE;
-  }
-  std::string device_id = MediaStorageUtil::MakeDeviceId(type, unique_id);
-
-  MountPointInfo mount_point_info;
-  mount_point_info.mount_device = mount_device;
+  StorageInfo mount_point_info;

[vandebo (ex-Chrome), 2013/03/01 22:21:48]

use the constructor?

[Greg Billock, 2013/03/05 19:20:55]

Done.

   mount_point_info.device_id = device_id;
-  mount_point_info.device_name = name;
-  mount_point_info.partition_size_in_bytes = partition_size_in_bytes;
+  mount_point_info.location = mount_point.value();
+  mount_point_info.name = name;
+  mount_point_info.total_size_in_bytes = partition_size_in_bytes;
+  mount_point_info.storage_label = volume_label;
+  mount_point_info.vendor_name = vendor_name;
+  mount_point_info.model_name = model_name;
 
   mount_info_map_[mount_point] = mount_point_info;
   mount_priority_map_[mount_device][mount_point] = removable;