Adds removable-drive listing for OS X.

chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc

Index: chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc
diff --git a/chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc b/chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc
index f227dde49e3e4ad9c34922be154df2c1b82a80ba..98b4e568eb91adfff1a4c2efc1638c372e5a4153 100644
--- a/chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc
+++ b/chrome/browser/extensions/api/image_writer_private/removable_storage_provider_mac.cc
@@ -2,13 +2,190 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <IOKit/storage/IOStorageProtocolCharacteristics.h>
+
+#include "base/strings/sys_string_conversions.h"
 #include "chrome/browser/extensions/api/image_writer_private/removable_storage_provider.h"
+#include "content/public/browser/browser_thread.h"
 
 namespace extensions {
 
-bool RemovableStorageProvider::PopulateDeviceList(
+void RemovableStorageProvider::GetAllDevicesImpl(
+    const DeviceListReadyCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  DAWrapper* da_wrapper = new DAWrapper();

[Robert Sesek, 2014/06/02 19:56:05]

Comment about ownership.

[Drew Haven, 2014/06/03 02:03:23]

I wasn't sure how to address this exactly.  We get to a point where we're
waiting on DiskArbitration callbacks but there are no callbacks on the message
queue that have references to this object.  The lifetime of this object should
roughly be that of the callback.  I don't think I can use callback ownership
because this class has to keep a copy of the callback to call later and so would
create a circular reference.  I opted for this pattern of making it a bare
pointer and then deleting it when the callback gets called.

To be sure it gets deleted I added a timer that times out and calls the callback
with a failure after a small interval.

I also added unit-tests to be sure that this behavior was occurring by checking
the value of a weak pointer to this object after the callback has run.

+  da_wrapper->GetDeviceList(
+      base::Bind(DAWrapper::WrapperDeleter, da_wrapper, callback));
+}
+
+DAWrapper::DAWrapper() : updates_pending_(0), callback_() {
+}
+
+DAWrapper::~DAWrapper() {
+  if (session_.get()) {
+    DASessionUnscheduleFromRunLoop(
+        session_, CFRunLoopGetCurrent(), kCFRunLoopCommonModes);
+  }
+}
+
+void DAWrapper::GetDeviceList(const DeviceListReadyCallback& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  device_list_ = new StorageDeviceList;
+  callback_ = callback;
+
+  session_.reset(DASessionCreate(NULL));
+
+  DARegisterDiskAppearedCallback(
+      session_, kDADiskDescriptionMatchMediaWhole, DiskAppearedCallback, this);
+  DARegisterDiskDisappearedCallback(session_,
+                                    kDADiskDescriptionMatchMediaWhole,
+                                    DiskDisappearedCallback,
+                                    this);
+  DASessionScheduleWithRunLoop(
+      session_, CFRunLoopGetCurrent(), kCFRunLoopCommonModes);

[Robert Sesek, 2014/06/02 19:56:05]

Really? I thought you said this was going to execute in the utility process.
You're scheduling this on the browser's UI thread.

[Drew Haven, 2014/06/03 02:03:23]

Discussed out of band.  See general comment.

+}
+
+void DAWrapper::ProcessDisk(base::ScopedCFTypeRef<CFDictionaryRef> dict) {
+  updates_pending_++;
+  content::BrowserThread::PostTask(

[Robert Sesek, 2014/06/02 19:56:05]

Why are you using message passing here? It's all being done on the UI thread
anyways…

[Drew Haven, 2014/06/03 02:03:23]

The reason is because we get a bunch of DiskAppearedCallbacks in the queue and
we want to know when we've processed all of them, so we basically just go
through and count them up before processing, then as we process disks we
decrement the counter and once we reach zero we know we've processed all the
callbacks.  I tried to explain that in the method comments in the header.  Let
me know if that makes sense, otherwise I can reword it.  I learned the trick
from StorageMonitorMac.

One case that is not covered yet is if there are no callbacks (no disks
somehow...) but I'll try to take care of that with a timeout.

[Robert Sesek, 2014/06/04 19:10:43]

But does that actually work as you describe? There's no guarantee that the
AddDisk() callback will run after a second DeviceAppearedCallback(), meaning you
could be incrementing to 1 updates_pending_, then decrementing it right back
down to 0. Am I missing something?

[Drew Haven, 2014/06/05 01:52:50]

As far as I can tell this is entirely undocumented and I suppose we're just
hoping it works that way.  I've been thinking about it and the only way around
it I can think of with DiskArb is to instantiate a global service, much like
StorageMonitor that will keep a watchful and on-going eye on the disks.  I don't
know of a way to immediately list all current disks.

The existing tool starts up it's monitor when the application starts up.

So, I guess we have to decide if the empirical evidence is strong enough
(risky), I'll have to integrate more deeply into something like StorageMonitor
(integration headaches) or create my own service for this (complexity and code
duplication).

Preference?

[Drew Haven, 2014/06/09 23:13:02]

I talked with Greg Billock who did the similar work in the Storage Monitor.  It
looks like it just appears to be what the API does but it isn't really
documented anywhere.

I think in the long run we want to merge this into the storage monitor and there
are open bugs to look at that, but at the moment this appears to work well.  My
guess is that when the session starts up on the loop it immediately posts an
event for each of the known disks that are already known by the system and so
they are all immediately on the queue.  I can't prove this though, since we
can't look at the source.

Is it possible we can leave this as is and look at further integration at
another time?

[Robert Sesek, 2014/06/10 20:52:22]

Then what is the rationale for not using storage monitor directly, if it's the
ultimate goal?
I don't think relying on undocumented behavior is the right way to go, unless
there's really nothing better. I was curious how `diskutil list` works, and it
actually does not use DiskArbitration to do this, but instead just uses IOKit.

(The following is from reverse engineering _DMgetDisks in the
DiskManagement.framework PrivateFramework, and it seems to work):

  NSMutableArray* array = [NSMutableArray array];
  CFMutableDictionaryRef matching = IOServiceMatching("IOMedia");
  CFDictionaryAddValue(matching, CFSTR("Whole"), kCFBooleanTrue);
  io_service_t iterator;
  IOServiceGetMatchingServices(kIOMasterPortDefault, matching, &iterator);
  io_object_t obj;
  while ((obj = IOIteratorNext(iterator))) {
    DADiskRef disk = DADiskCreateFromIOMedia(NULL, session, obj);
    [array addObject:disk];
    IOObjectRelease(obj);
    CFRelease(disk);
  }
  IOObjectRelease(iterator);


Unless you need continued monitoring of active disks, I think this would work
here; and then you won't need to worry about timeouts, updates_pending_, or
adding blocking work to the UI thread. This could just be done on the
file-blocking pool.

+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&DAWrapper::AddDisk, AsWeakPtr(), dict));
+}
+
+void DAWrapper::AddDisk(base::ScopedCFTypeRef<CFDictionaryRef> dict) {
+  if (DiskIsValidTarget(dict.get())) {
+    CFStringRef cf_bsd_name = base::mac::GetValueFromDictionary<CFStringRef>(
+        dict, kDADiskDescriptionMediaBSDNameKey);
+    CFStringRef cf_vendor = base::mac::GetValueFromDictionary<CFStringRef>(
+        dict, kDADiskDescriptionDeviceVendorKey);
+    CFStringRef cf_model = base::mac::GetValueFromDictionary<CFStringRef>(
+        dict, kDADiskDescriptionDeviceModelKey);
+
+    std::string bsd_name = base::SysCFStringRefToUTF8(cf_bsd_name);
+    std::string vendor = base::SysCFStringRefToUTF8(cf_vendor);
+    std::string model = base::SysCFStringRefToUTF8(cf_model);
+
+    CFNumberRef size_number = base::mac::GetValueFromDictionary<CFNumberRef>(
+        dict, kDADiskDescriptionMediaSizeKey);
+    uint64 size_in_bytes = 0;
+    if (size_number)
+      CFNumberGetValue(size_number, kCFNumberLongLongType, &size_in_bytes);
+
+    linked_ptr<api::image_writer_private::RemovableStorageDevice> device(
+        new api::image_writer_private::RemovableStorageDevice());
+    device->storage_unit_id = bsd_name;
+    device->capacity = size_in_bytes;
+    device->vendor = vendor;
+    device->model = model;
+    device_list_->data.push_back(device);
+  }
+
+  updates_pending_--;
+
+  if (updates_pending_ == 0) {
+    callback_.Run(device_list_, true);
+  }
+}
+
+// static
+void DAWrapper::DiskAppearedCallback(DADiskRef disk, void* context) {
+  DAWrapper* wrapper = static_cast<DAWrapper*>(context);
+  base::ScopedCFTypeRef<CFDictionaryRef> dict(DADiskCopyDescription(disk));
+  wrapper->ProcessDisk(dict);
+}
+
+// static
+void DAWrapper::DiskDisappearedCallback(DADiskRef disk, void* context) {
+}
+
+// static
+bool DAWrapper::DiskIsValidTarget(CFDictionaryRef dict) {
+  CFBooleanRef internal = (CFBooleanRef)CFDictionaryGetValue(

[Robert Sesek, 2014/06/02 19:56:05]

C-style casts are banned. Use GetValueFromDictionary like you do above.

[Drew Haven, 2014/06/03 02:03:23]

Done.

+      dict, kDADiskDescriptionDeviceInternalKey);
+  CFStringRef protocol = (CFStringRef)CFDictionaryGetValue(
+      dict, kDADiskDescriptionDeviceProtocolKey);
+  CFStringRef io_reg_path =
+      (CFStringRef)CFDictionaryGetValue(dict, kDADiskDescriptionDevicePathKey);
+  CFBooleanRef ejectable = (CFBooleanRef)CFDictionaryGetValue(
+      dict, kDADiskDescriptionMediaEjectableKey);
+  CFBooleanRef removable = (CFBooleanRef)CFDictionaryGetValue(
+      dict, kDADiskDescriptionMediaRemovableKey);
+  CFBooleanRef whole =
+      (CFBooleanRef)CFDictionaryGetValue(dict, kDADiskDescriptionMediaWholeKey);
+  CFStringRef kind =
+      (CFStringRef)CFDictionaryGetValue(dict, kDADiskDescriptionMediaKindKey);
+
+  // A drive is a USB stick iff:
+  // - it is not internal
+  // - it is attached to the USB bus
+  // - it is ejectable (because it will be ejected after written to)
+  // - it is removable
+  // - it is the whole drive (although the use of
+  //   kDADiskDescriptionMatchMediaWhole should have ensured this)
+  // - it is of type IOMedia (external DVD drives and the like are IOCDMedia
+  // or
+  //   IODVDMedia)
+  bool isUSBStick =

[Robert Sesek, 2014/06/02 19:56:05]

naming: is_usb_stick

[Drew Haven, 2014/06/03 02:03:23]

Done.

+      !CFBooleanGetValue(internal) &&
+      CFEqual(protocol, CFSTR(kIOPropertyPhysicalInterconnectTypeUSB)) &&
+      CFBooleanGetValue(ejectable) && CFBooleanGetValue(removable) &&
+      CFBooleanGetValue(whole) &&
+      CFStringCompare(kind, CFSTR("IOMedia"), 0) == kCFCompareEqualTo;
+
+  // A drive is an SD card iff:
+  // - it is attached to the USB bus
+  // - it is ejectable (because it will be ejected after written to)
+  // - it is removable
+  // - it is the whole drive (although the use of
+  //   kDADiskDescriptionMatchMediaWhole should have ensured this)
+  // - it is of type IOMedia (external DVD drives and the like are IOCDMedia
+  // or
+  //   IODVDMedia)
+  // - the IORegistry device path contains "AppleUSBCardReader"
+  bool isSDCard =

[Robert Sesek, 2014/06/02 19:56:05]

naming: is_sd_card

[Drew Haven, 2014/06/03 02:03:23]

Done.

+      CFEqual(protocol, CFSTR(kIOPropertyPhysicalInterconnectTypeUSB)) &&
+      CFBooleanGetValue(ejectable) && CFBooleanGetValue(removable) &&
+      CFBooleanGetValue(whole) &&
+      CFStringCompare(kind, CFSTR("IOMedia"), 0) == kCFCompareEqualTo &&
+      CFStringFind(io_reg_path, CFSTR("AppleUSBCardReader"), 0).location !=
+          kCFNotFound;
+
+  return isUSBStick || isSDCard;
+}
+
+// static
+void DAWrapper::WrapperDeleter(DAWrapper* wrapper,
+                               const DeviceListReadyCallback& callback,
+                               scoped_refptr<StorageDeviceList> device_list,
+                               bool success) {
+  delete wrapper;
+  callback.Run(device_list, success);
+}
+
+DAWrapperSpouse::DAWrapperSpouse(DAWrapper* spouse) : spouse_(spouse) {
+}
+DAWrapperSpouse::~DAWrapperSpouse() {
+}
+
+void DAWrapperSpouse::SetCallback(const DeviceListReadyCallback& callback) {
+  spouse_->callback_ = callback;
+}
+
+void DAWrapperSpouse::SetDeviceList(
     scoped_refptr<StorageDeviceList> device_list) {
-  return false;
+  spouse_->device_list_ = device_list;
+}
+
+void DAWrapperSpouse::ProcessDisk(base::ScopedCFTypeRef<CFDictionaryRef> disk) {
+  spouse_->ProcessDisk(disk);
+}
+
+// static
+bool DAWrapperSpouse::DiskIsValidTarget(CFDictionaryRef dict) {
+  return DAWrapper::DiskIsValidTarget(dict);
 }
 
 } // namespace extensions