DirectInput is deprecated and sort of terrible. The Raw Input API is much simpler to use, and we can rip out all the threading code in the Windows gamepad backend using it.

Comment on attachment 8406258 [details] [diff] [review] Replace Windows Gamepad DirectInput backend with Raw Input Jim: do you think you could review this? It's mostly Win32 API, using the Raw Input API: http://msdn.microsoft.com/en-us/library/windows/desktop/ms645536%28v=vs.85%29.aspx This is replacing the existing DirectInput usage. It's nicer because it's single-threaded and the code is less complex overall.

Comment on attachment 8406258 [details] [diff] [review] Replace Windows Gamepad DirectInput backend with Raw Input Review of attachment 8406258 [details] [diff] [review]: ----------------------------------------------------------------- I don't have a joystick to test, but I assume you've run this through it's paces. Generally looks good, just a few formatting nits and couple issues with return values checks. ::: hal/windows/WindowsGamepad.cpp @@ +91,5 @@ > +bool > +GetPreparsedData(HANDLE handle, nsTArray<uint8_t>& data) > +{ > + UINT size; > + if (GetRawInputDeviceInfo(handle, RIDI_PREPARSEDDATA, nullptr, &size) < 0) Looks like this should be > 0 since with pData set to nullptr a success value is 0. also nit - {} @@ +120,3 @@ > } > + > + int value = dpad_value - gamepad->dpadCaps.LogicalMin; a comment here explaining what this value is would be helpful. @@ +138,5 @@ > +bool > +SupportedUsage(USHORT page, USHORT usage) > +{ > + for (unsigned i = 0; i < ArrayLength(kUsagePages); i++) { > + if (page == kUsagePages[i].usagePage && usage == kUsagePages[i].usage) nit - {} @@ +240,5 @@ > +void > +WindowsGamepadService::ScanForRawInputDevices() > +{ > + UINT numDevices; > + GetRawInputDeviceList(nullptr, &numDevices, sizeof(RAWINPUTDEVICELIST)); numDevices is uninitialized here, and you're not checking the result. I think a check similar to what you did with GetRawInputDeviceInfo would be prudent. @@ +288,5 @@ > + } > + bool LessThan(const HIDP_VALUE_CAPS& c1, const HIDP_VALUE_CAPS& c2) const > + { > + if (c1.UsagePage == c2.UsagePage) > + return c1.Range.UsageMin < c2.Range.UsageMin; nit - {} @@ +299,5 @@ > +WindowsGamepadService::GetRawGamepad(HANDLE handle) > +{ > + for (unsigned i = 0; i < mGamepads.Length(); i++) { > + if (mGamepads[i].type == kRawInputGamepad > + && mGamepads[i].handle == handle) { did we change coding standards here? Usually the && would be on the end of the first line, but I've been seeing a lot of this type of formatting lately. @@ +320,5 @@ > + > + // Device name is a mostly-opaque string. > + wchar_t devname[256]; > + size = sizeof(devname); > + if (GetRawInputDeviceInfo(handle, RIDI_DEVICENAME, &devname, &size) < 0) { <= 0 I think? 0 would mean no device name is configured, which I think is unexpected. Plus the CreateFile call below will fail with it anyway. @@ +332,5 @@ > + return false; > + } > + > + // Product string is a human-readable name. > + wchar_t name[128] = { 0 }; is the 128 specified in the docs? Maybe make this a const with a comment explaining why we use 128. @@ +410,5 @@ > + } > + > + gamepad.numAxes = std::min(axes.Length(), kMaxAxes); > + for (unsigned i = 0; i < gamepad.numAxes; i++) { > + if (i >= kMaxAxes) nit - {} @@ +465,5 @@ > + > + // Second, get the preparsed data > + nsTArray<uint8_t> parsedbytes; > + if (!GetPreparsedData(raw->header.hDevice, parsedbytes)) > + return false; nit - {}

Thanks for the review! (In reply to Jim Mathies [:jimm] from comment #3) > ::: hal/windows/WindowsGamepad.cpp > @@ +91,5 @@ > > +bool > > +GetPreparsedData(HANDLE handle, nsTArray<uint8_t>& data) > > +{ > > + UINT size; > > + if (GetRawInputDeviceInfo(handle, RIDI_PREPARSEDDATA, nullptr, &size) < 0) > > Looks like this should be > 0 since with pData set to nullptr a success > value is 0. This is returning false (indicating failure) if the return is < 0, I think it's already doing what you want. > @@ +299,5 @@ > > +WindowsGamepadService::GetRawGamepad(HANDLE handle) > > +{ > > + for (unsigned i = 0; i < mGamepads.Length(); i++) { > > + if (mGamepads[i].type == kRawInputGamepad > > + && mGamepads[i].handle == handle) { > > did we change coding standards here? Usually the && would be on the end of > the first line, but I've been seeing a lot of this type of formatting lately. Nope, I apparently misread the style guide somehow. I think this first line here confused me: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators > @@ +332,5 @@ > > + return false; > > + } > > + > > + // Product string is a human-readable name. > > + wchar_t name[128] = { 0 }; > > is the 128 specified in the docs? Maybe make this a const with a comment > explaining why we use 128. Yeah, it's mentioned here: http://msdn.microsoft.com/en-us/library/windows/hardware/ff539681%28v=vs.85%29.aspx "For USB devices, the maximum string length is 126 wide characters (not including the terminating NULL character)." I'll add a comment.

Should have pushed this to Try earlier. The builders don't have the import lib for hid.dll, so I can't link directly to it (I'm not sure if they would need a newer SDK or just some other SDK installed or what.) I just switched to dynamically loading it instead. The only real change here is the HIDLoader class, the rest is just changing direct calls to calls through the loader.

Comment on attachment 8413699 [details] [diff] [review] Replace Windows Gamepad DirectInput backend with Raw Input. Review of attachment 8413699 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

Thanks! New try run is very green: https://tbpl.mozilla.org/?tree=Try&rev=4299de24b9db https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb66b16b70c

Backed out for B2G desktop Windows build failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb6177f3597

I'm assuming this is non-unified build bustage. Easy to fix, at least.

Fixed that bustage (tested in a local --disable-unified-compilation build) and re-pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/79c804f8760d