Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14328036:
Implement support for USB Xbox360 controllers without a driver on Mac. (Closed)
|
Created:
7 years, 8 months ago by jeremya Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, sail+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement support for USB Xbox360 controllers without a driver on Mac.
Without this patch, users have to install a driver that exposes a standard HID
device for the controller. Unfortunately, apart from the problems with
requiring users to install a driver, the only such available driver
(http://tattiebogle.net/index.php/ProjectRoot/Xbox360Controller/OsxDriver)
crashes the kernel when the controller is unplugged.
This change uses IOKit to talk directly to the Xbox controller without the need
for a driver, and exposes it through the normal HTML5 gamepad API.
BUG=232238
R=scottmg@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201717
Patch Set 1 : . #
Total comments: 18
Patch Set 2 : comments #
Total comments: 18
Patch Set 3 : comments #Patch Set 4 : fix compile #
Total comments: 2
Patch Set 5 : CFCastStrict #
Total comments: 106
Patch Set 6 : addressed mark's comments #
Total comments: 4
Patch Set 7 : rebase & comments #Patch Set 8 : move conversions back to xbox-specific code; #if out the TYPE_UI stuff just for mac. #
Total comments: 1
Messages
Total messages: 26 (0 generated)
cool. i tried to apply to test, but doesn't build because of the recent removal of scoped_array. what's the behaviour when the user has had the disfortune to install that driver? we've probably indirectly encouraged a lot of people to install it, so it'd be nice to make it sort of obvious that it can be nuked now. or at least make sure things don't work less well. i'm afraid i can offer much useful input on xbox_data_fetcher_mac.* but i'll try to understand it as well as i can. we should find a mac/usb/iokit person to review that code though. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:332: // but the device is connected in slot 3? hm. also in DeviceAdd. yup, you're right :( https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:346: @"Xbox360 Controller (STANDARD GAMEPAD Vendor: 045e Product: 028e)"; space after Xbox to match https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ga...
On 2013/04/19 17:03:50, scottmg wrote: > cool. i tried to apply to test, but doesn't build because of the recent removal > of scoped_array. Oh dear, what are we supposed to use instead? > what's the behaviour when the user has had the disfortune to install that > driver? we've probably indirectly encouraged a lot of people to install it, so > it'd be nice to make it sort of obvious that it can be nuked now. or at least > make sure things don't work less well. I suspect that what'll happen is that the driver will grab the USB device before this code has a chance, and offer it up as a HID device, which the regular HID code will handle. I'll test though. Perhaps also you could add me to the gamepadjs repo on github so I can update the docs there? (and fix it to work with the newer API?) I'm nornagon on github. > i'm afraid i can offer much useful input on xbox_data_fetcher_mac.* but i'll try > to understand it as well as i can. we should find a mac/usb/iokit person to > review that code though. Any suggestions? :)
On 2013/04/20 01:47:17, jeremya wrote: > On 2013/04/19 17:03:50, scottmg wrote: > > cool. i tried to apply to test, but doesn't build because of the recent > removal > > of scoped_array. > > Oh dear, what are we supposed to use instead? https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d... > > > what's the behaviour when the user has had the disfortune to install that > > driver? we've probably indirectly encouraged a lot of people to install it, so > > it'd be nice to make it sort of obvious that it can be nuked now. or at least > > make sure things don't work less well. > > I suspect that what'll happen is that the driver will grab the USB device before > this code has a chance, and offer it up as a HID device, which the regular HID > code will handle. I'll test though. Perhaps also you could add me to the > gamepadjs repo on github so I can update the docs there? (and fix it to work > with the newer API?) I'm nornagon on github. OK, that sounds reasonable. Hopefully not too many people installed it. (I'm going to delete that repo ... sometime ... as the spec's going to roll in almost all of its behaviour anyway). > > > i'm afraid i can offer much useful input on xbox_data_fetcher_mac.* but i'll > try > > to understand it as well as i can. we should find a mac/usb/iokit person to > > review that code though. > > Any suggestions? :) avi@ did the initial mac code review iirc, or possibly rsesek@ as a general mac-familiar.
+avi,rsesek for IOKit review (xbox_data_fetcher_mac) :) https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:332: // but the device is connected in slot 3? hm. also in DeviceAdd. On 2013/04/19 17:03:50, scottmg wrote: > yup, you're right :( Cool, I'll fix up this logic (and maybe see if I can find a better way to express it)
+mark, he loves this kind of stuff. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:308: using WebKit::WebGamepads; It seems a bit silly that every function puts these using declarations at the top. Just pull them out to file level. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:9: #include <IOKit/IOCFPlugIn.h> alphabetize https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:180: IOObjectRelease(service); If you're consuming the service, you may want to note that in the function comments. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:185: // Use IOUSBDeviceStruct320 for support on MacOS 10.6. I don't understand your comment here. "For support on 10.6" as opposed to what? https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:460: IOObjectRelease(ref); Huh? I thought OpenDevice consumed the ref... base/mac/scoped_ioobject.h's ScopedIOObject perhaps? https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:478: IOObjectRelease(ref); ScopedIOObject
https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:308: using WebKit::WebGamepads; On 2013/04/22 21:00:51, Avi wrote: > It seems a bit silly that every function puts these using declarations at the > top. Just pull them out to file level. Done. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:346: @"Xbox360 Controller (STANDARD GAMEPAD Vendor: 045e Product: 028e)"; On 2013/04/19 17:03:50, scottmg wrote: > space after Xbox to match > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ga... Done, also changed to not duplicate the vendor/product ids. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:9: #include <IOKit/IOCFPlugIn.h> On 2013/04/22 21:00:51, Avi wrote: > alphabetize Done. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:180: IOObjectRelease(service); On 2013/04/22 21:00:51, Avi wrote: > If you're consuming the service, you may want to note that in the function > comments. Oops, looks like this was actually being double-released. Removed. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:185: // Use IOUSBDeviceStruct320 for support on MacOS 10.6. On 2013/04/22 21:00:51, Avi wrote: > I don't understand your comment here. "For support on 10.6" as opposed to what? There are various versions of the IOUSBDeviceStruct struct, this one is 3.2.0 which is the latest one that is supported on 10.6 (it actually supports back to 10.5.4). See https://github.com/opensource-apple/IOUSBFamily/blob/master/IOUSBFamily/Heade... I've clarified the comment. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:460: IOObjectRelease(ref); On 2013/04/22 21:00:51, Avi wrote: > Huh? I thought OpenDevice consumed the ref... > > base/mac/scoped_ioobject.h's ScopedIOObject perhaps? Right you are :) I added the releasing afterwards, and missed this. I'll take a look at ScopedIOObject; didn't realise that existed. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/xb... content/browser/gamepad/xbox_data_fetcher_mac.cc:478: IOObjectRelease(ref); On 2013/04/22 21:00:51, Avi wrote: > ScopedIOObject Done.
https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:148: WebKit::WebGamepad& pad = data_.items[slot]; ... which means you don't need to qualify these names where they're used throughout the file now. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:180: service = 0; Why are you zeroing out |service|? It's a local variable. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:185: // that is supported on Mac OS 10.6. This comment, which answers the legitimate question of "why are we querying for this particular interface?" with system version information, makes sense, but some others don't... https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:266: (*plugin_interface)->Release(plugin_interface); You nulled out |plugin| above on line 191 when you released it. Why do it there and not here? If you're not consistent it makes me wonder. Would it be worthwhile to make a templated scoped container for interfaces that automatically Releases them when they fall out of scope? I'm seeing all the early returns in this function and getting a bit nervous. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:321: // If its useful, the deviceId can be used to track controllers through s/its/it's/ https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:333: UInt8* buffer = new UInt8[length]; Can you comment that this is released in WriteComplete? It took me a while to work out. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:406: memcpy(&data, buffer, sizeof(data)); Perhaps you can reinterpret cast to get a ButtonData pointer to the buffer and pass that pointer into NormalizeButtonData, and avoid copying. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.h (right): https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:9: #include <CoreFoundation/CoreFoundation.h> abcde... https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:99: // The device itself. Available on MacOS 10.5.4 and up. The device is available on 10.5.4 up? Or IOUSBDeviceStruct320? I'm not clear on why you need to call out system version compatibility in this comment and the one below. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:119: scoped_array<uint8> read_buffer_; You really need to get off scoped_array. scoped_ptr<uint8[]> should work fine.
I started reading this yesterday but didn’t get to the meat of the IOKit stuff. Here’s the one comment I had up until that point, which may well now be irrelevant. https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:309: size_t slot; Don’t declare stuff ’til you use it. You can declare slot right above the for.
(wfh, haven't compiled this yet) https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/9001/content/browser/gamepad/ga... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:309: size_t slot; On 2013/04/23 19:46:59, Mark Mentovai wrote: > Don’t declare stuff ’til you use it. You can declare slot right above the for. Done. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:148: WebKit::WebGamepad& pad = data_.items[slot]; On 2013/04/23 15:12:38, Avi wrote: > ... which means you don't need to qualify these names where they're used > throughout the file now. Done. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:180: service = 0; On 2013/04/23 15:12:38, Avi wrote: > Why are you zeroing out |service|? It's a local variable. Not sure. Removed. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:185: // that is supported on Mac OS 10.6. On 2013/04/23 15:12:38, Avi wrote: > This comment, which answers the legitimate question of "why are we querying for > this particular interface?" with system version information, makes sense, but > some others don't... Some other comments generally, or some other comments about the system version? I clarified this comment further to use "API" instead of "interface", since "interface" means something already in USB (hence "InterfaceInterface" below...) https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:266: (*plugin_interface)->Release(plugin_interface); On 2013/04/23 15:12:38, Avi wrote: > You nulled out |plugin| above on line 191 when you released it. Why do it there > and not here? If you're not consistent it makes me wonder. > > Would it be worthwhile to make a templated scoped container for interfaces that > automatically Releases them when they fall out of scope? I'm seeing all the > early returns in this function and getting a bit nervous. These two can be released pretty much as soon as they're used, but I've made a scoped container anyway, for peace of mind :) https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:333: UInt8* buffer = new UInt8[length]; On 2013/04/23 15:12:38, Avi wrote: > Can you comment that this is released in WriteComplete? It took me a while to > work out. Done. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:406: memcpy(&data, buffer, sizeof(data)); On 2013/04/23 15:12:38, Avi wrote: > Perhaps you can reinterpret cast to get a ButtonData pointer to the buffer and > pass that pointer into NormalizeButtonData, and avoid copying. I had that initially, but comments in base/basictypes.h on bit_cast<> had me worried about type punning. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.h (right): https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:99: // The device itself. Available on MacOS 10.5.4 and up. On 2013/04/23 15:12:38, Avi wrote: > The device is available on 10.5.4 up? Or IOUSBDeviceStruct320? I'm not clear on > why you need to call out system version compatibility in this comment and the > one below. I guess there's not really any need to call it out at all. https://codereview.chromium.org/14328036/diff/16001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:119: scoped_array<uint8> read_buffer_; On 2013/04/23 15:12:38, Avi wrote: > You really need to get off scoped_array. scoped_ptr<uint8[]> should work fine. Done.
I'm pretty happy. lgtm Let's see how Mark feels... https://codereview.chromium.org/14328036/diff/30001/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/30001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:473: (CFNumberRef)IORegistryEntryCreateCFProperty( CFCast<CFNumberRef>, probably CFCastStrict.
https://codereview.chromium.org/14328036/diff/30001/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/30001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:473: (CFNumberRef)IORegistryEntryCreateCFProperty( On 2013/04/24 03:30:12, Avi wrote: > CFCast<CFNumberRef>, probably CFCastStrict. Done.
https://codereview.chromium.org/14328036/diff/29003/base/mac/scoped_ioplugini... File base/mac/scoped_ioplugininterface.h (right): https://codereview.chromium.org/14328036/diff/29003/base/mac/scoped_ioplugini... base/mac/scoped_ioplugininterface.h:21: typedef T** IFT; What is IFT supposed to stand for? CFT, IOT, and NST all made sense because they were just the prefixes of the type families they were used with. What’s this one about? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:328: // XXX looks like there's a bug here? what if the first slot is empty, What’s the story here? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:384: // XXX should have xbox_enabled_ too. And this? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:78: float x_val = (float)x; Use c_plus_plus<style>(casts), not (c_style)casts. But I’m surprised you’d need a cast at all here. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:82: float real_magnitude = sqrtf(x_val * x_val + y_val * y_val); 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:87: float magnitude = real_magnitude > 32767 ? 32767 : real_magnitude; Don’t write your own min function, #include <algorithm> and use std::min. Write 32767.0f if you like. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:106: static float NormalizeTrigger(uint8 value) { You are in an anonymous namespace, the “static” is meaningless. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:108: (float)(value - kTriggerDeadzone) / (kuint8max - kTriggerDeadzone); Don’t use C-style casts. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:108: (float)(value - kTriggerDeadzone) / (kuint8max - kTriggerDeadzone); Don’t use kuint8max, #include <limits> and write std::numeric_limits<uint8>::max. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:113: normalized_data->buttons[0] = data.a ? 1.f : 0.f; If these represent binary state, is it sensible for them to be floats? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:113: normalized_data->buttons[0] = data.a ? 1.f : 0.f; 1.0f and 0.0f are normal for floating-point constants. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:166: kern_return_t kr; Don’t declare stuff ’til you use it. Move the declaration down, you can declare it inline with the call to IOCreatePlugInInterfaceForService. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:167: HRESULT res; Same. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:196: // value 1. Try to set it and fail out if it couldn't be configured. fail, not fail out https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:201: kr = (*device_)->SetConfiguration(device_, config_desc->bConfigurationValue); You are positive that you’re talking to an Xbox controller when you reach this point, correct? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:217: // For more detail, see https://github.com/Grumbel/xboxdrv/blob/master/PROTOCOL Spill the URL to a new line, because it’s embarrassing to wrap one character. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:224: kr = (*device_)->CreateInterfaceIterator(device_, &request, &iter); Do you need to IOObjectRelease iter, or have a ScopedIOObject supervise it? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:228: // There should be exactly one usb interface which matches the requested In comments, USB should be capitalized. This happens elsewhere in this file too. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:275: // The CFRunLoop retains the source. 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:285: for (int i = 1; i <= num_endpoints; i++) { 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:328: // This buffer will be released in WriteComplete when WritePipeAsync Blank line before this. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:331: buffer[0] = (UInt8)CONTROL_MESSAGE_SET_LED; C++-style casts. Line 333 also. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:340: if (kr != KERN_SUCCESS) { Do you need to delete[] the buffer if this happens (because WriteComplete won’t be called?) https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:347: return kVendorMicrosoft; 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:357: // the data got to the controller or not. Maybe it doesn’t matter if the data got to the controller, but you still want to delete[] buffer instead of leaking it. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:358: if (result != KERN_SUCCESS) KERN_SUCCESS is a kern_return_t, not an IOReturn. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:361: UInt8* buffer = (UInt8*)context; C++-style casts. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:366: uint32 bytesRead = (uint32)arg0; C++-style casts. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:371: // probably been destroyed by a meteorite. :) https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:383: if (length < 3) return; 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:392: // Length in packet doesn't match length reported by USB. So just drop it? Not worth reporting via IOError or another mechanism? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:403: memcpy(&data, buffer, sizeof(data)); Why copy? Why not cast buffer to a ButtonData*? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:413: led_pattern_ = (LEDPattern)buffer[0]; 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... . https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:506: CFRunLoopAddSource(CFRunLoopGetMain(), source_, kCFRunLoopDefaultMode); 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? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:508: CFRelease(source_); 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:512: IOReturn ret; Don’t declare this until you use it on line 519. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:561: controllers_.insert(controller); 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 https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:566: controllers_.erase(controller); I would do this after calling the delegate method. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.h (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:20: typedef enum { typedef enum is C. This is C++. Write enum StatusMessageType {. Same on line 32. But you might not even need the type name: you never use StatusMessageType anywhere, you only use its member constants. So you can just get away with enum { on this line, and the type can be anonymous. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:23: STATUS_MESSAGE_UNKNOWN = 2, Huh? If you don’t know what this message is and you don’t use it, why bother defining it? Have you seen these status messages but just don’t know what to do with them? If you want to keep this, a comment about what’s going on might clear things up, like you did with the also-unused STATUS_MESSAGE_RUMBLE. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:29: } StatusMessageType; This enum is not part of the public interface and is not used by any other class. It can be private, or it can even be moved out of the header and into an anonymous namespace in the .cc. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:31: // The LEDs are numbered top left, top right, bottom left, bottom right. Why is this information in a comment and not in the constant names, like LED_FLASH_TOP_LEFT, etc.? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:54: LED_FLASH_SLOW = 12, // Flash about once per 3 seconds Style nit: two spaces between the last non-comment thing on a line and the //. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:78: }; The delegate shouldn’t be destroyed by “delete”ing a Delegate*, right? Declare (but don’t define) a private destructor? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:80: XboxController(Delegate* delegate_); explicit. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:140: }; Private destructor declaration? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:142: XboxDataFetcher(Delegate* delegate); explicit?
https://codereview.chromium.org/14328036/diff/29003/base/mac/scoped_ioplugini... File base/mac/scoped_ioplugininterface.h (right): https://codereview.chromium.org/14328036/diff/29003/base/mac/scoped_ioplugini... base/mac/scoped_ioplugininterface.h:21: typedef T** IFT; On 2013/04/24 16:57:20, Mark Mentovai wrote: > What is IFT supposed to stand for? > > CFT, IOT, and NST all made sense because they were just the prefixes of the type > families they were used with. What’s this one about? "Interface Type", but I guess I should pick a better name. "InterfaceT"? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/g... File content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:328: // XXX looks like there's a bug here? what if the first slot is empty, On 2013/04/24 16:57:20, Mark Mentovai wrote: > What’s the story here? I've refactored this code to hopefully remove the bug now :) https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/g... content/browser/gamepad/gamepad_platform_data_fetcher_mac.mm:384: // XXX should have xbox_enabled_ too. On 2013/04/24 16:57:20, Mark Mentovai wrote: > And this? enabled_ only gates the HID stuff -- that can fail orthogonally to the Xbox stuff. I've updated the code so that xbox_fetcher_ can be null if it fails to register, and enabled_ doesn't gate the Xbox stuff. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:78: float x_val = (float)x; On 2013/04/24 16:57:20, Mark Mentovai wrote: > Use c_plus_plus<style>(casts), not (c_style)casts. But I’m surprised you’d need > a cast at all here. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:82: float real_magnitude = sqrtf(x_val * x_val + y_val * y_val); On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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. Old? I prefer "classic" ;) (done.) https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:87: float magnitude = real_magnitude > 32767 ? 32767 : real_magnitude; On 2013/04/24 16:57:20, Mark Mentovai wrote: > Don’t write your own min function, #include <algorithm> and use std::min. Write > 32767.0f if you like. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:106: static float NormalizeTrigger(uint8 value) { On 2013/04/24 16:57:20, Mark Mentovai wrote: > You are in an anonymous namespace, the “static” is meaningless. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:108: (float)(value - kTriggerDeadzone) / (kuint8max - kTriggerDeadzone); On 2013/04/24 16:57:20, Mark Mentovai wrote: > Don’t use kuint8max, #include <limits> and write > std::numeric_limits<uint8>::max. C++, making things more verbose since 1989! https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:108: (float)(value - kTriggerDeadzone) / (kuint8max - kTriggerDeadzone); On 2013/04/24 16:57:20, Mark Mentovai wrote: > Don’t use C-style casts. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:113: normalized_data->buttons[0] = data.a ? 1.f : 0.f; On 2013/04/24 16:57:20, Mark Mentovai wrote: > If these represent binary state, is it sensible for them to be floats? 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:113: normalized_data->buttons[0] = data.a ? 1.f : 0.f; On 2013/04/24 16:57:20, Mark Mentovai wrote: > 1.0f and 0.0f are normal for floating-point constants. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:166: kern_return_t kr; On 2013/04/24 16:57:20, Mark Mentovai wrote: > Don’t declare stuff ’til you use it. Move the declaration down, you can declare > it inline with the call to IOCreatePlugInInterfaceForService. I moved it down, but didn't inline it because it's re-used several times later. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:167: HRESULT res; On 2013/04/24 16:57:20, Mark Mentovai wrote: > Same. 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.) https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:196: // value 1. Try to set it and fail out if it couldn't be configured. On 2013/04/24 16:57:20, Mark Mentovai wrote: > fail, not fail out Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:201: kr = (*device_)->SetConfiguration(device_, config_desc->bConfigurationValue); On 2013/04/24 16:57:20, Mark Mentovai wrote: > You are positive that you’re talking to an Xbox controller when you reach this > point, correct? At least certain that it is a device with the vendor and product ID of an Xbox controller. Is that insufficiently certain? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:217: // For more detail, see https://github.com/Grumbel/xboxdrv/blob/master/PROTOCOL On 2013/04/24 16:57:20, Mark Mentovai wrote: > Spill the URL to a new line, because it’s embarrassing to wrap one character. I'm so embarrassed ;) https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:224: kr = (*device_)->CreateInterfaceIterator(device_, &request, &iter); On 2013/04/24 16:57:20, Mark Mentovai wrote: > Do you need to IOObjectRelease iter, or have a ScopedIOObject supervise it? Good catch. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:228: // There should be exactly one usb interface which matches the requested On 2013/04/24 16:57:20, Mark Mentovai wrote: > In comments, USB should be capitalized. This happens elsewhere in this file too. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:275: // The CFRunLoop retains the source. On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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. ScopedCFTypeRef to the rescue! Done here and in XboxDataFetcher, too. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:285: for (int i = 1; i <= num_endpoints; i++) { On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:328: // This buffer will be released in WriteComplete when WritePipeAsync On 2013/04/24 16:57:20, Mark Mentovai wrote: > Blank line before this. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:331: buffer[0] = (UInt8)CONTROL_MESSAGE_SET_LED; On 2013/04/24 16:57:20, Mark Mentovai wrote: > C++-style casts. Line 333 also. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:340: if (kr != KERN_SUCCESS) { On 2013/04/24 16:57:20, Mark Mentovai wrote: > Do you need to delete[] the buffer if this happens (because WriteComplete won’t > be called?) I bet I do! Good catch. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:347: return kVendorMicrosoft; On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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. Ok -- I've added a check in OpenDevice(). https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:357: // the data got to the controller or not. On 2013/04/24 16:57:20, Mark Mentovai wrote: > Maybe it doesn’t matter if the data got to the controller, but you still want to > delete[] buffer instead of leaking it. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:358: if (result != KERN_SUCCESS) On 2013/04/24 16:57:20, Mark Mentovai wrote: > KERN_SUCCESS is a kern_return_t, not an IOReturn. Done, though they're the same thing: http://www.opensource.apple.com/source/xnu/xnu-1486.2.11/iokit/IOKit/IOReturn.h https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:361: UInt8* buffer = (UInt8*)context; On 2013/04/24 16:57:20, Mark Mentovai wrote: > C++-style casts. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:366: uint32 bytesRead = (uint32)arg0; On 2013/04/24 16:57:20, Mark Mentovai wrote: > C++-style casts. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:383: if (length < 3) return; On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:392: // Length in packet doesn't match length reported by USB. On 2013/04/24 16:57:20, Mark Mentovai wrote: > So just drop it? Not worth reporting via IOError or another mechanism? 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:403: memcpy(&data, buffer, sizeof(data)); On 2013/04/24 16:57:20, Mark Mentovai wrote: > Why copy? Why not cast buffer to a ButtonData*? 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! https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:413: led_pattern_ = (LEDPattern)buffer[0]; On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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... > . I believe you :P https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:506: CFRunLoopAddSource(CFRunLoopGetMain(), source_, kCFRunLoopDefaultMode); On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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? 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()). https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:508: CFRelease(source_); On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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. Ah, I see! Fixed. I assume destroying port_ will also invalidate the source? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:512: IOReturn ret; On 2013/04/24 16:57:20, Mark Mentovai wrote: > Don’t declare this until you use it on line 519. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:561: controllers_.insert(controller); On 2013/04/24 16:57:20, Mark Mentovai wrote: > 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 Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:566: controllers_.erase(controller); On 2013/04/24 16:57:20, Mark Mentovai wrote: > I would do this after calling the delegate method. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.h (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:20: typedef enum { On 2013/04/24 16:57:20, Mark Mentovai wrote: > typedef enum is C. This is C++. Write enum StatusMessageType {. Same on line 32. > > But you might not even need the type name: you never use StatusMessageType > anywhere, you only use its member constants. So you can just get away with enum > { on this line, and the type can be anonymous. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:23: STATUS_MESSAGE_UNKNOWN = 2, On 2013/04/24 16:57:20, Mark Mentovai wrote: > Huh? If you don’t know what this message is and you don’t use it, why bother > defining it? Have you seen these status messages but just don’t know what to do > with them? If you want to keep this, a comment about what’s going on might clear > things up, like you did with the also-unused STATUS_MESSAGE_RUMBLE. I'll just remove it. (A lot of this code came from https://github.com/josephg/Cocontroller) https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:29: } StatusMessageType; On 2013/04/24 16:57:20, Mark Mentovai wrote: > This enum is not part of the public interface and is not used by any other > class. It can be private, or it can even be moved out of the header and into an > anonymous namespace in the .cc. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:31: // The LEDs are numbered top left, top right, bottom left, bottom right. On 2013/04/24 16:57:20, Mark Mentovai wrote: > Why is this information in a comment and not in the constant names, like > LED_FLASH_TOP_LEFT, etc.? Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:54: LED_FLASH_SLOW = 12, // Flash about once per 3 seconds On 2013/04/24 16:57:20, Mark Mentovai wrote: > Style nit: two spaces between the last non-comment thing on a line and the //. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:78: }; On 2013/04/24 16:57:20, Mark Mentovai wrote: > The delegate shouldn’t be destroyed by “delete”ing a Delegate*, right? Declare > (but don’t define) a private destructor? Apparently that's an error: ../../content/browser/gamepad/gamepad_platform_data_fetcher_mac.h:31:39: error: base class 'XboxDataFetcher::Delegate' has private destructor public XboxDataFetcher::Delegate { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../content/browser/gamepad/xbox_data_fetcher_mac.h:133:13: note: declared private here virtual ~Delegate(); ^ https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:80: XboxController(Delegate* delegate_); On 2013/04/24 16:57:20, Mark Mentovai wrote: > explicit. Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:140: }; On 2013/04/24 16:57:20, Mark Mentovai wrote: > Private destructor declaration? Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:140: }; On 2013/04/24 16:57:20, Mark Mentovai wrote: > Private destructor declaration? Done. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:142: XboxDataFetcher(Delegate* delegate); On 2013/04/24 16:57:20, Mark Mentovai wrote: > explicit? Done.
ping :)
Sorry, I was out on Monday and Tuesday. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:113: normalized_data->buttons[0] = data.a ? 1.f : 0.f; jeremya wrote: > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > If these represent binary state, is it sensible for them to be floats? > > 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. 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:201: kr = (*device_)->SetConfiguration(device_, config_desc->bConfigurationValue); jeremya wrote: > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > You are positive that you’re talking to an Xbox controller when you reach this > > point, correct? > > At least certain that it is a device with the vendor and product ID of an Xbox > controller. Is that insufficiently certain? Yeah, vendor and product ID is enough, but you did account for this as I intended in this change. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:358: if (result != KERN_SUCCESS) jeremya wrote: > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > KERN_SUCCESS is a kern_return_t, not an IOReturn. > > Done, though they're the same thing: > http://www.opensource.apple.com/source/xnu/xnu-1486.2.11/iokit/IOKit/IOReturn.h 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:403: memcpy(&data, buffer, sizeof(data)); jeremya wrote: > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > Why copy? Why not cast buffer to a ButtonData*? > > 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! 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. > An object shall have its stored value accessed only by an lvalue > expression that has one of the following types[73]: […] > - a character type. […] > [73] The intent of this list is to specify those circumstances in > which an object may or may not be aliased. Ref. C++03 §3.10 (“Lvalues and rvalues”) clause 15 > If a program attempts to access the stored value of an object through > an lvalue of other than one of the following types the behavior is > undefined[48]: […] > - a char or unsigned char type. […] > [48] The intent of this list is to specify those circumstances in > which an object may or may not be aliased. Ref. similar language in C++11 §3.10 clause 10. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:506: CFRunLoopAddSource(CFRunLoopGetMain(), source_, kCFRunLoopDefaultMode); jeremya wrote: > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > 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? > > 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()). 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:508: CFRelease(source_); jeremya wrote: > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > 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. > > Ah, I see! Fixed. I assume destroying port_ will also invalidate the source? 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.h (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.h:78: }; jeremya wrote: > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > The delegate shouldn’t be destroyed by “delete”ing a Delegate*, right? Declare > > (but don’t define) a private destructor? > > Apparently that's an error: > > ../../content/browser/gamepad/gamepad_platform_data_fetcher_mac.h:31:39: error: > base class 'XboxDataFetcher::Delegate' has private destructor > public XboxDataFetcher::Delegate { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../content/browser/gamepad/xbox_data_fetcher_mac.h:133:13: note: declared > private here > virtual ~Delegate(); > ^ Oh duh, you’d need a protected destructor, not a private one. But that’d mean you’d need an implementation. So maybe not worth doing just for a little “delete” protection. https://codereview.chromium.org/14328036/diff/19002/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/19002/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:182: kr = IOCreatePlugInInterfaceForService(service, Whether or not you declare kr on this line has nothing to do with whether kr is repurposed for other uses. The declaration belongs here. https://codereview.chromium.org/14328036/diff/19002/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:191: IOReturn res = (*plugin)->QueryInterface( Sadly, these were all correct as HRESULT, not as IOReturn. You should switch them back. If you wanted an explicit “success” condition to test, use SUCCEEDED(res). An HRESULT is an SInt32 (<CoreFoundation/CFPlugInCOM.h>) which is a signed int on 64-bit and a signed long on 32-bit (<MacTypes.h>). An IOReturn is a kern_return_t (<IOKit/IOReturn.h>) which is an int (<mach/kern_return.h>). In addition to the types not being compatible, the namespaces are incompatible as well. WTF, I know, right?
scottmg - do you have a better understanding than I about the TYPE_IO vs TYPE_UI thing? https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:113: normalized_data->buttons[0] = data.a ? 1.f : 0.f; On 2013/05/01 18:59:20, Mark Mentovai wrote: > jeremya wrote: > > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > > If these represent binary state, is it sensible for them to be floats? > > > > 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. > > 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. 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... https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:403: memcpy(&data, buffer, sizeof(data)); On 2013/05/01 18:59:20, Mark Mentovai wrote: > jeremya wrote: > > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > > Why copy? Why not cast buffer to a ButtonData*? > > > > 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! > > 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. > > > An object shall have its stored value accessed only by an lvalue > > expression that has one of the following types[73]: > […] > > - a character type. > […] > > [73] The intent of this list is to specify those circumstances in > > which an object may or may not be aliased. > > Ref. C++03 §3.10 (“Lvalues and rvalues”) clause 15 > > > If a program attempts to access the stored value of an object through > > an lvalue of other than one of the following types the behavior is > > undefined[48]: > […] > > - a char or unsigned char type. > […] > > [48] The intent of this list is to specify those circumstances in > > which an object may or may not be aliased. > > Ref. similar language in C++11 §3.10 clause 10. Mind. Blown. Awesome! https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:506: CFRunLoopAddSource(CFRunLoopGetMain(), source_, kCFRunLoopDefaultMode); On 2013/05/01 18:59:20, Mark Mentovai wrote: > jeremya wrote: > > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > > 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? > > > > 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()). > > 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. 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. https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:508: CFRelease(source_); On 2013/05/01 18:59:20, Mark Mentovai wrote: > jeremya wrote: > > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > > 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. > > > > Ah, I see! Fixed. I assume destroying port_ will also invalidate the source? > > 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. Surprising, but done. https://codereview.chromium.org/14328036/diff/19002/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/19002/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:182: kr = IOCreatePlugInInterfaceForService(service, On 2013/05/01 18:59:20, Mark Mentovai wrote: > Whether or not you declare kr on this line has nothing to do with whether kr is > repurposed for other uses. The declaration belongs here. Done. https://codereview.chromium.org/14328036/diff/19002/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:191: IOReturn res = (*plugin)->QueryInterface( On 2013/05/01 18:59:20, Mark Mentovai wrote: > Sadly, these were all correct as HRESULT, not as IOReturn. You should switch > them back. If you wanted an explicit “success” condition to test, use > SUCCEEDED(res). > > An HRESULT is an SInt32 (<CoreFoundation/CFPlugInCOM.h>) which is a signed int > on 64-bit and a signed long on 32-bit (<MacTypes.h>). An IOReturn is a > kern_return_t (<IOKit/IOReturn.h>) which is an int (<mach/kern_return.h>). In > addition to the types not being compatible, the namespaces are incompatible as > well. > > WTF, I know, right? Done. Sigh :P
https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/29003/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:113: normalized_data->buttons[0] = data.a ? 1.f : 0.f; jeremya wrote: > On 2013/05/01 18:59:20, Mark Mentovai wrote: > > jeremya wrote: > > > On 2013/04/24 16:57:20, Mark Mentovai wrote: > > > > If these represent binary state, is it sensible for them to be floats? > > > > > > 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. > > > > 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. > > 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... Oh, I thought we could do it in Xbox-specific code still.
If you’re not doing libevent IO, you don’t need a TYPE_IO loop. You can make the loop type conditional on OS if necessary. Since you’re using CFRunLoop stuff, you need a TYPE_UI loop on Mac.
On 2013/05/21 13:47:50, Mark Mentovai wrote: > If you’re not doing libevent IO, you don’t need a TYPE_IO loop. You can make the > loop type conditional on OS if necessary. Since you’re using CFRunLoop stuff, > you need a TYPE_UI loop on Mac. Any type should be fine for Windows, use UI. The Linux udev code watches file handles via the message loop, so it will have to be TYPE_IO loop on Linux: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ga...
Moved the conversions back to xbox-specific code, though it's a little clumsy because of the triggers. Also added a #if for the mac message loop type.
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/14328036/58001
Message was sent while issue was closed.
Change committed as 201717
Message was sent while issue was closed.
You broke the build. https://codereview.chromium.org/14328036/diff/58001/content/browser/gamepad/x... File content/browser/gamepad/xbox_data_fetcher_mac.cc (right): https://codereview.chromium.org/14328036/diff/58001/content/browser/gamepad/x... content/browser/gamepad/xbox_data_fetcher_mac.cc:390: uint32 bytesRead = reinterpret_cast<uint32>(arg0); This broke the Mac 64 build (http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Mac%2010.8%20x64...): /Volumes/data/b/build/slave/Chromium_Mac_10_8_x64__experimental_/build/src/content/browser/gamepad/xbox_data_fetcher_mac.cc:390:22:error: cast from pointer to smaller type 'uint32' (aka 'unsigned int') loses information uint32 bytesRead = reinterpret_cast<uint32>(arg0); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. The correct thing to do here imo is to reinterpret_cast to uintptr_t and then assign over. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
