Add Media device notification to SystemMonitor and Mac impl

content/browser/mac/media_device_notifications.mm

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Avi (use Gerrit), 2012/02/25 22:13:00]

File-wide comments:

- I do not want to see CFRelease scattered all over the place. Use
scoped_cfobject.
- Why are you using CFStringCompare with kCFCompareEqualTo? Why not CFEqual?
- This is .mm, Objective-C++. When appropriate, save yourself pain, NSToCFCast
the objects, and use ObjC to make your life easier.
- Please use appropriate constants rather than hard-coding strings.

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

ScopedCFTypeRef?  Done.
I don't know any better - not a Mac programmer.  Please educate me where
appropriate.

[Avi (use Gerrit), 2012/02/28 01:26:46]

No problem. If all you're trying to do is do equality comparisons, CFEqual is
simpler and shorter.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/mac/media_device_notifications.h"
+
+#include <Carbon/Carbon.h>
+#include <CoreFoundation/CoreFoundation.h>
+
+#include "base/file_path.h"
+#include "base/mac/foundation_util.h"
+#include "base/sys_string_conversions.h"
+#include "base/system_monitor/system_monitor.h"
+
+namespace content {
+
+namespace {
+
+bool GetDeviceInfo(unsigned long device_number, std::string* name,
+                   FilePath* location) {
+  ICACopyObjectPropertyDictionaryPB properties_request;
+  properties_request.object = device_number;
+  CFDictionaryRef device_properties;
+  properties_request.theDict = &device_properties;
+  OSErr ret = ICACopyObjectPropertyDictionary(&properties_request, NULL);
+  CHECK_EQ(ret, noErr);
+
+  // For now, we only support mass storage media devices.

[Avi (use Gerrit), 2012/02/25 22:13:00]

That's a huge limitation; e.g. iPhones are an ICA device that isn't
mass-storage. That's probably worth a TODO.

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

Changed to a todo.  Supporting all of OSX's media devices is on the roadmap. 
Supporting just mass storage was a simple thing to do to unblock another effort.

+  CFStringRef volume = base::mac::GetValueFromDictionary<CFStringRef>(
+      device_properties, CFSTR("volume"));

[Avi (use Gerrit), 2012/02/25 22:13:00]

Is there a constant you can use?

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

Not that I'm aware of.  "volume" does not appear in
/Volumes/Leopard/Developer/SDKs/MacOSX10.5.sdk/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/ImageCapture.framework/*

+  if (volume == NULL) {
+    CFRelease(device_properties);
+    return false;
+  }
+  *location = FilePath("/Volumes/").Append(base::SysCFStringRefToUTF8(volume));

[Avi (use Gerrit), 2012/02/25 22:13:00]

This scares me because this is almost correct and yet wrong. In the dictionary
that you get back, do you get any media identifiers? Use the Disk Arbitration
utils or something else to convert it to a mount path. Assuming that the mount
path is going to be /Volumes/volumename is a dangerous assumption.

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

Done.

+
+  CFStringRef device_name = base::mac::GetValueFromDictionary<CFStringRef>(
+      device_properties, CFSTR("ICAUserAssignedDeviceNameKey"));

[Avi (use Gerrit), 2012/02/25 22:13:00]

kICAUserAssignedDeviceNameKey. Constants, please!

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

Done. (Added my own, there doesn't seem to be one in the system header)

+  if (device_name == NULL) {
+    device_name = base::mac::GetValueFromDictionary<CFStringRef>(
+        device_properties, CFSTR("ifil"));

[Avi (use Gerrit), 2012/02/25 22:13:00]

kICAPropertyImageFilename? 

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

It's the wrong type.  Changed to a constant though.

+  }
+  if (device_name == NULL) {
+    CFRelease(device_properties);
+    return false;
+  }
+  *name = base::SysCFStringRefToUTF8(device_name);
+
+  CFRelease(device_properties);
+  return true;
+}
+
+void MediaDeviceNotificationCallback(CFStringRef notification_type,
+                                     CFDictionaryRef notification_dictionary) {
+  bool attach = false;
+  if (CFStringCompare(notification_type, kICANotificationTypeDeviceAdded, 0) ==
+      kCFCompareEqualTo) {
+    attach = true;
+  } else if (CFStringCompare(notification_type,
+                             kICANotificationTypeDeviceRemoved, 0) !=
+             kCFCompareEqualTo) {
+    return;
+  }
+
+  base::SystemMonitor* system_monitor = base::SystemMonitor::Get();
+
+  CFNumberRef device_number_object =
+      base::mac::GetValueFromDictionary<CFNumberRef>(
+          notification_dictionary, kICANotificationDeviceICAObjectKey);
+  unsigned long device_number;
+  CFNumberGetValue(device_number_object, kCFNumberIntType, &device_number);

[Avi (use Gerrit), 2012/02/25 22:13:00]

Objective C will likely be nice here.

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

Done.

+  if (attach) {
+    std::string device_name;
+    FilePath location;
+    if (GetDeviceInfo(device_number, &device_name, &location)) {
+      system_monitor->ProcessMediaDeviceAttached(device_number, device_name,
+                                                 location);
+    }
+  } else {
+    system_monitor->ProcessMediaDeviceDetached(device_number);
+  }
+}
+
+}  // namespace
+
+void StartMediaDeviceNotifications() {
+  CFStringRef events_of_interest_array[] = {kICANotificationTypeDeviceAdded,
+                                            kICANotificationTypeDeviceRemoved};
+  CFArrayRef events_of_interest =

[Avi (use Gerrit), 2012/02/25 22:13:00]

Does ICARegisterForEventNotificationPB own this? Does
ICARegisterForEventNotification take a ref? Are we going to be leaking this?

[vandebo (ex-Chrome), 2012/02/28 00:51:33]

This sample app implies that ICARegisterForEventNotification takes ownership:
https://developer.apple.com/library/mac/#samplecode/SampleScannerApp/Listings...

[Avi (use Gerrit), 2012/02/28 01:26:46]

You got ICA sample code to compile?! I tried two different ones and neither
compiled.

But the sample code you bring proves the opposite. -[NSArray arrayWithObjects:]
returns an autoreleased array, so if the Apple sample code works, it shows that
ICARegisterForEventNotification retains the array itself and does not take over
the ownership.

[vandebo (ex-Chrome), 2012/02/28 18:35:05]

I didn't try compiling the sample code, just used it as a reference.  I guess
it's good to know that they don't necessarily work/correctly.  As you probably
saw, I switched to the array construction style in the sample app (because it
was shorter), so ownership should be ok now.

+      CFArrayCreate(NULL, (const void**)&events_of_interest_array, 2,
+                    &kCFTypeArrayCallBacks);
+
+  ICARegisterForEventNotificationPB notification_request;
+  notification_request.objectOfInterest = 0;  // Zero means all objects
+  notification_request.eventsOfInterest = events_of_interest;
+  notification_request.notificationProc = &MediaDeviceNotificationCallback;
+  notification_request.options = NULL;
+  OSErr err = ICARegisterForEventNotification(&notification_request, NULL);
+  CHECK_EQ(err, noErr);
+}
+
+}  // namespace content