add device notification to Mac

content/browser/device_monitor_mac.mm

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

[Mark Mentovai, 2012/08/07 18:39:10]

Why is this a .mm file? You haven’t used any Objective-C. It should be a .cc
file.

[no longer working on chromium, 2012/08/08 08:42:42]

It is a following design from gamepad/platform_data_fetcher_mac.mm.
I can change it to .cc file.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/device_monitor_mac.h"
+
+#include <IOKit/usb/IOUSBLib.h>
+
+#include "base/logging.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/mac/scoped_ioobject.h"
+
+namespace content {
+
+namespace {
+
+struct {

[Mark Mentovai, 2012/08/07 18:39:10]

const?

[no longer working on chromium, 2012/08/08 08:42:42]

Done.

+  base::SystemMonitor::DeviceType device_type;
+  const io_name_t service_type;

[Mark Mentovai, 2012/08/07 18:39:10]

…but this doesn’t really need to be const

[no longer working on chromium, 2012/08/08 08:42:42]

I think it is a c_string, isn't it better to put it const unless we use const
struct?
 

+} kDeviceServices[] = {
+  // Add new services here if needed.
+  { base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE, kIOMatchedNotification },
+  { base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE, kIOTerminatedNotification },
+  { base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE, kIOMatchedNotification },
+  { base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE, kIOTerminatedNotification },
+};
+
+CFMutableDictionaryRef CreateMatchingDictionary(
+    SInt32 interface_class_code, SInt32 interface_subclass_code) {
+  CFMutableDictionaryRef matching_dictionary = IOServiceMatching(
+      kIOUSBInterfaceClassName);
+  base::mac::ScopedCFTypeRef<CFNumberRef> number_ref(CFNumberCreate(
+      kCFAllocatorDefault, kCFNumberSInt32Type, &interface_class_code));
+  DCHECK(number_ref);
+  CFDictionaryAddValue(matching_dictionary, CFSTR(kUSBInterfaceClass),
+                       number_ref);
+
+  number_ref.reset(CFNumberCreate(kCFAllocatorDefault,
+                                  kCFNumberSInt32Type,
+                                  &interface_subclass_code));
+  DCHECK(number_ref);
+  CFDictionaryAddValue(matching_dictionary, CFSTR(kUSBInterfaceSubClass),
+                       number_ref);
+
+  return matching_dictionary;
+}
+
+void AddCallbackToIOService(IONotificationPortRef port,
+                            const io_name_t type,
+                            CFMutableDictionaryRef dictionary,
+                            IOServiceMatchingCallback callback,
+                            void* context,
+                            io_iterator_t* notification) {
+   kern_return_t err = IOServiceAddMatchingNotification(port,

[Mark Mentovai, 2012/08/07 18:39:10]

Weird indentation.

[no longer working on chromium, 2012/08/08 08:42:42]

thanks.

+                                                       type,
+                                                       dictionary,
+                                                       callback,
+                                                       context,
+                                                       notification);
+  if (err) {
+    NOTREACHED() << "Failed to register the IO matched notification for type "
+                 << type;
+    return;
+  }
+  DCHECK(*notification);
+
+  // Iterate over set of matching devices to access already-present devices
+  // and to arm the notification.
+  base::mac::ScopedIOObject<io_service_t> this_object(
+      IOIteratorNext(*notification));
+  for (; this_object; this_object.reset(IOIteratorNext(*notification)));

[Mark Mentovai, 2012/08/07 18:39:10]

Use {} to show an empty body. Also see the comment at line 136.

[no longer working on chromium, 2012/08/08 08:42:42]

Done.

I replied the comment at line 136.

+}
+
+}  // namespace
+
+DeviceMonitorMac::DeviceMonitorMac() {
+  CFRunLoopRef runloop = CFRunLoopGetCurrent();

[Mark Mentovai, 2012/08/07 18:39:10]

You can save doing this until you’re ready to add the source to the run loop. Or
you can just write CFRunLoopGetCurrent() in-line in the CFRunLoopAddSource call,
as you did with CFRunLoopRemoveSource in the destructor.

[no longer working on chromium, 2012/08/08 08:42:42]

Done.

+
+  // Add the notification port to the run loop.
+  notification_port_ = IONotificationPortCreate(kIOMasterPortDefault);
+  DCHECK(notification_port_);
+  CFRunLoopSourceRef notification_cfsource =
+      IONotificationPortGetRunLoopSource(notification_port_);
+
+  RegisterServices();
+
+  CFRunLoopAddSource(runloop, notification_cfsource, kCFRunLoopCommonModes);
+}
+
+DeviceMonitorMac::~DeviceMonitorMac() {
+  // Stop the notifications and free the objects.
+  for (size_t i = 0; i < arraysize(kDeviceServices); ++i) {

[Mark Mentovai, 2012/08/07 18:39:10]

This looks a little freaky. Why don’t you use a std::vector<io_iterator_t> that
you can push your iterators on to, and then use notification_iterators_.size()
here.

Then if kDeviceServices gets screwed up and out of sync with RegisterServices,
you don’t crash because notification_iterators and kDeviceServices don‘t line
up.

[no longer working on chromium, 2012/08/08 08:42:42]

a scoped_array is much more suitable here because it makes the ownership of the
memory much clearer.

If we use a std::vector, we need to use std::vector<io_iterator_t*> in order to
release the memory correctly. And this needs more code and also becomes less
readable.

On the other hand, it is intentional to keep the KDeviceServices in line with
notification_iterators_, in order to avoid leak.

But I agree it is good point that there might be cases that KDeviceServices
might be out of sync with RegisterServices, so I want to keep the
KDeviceServices simple and easy to extend. I have already checked the apple
document, the current KDeviceServices structure is a must to be able to add new
service to the notifications.

Hope it is fine to keep this.

[Mark Mentovai, 2012/08/08 12:32:57]

Aside from the ability to resize a std::vector (which you could use in this
case), the std::vector<io_iterator_t*> is equivalent in usage to a
scoped_array<io_iterator_t>. How is the vector “less readable” or “more bad?”

+    IOObjectRelease(notification_iterators_[i]);

[Avi (use Gerrit), 2012/08/06 20:29:47]

Aha, cool.

+  }
+
+  // Remove the sleep notification port from the application runloop.
+  CFRunLoopRemoveSource(CFRunLoopGetCurrent(),
+                        IONotificationPortGetRunLoopSource(notification_port_),
+                        kCFRunLoopCommonModes);
+
+  // Destroy the notification port allocated by IONotificationPortCreate.
+  IONotificationPortDestroy(notification_port_);
+}
+
+void DeviceMonitorMac::RegisterServices() {
+  notification_iterators_.reset(new io_iterator_t[arraysize(kDeviceServices)]);
+  CFMutableDictionaryRef matching_dictionary;
+  for (size_t i = 0; i < arraysize(kDeviceServices); ++i) {
+    switch (kDeviceServices[i].device_type) {

[wjia(left Chromium), 2012/08/07 20:57:27]

After chatting with mmentovai@, my understanding is most of devices are handled
in the same way as audio and video devices. You can include interface_class_code
and subclass_code in struct for kDeviceServices[]. Having switch here defies the
purpose of having kDeviceServices.

[no longer working on chromium, 2012/08/08 08:42:42]

NO.
I thought I had already explained clearly about this offline.The number of
matching dictionaries can be varying from 0 to multiple. If you don't believe
it, please check out 
/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/IOKit.framework/Versions/A/Headers
or the relevant apple documents.

It is never good to define a pattern which limits the user cases to use one
class_code and one subclass_code.
A very strong supportive point is, as Mark points out his concern about the case
where KDeviceServices might be out of sync with RegisterServices. If we add the
interface_class_code and subclass_code to the struct, this becomes quite likely
in case the new service needs more or less matching syntact, users might skip
the KDeviceServices and add their own code.

+      case base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE:
+        matching_dictionary = CreateMatchingDictionary(
+            kUSBAudioInterfaceClass, kUSBAudioControlSubClass);
+        break;
+      case base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE:
+        matching_dictionary = CreateMatchingDictionary(
+            kUSBVideoInterfaceClass, kUSBVideoControlSubClass);
+        break;
+      default:
+        NOTREACHED();
+        return;
+    }
+
+    // Add callback to the service.
+    AddCallbackToIOService(notification_port_,
+                           kDeviceServices[i].service_type,
+                           matching_dictionary,
+                           &DeviceChangedCallback,
+                           static_cast<void*>(&kDeviceServices[i].device_type),
+                           &notification_iterators_[i]);
+  }
+}
+
+void DeviceMonitorMac::DeviceChangedCallback(void *context,
+                                             io_iterator_t iterator) {
+  base::mac::ScopedIOObject<io_service_t> this_object(IOIteratorNext(iterator));
+  for (; this_object; this_object.reset(IOIteratorNext(iterator))) {

[Mark Mentovai, 2012/08/07 18:39:10]

I’d write the initialization inside the body of the for, like

   for (base::mac::ScopedIOObject<io_service_t>
object(IOIteratorNext(iterator));
       object;
       object.reset(IOIteratorNext(iterator))) {

Also on line 71.

[no longer working on chromium, 2012/08/08 08:42:42]

:) I think I have got different opinions on this style thing. I used to do it in
the way you recommended, but in some of my early CLs, I was asked to extract the
declaration out of the for loop if it does not fit in one line. And their
opinion is that it helps readability.

I am pretty OK to change it if you have a strong opinion on this, please just
let me know.

[Mark Mentovai, 2012/08/08 12:32:57]

Whoever’s opinion you got in the past on this was wrong. :)

The scoping and lifetime are much clearer when the declaration is inside the
loop declaration. And you can resolve the “doesn’t fit” complaint by choosing a
shorter name: “object” instead of “this_object” as I’ve recommended. That’s a
good idea anyway, since you don’t have a “that_object”. You can also call it
“service” since you’re dealing with an io_service_t.

+    if (context) {
+      base::SystemMonitor::DeviceType device_type =
+          *reinterpret_cast<base::SystemMonitor::DeviceType*>(context);
+      DCHECK(device_type == base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE ||
+             device_type == base::SystemMonitor::DEVTYPE_VIDEO_CAPTURE);
+      // TODO(xians): Remove the global variable for SystemMonitor.
+      base::SystemMonitor::Get()->ProcessDevicesChanged(device_type);
+    }
+  }
+}
+
+}  // namespace content