[Media Galleries] Populate volume metadata in ChromeOS/Linux

chrome/browser/system_monitor/removable_storage_notifications.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_SYSTEM_MONITOR_REMOVABLE_STORAGE_NOTIFICATIONS_H_
 #define CHROME_BROWSER_SYSTEM_MONITOR_REMOVABLE_STORAGE_NOTIFICATIONS_H_
 
 #include "base/file_path.h"
 #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;
 class MediaGalleriesPrivateApiTest;
 
 namespace chrome {
 
 class MediaFileSystemRegistryTest;
 class RemovableStorageObserver;
 
 // Base class for platform-specific instances watching for removable storage
 // attachments/detachments.
 class RemovableStorageNotifications {
  public:
   struct StorageInfo {
     StorageInfo();
     StorageInfo(const std::string& id,

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

This constructor implies that sometimes the new fields won't be valid.  Can we
distinguish that case from when the new fields simply aren't available?  I'd
rather have all the fields be valid all the time, which means the constructor
should take everything.

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

That's correct -- we won't necessarily collect all this information, or it may
not be collectable. On ChromeOS we can get it all pretty readily. For MTP,
Windows, and Mac some of the fields either aren't known, don't apply, or we
don't currently  collect them. I do think we should start attempting to do this
uniformly, though. I mostly left this alone to not be as invasive while adding
new fields.

On 2013/02/08 02:03:31, vandebo wrote:

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

If we don't collect something on a platform that's fine, but we should fill in
everything that we do populate for that platform.  As such, I think the
constructor should take all of those items.  It's a pet-peeve of mine to have
fields that are only valid some of the time, especially when there's no way to
tell if it should be valid or not.

                 const string16& device_name,
                 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.
   class Receiver {
    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;
   };
 
   // Returns a pointer to an object owned by the BrowserMainParts, with lifetime
   // somewhat shorter than a process Singleton.
   static RemovableStorageNotifications* GetInstance();
 
   // Finds the device that contains |path| and populates |device_info|.
   // Should be able to handle any path on the local system, not just removable
   // storage. Returns false if unable to find the device.
   virtual bool GetDeviceInfoForPath(
       const base::FilePath& path,
       StorageInfo* device_info) const = 0;
 
   // Returns the storage size of the device present at |location|. If the
   // device information is unavailable, returns zero.
   virtual uint64 GetStorageSize(const std::string& location) const = 0;

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

If we have storage size in the storage info, then we don't need this method any
more, right?

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

Right. I'm planning to get rid of it. The use is in
MediaStorageUtil::GetDeviceInfoFromPath, which needs a lot of attention.

 
 #if defined(OS_WIN)
   // Gets the MTP device storage information specified by |storage_device_id|.
   // On success, returns true and fills in |device_location| with device
   // interface details and |storage_object_id| with the string ID that
   // uniquely identifies the object on the device. This ID need not be
   // persistent across sessions.
   virtual bool GetMTPStorageInfoFromDeviceId(
       const std::string& storage_device_id,
       string16* device_location,
@@ skipping 35 matching lines @@
   // For manipulating removable_storage_map_ structure.
   mutable base::Lock storage_lock_;
 
   // Map of all the attached removable storage devices.
   RemovableStorageMap storage_map_;
 };
 
 } // namespace chrome
 
 #endif  // CHROME_BROWSER_SYSTEM_MONITOR_REMOVABLE_STORAGE_NOTIFICATIONS_H_