[Media Galleries] Introduce a new type for Mac Image Capture.

chrome/browser/system_monitor/media_storage_util.cc

Index: chrome/browser/system_monitor/media_storage_util.cc
diff --git a/chrome/browser/system_monitor/media_storage_util.cc b/chrome/browser/system_monitor/media_storage_util.cc
index e4f69b4de0b6d93e40ecdea87e38a638317bb364..a4619e26f4eaeba521fe1268409695bac62436fd 100644
--- a/chrome/browser/system_monitor/media_storage_util.cc
+++ b/chrome/browser/system_monitor/media_storage_util.cc
@@ -34,6 +34,8 @@
 using base::SystemMonitor;
 using content::BrowserThread;
 
+const char kRootPath[] = "/";
+
 namespace chrome {
 
 namespace {
@@ -58,6 +60,7 @@ const char kRemovableMassStorageWithDCIMPrefix[] = "dcim:";
 const char kRemovableMassStorageNoDCIMPrefix[] = "nodcim:";
 const char kFixedMassStoragePrefix[] = "path:";
 const char kMtpPtpPrefix[] = "mtp:";
+const char kMacImageCapture[] = "ic:";
 
 static bool (*g_test_get_device_info_from_path_function)(  // NOLINT
     const FilePath& path, std::string* device_id, string16* device_name,
@@ -70,6 +73,18 @@ void ValidatePathOnFileThread(
                           base::Bind(callback, file_util::PathExists(path)));
 }
 
+bool IsMTPDeviceAttached(const std::string& id) {

[Lei Zhang, 2012/12/10 21:53:41]

Isn't this function really "IsDeviceAttached()" ?

[Lei Zhang, 2012/12/10 21:53:41]

Can this just call FindRemovableStorageLocationById() and see if the return
value is empty? Are you special-casing this because the ImageCapture devices
have no location?

[Greg Billock, 2012/12/10 22:39:41]

I think that's what I was doing. I didn't want to add a location (they really
have none), but there ends up being too much reliance on that. I think in the
end that's a really compelling indication that this (overloading location with
bogus data) is a bad plan we should move away from.

There's tons of baked-in assumptions about this location, virtually all of which
are based on false premises for MTP devices. It leaves things very fragile --
anyone dealing with that location will be justified in doing something like
checking for path existence or something, which will break the code.

+  RemovableStorageInfo media_devices =
+      SystemMonitor::Get()->GetAttachedRemovableStorage();
+  for (RemovableStorageInfo::const_iterator it = media_devices.begin();
+       it != media_devices.end();
+       ++it) {
+    if (it->device_id == id)
+      return true;
+  }
+  return false;
+}
+
 FilePath::StringType FindRemovableStorageLocationById(
     const std::string& device_id) {
   RemovableStorageInfo media_devices =
@@ -104,10 +119,15 @@ void FilterAttachedDevicesOnFileThread(MediaStorageUtil::DeviceIdSet* devices) {
     }
 
     DCHECK(type == MediaStorageUtil::MTP_OR_PTP ||
+           type == MediaStorageUtil::MAC_IMAGE_CAPTURE ||
            type == MediaStorageUtil::REMOVABLE_MASS_STORAGE_WITH_DCIM ||
            type == MediaStorageUtil::REMOVABLE_MASS_STORAGE_NO_DCIM);
-    if (FindRemovableStorageLocationById(*it).empty())
+    if (type == MediaStorageUtil::MAC_IMAGE_CAPTURE) {
+      if (!IsMTPDeviceAttached(*it))
+        missing_devices.insert(*it);
+    } else if (FindRemovableStorageLocationById(*it).empty()) {
       missing_devices.insert(*it);
+    }
   }
 
   for (MediaStorageUtil::DeviceIdSet::const_iterator it =
@@ -120,6 +140,7 @@ void FilterAttachedDevicesOnFileThread(MediaStorageUtil::DeviceIdSet* devices) {
 
 }  // namespace
 
+// NOTE: instead of all this parsing, why not just pass the type itself?

[sail, 2012/12/10 19:38:48]

this comment isn't clear, is this supposed to be a TODO?
maybe rephrase?

[Greg Billock, 2012/12/10 19:47:37]

Removing. This was a note-to-self.

On 2012/12/10 19:38:48, sail wrote:

 // static
 std::string MediaStorageUtil::MakeDeviceId(Type type,
                                            const std::string& unique_id) {
@@ -133,6 +154,8 @@ std::string MediaStorageUtil::MakeDeviceId(Type type,
       return std::string(kFixedMassStoragePrefix) + unique_id;
     case MTP_OR_PTP:
       return std::string(kMtpPtpPrefix) + unique_id;
+    case MAC_IMAGE_CAPTURE:
+      return std::string(kMacImageCapture) + unique_id;
   }
   NOTREACHED();
   return std::string();
@@ -154,6 +177,8 @@ bool MediaStorageUtil::CrackDeviceId(const std::string& device_id,
     found_type = FIXED_MASS_STORAGE;
   } else if (prefix == kMtpPtpPrefix) {
     found_type = MTP_OR_PTP;
+  } else if (prefix == kMacImageCapture) {
+    found_type = MAC_IMAGE_CAPTURE;
   } else {
     NOTREACHED();
     return false;
@@ -170,7 +195,8 @@ bool MediaStorageUtil::CrackDeviceId(const std::string& device_id,
 bool MediaStorageUtil::IsMediaDevice(const std::string& device_id) {
   Type type;
   return CrackDeviceId(device_id, &type, NULL) &&
-      (type == REMOVABLE_MASS_STORAGE_WITH_DCIM || type == MTP_OR_PTP);
+      (type == REMOVABLE_MASS_STORAGE_WITH_DCIM || type == MTP_OR_PTP ||
+       type == MAC_IMAGE_CAPTURE);
 }
 
 // static
@@ -182,7 +208,22 @@ bool MediaStorageUtil::IsRemovableDevice(const std::string& device_id) {
 // static
 bool MediaStorageUtil::IsMassStorageDevice(const std::string& device_id) {
   Type type;
-  return CrackDeviceId(device_id, &type, NULL) && type != MTP_OR_PTP;
+  return CrackDeviceId(device_id, &type, NULL) &&
+         (type == REMOVABLE_MASS_STORAGE_WITH_DCIM ||
+          type == REMOVABLE_MASS_STORAGE_NO_DCIM ||
+          type == FIXED_MASS_STORAGE);
+}
+
+// static
+bool MediaStorageUtil::CanCreateFileSystem(const std::string& device_id,
+                                           const FilePath& path) {
+  Type type;
+  if (!CrackDeviceId(device_id, &type, NULL)) return false;

[sail, 2012/12/10 19:38:48]

return on separate line

[Greg Billock, 2012/12/10 19:47:37]

Done.

+
+  if (type == MAC_IMAGE_CAPTURE)
+    return true;
+
+  return path.IsAbsolute() && path.ReferencesParent();

[Lei Zhang, 2012/12/10 21:53:41]

the second part is "not path.ReferencesParent()"

[Greg Billock, 2012/12/10 22:39:41]

Yes, I'd noticed this up in my client. I don't remember why this was backwards,
but now repaired.

 }
 
 // static
@@ -310,6 +351,14 @@ FilePath MediaStorageUtil::FindDevicePathById(const std::string& device_id) {
     return FilePath::FromUTF8Unsafe(unique_id);
   }
 
+  // This should return "/" because when the prefs returns an absolute path,
+  // it just smacks it on the end of whatever this method returns.
+  // For ImageCapture, we're not using the path as a path anyway, so return
+  // something to make the fake path arithmetic work out.
+  if (type == MAC_IMAGE_CAPTURE) {
+    return FilePath(kRootPath + device_id);
+  }
+
   DCHECK(type == MTP_OR_PTP ||
          type == REMOVABLE_MASS_STORAGE_WITH_DCIM ||
          type == REMOVABLE_MASS_STORAGE_NO_DCIM);