Implement support for USB Xbox360 controllers without a driver on Mac.

content/browser/gamepad/xbox_data_fetcher_mac.cc

Index: content/browser/gamepad/xbox_data_fetcher_mac.cc
diff --git a/content/browser/gamepad/xbox_data_fetcher_mac.cc b/content/browser/gamepad/xbox_data_fetcher_mac.cc
new file mode 100644
index 0000000000000000000000000000000000000000..01f4511810c6d9d5c702651878cc37f0ace10872
--- /dev/null
+++ b/content/browser/gamepad/xbox_data_fetcher_mac.cc
@@ -0,0 +1,592 @@
+// Copyright 2013 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.
+
+#include "content/browser/gamepad/xbox_data_fetcher_mac.h"
+
+#include <CoreFoundation/CoreFoundation.h>
+#include <IOKit/IOCFPlugIn.h>
+#include <IOKit/IOKitLib.h>
+#include <IOKit/usb/IOUSBLib.h>
+#include <IOKit/usb/USB.h>
+
+#include "base/logging.h"
+#include "base/mac/foundation_util.h"
+#include "base/mac/scoped_cftyperef.h"
+
+namespace {
+const int kVendorMicrosoft = 0x045e;
+const int kProduct360Controller = 0x028e;
+
+const int kReadEndpoint = 1;
+const int kControlEndpoint = 2;
+
+#pragma pack(push, 1)
+struct ButtonData {
+  bool dpad_up    : 1;
+  bool dpad_down  : 1;
+  bool dpad_left  : 1;
+  bool dpad_right : 1;
+
+  bool start             : 1;
+  bool back              : 1;
+  bool stick_left_click  : 1;
+  bool stick_right_click : 1;
+
+  bool bumper_left  : 1;
+  bool bumper_right : 1;
+  bool guide        : 1;
+  bool dummy1       : 1;  // Always 0.
+
+  bool a : 1;
+  bool b : 1;
+  bool x : 1;
+  bool y : 1;
+
+  uint8 trigger_left;
+  uint8 trigger_right;
+
+  int16 stick_left_x;
+  int16 stick_left_y;
+  int16 stick_right_x;
+  int16 stick_right_y;
+
+  // Always 0.
+  uint32 dummy2;
+  uint16 dummy3;
+};
+#pragma pack(pop)
+
+COMPILE_ASSERT(sizeof(ButtonData) == 0x12, xbox_button_data_wrong_size);
+
+enum {
+  CONTROL_MESSAGE_SET_RUMBLE = 0,
+  CONTROL_MESSAGE_SET_LED = 1,
+};
+
+// From MSDN:
+// http://msdn.microsoft.com/en-us/library/windows/desktop/ee417001(v=vs.85).aspx#dead_zone
+const int16 kLeftThumbDeadzone = 7849;
+const int16 kRightThumbDeadzone = 8689;
+const uint8 kTriggerDeadzone = 30;
+
+void NormalizeAxis(int16 x,
+                   int16 y,
+                   int16 deadzone,
+                   float* x_out,
+                   float* y_out) {
+  float x_val = (float)x;

[Mark Mentovai, 2013/04/24 16:57:20]

Use c_plus_plus<style>(casts), not (c_style)casts. But I’m surprised you’d need
a cast at all here.

[jeremya, 2013/04/26 03:47:40]

Done.

+  float y_val = (float)y;
+
+  // Determine how far the stick is pushed.
+  float real_magnitude = sqrtf(x_val * x_val + y_val * y_val);

[Mark Mentovai, 2013/04/24 16:57:20]

sqrtf…those C names are so OLD. And you’d need to #include <math.h> to use them.
But don’t do that.

Instead:

#include <cmath> and use std::sqrt, which will choose the single-precision
“float” version based on the argument type.

[jeremya, 2013/04/26 03:47:40]

Old? I prefer "classic" ;) (done.)

+
+  // Check if the controller is outside a circular dead zone.
+  if (real_magnitude > deadzone) {
+    // Clip the magnitude at its expected maximum value.
+    float magnitude = real_magnitude > 32767 ? 32767 : real_magnitude;

[Mark Mentovai, 2013/04/24 16:57:20]

Don’t write your own min function, #include <algorithm> and use std::min. Write
32767.0f if you like.

[jeremya, 2013/04/26 03:47:40]

Done.

+
+    // Adjust magnitude relative to the end of the dead zone.
+    magnitude -= deadzone;
+
+    // Normalize the magnitude with respect to its expected range giving a
+    // magnitude value of 0.0 to 1.0
+    float ratio = (magnitude / (32767 - deadzone)) / real_magnitude;
+
+    // Y is negated because xbox controllers have an opposite sign from
+    // the 'standard controller' recommendations.
+    *x_out = x_val * ratio;
+    *y_out = -y_val * ratio;
+  } else {
+    // If the controller is in the deadzone zero out the magnitude.
+    *x_out = *y_out = 0.0f;
+  }
+}
+
+static float NormalizeTrigger(uint8 value) {

[Mark Mentovai, 2013/04/24 16:57:20]

You are in an anonymous namespace, the “static” is meaningless.

[jeremya, 2013/04/26 03:47:40]

Done.

+  return value < kTriggerDeadzone ? 0 :
+      (float)(value - kTriggerDeadzone) / (kuint8max - kTriggerDeadzone);

[Mark Mentovai, 2013/04/24 16:57:20]

Don’t use C-style casts.

[Mark Mentovai, 2013/04/24 16:57:20]

Don’t use kuint8max, #include <limits> and write
std::numeric_limits<uint8>::max.

[jeremya, 2013/04/26 03:47:40]

C++, making things more verbose since 1989!

[jeremya, 2013/04/26 03:47:40]

Done.

+}
+
+void NormalizeButtonData(const ButtonData& data,
+                         XboxController::Data* normalized_data) {
+  normalized_data->buttons[0] = data.a ? 1.f : 0.f;

[Mark Mentovai, 2013/04/24 16:57:20]

If these represent binary state, is it sensible for them to be floats?

[Mark Mentovai, 2013/04/24 16:57:20]

1.0f and 0.0f are normal for floating-point constants.

[jeremya, 2013/04/26 03:47:40]

The reason they're floats is for easy conversion into the WebKit::WebGamepad
data structure (see GamepadPlatformDataFetcherMac::XboxValueChanged()). I
figured it was a reasonably small memory/perf price to pay for a little extra
simplicity in the code. Happy to optimise it if you want though.

[jeremya, 2013/04/26 03:47:40]

Done.

[Mark Mentovai, 2013/05/01 18:59:20]

jeremya wrote:
I think that XboxValueChanged can still keep a pretty simple structure even if
Data is carrying bools that are conceptually correct. You’re just moving the
conversions from this function into that one.

[jeremya, 2013/05/21 06:23:25]

Done, though it feels a little weird to have all this xbox-specific
normalization stuff in the gamepad data fetcher rather than in the xbox data
fetcher...

[Mark Mentovai, 2013/05/21 13:46:18]

jeremya wrote:
Oh, I thought we could do it in Xbox-specific code still.

+  normalized_data->buttons[1] = data.b ? 1.f : 0.f;
+  normalized_data->buttons[2] = data.x ? 1.f : 0.f;
+  normalized_data->buttons[3] = data.y ? 1.f : 0.f;
+  normalized_data->buttons[4] = data.bumper_left ? 1.f : 0.f;
+  normalized_data->buttons[5] = data.bumper_right ? 1.f : 0.f;
+  normalized_data->buttons[6] = NormalizeTrigger(data.trigger_left);
+  normalized_data->buttons[7] = NormalizeTrigger(data.trigger_right);
+  normalized_data->buttons[8] = data.back ? 1.f : 0.f;
+  normalized_data->buttons[9] = data.start ? 1.f : 0.f;
+  normalized_data->buttons[10] = data.stick_left_click ? 1.f : 0.f;
+  normalized_data->buttons[11] = data.stick_right_click ? 1.f : 0.f;
+  normalized_data->buttons[12] = data.dpad_up ? 1.f : 0.f;
+  normalized_data->buttons[13] = data.dpad_down ? 1.f : 0.f;
+  normalized_data->buttons[14] = data.dpad_left ? 1.f : 0.f;
+  normalized_data->buttons[15] = data.dpad_right ? 1.f : 0.f;
+  normalized_data->buttons[16] = data.guide ? 1.f : 0.f;
+  NormalizeAxis(data.stick_left_x,
+                data.stick_left_y,
+                kLeftThumbDeadzone,
+                &normalized_data->axes[0],
+                &normalized_data->axes[1]);
+  NormalizeAxis(data.stick_right_x,
+                data.stick_right_y,
+                kRightThumbDeadzone,
+                &normalized_data->axes[2],
+                &normalized_data->axes[3]);
+}
+
+}  // namespace
+
+XboxController::XboxController(Delegate* delegate)
+    : device_(NULL),
+      interface_(NULL),
+      device_is_open_(false),
+      interface_is_open_(false),
+      source_(NULL),
+      read_buffer_size_(0),
+      led_pattern_(LED_NUM_PATTERNS),
+      location_id_(0),
+      delegate_(delegate) {
+}
+
+XboxController::~XboxController() {
+  if (source_)
+    CFRunLoopSourceInvalidate(source_);
+  if (interface_ && interface_is_open_)
+    (*interface_)->USBInterfaceClose(interface_);
+  if (device_ && device_is_open_)
+    (*device_)->USBDeviceClose(device_);
+}
+
+bool XboxController::OpenDevice(io_service_t service) {
+  kern_return_t kr;

[Mark Mentovai, 2013/04/24 16:57:20]

Don’t declare stuff ’til you use it. Move the declaration down, you can declare
it inline with the call to IOCreatePlugInInterfaceForService.

[jeremya, 2013/04/26 03:47:40]

I moved it down, but didn't inline it because it's re-used several times later.

+  HRESULT res;

[Mark Mentovai, 2013/04/24 16:57:20]

Same.

[jeremya, 2013/04/26 03:47:40]

Done. Also, s/HRESULT/IOReturn/, because wtf. (though I think both of those are
typedefed to kern_return_t, which is prbly typedefed to int.)

+
+  IOCFPlugInInterface **plugin;
+  SInt32 score;  // Unused, but required for IOCreatePlugInInterfaceForService.
+  kr = IOCreatePlugInInterfaceForService(service,
+                                         kIOUSBDeviceUserClientTypeID,
+                                         kIOCFPlugInInterfaceID,
+                                         &plugin,
+                                         &score);
+  if (kr != KERN_SUCCESS)
+    return false;
+  base::mac::ScopedIOPluginInterface<IOCFPlugInInterface> plugin_ref(plugin);
+
+  // IOUSBDeviceStruct320 is the latest version of the device API that is
+  // supported on Mac OS 10.6.
+  res = (*plugin)->QueryInterface(
+      plugin,
+      CFUUIDGetUUIDBytes(kIOUSBDeviceInterfaceID320),
+      (LPVOID *)&device_);
+  if (res || !device_)
+    return false;
+
+  // Open the device and configure it.
+  kr = (*device_)->USBDeviceOpen(device_);
+  if (kr != KERN_SUCCESS)
+    return false;
+  device_is_open_ = true;
+
+  // Xbox controllers have one configuration option which has configuration
+  // value 1. Try to set it and fail out if it couldn't be configured.

[Mark Mentovai, 2013/04/24 16:57:20]

fail, not fail out

[jeremya, 2013/04/26 03:47:40]

Done.

+  IOUSBConfigurationDescriptorPtr config_desc;
+  kr = (*device_)->GetConfigurationDescriptorPtr(device_, 0, &config_desc);
+  if (kr != KERN_SUCCESS)
+    return false;
+  kr = (*device_)->SetConfiguration(device_, config_desc->bConfigurationValue);

[Mark Mentovai, 2013/04/24 16:57:20]

You are positive that you’re talking to an Xbox controller when you reach this
point, correct?

[jeremya, 2013/04/26 03:47:40]

At least certain that it is a device with the vendor and product ID of an Xbox
controller. Is that insufficiently certain?

[Mark Mentovai, 2013/05/01 18:59:20]

jeremya wrote:
Yeah, vendor and product ID is enough, but you did account for this as I
intended in this change.

+  if (kr != KERN_SUCCESS)
+    return false;
+
+  // The device has 4 interfaces. They are as follows:
+  // Protocol 1:
+  //  - Endpoint 1 (in) : Controller events, including button presses.
+  //  - Endpoint 2 (out): Rumble pack and LED control
+  // Protocol 2 has a single endpoint to read from a connected ChatPad device.
+  // Protocol 3 is used by a connected headset device.
+  // The device also has an interface on subclass 253, protocol 10 with no
+  // endpoints.  It is unused.
+  //
+  // We don't currently support the ChatPad or headset, so protocol 1 is the
+  // only protocol we care about.
+  //
+  // For more detail, see https://github.com/Grumbel/xboxdrv/blob/master/PROTOCOL

[Mark Mentovai, 2013/04/24 16:57:20]

Spill the URL to a new line, because it’s embarrassing to wrap one character.

[jeremya, 2013/04/26 03:47:40]

I'm so embarrassed ;)

+  IOUSBFindInterfaceRequest request;
+  request.bInterfaceClass = 255;
+  request.bInterfaceSubClass = 93;
+  request.bInterfaceProtocol = 1;
+  request.bAlternateSetting = kIOUSBFindInterfaceDontCare;
+  io_iterator_t iter;
+  kr = (*device_)->CreateInterfaceIterator(device_, &request, &iter);

[Mark Mentovai, 2013/04/24 16:57:20]

Do you need to IOObjectRelease iter, or have a ScopedIOObject supervise it?

[jeremya, 2013/04/26 03:47:40]

Good catch. Done.

+  if (kr != KERN_SUCCESS)
+    return false;
+
+  // There should be exactly one usb interface which matches the requested

[Mark Mentovai, 2013/04/24 16:57:20]

In comments, USB should be capitalized. This happens elsewhere in this file too.

[jeremya, 2013/04/26 03:47:40]

Done.

+  // settings.
+  io_service_t usb_interface = IOIteratorNext(iter);
+  if (!usb_interface)
+    return false;
+
+  // We need to make an InterfaceInterface to communicate with the device
+  // endpoint. This is the same process as earlier: first make a
+  // PluginInterface from the io_service then make the InterfaceInterface from
+  // that.
+  IOCFPlugInInterface **plugin_interface;
+  kr = IOCreatePlugInInterfaceForService(usb_interface,
+                                         kIOUSBInterfaceUserClientTypeID,
+                                         kIOCFPlugInInterfaceID,
+                                         &plugin_interface,
+                                         &score);
+  if (kr != KERN_SUCCESS || !plugin_interface)
+    return false;
+  base::mac::ScopedIOPluginInterface<IOCFPlugInInterface> interface_ref(
+      plugin_interface);
+
+  // Release the usb interface, and any subsequent interfaces returned by the
+  // iterator. (There shouldn't be any, but in case a future device does
+  // contain more interfaces, this will serve to avoid memory leaks.)
+  do {
+    IOObjectRelease(usb_interface);
+  } while ((usb_interface = IOIteratorNext(iter)));
+
+  // Actually create the interface.
+  kr = (*plugin_interface)->QueryInterface(
+      plugin_interface,
+      CFUUIDGetUUIDBytes(kIOUSBInterfaceInterfaceID300),
+      (LPVOID *)&interface_);
+
+  if (kr != KERN_SUCCESS || !interface_)
+    return false;
+
+  // Actually open the interface.
+  kr = (*interface_)->USBInterfaceOpen(interface_);
+  if (kr != KERN_SUCCESS)
+    return false;
+  interface_is_open_ = true;
+
+  kr = (*interface_)->CreateInterfaceAsyncEventSource(interface_, &source_);
+  if (kr != KERN_SUCCESS || !source_)
+    return false;
+  CFRunLoopAddSource(CFRunLoopGetMain(), source_, kCFRunLoopDefaultMode);
+  // The CFRunLoop retains the source.

[Mark Mentovai, 2013/04/24 16:57:20]

Yeah, while it’s alive. But you still need to call CFRunLoopSourceInvalidate on
it, and it would suck to do that after the CFRunLoop went away and took the
source_ with it. Yes, this would be weird. Computers are weird. So don’t release
source_ here, do it in the destructor. There’s literally zero cost since you’re
saving it as a member anyway.

[jeremya, 2013/04/26 03:47:40]

ScopedCFTypeRef to the rescue! Done here and in XboxDataFetcher, too.

+  CFRelease(source_);
+
+  // The interface should have two pipes. Pipe 1 with direction kUSBIn and pipe
+  // 2 with direction kUSBOut. Both pipes should have type kUSBInterrupt.
+  uint8 num_endpoints;
+  kr = (*interface_)->GetNumEndpoints(interface_, &num_endpoints);
+  if (kr != KERN_SUCCESS || num_endpoints < 2)
+    return false;
+
+  for (int i = 1; i <= num_endpoints; i++) {

[Mark Mentovai, 2013/04/24 16:57:20]

Do you really need to GetPipeProperties on endpoints > 2, just to check that the
transfer_type is kUSBInterrupt? If not, then this loop should only be over pipes
1 and 2, and not possible additional endpoints.

[jeremya, 2013/04/26 03:47:40]

Done.

+    uint8 direction;
+    uint8 number;
+    uint8 transfer_type;
+    uint16 max_packet_size;
+    uint8 interval;
+
+    kr = (*interface_)->GetPipeProperties(interface_,
+                                          i,
+                                          &direction,
+                                          &number,
+                                          &transfer_type,
+                                          &max_packet_size,
+                                          &interval);
+    if (kr != KERN_SUCCESS || transfer_type != kUSBInterrupt)
+      return false;
+    if (i == kReadEndpoint) {
+      if (direction != kUSBIn)
+        return false;
+      if (max_packet_size > 32)
+        return false;
+      read_buffer_.reset(new uint8[max_packet_size]);
+      read_buffer_size_ = max_packet_size;
+      QueueRead();
+    } else if (i == kControlEndpoint) {
+      if (direction != kUSBOut)
+        return false;
+    }
+  }
+
+  // The location ID is unique per controller, and can be used to track
+  // controllers through reconnections (though if a controller is detached from
+  // one USB hub and attached to another, the location ID will change).
+  kr = (*device_)->GetLocationID(device_, &location_id_);
+  if (kr != KERN_SUCCESS)
+    return false;
+
+  return true;
+}
+
+void XboxController::SetLEDPattern(LEDPattern pattern) {
+  led_pattern_ = pattern;
+  const UInt8 length = 3;
+  // This buffer will be released in WriteComplete when WritePipeAsync

[Mark Mentovai, 2013/04/24 16:57:20]

Blank line before this.

[jeremya, 2013/04/26 03:47:40]

Done.

+  // finishes.
+  UInt8* buffer = new UInt8[length];
+  buffer[0] = (UInt8)CONTROL_MESSAGE_SET_LED;

[Mark Mentovai, 2013/04/24 16:57:20]

C++-style casts. Line 333 also.

[jeremya, 2013/04/26 03:47:40]

Done.

+  buffer[1] = length;
+  buffer[2] = (UInt8)pattern;
+  kern_return_t kr = (*interface_)->WritePipeAsync(interface_,
+                                                   kControlEndpoint,
+                                                   buffer,
+                                                   (UInt32)length,
+                                                   WriteComplete,
+                                                   buffer);
+  if (kr != KERN_SUCCESS) {

[Mark Mentovai, 2013/04/24 16:57:20]

Do you need to delete[] the buffer if this happens (because WriteComplete won’t
be called?)

[jeremya, 2013/04/26 03:47:40]

I bet I do! Good catch.

+    IOError();
+    return;
+  }
+}
+
+int XboxController::GetVendorId() const {
+  return kVendorMicrosoft;

[Mark Mentovai, 2013/04/24 16:57:20]

Again: are you sure? Because I didn’t see anything in the XboxController class
actually check the vendor and product.

“XboxDataFetcher only registers that vendor and product” is not an answer
because XboxController is exposed as its own public interface.

[jeremya, 2013/04/26 03:47:40]

Ok -- I've added a check in OpenDevice().

+}
+
+int XboxController::GetProductId() const {
+  return kProduct360Controller;
+}
+
+void XboxController::WriteComplete(void* context, IOReturn result, void* arg0) {
+  // Ignoring any errors sending data, because they will usually only occur
+  // when the device is disconnected, in which case it really doesn't matter if
+  // the data got to the controller or not.

[Mark Mentovai, 2013/04/24 16:57:20]

Maybe it doesn’t matter if the data got to the controller, but you still want to
delete[] buffer instead of leaking it.

[jeremya, 2013/04/26 03:47:40]

Done.

+  if (result != KERN_SUCCESS)

[Mark Mentovai, 2013/04/24 16:57:20]

KERN_SUCCESS is a kern_return_t, not an IOReturn.

[jeremya, 2013/04/26 03:47:40]

Done, though they're the same thing:
http://www.opensource.apple.com/source/xnu/xnu-1486.2.11/iokit/IOKit/IOReturn.h

[Mark Mentovai, 2013/05/01 18:59:20]

jeremya wrote:
It’s still more readable when you don’t conceptually mix types and their
associated constants. If there’s an equivalent way to write it without mixing,
it keeps people from having to do the header hunt to figure out if it’s really
OK.

+    return;
+
+  UInt8* buffer = (UInt8*)context;

[Mark Mentovai, 2013/04/24 16:57:20]

C++-style casts.

[jeremya, 2013/04/26 03:47:40]

Done.

+  delete[] buffer;
+}
+
+void XboxController::GotData(void* context, IOReturn result, void* arg0) {
+  uint32 bytesRead = (uint32)arg0;

[Mark Mentovai, 2013/04/24 16:57:20]

C++-style casts.

[jeremya, 2013/04/26 03:47:40]

Done.

+  XboxController* controller = static_cast<XboxController*>(context);
+
+  if (result != kIOReturnSuccess) {
+    // This will happen if the device was disconnected. The gamepad has
+    // probably been destroyed by a meteorite.

[Mark Mentovai, 2013/04/24 16:57:20]

:)

+    controller->IOError();
+    return;
+  }
+
+  controller->ProcessPacket(bytesRead);
+
+  // Queue up another read.
+  controller->QueueRead();
+}
+
+void XboxController::ProcessPacket(uint32 length) {
+  if (length < 3) return;

[Mark Mentovai, 2013/04/24 16:57:20]

I imagine it’s possible for (possibly future) messages of unknown type to not
carry any payload. You should only check for the two fields you know must be
present, type and length. This will mean adding a check for the known message
size in the STATUS_MESAGE_LED case.

[jeremya, 2013/04/26 03:47:40]

Done.

+  DCHECK(length <= read_buffer_size_);
+  if (length > read_buffer_size_) {
+    IOError();
+    return;
+  }
+  uint8* buffer = read_buffer_.get();
+
+  if (buffer[1] != length)
+    // Length in packet doesn't match length reported by USB.

[Mark Mentovai, 2013/04/24 16:57:20]

So just drop it? Not worth reporting via IOError or another mechanism?

[jeremya, 2013/04/26 03:47:40]

Nope, sometimes the controller messes up its packets and sends bad data (but
recovers). We don't want to lose the connection in that case.

+    return;
+
+  uint8 type = buffer[0];
+  buffer += 2;
+  length -= 2;
+  switch (type) {
+    case STATUS_MESSAGE_BUTTONS: {
+      if (length != sizeof(ButtonData))
+        return;
+      ButtonData data;
+      memcpy(&data, buffer, sizeof(data));

[Mark Mentovai, 2013/04/24 16:57:20]

Why copy? Why not cast buffer to a ButtonData*?

[jeremya, 2013/04/26 03:47:40]

The comments on bit_cast<> in base/basictypes.h lead me to believe that a direct
cast could cause badness. Basically: type punning causes mis-optimizations. I
was surprised too!

[Mark Mentovai, 2013/05/01 18:59:20]

jeremya wrote:
I know. This was a trick question.

There is a narrow exception to the aliasing rules that allows character types to
alias other types, because they’re used so frequently (as read buffers, for
example, like here) and they’re so handy.

buffer is a uint8* which is a char*. It’s safe to cast this to ButtonData*
without worrying about aliasing. The char* aliases the true underlying type,
ButtonData*.

Ref. C99 §6.5 (“Expressions”) clause 7.
[…]
[…]
Ref. C++03 §3.10 (“Lvalues and rvalues”) clause 15
[…]
[…]
Ref. similar language in C++11 §3.10 clause 10.

[jeremya, 2013/05/21 06:23:25]

Mind. Blown. Awesome!

+      Data normalized_data;
+      NormalizeButtonData(data, &normalized_data);
+      delegate_->XboxControllerGotData(this, normalized_data);
+      break;
+    }
+    case STATUS_MESSAGE_LED:
+      // The controller sends one of these messages every time the LED pattern
+      // is set, as well as once when it is plugged in.
+      if (led_pattern_ == LED_NUM_PATTERNS && buffer[0] < LED_NUM_PATTERNS)
+        led_pattern_ = (LEDPattern)buffer[0];

[Mark Mentovai, 2013/04/24 16:57:20]

C++-style casts.

I keep harping on this so I’ll give you a link to the style guide so you can see
it’s not just me being a jerk about some personal pet peeve:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...
.

[jeremya, 2013/04/26 03:47:40]

I believe you :P

+      break;
+    default:
+      // Unknown packet: ignore!
+      break;
+  }
+}
+
+void XboxController::QueueRead() {
+  kern_return_t kr = (*interface_)->ReadPipeAsync(interface_,
+                                                  kReadEndpoint,
+                                                  read_buffer_.get(),
+                                                  read_buffer_size_,
+                                                  GotData,
+                                                  this);
+  if (kr != KERN_SUCCESS)
+    IOError();
+}
+
+void XboxController::IOError() {
+  delegate_->XboxControllerError(this);
+}
+
+//-----------------------------------------------------------------------------
+
+XboxDataFetcher::XboxDataFetcher(Delegate* delegate)
+    : delegate_(delegate),
+      listening_(false),
+      port_(NULL),
+      source_(NULL) {
+}
+
+XboxDataFetcher::~XboxDataFetcher() {
+  while (!controllers_.empty()) {
+    RemoveController(*controllers_.begin());
+  }
+  UnregisterFromNotifications();
+}
+
+void XboxDataFetcher::DeviceAdded(void* context, io_iterator_t iterator) {
+  DCHECK(context);
+  XboxDataFetcher* fetcher = static_cast<XboxDataFetcher*>(context);
+  io_service_t ref;
+  while ((ref = IOIteratorNext(iterator))) {
+    base::mac::ScopedIOObject<io_service_t> scoped_ref(ref);
+    XboxController* controller = new XboxController(fetcher);
+    if (controller->OpenDevice(ref)) {
+      fetcher->AddController(controller);
+    } else {
+      delete controller;
+    }
+  }
+}
+
+void XboxDataFetcher::DeviceRemoved(void* context, io_iterator_t iterator) {
+  DCHECK(context);
+  XboxDataFetcher* fetcher = static_cast<XboxDataFetcher*>(context);
+  io_service_t ref;
+  while ((ref = IOIteratorNext(iterator))) {
+    base::mac::ScopedIOObject<io_service_t> scoped_ref(ref);
+    base::mac::ScopedCFTypeRef<CFNumberRef> number(
+        base::mac::CFCastStrict<CFNumberRef>(IORegistryEntryCreateCFProperty(
+            ref,
+            CFSTR(kUSBDevicePropertyLocationID),
+            kCFAllocatorDefault,
+            kNilOptions)));
+    UInt32 location_id = 0;
+    CFNumberGetValue(number, kCFNumberSInt32Type, &location_id);
+    fetcher->RemoveControllerByLocationID(location_id);
+  }
+}
+
+void XboxDataFetcher::RegisterForNotifications() {
+  if (listening_)
+    return;
+  base::mac::ScopedCFTypeRef<CFNumberRef> vendor_cf(
+      CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type,
+                     &kVendorMicrosoft));
+  base::mac::ScopedCFTypeRef<CFNumberRef> product_cf(
+      CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type,
+                     &kProduct360Controller));
+  base::mac::ScopedCFTypeRef<CFMutableDictionaryRef> matching_dict(
+      IOServiceMatching(kIOUSBDeviceClassName));
+  if (!matching_dict)
+    return;
+  CFDictionarySetValue(matching_dict, CFSTR(kUSBVendorID), vendor_cf);
+  CFDictionarySetValue(matching_dict, CFSTR(kUSBProductID), product_cf);
+  port_ = IONotificationPortCreate(kIOMasterPortDefault);
+  if (!port_)
+    return;
+  source_ = IONotificationPortGetRunLoopSource(port_);
+  if (!source_)
+    return;
+  CFRunLoopAddSource(CFRunLoopGetMain(), source_, kCFRunLoopDefaultMode);

[Mark Mentovai, 2013/04/24 16:57:20]

You probably want CFRunLoopGetCurrent() instead of CFRunLoopGetMain(). You want
to attach to the current thread’s run loop even if the current thread is not the
main thread, right?

[jeremya, 2013/04/26 03:47:40]

I tried this -- it looks like there is no CFRunLoopGetCurrent(), or at least,
it's not being run. (No data got delivered when I tried Current() instead of
Main()).

[Mark Mentovai, 2013/05/01 18:59:20]

jeremya wrote:
That tells me that there’s some cross-threadedness happening, which I wasn’t
expecting, and I’m not sure if you were expecting it either.

What thread is everything supposed to be running on here?

Add DCHECKs to make sure things only run on the correct threads.

[jeremya, 2013/05/21 06:23:25]

It should all be running on the gamepad polling thread (created by
gamepad_provider.cc). I just checked manually, and CFRunLoopGetMain() was indeed
causing stuff to be run on the main browser thread.

I figured out why it wasn't working with CFRunLoopGetCurrent(): the thread was
being created with TYPE_IO, which on mac uses libevent, which doesn't play ball
with CFRunLoop. I switched the thread to be TYPE_UI and it works, but I'm not
100% sure that works on other platforms, though I think based on some other
confusion in gamepad_platform_data_fetcher_mac.mm between GetMain() and
GetCurrent() that it'd be fine for non-Xbox controllers on mac.

From what I can tell of the linux code it doesn't actually depend on the type of
message loop used, but I haven't tested there.

I can conditionally make it a TYPE_UI thread only on mac if desired.

+  // The CFRunLoop retains the source.
+  CFRelease(source_);

[Mark Mentovai, 2013/04/24 16:57:20]

You can’t do this, you don’t own it. port_ has a reference and the run loop has
a reference. When you eventually destroy port_ in UnregisterFromNotifications,
that should cause port_ to release its reference.

[jeremya, 2013/04/26 03:47:40]

Ah, I see! Fixed. I assume destroying port_ will also invalidate the source?

[Mark Mentovai, 2013/05/01 18:59:20]

jeremya wrote:
No, you’re still on the hook for that. But I’d say you’re allowed to invalidate
it because you added it and you know it’s still alive because port_ , which you
do own and are sure is still alive, has kept source_ alive.

[jeremya, 2013/05/21 06:23:25]

Surprising, but done.

+
+  listening_ = true;
+
+  IOReturn ret;

[Mark Mentovai, 2013/04/24 16:57:20]

Don’t declare this until you use it on line 519.

[jeremya, 2013/04/26 03:47:40]

Done.

+
+  // IOServiceAddMatchingNotification() releases the dictionary when it's done.
+  // Retain it before each call to IOServiceAddMatchingNotification to keep
+  // things balanced.
+  CFRetain(matching_dict);
+  io_iterator_t device_added_iter;
+  ret = IOServiceAddMatchingNotification(port_,
+                                         kIOFirstMatchNotification,
+                                         matching_dict,
+                                         DeviceAdded,
+                                         this,
+                                         &device_added_iter);
+  device_added_iter_.reset(device_added_iter);
+  if (ret != kIOReturnSuccess) {
+    LOG(ERROR) << "Error listening for Xbox controller add events: " << ret;
+    return;
+  }
+  DeviceAdded(this, device_added_iter_.get());
+
+  CFRetain(matching_dict);
+  io_iterator_t device_removed_iter;
+  ret = IOServiceAddMatchingNotification(port_,
+                                         kIOTerminatedNotification,
+                                         matching_dict,
+                                         DeviceRemoved,
+                                         this,
+                                         &device_removed_iter);
+  device_removed_iter_.reset(device_removed_iter);
+  if (ret != kIOReturnSuccess) {
+    LOG(ERROR) << "Error listening for Xbox controller remove events: " << ret;
+    return;
+  }
+  DeviceRemoved(this, device_removed_iter_.get());
+}
+
+void XboxDataFetcher::UnregisterFromNotifications() {
+  if (!listening_)
+    return;
+  listening_ = false;
+  if (source_)
+    CFRunLoopSourceInvalidate(source_);
+  source_ = NULL;
+  if (port_)
+    IONotificationPortDestroy(port_);
+  port_ = NULL;
+}
+
+void XboxDataFetcher::AddController(XboxController* controller) {
+  controllers_.insert(controller);

[Mark Mentovai, 2013/04/24 16:57:20]

As an integrity check, do you want to verify that no controller with the same
location is already present in the set? This can be a debug-only thing if you
want

[jeremya, 2013/04/26 03:47:40]

Done.

+  delegate_->XboxDeviceAdd(controller);
+}
+
+void XboxDataFetcher::RemoveController(XboxController* controller) {
+  controllers_.erase(controller);

[Mark Mentovai, 2013/04/24 16:57:20]

I would do this after calling the delegate method.

[jeremya, 2013/04/26 03:47:40]

Done.

+  delegate_->XboxDeviceRemove(controller);
+  delete controller;
+}
+
+void XboxDataFetcher::RemoveControllerByLocationID(uint32 location_id) {
+  XboxController* controller = NULL;
+  for (std::set<XboxController*>::iterator i = controllers_.begin();
+       i != controllers_.end();
+       ++i) {
+    if ((*i)->location_id() == location_id) {
+      controller = *i;
+      break;
+    }
+  }
+  if (controller)
+    RemoveController(controller);
+}
+
+void XboxDataFetcher::XboxControllerGotData(XboxController* controller,
+                                            const XboxController::Data& data) {
+  delegate_->XboxValueChanged(controller, data);
+}
+
+void XboxDataFetcher::XboxControllerError(XboxController* controller) {
+  RemoveController(controller);
+}