SystemMonitor: Pull device type into the device id

chrome/browser/media_gallery/media_storage_util.cc

Index: chrome/browser/media_gallery/media_storage_util.cc
diff --git a/chrome/browser/media_gallery/media_storage_util.cc b/chrome/browser/media_gallery/media_storage_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..73ede71174fc93389e921053e3457666a4652f28
--- /dev/null
+++ b/chrome/browser/media_gallery/media_storage_util.cc
@@ -0,0 +1,235 @@
+// 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 implementation.

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

nit: Comment is not required.

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

In Lei's readability review the reviewer suggested a one line comment at the top
of the file.  This mimics what's in media_file_system_registry.cc

+
+#include "chrome/browser/media_gallery/media_storage_util.h"
+
+#include <vector>
+
+#include "base/callback.h"
+#include "base/file_util.h"
+#include "base/lazy_instance.h"

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

lazy_instance is included both in .h and .cc file.

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

And used in neither.  Fixed.

+#include "base/logging.h"
+#include "base/system_monitor/system_monitor.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace chrome {
+
+namespace {
+
+typedef std::vector<base::SystemMonitor::MediaDeviceInfo> MediaDevicesInfo;
+
+const char kUsbMassStorageWithDCIMPrefix[] = "dcim:";

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

nit: // Media device unique id prefix constants.

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

Done.  (Added a more "why" comment than you suggested.)

+const char kUsbMassStorageNoDCIMPrefix[] = "usb:";
+const char kOtherMassStoragePrefix[] = "path:";
+const char kUsbMtpPrefix[] = "mtp:";
+
+void EmptyPathIsFalseCallback(const base::Callback<void(bool)>& callback,

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

Document this helper function.

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

I think the name and body are pretty self descriptive.

+                              FilePath path) {

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

nit: "const FilePath&"

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

This can't be const because it has to match the FilePathCallback type.

+  callback.Run(!path.empty());
+}
+
+void ValidatePathOnFileThread(const FilePath& path,
+                              const base::Callback<void(FilePath)>& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));

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

nit: Add using content::BrowserThread;

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

Done.

+  FilePath result;
+  if (file_util::PathExists(path))
+    result = path;
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                   base::Bind(callback, path));
+}
+
+FilePath MakePathRelative(FilePath path) {

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

nit: GetRelativePath
nit: const FilePath&
nit: Document the function stating that the source path is the relative path
when the source path is not absolute, etc.,
nit: If possible, add an E.g.,

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

Done.

+  if (!path.IsAbsolute())
+    return path;
+
+  FilePath relative;
+  std::vector<FilePath::StringType> components;
+  path.GetComponents(&components);
+
+  // On Windows, the first component may be the drive letter with the second
+  // being \\.
+  int start = 1;
+  if (components[1].size() == 1 && FilePath::IsSeparator(components[1][0]))
+    start = 2;
+
+  for (size_t i = start; i < components.size(); i++)
+    relative = relative.Append(components[i]);
+  return relative;
+}
+
+}  // namespace
+
+// static
+std::string MediaStorageUtil::MakeDeviceId(Type type,

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

nit: const Type&
nit: DCHECK(!unique_id.empty());

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

I don't think we usually use const ref's for enums.
Done.

+                                           const std::string& unique_id) {
+  switch (type) {
+    case USB_MASS_STORAGE_WITH_DCIM:
+      return std::string(kUsbMassStorageWithDCIMPrefix) + unique_id;
+    case USB_MASS_STORAGE_NO_DCIM:
+      return std::string(kUsbMassStorageNoDCIMPrefix) + unique_id;
+    case OTHER_MASS_STORAGE:
+      return std::string(kOtherMassStoragePrefix) + unique_id;
+    case USB_MTP:
+      return std::string(kUsbMtpPrefix) + unique_id;
+    default:
+      NOTREACHED();
+      return std::string();

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

Can we  return a bool to notify failure?

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

Failure shouldn't happen, hence the NOTREACHED.

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

(repeating our offline discussion): There is a chance that this function is
called with invalid type which may cause this function to return an empty
string. Therefore, it is better to return a bool value to notify the failure.

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

Our in person conversation was about corrupt preferences, i.e. CrackDeviceId
being able to fail.  MakeDeviceId still should never fail.  I added a comment in
the header file saying that if an invalid type is passed in the result is an
empty string.

+  }
+}
+
+// static
+void MediaStorageUtil::CrackDeviceId(const std::string& device_id,
+                                     Type* type, std::string* unique_id) {
+  size_t prefix_length = device_id.find_first_of(':');
+  std::string prefix = device_id.substr(0, prefix_length);
+
+  if (type) {
+    if (prefix == kUsbMassStorageWithDCIMPrefix) {
+      *type = USB_MASS_STORAGE_WITH_DCIM;
+    } else if (prefix == kUsbMassStorageNoDCIMPrefix) {
+      *type = USB_MASS_STORAGE_NO_DCIM;
+    } else if (prefix == kOtherMassStoragePrefix) {
+      *type = OTHER_MASS_STORAGE;
+    } else if (prefix == kUsbMtpPrefix) {
+      *type = USB_MTP;
+    } else {
+      NOTREACHED();

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

Can we  return a bool to notify failure?

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

Failure shouldn't happen, hence the NOTREACHED.

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

same here.

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

Done.

+    }
+  }
+
+  if (unique_id)
+    *unique_id = device_id.substr(prefix_length + 1);
+}
+
+// static
+bool MediaStorageUtil::IsMediaDevice(const std::string& device_id) {
+  Type type;
+  CrackDeviceId(device_id, &type, NULL);
+  return type == USB_MASS_STORAGE_WITH_DCIM || type == USB_MTP;
+}
+
+// static
+bool MediaStorageUtil::IsRemovableDevice(const std::string& device_id) {
+  Type type;
+  CrackDeviceId(device_id, &type, NULL);
+  return type != OTHER_MASS_STORAGE;
+}
+
+// static
+void MediaStorageUtil::IsDeviceAttached(
+    const std::string& device_id, const base::Callback<void(bool)>& callback) {
+  Type type;
+  std::string unique_id;
+  CrackDeviceId(device_id, &type, &unique_id);
+
+  switch (type) {
+    case USB_MTP:  // Fall through
+    case USB_MASS_STORAGE_WITH_DCIM: {
+      // We should be able to find media devices in SystemMonitor.
+      MediaDevicesInfo media_devices =
+          base::SystemMonitor::Get()->GetAttachedMediaDevices();

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

Add a helper function in SystemMonitor to check whether the device is attached
or not.

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

What advantage would that have?

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

As we discussed, you can leave this as it is.

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

As you suggested, I pulled the duplicated code into a helper function.

+      for (MediaDevicesInfo::const_iterator it = media_devices.begin();
+           it != media_devices.end();
+           ++it) {
+        if (it->unique_id == device_id) {
+          callback.Run(true);
+          return;
+        }
+      }
+      callback.Run(false);
+      break;
+    }
+    case USB_MASS_STORAGE_NO_DCIM:
+      FindUSBDeviceById(unique_id,
+                        base::Bind(&EmptyPathIsFalseCallback, callback));

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

nit: Can we rename "EmptyPathIsFalseCallback" ?

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

Do you have a suggestion?  I think it's a descriptive name.

+      break;
+    case OTHER_MASS_STORAGE:
+      // For this type, the unique_id is the path.
+      content::BrowserThread::PostTask(
+          content::BrowserThread::FILE, FROM_HERE,
+          base::Bind(&ValidatePathOnFileThread,
+                     FilePath::FromUTF8Unsafe(unique_id),
+                     base::Bind(&EmptyPathIsFalseCallback, callback)));
+      break;
+    default:
+      NOTREACHED();
+  }
+}
+
+// static
+void MediaStorageUtil::GetDeviceInfoFromPath(
+    const FilePath& input,
+    const base::Callback<void(std::string, FilePath, string16)>& callback) {
+  // TODO(vandebo) This needs to be implemented per platform.  Below is no
+  // worse than what the code already does.
+  // * Find mount point parent (determines relative file path)
+  // * Search System monitor, just in case.
+  // * If it's a USB device, generate device id, else use device root path as id
+  std::string device_id =
+      MakeDeviceId(OTHER_MASS_STORAGE, input.AsUTF8Unsafe());
+  FilePath relative_path = MakePathRelative(input);
+  string16 display_name = input.BaseName().LossyDisplayName();
+
+  callback.Run(device_id, relative_path, display_name);
+  return;
+}
+
+// static
+void MediaStorageUtil::FindDevicePathById(
+    const std::string& device_id,
+    const base::Callback<void(FilePath)>& callback) {
+  Type type;
+  std::string unique_id;
+  CrackDeviceId(device_id, &type, &unique_id);
+
+  switch (type) {
+    case USB_MTP:
+      callback.Run(FilePath());
+      break;
+    case USB_MASS_STORAGE_NO_DCIM:
+      FindUSBDeviceById(unique_id, callback);
+      break;
+    case OTHER_MASS_STORAGE:
+      // For this type, the unique_id is the path.
+      content::BrowserThread::PostTask(
+          content::BrowserThread::FILE, FROM_HERE,
+          base::Bind(&ValidatePathOnFileThread,
+                     FilePath::FromUTF8Unsafe(unique_id), callback));
+      break;
+    case USB_MASS_STORAGE_WITH_DCIM: {
+      FilePath result;
+      MediaDevicesInfo media_devices =
+          base::SystemMonitor::Get()->GetAttachedMediaDevices();

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

nit: using base::SystemMonitor;

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

Done.

+      for (MediaDevicesInfo::const_iterator it = media_devices.begin();
+           it != media_devices.end();
+           ++it) {
+        if (it->unique_id == device_id) {
+          result = FilePath(it->location);
+          break;
+        }
+      }
+      callback.Run(result);
+      break;
+    }
+    default:
+      NOTREACHED();
+  }
+}
+
+MediaStorageUtil::MediaStorageUtil() {}
+
+// static
+void MediaStorageUtil::FindUSBDeviceById(
+    const std::string unique_id,
+    const base::Callback<void(FilePath)>& callback) {
+  // TODO(vandebo) This needs to be implemented per platform.
+  // Type is USB_MASS_STORAGE_NO_DCIM, so it's a device possibly mounted
+  // somewhere...
+  NOTREACHED();
+  callback.Run(FilePath());
+}
+
+}  // namespace chrome