Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8799022:
Add Mac implementation of data_fetcher for gamepad. (Closed)
|
Created:
9 years ago by scottmg Modified:
9 years ago Reviewers:
Avi (use Gerrit) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, dpranke-watch+content_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Mac implementation of data_fetcher for gamepad.
BUG=79050
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113538
Patch Set 1 #Patch Set 2 : working for test device now #Patch Set 3 : some renaming per usb hid spec #
Total comments: 18
Patch Set 4 : review fixes, convert some from CF to NS #Patch Set 5 : more NS->CF, implement remove #
Total comments: 21
Patch Set 6 : review fixes #
Total comments: 8
Patch Set 7 : no unsigned, proper scoped type, fix fmt string #
Total comments: 1
Patch Set 8 : fix indent #
Messages
Total messages: 20 (0 generated)
Hi Nico, I know you're not an OWNERS for this folder, but would you mind having a look at the Mac bits here in data_fetcher_mac.mm? I'm not very Mac-familiar so I'm not sure if I've done anything horrible.
Dealing with Core Foundation is a bit clunky. We have a whole bunch of helpers that you should avail yourself of, and I noted them inline. But you added this as a .mm file, an Objective-C++ file. If you are doing so, you have the freedom to build your complicated arrays/dictionaries using NSArrays/NSDictionaries in about half the lines of code and double the readability. I'd recommend that. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.h (right): http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.h:27: IOHIDManagerRef hid_manager_ref_; scoped_cftyperef http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:31: CFRelease(page_number_ref); Rather than create/release, prefer scoped_cftyperef. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:65: CFMutableDictionaryRef joystick_criterion = CreateDeviceMatchingDictionary( Make this a scoped_cftyperef... http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:68: CFMutableDictionaryRef game_criterion = CreateDeviceMatchingDictionary( ...and this... http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:75: CFArrayRef criteria = CFArrayCreate( ...and this should be held in a scoped_cftyperef (eliminating the need for a release on line 82)... http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:80: NULL); ...and this should be kCFTypeArrayCallBacks, to allow the array to retain/release its items properly. Without it, I believe you leak the two criteria. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:119: CFRelease(hid_manager_ref_); Make this member variable a scoped_cftyperef and you don't need this. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:160: const_cast<void*>(CFArrayGetValueAtIndex(elements, i))); This screams for CFCast from foundation_util. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:205: CFNumberRef vendor_id_ref = reinterpret_cast<CFNumberRef>( CFCast? (and the two others below)
Sounds like avi wants to review this.
Makes more sense; I can give owner approval too. I didn't mean to step on your foot, though; I was flipping through changes last night and felt like commenting.
On 2011/12/06 04:54:19, Avi wrote: > Dealing with Core Foundation is a bit clunky. We have a whole bunch of helpers > that you should avail yourself of, and I noted them inline. > > But you added this as a .mm file, an Objective-C++ file. If you are doing so, > you have the freedom to build your complicated arrays/dictionaries using > NSArrays/NSDictionaries in about half the lines of code and double the > readability. I'd recommend that. Thanks for taking a loot, and for the pointer to the base/ helpers. I will rework it shortly. I added it as .mm without really knowing what I was doing. I'm totally ignorant of Mac dev: is there any benefit to avoiding .mm and NSxxx and keeping it as .cc?
(In addition to looting, looking was also good...)
If you have multiple platforms sharing ifdef-ed versions inside one .cc file, then CF is great. But for a Mac impl file like that, you don't gain much from going pure CF.
Thanks! Using NS is much cleaner than the CF version. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.h (right): http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.h:27: IOHIDManagerRef hid_manager_ref_; On 2011/12/06 04:54:19, Avi wrote: > scoped_cftyperef Done. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:31: CFRelease(page_number_ref); On 2011/12/06 04:54:19, Avi wrote: > Rather than create/release, prefer scoped_cftyperef. Done. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:65: CFMutableDictionaryRef joystick_criterion = CreateDeviceMatchingDictionary( On 2011/12/06 04:54:19, Avi wrote: > Make this a scoped_cftyperef... Done. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:68: CFMutableDictionaryRef game_criterion = CreateDeviceMatchingDictionary( On 2011/12/06 04:54:19, Avi wrote: > ...and this... Done. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:75: CFArrayRef criteria = CFArrayCreate( On 2011/12/06 04:54:19, Avi wrote: > ...and this should be held in a scoped_cftyperef (eliminating the need for a > release on line 82)... Done. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:80: NULL); On 2011/12/06 04:54:19, Avi wrote: > ...and this should be kCFTypeArrayCallBacks, to allow the array to > retain/release its items properly. Without it, I believe you leak the two > criteria. Done. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:119: CFRelease(hid_manager_ref_); On 2011/12/06 04:54:19, Avi wrote: > Make this member variable a scoped_cftyperef and you don't need this. Done. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:160: const_cast<void*>(CFArrayGetValueAtIndex(elements, i))); On 2011/12/06 04:54:19, Avi wrote: > This screams for CFCast from foundation_util. I mucked around for a bit here, but I'm not clear on how to use it with IOHIDElementRef. CFCast is defined explicitly in foundation_util.mm for Foundation types (not including IOxxx), and it's not an NS type for ObjCCast. http://codereview.chromium.org/8799022/diff/4001/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:205: CFNumberRef vendor_id_ref = reinterpret_cast<CFNumberRef>( On 2011/12/06 04:54:19, Avi wrote: > CFCast? (and the two others below) Done.
On 2011/12/07 01:20:49, scottmg wrote: > Thanks! Using NS is much cleaner than the CF version. I know! Right? > On 2011/12/06 04:54:19, Avi wrote: > > This screams for CFCast from foundation_util. > I mucked around for a bit here, but I'm not clear on how to use it with > IOHIDElementRef. CFCast is defined explicitly in foundation_util.mm for > Foundation types (not including IOxxx), and it's not an NS type for ObjCCast. It might be worth extending CFCast to know about the IOxxx types. Not in this CL.
Very nice! Mostly style violations, but a few real issues. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.h (right): http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.h:19: #import <Foundation/NSArray.h> Just do @class NSArray; here instead; forward-declare for the win. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:14: #import <Foundation/NSArray.h> #import <Foundation/Foundation.h> if not #import <Cocoa/Cocoa.h> http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:23: [NSNumber numberWithInt: usage_page], no space after : here, and on line 25. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:66: // Register for plug/unplug notifications full sentences end with periods . http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:175: using base::mac::CFToNSCast; I was not aware that the style guide permitted this until I looked it up. Nice! http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:195: NSNumber* vendor_id = CFToNSCast(base::mac::CFCastStrict<CFNumberRef>( We do "using base::mac::CFToNSCast" to cut clutter but don't do "using base::mac::CFCastStrict"? http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:204: [product UTF8String], Nope. NSObjects have a formatting code of %@; use that and just pass the object pointer. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:207: NSData* as16 = [ident dataUsingEncoding: NSUTF16StringEncoding]; no space after : http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:213: length: kOutputLengthBytes - sizeof(WebKit::WebUChar)]; Bad style. Besides no space after :, put getBytes on the first line, and align on colons. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:215: NSArray* elements = CFToNSCast(IOHIDDeviceCopyMatchingElements( Nope. CF-style functions that have "Copy" in their name return an object that needs to be released. Your choices are to capture the raw CFArray returned in a scoped_cftyperef and do the CFToNSCast in the call to AddButtonsAndAxes, or use base::mac::CFTypeRefToNSObjectAutorelease. The former is cleaner.
Thanks for your patient Mac-n00b-review. :) http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.h (right): http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.h:19: #import <Foundation/NSArray.h> On 2011/12/07 04:12:46, Avi wrote: > Just do > > @class NSArray; > > here instead; forward-declare for the win. Done. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:14: #import <Foundation/NSArray.h> On 2011/12/07 04:12:46, Avi wrote: > #import <Foundation/Foundation.h> if not #import <Cocoa/Cocoa.h> Done. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:23: [NSNumber numberWithInt: usage_page], On 2011/12/07 04:12:46, Avi wrote: > no space after : here, and on line 25. Done. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:66: // Register for plug/unplug notifications On 2011/12/07 04:12:46, Avi wrote: > full sentences end with periods . Done. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:175: using base::mac::CFToNSCast; On 2011/12/07 04:12:46, Avi wrote: > I was not aware that the style guide permitted this until I looked it up. Nice! Darin schooled me on that last review. :) http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:195: NSNumber* vendor_id = CFToNSCast(base::mac::CFCastStrict<CFNumberRef>( On 2011/12/07 04:12:46, Avi wrote: > We do "using base::mac::CFToNSCast" to cut clutter but don't do "using > base::mac::CFCastStrict"? Done. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:204: [product UTF8String], On 2011/12/07 04:12:46, Avi wrote: > Nope. NSObjects have a formatting code of %@; use that and just pass the object > pointer. Done. (Magic!) http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:207: NSData* as16 = [ident dataUsingEncoding: NSUTF16StringEncoding]; On 2011/12/07 04:12:46, Avi wrote: > no space after : Done. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:213: length: kOutputLengthBytes - sizeof(WebKit::WebUChar)]; On 2011/12/07 04:12:46, Avi wrote: > Bad style. Besides no space after :, put getBytes on the first line, and align > on colons. Done. http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:215: NSArray* elements = CFToNSCast(IOHIDDeviceCopyMatchingElements( On 2011/12/07 04:12:46, Avi wrote: > Nope. CF-style functions that have "Copy" in their name return an object that > needs to be released. Your choices are to capture the raw CFArray returned in a > scoped_cftyperef and do the CFToNSCast in the call to AddButtonsAndAxes, or use > base::mac::CFTypeRefToNSObjectAutorelease. The former is cleaner. Is scoped_nsobject<NSArray> ok, or are the retain/release different somehow? (I sat and wondered what the heck the rules were for ownership/copying by couldn't figure it out from the docs... still missing some high level knowledge here).
Just found another issue. No implicit int. (This isn't the '70s :) ) You probably mean size_t in just about all the places you declare variables as "unsigned". Next rev of the CL, I don't want to find _any_ uses of the keyword unsigned. (NB: the style guide does not permit unsigned use. We allow size_t, though, where appropriate.) http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/9002/content/browser/gamepad/data... content/browser/gamepad/data_fetcher_mac.mm:215: NSArray* elements = CFToNSCast(IOHIDDeviceCopyMatchingElements( On 2011/12/07 04:39:36, scottmg wrote: > Is scoped_nsobject<NSArray> ok, or are the retain/release different somehow? Wellllll.... they used to be the same until they weren't. Back in the good ole days, you could CFRetain/CFRelease ObjC objects and send -release/-retain to CF objects. Then garbage collection came along, and it worked with ObjC objects but CF objects remained ref counted. If you look at the CFTypeRefToNSObjectAutorelease function, you'll see it's written to handle both cases, specially bridging between the two lands. And then ARC came along, and again, ObjC objects got the magic while CF objects didn't. And with ARC, there's a completely different set of bridging functions to move objects between the worlds of automatic and manual ref counting. The way out is to just keep things in containers for their type. If it's a CF type, hold it in a scoped_cftyperef. If it's an NSObject, hold it in a scoped_nsobject. Use the casting functions on use and you won't have to deal with ownership issues that way. http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:20: NSDictionary* DeviceMatching(unsigned usage_page, unsigned usage) { You define the values used to call this function (lines 34-40) as uint32_t. Make the parameter types here match. (Relying on implicit int here is so '70s.) http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:214: scoped_nsobject<NSArray> elements(CFToNSCast( As discussed, you will want to use a scoped_cftyperef here and do the CFToNSCast as you pass the parameter to AddButtonsAndAxes.
http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:203: @"%@s (Vendor: %04x Product: %04x)", Not %@s. Just %@. ("%@" => substitute in the object's description, which in the case of a string is the string value itself.) http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:286: } // namespace content Two spaces before // when they follow actual content.
http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:20: NSDictionary* DeviceMatching(unsigned usage_page, unsigned usage) { On 2011/12/07 05:26:06, Avi wrote: > You define the values used to call this function (lines 34-40) as uint32_t. Make > the parameter types here match. (Relying on implicit int here is so '70s.) Done. http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:203: @"%@s (Vendor: %04x Product: %04x)", On 2011/12/07 05:39:13, Avi wrote: > Not %@s. Just %@. ("%@" => substitute in the object's description, which in the > case of a string is the string value itself.) Done. http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:214: scoped_nsobject<NSArray> elements(CFToNSCast( On 2011/12/07 05:26:06, Avi wrote: > As discussed, you will want to use a scoped_cftyperef here and do the CFToNSCast > as you pass the parameter to AddButtonsAndAxes. Done. http://codereview.chromium.org/8799022/diff/14003/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:286: } // namespace content On 2011/12/07 05:39:13, Avi wrote: > Two spaces before // when they follow actual content. Done.
I'm no gamepad expert, but the code looks quite plausible; if it works, I have but a small style nit. LGTM! http://codereview.chromium.org/8799022/diff/19001/content/browser/gamepad/dat... File content/browser/gamepad/data_fetcher_mac.mm (right): http://codereview.chromium.org/8799022/diff/19001/content/browser/gamepad/dat... content/browser/gamepad/data_fetcher_mac.mm:215: IOHIDDeviceCopyMatchingElements(device, NULL, kIOHIDOptionsTypeNone)); Still four space indent for a line continuation.
On 2011/12/07 19:24:14, Avi wrote: > I'm no gamepad expert, but the code looks quite plausible; if it works, I have > but a small style nit. LGTM! Thanks for the review! > > http://codereview.chromium.org/8799022/diff/19001/content/browser/gamepad/dat... > File content/browser/gamepad/data_fetcher_mac.mm (right): > > http://codereview.chromium.org/8799022/diff/19001/content/browser/gamepad/dat... > content/browser/gamepad/data_fetcher_mac.mm:215: > IOHIDDeviceCopyMatchingElements(device, NULL, kIOHIDOptionsTypeNone)); > Still four space indent for a line continuation. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/8799022/24001
Try job failure for 8799022-24001 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/8799022/24001
Change committed as 113538 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
