SystemMonitor: Pull device type into the device id

chrome/browser/media_gallery/media_storage_util.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.
+
+// chrome::MediaStorageUtil provides information about storage devices attached
+// to the computer.
+
+#ifndef CHROME_BROWSER_MEDIA_GALLERY_MEDIA_STORAGE_UTIL_H_
+#define CHROME_BROWSER_MEDIA_GALLERY_MEDIA_STORAGE_UTIL_H_
+
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/file_path.h"
+#include "base/lazy_instance.h"
+
+namespace chrome {
+
+class MediaStorageUtil {

[kmadhusu, 2012/08/17 18:53:35]

nit: Either move the comments from line 5 to here or Add "See comment at top of
file for a complete description"

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Seems unnecessary to me.

+ public:
+  enum Type {
+    // A USB mass storage device with a DCIM directory.
+    USB_MASS_STORAGE_WITH_DCIM,
+    // A USB mass storage device without a DCIM directory.
+    USB_MASS_STORAGE_NO_DCIM,

[kmadhusu, 2012/08/17 18:53:35]

You might want to add TODO's
(1) To handle mass storage with NO DCIM directory 
(2) To identify whether the mass storage is USB or not.

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Where would you like me to add those TODOs?

[kmadhusu, 2012/08/20 17:13:49]

You can add these TODO's in IsMediaDevice function

[vandebo (ex-Chrome), 2012/08/20 18:35:14]

Actually there's already TODO's in the implementation of GetDeviceInfoFromPath.

+    // Mass storage not connected through USB - a proxy for fixed/removable.
+    OTHER_MASS_STORAGE,
+    // A MTP or PTP device.
+    USB_MTP,
+  };
+
+  // Makes a device id given properties of the device. A prefix dependant on

[kmadhusu, 2012/08/17 18:53:35]

nit: dependent

[kmadhusu, 2012/08/17 18:53:35]

nit: Returns device id ....

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

+  // |type| is added so |unique_id| need only be unique within the given type.
+  static std::string MakeDeviceId(Type type, const std::string& unique_id);
+
+  // Extracts the device type from |device_id|.

[kmadhusu, 2012/08/17 18:53:35]

nit: Extracts device |type| and |unique_id| given |device_id|.

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

+  static void CrackDeviceId(const std::string& device_id,
+                            Type* type, std::string* unique_id);
+
+  // Looks inside |device_id| to determine if it is a media device
+  // (type is USB_MASS_STORAGE_WITH_DCIM or USB_MTP).

[kmadhusu, 2012/08/17 18:53:35]

A device of type USB_MASS_STORAGE_WITH_NO_DCIM can also be a media device right?

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

No, it would be type USB_MASS_STORAGE_WITH_DCIM in that case.

+  static bool IsMediaDevice(const std::string& device_id);
+
+  // Looks inside |device_id| to determine if it is a media device
+  // (type isn't OTHER_MASS_STORAGE).
+  static bool IsRemovableDevice(const std::string& device_id);

[kmadhusu, 2012/08/17 18:53:35]

If IsMediaDevice() returns true of USB_MASS_STORAGE_WITH_DCIM,
USB_MASS_STORAGE_WITH_NO_DCIM and USB_MTP, we can remove this function.

+
+  // Determines if the device is attached to the computer.
+  static void IsDeviceAttached(const std::string& device_id,
+                               const base::Callback<void(bool)>& callback);

[kmadhusu, 2012/08/17 18:53:35]

nit: Can we create a typedef for the Query callbacks? (here and at other places)

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

+
+  // Given a path |input|, get the device_id, relative path from the root of

[kmadhusu, 2012/08/17 18:53:35]

nit: Can we rename |input| to |path|? Since it is a reference param, it is
implicit that it is a input param.

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

+  // the device, and the device name.
+  static void GetDeviceInfoFromPath(
+      const FilePath& input,
+      const base::Callback<void(std::string /*device id*/,

[kmadhusu, 2012/08/17 18:53:35]

nit: /* ABC */ (Space after /*)

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Actually there's no clear guidance on this in the style guide.  The only
occurrence of an inline comment uses the no-space style. 

+                                FilePath /*path relative to device root*/,
+                                string16 /*display name*/)>& callback);
+
+  // Get a FilePath for the given |device_id|.  If the device isn't connected
+  // or isn't a mass storage type, the FilePath will be empty.
+  static void FindDevicePathById(
+      const std::string& device_id,
+      const base::Callback<void(FilePath)>& callback);
+
+ private:
+  MediaStorageUtil();

[kmadhusu, 2012/08/17 18:53:35]

nit: Document this function // Do not instantiate this class

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

+
+  static void FindUSBDeviceById(const std::string unique_id,

[kmadhusu, 2012/08/17 18:53:35]

nit: Document this function (To specify that this is a platform specific code).

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

+                                const base::Callback<void(FilePath)>& callback);

[kmadhusu, 2012/08/17 18:53:35]

DISALLOW_COPY_AND_ASSIGN(MediaStorageUtil);

[vandebo (ex-Chrome), 2012/08/17 22:55:51]

Done.

+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_MEDIA_GALLERY_MEDIA_STORAGE_UTIL_H_