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

chrome/browser/system_monitor/media_storage_util.cc

 // 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.
 
 #include "chrome/browser/system_monitor/media_storage_util.h"
 
 #include <vector>
 
@@ skipping 16 matching lines @@
 #include "chrome/browser/system_monitor/removable_device_notifications_window_win.h"
 #endif
 
 #if defined(OS_LINUX)  // Implies OS_CHROMEOS
 #include "chrome/browser/system_monitor/media_transfer_protocol_device_observer_linux.h"
 #endif
 
 using base::SystemMonitor;
 using content::BrowserThread;
 
+const char kRootPath[] = "/";
+
 namespace chrome {
 
 namespace {
 
 typedef std::vector<SystemMonitor::RemovableStorageInfo> RemovableStorageInfo;
 
 // MediaDeviceNotification.DeviceInfo histogram values.
 enum DeviceInfoHistogramBuckets {
   MASS_STORAGE_DEVICE_NAME_AND_UUID_AVAILABLE,
   MASS_STORAGE_DEVICE_UUID_MISSING,
   MASS_STORAGE_DEVICE_NAME_MISSING,
   MASS_STORAGE_DEVICE_NAME_AND_UUID_MISSING,
   MTP_STORAGE_DEVICE_NAME_AND_UUID_AVAILABLE,
   MTP_STORAGE_DEVICE_UUID_MISSING,
   MTP_STORAGE_DEVICE_NAME_MISSING,
   MTP_STORAGE_DEVICE_NAME_AND_UUID_MISSING,
   DEVICE_INFO_BUCKET_BOUNDARY
 };
 
 // Prefix constants for different device id spaces.
 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,
     FilePath* relative_path) = NULL;
 
 void ValidatePathOnFileThread(
     const FilePath& path, const MediaStorageUtil::BoolCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           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 =
       SystemMonitor::Get()->GetAttachedRemovableStorage();
   for (RemovableStorageInfo::const_iterator it = media_devices.begin();
        it != media_devices.end();
        ++it) {
     if (it->device_id == device_id)
       return it->location;
   }
@@ skipping 14 matching lines @@
       continue;
     }
 
     if (type == MediaStorageUtil::FIXED_MASS_STORAGE) {
       if (!file_util::PathExists(FilePath::FromUTF8Unsafe(unique_id)))
         missing_devices.insert(*it);
       continue;
     }
 
     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 =
            missing_devices.begin();
        it != missing_devices.end();
        ++it) {
     devices->erase(*it);
   }
 }
 
 }  // 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) {
   DCHECK(!unique_id.empty());
   switch (type) {
     case REMOVABLE_MASS_STORAGE_WITH_DCIM:
       return std::string(kRemovableMassStorageWithDCIMPrefix) + unique_id;
     case REMOVABLE_MASS_STORAGE_NO_DCIM:
       return std::string(kRemovableMassStorageNoDCIMPrefix) + unique_id;
     case FIXED_MASS_STORAGE:
       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();
 }
 
 // static
 bool 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 = prefix_length != std::string::npos ?
                        device_id.substr(0, prefix_length + 1) : "";
 
   Type found_type;
   if (prefix == kRemovableMassStorageWithDCIMPrefix) {
     found_type = REMOVABLE_MASS_STORAGE_WITH_DCIM;
   } else if (prefix == kRemovableMassStorageNoDCIMPrefix) {
     found_type = REMOVABLE_MASS_STORAGE_NO_DCIM;
   } else if (prefix == kFixedMassStoragePrefix) {
     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;
   }
   if (type)
     *type = found_type;
 
   if (unique_id)
     *unique_id = device_id.substr(prefix_length + 1);
   return true;
 }
 
 // static
 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
 bool MediaStorageUtil::IsRemovableDevice(const std::string& device_id) {
   Type type;
   return CrackDeviceId(device_id, &type, NULL) && type != FIXED_MASS_STORAGE;
 }
 
 // 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
 void MediaStorageUtil::IsDeviceAttached(const std::string& device_id,
                                         const BoolCallback& callback) {
   Type type;
   std::string unique_id;
   if (!CrackDeviceId(device_id, &type, &unique_id)) {
     callback.Run(false);
     return;
   }
 
   if (type == FIXED_MASS_STORAGE) {
@@ skipping 104 matching lines @@
   Type type;
   std::string unique_id;
   if (!CrackDeviceId(device_id, &type, &unique_id))
     return FilePath();
 
   if (type == FIXED_MASS_STORAGE) {
     // For this type, the unique_id is the path.
     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);
   return FilePath(FindRemovableStorageLocationById(device_id));
 }
 
 // static
 void MediaStorageUtil::RecordDeviceInfoHistogram(bool mass_storage,
                                                  const std::string& device_uuid,
                                                  const string16& device_name) {
@@ skipping 18 matching lines @@
 
 // static
 void MediaStorageUtil::SetGetDeviceInfoFromPathFunctionForTesting(
     GetDeviceInfoFromPathFunction function) {
   g_test_get_device_info_from_path_function = function;
 }
 
 MediaStorageUtil::MediaStorageUtil() {}
 
 }  // namespace chrome