[Media Galleries] Populate volume metadata in ChromeOS/Linux

chrome/browser/system_monitor/removable_storage_notifications.h

Index: chrome/browser/system_monitor/removable_storage_notifications.h
diff --git a/chrome/browser/system_monitor/removable_storage_notifications.h b/chrome/browser/system_monitor/removable_storage_notifications.h
index e916dea83fdc0ed3f1fadb65729e269f983a5234..d6df0f4e885192f2ecde3739912116970bdf55ae 100644
--- a/chrome/browser/system_monitor/removable_storage_notifications.h
+++ b/chrome/browser/system_monitor/removable_storage_notifications.h
@@ -9,6 +9,7 @@
 #include "base/observer_list_threadsafe.h"
 #include "base/string16.h"
 #include "base/synchronization/lock.h"
+#include "chrome/browser/system_monitor/media_storage_util.h"
 
 class ChromeBrowserMainPartsLinux;
 class ChromeBrowserMainPartsMac;
@@ -30,13 +31,37 @@ class RemovableStorageNotifications {
                 const base::FilePath::StringType& device_location);
 
     // Unique device id - persists between device attachments.
+    // This is the string that should be used as the label for a particular
+    // storage device when interacting with the API. Clients should treat
+    // this as an opaque string.
     std::string device_id;
 
     // Human readable removable storage device name.
     string16 name;

[vandebo (ex-Chrome), 2013/02/08 02:03:31]

Do you want to remove this and instead have a function that takes a StorageInfo
and constructors a human readable name?

[Greg Billock, 2013/02/08 16:42:02]

Yes. I left it alone for now -- my idea was to strictly add fields and leave all
existing functionality untouched, then convert clients to use the new fields.
Shall I just bite off the whole thing?

[vandebo (ex-Chrome), 2013/02/08 23:20:36]

I think that's a sufficiently focused CL: Push name generation of the stack and
split out the info needed for names here.

 
     // Current attached removable storage device location.
+    // TODO(gbillock): Just use a FilePath here?

[vandebo (ex-Chrome), 2013/02/08 02:03:31]

For MTP devices it is not a real file path.

[Greg Billock, 2013/02/08 16:42:02]

Right. They'd leave the FilePath as empty(). Currently we do some very shocking
things with MTP and paths.

[vandebo (ex-Chrome), 2013/02/08 23:20:36]

But we need a location string to be able to connect to the device, where would
we store that?

     base::FilePath::StringType location;
+
+    // Label given to this storage device by the user.
+    // May be empty if not found or the device is unlabeled.
+    string16 storage_label;
+
+    // Vendor name for the removable device. (Human readable)
+    // May be empty if not collected.
+    string16 vendor_name;
+
+    // Model name for the removable device. (Human readable)
+    // May be empty if not collected.
+    string16 model_name;
+
+    // Size of the removable device in bytes.
+    // Zero if not collected or unknown.
+    uint64 total_size_in_bytes;
+
+    // Type of device.
+    // TODO(gbillock): Move this enum to this class.
+    MediaStorageUtil::Type device_type;

[vandebo (ex-Chrome), 2013/02/08 02:03:31]

This is already in device_id, there's no reason to break it out separately.

[Greg Billock, 2013/02/08 16:42:02]

The existence of CrackDeviceId and MakeDeviceId mean that field is not opaque,
and is really two fields, both of which are accessible. I think that's the right
visibility choice, but we should move to storing the two fields as two separate
fields, not combining them. Even though it's done losslessly it just makes the
existing code more difficult. Is there a motivation in the way prefs are stored
for this? I haven't read that as carefully as the system_monitor stuff.

[vandebo (ex-Chrome), 2013/02/08 23:20:36]

Prefs only stores a device id.  To make them store both a device id and a type
would be a non-backward compatible change, requiring conversion code; I don't
think it's worth it.  If we do want to do it though, lets do that in it's own
CL.

   };
 
   // This interface is provided to generators of storage notifications.
@@ -44,9 +69,7 @@ class RemovableStorageNotifications {
    public:
     virtual ~Receiver();
 
-    virtual void ProcessAttach(const std::string& id,
-                               const string16& name,
-                               const base::FilePath::StringType& location) = 0;
+    virtual void ProcessAttach(const StorageInfo& info) = 0;
     virtual void ProcessDetach(const std::string& id) = 0;
   };