The Gamepad HAL code doesn't currently handle the OOP scenario, mostly because I don't have a Gonk backend yet anyway, and I didn't want to complicate things for initial landing. I don't think the architecture should be too bad. Currently a nsGlobalWindow calls into the singleton GamepadService to register itself for gamepad events. The GamepadService calls hal::StartMonitoringGamepadStatus, which looks for gamepads and calls directly back into the GamepadService to feed it events. In an OOP scenario, I think the only change should be that hal::StartMonitoringGamepadStatus should take an observer interface as a parameter, and the backends would feed all updates through the observer interface instead of directly to the GamepadService. For the existing in-process case the GamepadService could just implement the GamepadObserver interface and it would work basically as it does now. For the OOP scenario we'd need to add it to the PHal protocol: http://mxr.mozilla.org/mozilla-central/source/hal/sandbox/PHal.ipdl and implement the proxying in SandboxHal: http://mxr.mozilla.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp The RegisterBatteryObserver code is probably a good place to copy.

Progress happening at https://github.com/qdot/mozilla-central/tree/852944-oop-joystick. Hopefully can crank this out pretty quick.

Cool! From a brief skim of your patchset it looks very similar to what I had in my head when I wrote comment 0.

Moved to observer model similar to battery and other HAL sandbox features. Messages created and hooked up. Things still left to do (that I currently know about): - Add gamepad index creation management in the parent process - PlatformGamepadService doesn't really need to be an object, can just be namespaced functions

Other things still left to do now that I've taken a look through the diff: - Fix test service - Readd comments - Figure out if we need GetCurrentGamepadChangeInfo, if so, update platforms - Change GamepadChangeEvent to GamepadHALEvent - Update CocoaGamepad HAL with new Service names

Comment on attachment 832013 [details] [diff] [review] Patch 1 (v2) - Make Gamepad API work in content processes Review of attachment 832013 [details] [diff] [review]: ----------------------------------------------------------------- ::: hal/sandbox/PHal.ipdl @@ +97,5 @@ > > +struct GamepadButton { > + bool pressed; > + double value; > +}; Whoops. Don't need this. Will remove after I get review round back.

Fixed platform includes, removed unneeded ipdl

Comment on attachment 832014 [details] [diff] [review] Patch 2 (v1) - Update mochitests to reflect new Gameapi API signatures Realized the mochis aren't actually testing any of the work I did, and could easily sit behind mozilla::hal::GamepadFunctions to also test our observer model and index allotment. New patch coming soon.

Now routing mochitests through hal::GamepadFunctions to test observer and indexes as well as functionality

Comment on attachment 832036 [details] [diff] [review] Patch 1 (v3) - Make Gamepad API work in content processes Review of attachment 832036 [details] [diff] [review]: ----------------------------------------------------------------- ::: hal/sandbox/PHal.ipdl @@ +96,5 @@ > }; > > +struct GamepadAdded { > + nsString id; > + uint32_t index; Wouldn't this be better using an actor? You have an index in all of these...

Brought branch up to date, moved to gecko-dev repo. Branch now at https://github.com/qdot/gecko-dev/tree/852944-gamepad-oop Now I just need to actually finish that "actor per gamepad per process" deal. :|

So as it turns out, the way I stubbed this out means that with e10s enabled on desktop the Gamepad API just silently fails to do anything. That sucks. We should fix this before we ship e10s on desktop.

This should block e10s uplift to Aurora.

Ok brought patches up to current master, tests seem to work. Will try to get bent's comments addressed ASAP. Will also have to redo windows hal changes since the xinput stuff changed a bunch, but I don't see that being a ton of work.

Ok, IPC Gamepad API is back on track, but this new patch set looks far different than the first version from Nov 2013. Instead of adding more functions onto HAL, I've moved all of gamepad into DOM, and thrown functions on top of PContent instead. This was on recommendation from :bent. This also allows us to easily implement rumble since we are now on a protocol that gives us two-way communication, versus HAL's weird observer model system. Patches 1 and 2 are almost all moves or removals, so I've put those up for review now. Should be pretty simple. Patches 3-5 are close, but still WIP. Currently, things /mostly/ work on my linux test machine. I can see/use gamepads in both e10s and non-e10s instances, though I'm having some problems with losing the ability to see new gamepads after a while (getGamepads() just returns empty arrays constantly). I also need to test on windows/os x/possibly android?, which I'll start as soon as try comes back up.

Fixed issue with windows not being able to find HAL_LOG

Fixed issue with AddGamepad not returning indexes, which fixed issues I was seeing. Ready for review.

Right now, these tests are still only non-e10s. I've gotta figure out a way to get GamepadServiceTest calls back to the main process. May file a followup bug for that.

Removed stub function additions from HalInternal.

Removed a bunch of unneeded includes, moved GameServiceTest changes to later patch.

Removed a bunch of unneeded includes

Removed unneeded includes

Try with mochitest 4 on all platforms, fails on android due to missing include: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a69f80ee5334 Fixed android includes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba12391e000

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #27) > Right now, these tests are still only non-e10s. I've gotta figure out a way > to get GamepadServiceTest calls back to the main process. May file a > followup bug for that. I think the simplest way to do this would be to use SpecialPowers.loadChromeScript, which loads a script for you in the chrome process and gives you a proxy with a MessageManager API so you can communicate with it. You should be able to do that and then have message listeners in the chrome script to poke GamepadServiceTest for you. Here's an example of loadChromeScript: https://hg.mozilla.org/mozilla-central/annotate/dcc86f78d75e/b2g/chrome/content/test/mochitest/RecordingStatusHelper.js#l25 and the corresponding chrome script: https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/test/mochitest/RecordingStatusChromeScript.js Thanks for getting back to this, I'll try to give these patches a once-over tomorrow.

Oh, awesome! I used loadChromeScript for some of our e10s testing of the settings API, but that was super simple and I didn't think about sending messages to the chrome script. That'll work nicely, and then I can just make a proxy for the GamepadTestService. Will try to slide that into this bug soon, though will attach to a followup if r+'s suddenly pour in somehow.

Any chance this can get a review soon?

Comment on attachment 8569514 [details] [diff] [review] Patch 3 (v3) - IPC Implementation of Gamepad API Review of attachment 8569514 [details] [diff] [review]: ----------------------------------------------------------------- it looks ok for me but please fix: 1. don't reuse the same ID for gamepad. 2. a better approach to count the number of listeners. ::: dom/gamepad/GamepadFunctions.cpp @@ +10,5 @@ > +#include "mozilla/unused.h" > + > +namespace mozilla { > +namespace dom { > +namespace GamepadFunctions { namespace { @@ +36,5 @@ > + if (GamepadService::IsServiceRunning()) { > + nsRefPtr<GamepadService> svc = GamepadService::GetService(); > + svc->Update(e); > + } > +} } // anonymous namespace @@ +47,5 @@ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(IsMainProcess()); > + int index = 0; > + while(gIndexSet.Contains(index)) { > + ++index; can you have a static uint32_t gLastIndex ? @@ +61,5 @@ > +RemoveGamepad(uint32_t aIndex) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(IsMainProcess()); > + gIndexSet.Remove(aIndex); hold on! This means that we can reuse the same index? I guess the index should be 'unique'. ::: dom/gamepad/GamepadFunctions.h @@ +12,5 @@ > + > +// Functions for building and transmitting IPDL messages through the HAL > +// sandbox. Used by platform specific Gamepad implementations > +namespace GamepadFunctions > +{ same line for the { ::: dom/gamepad/GamepadMonitoring.cpp @@ +9,5 @@ > +namespace mozilla { > +namespace dom { > +namespace GamepadMonitoring > +{ > + { in the same line @@ +11,5 @@ > +namespace GamepadMonitoring > +{ > + > +bool > +IsMainProcess() anonymous namespace? @@ +24,5 @@ > + MOZ_ASSERT(IsMainProcess()); > + nsTArray<ContentParent*> t; > + ContentParent::GetAll(t); > + for(uint32_t i = 0; i < t.Length(); ++i) { > + if (t[i]->IsGamepadListener()) { Yes, I guess I prefer: t[i]->HasGamepadListener() // or something similar. ::: dom/gamepad/GamepadMonitoring.h @@ +5,5 @@ > +#ifndef mozilla_dom_GamepadMonitoring_h_ > +#define mozilla_dom_GamepadMonitoring_h_ > + > +#include "mozilla/dom/GamepadBinding.h" > +#include "mozilla/dom/PContentParent.h" these 2 are not needed here. @@ +13,5 @@ > + > +// Functions for platform specific gamepad monitoring. > + > +namespace GamepadMonitoring > +{ { in the same line. @@ +15,5 @@ > + > +namespace GamepadMonitoring > +{ > +void MaybeStopGamepadMonitoring(); > +void StartGamepadMonitoring(); where is it implemented? @@ +16,5 @@ > +namespace GamepadMonitoring > +{ > +void MaybeStopGamepadMonitoring(); > +void StartGamepadMonitoring(); > +void StopGamepadMonitoring(); same for this. other patches? ::: dom/gamepad/GamepadService.cpp @@ +50,5 @@ > > +bool > +IsMainProcess() > +{ > + return XRE_GetProcessType() == GeckoProcessType_Default; you actually use this only once. What about if you do: mIsMainProcess = (XRE_GetProcessType() == GeckoProcessType_Default); Otherwise put this into an anonymous namespace or add it to the GamepadFunctions. @@ +90,4 @@ > mTimer->Cancel(); > } > if (mStarted) { > + if (!mIsMainProcess) { NIT: if (A) { ... } else { ... } instead if (!A) { ... } else { ... } you can ignore this comment if you don't like this stylistic suggestion :) @@ +430,3 @@ > return; > } > should this gamepad == aGamepad? Can we assert it? @@ +485,4 @@ > if (aHasSeen) { > aWindow->SetHasSeenGamepadInput(true); > nsCOMPtr<nsISupports> window = ToSupports(aWindow); > + nsRefPtr<Gamepad> gamepad = GetGamepad(aIndex); can we add: MOZ_ASSERT(gamepad) ? @@ +546,5 @@ > +{ > + if (aEvent.type() == GamepadChangeEvent::TGamepadAdded) { > + const GamepadAdded& a = aEvent.get_GamepadAdded(); > + AddGamepad(a.index(), a.id(), > + (GamepadMappingType)a.mapping(), static_cast<GamepadMappingType>(a.mapping()), @@ +553,5 @@ > + const GamepadRemoved& a = aEvent.get_GamepadRemoved(); > + RemoveGamepad(a.index()); > + } else if (aEvent.type() == GamepadChangeEvent::TGamepadButtonInformation) { > + const GamepadButtonInformation& a = aEvent.get_GamepadButtonInformation(); > + // TODO: We currently have no way to get analog button values? What do you mean? Are you planning to do a follow-up for this? ::: dom/gamepad/GamepadService.h @@ +49,1 @@ > uint32_t aNumButtons, uint32_t aNumAxes); indentation @@ +110,5 @@ > // true when shutdown has begun > bool mShuttingDown; > > + // true if we're not the main process and need to use IPC > + bool mIsMainProcess; This can be in the private section. @@ +127,4 @@ > > // Gamepads connected to the system. Copies of these are handed out > // to each window. > + nsRefPtrHashtable<nsUint32HashKey, mozilla::dom::Gamepad > mGamepads; no additional space before > ::: dom/ipc/ContentChild.cpp @@ +2596,5 @@ > +bool > +ContentChild::RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent) > +{ > +#ifdef MOZ_GAMEPAD > + nsRefPtr<GamepadService> svc(GamepadService::GetService()); can this getter fail? Should we do: if (svc) { srv->Update(aGamepadEvent); } ::: dom/ipc/ContentParent.cpp @@ +4727,5 @@ > } > > +bool > +ContentParent::RecvGamepadListenerAdded() > +{ What about if we have more listeners? I see 2 options: 1. we support it, then mHasGameListener should be a uint32_t and count them. 2. we don't and we kill the child process: if (mIsGameListener) { NS_WARNING("No more than 1 gamepad listener."); return false; } I guess, just because the listeners are the services, we should support just 1 listener per ContentParent. And in case we have more than 1, kill the child process returning false. @@ +4737,5 @@ > +} > + > +bool > +ContentParent::RecvGamepadListenerRemoved() > +{ same here: if (!mIs/HasGamepadListeners) { NS_WARNING("A good warning message."); return false; } ::: dom/ipc/ContentParent.h @@ +350,4 @@ > > virtual bool RecvFinishShutdown() MOZ_OVERRIDE; > > + bool IsGamepadListener() const { return mIsGamepadListener; } you know english better than me, but... should it be "hasGamepadListener" ? @@ +796,4 @@ > virtual bool RecvPDocAccessibleConstructor(PDocAccessibleParent* aDoc, > PDocAccessibleParent* aParentDoc, const uint64_t& aParentID) MOZ_OVERRIDE; > > + virtual bool RecvGamepadListenerAdded(); override @@ +796,5 @@ > virtual bool RecvPDocAccessibleConstructor(PDocAccessibleParent* aDoc, > PDocAccessibleParent* aParentDoc, const uint64_t& aParentID) MOZ_OVERRIDE; > > + virtual bool RecvGamepadListenerAdded(); > + virtual bool RecvGamepadListenerRemoved(); override your tree is not up-to-date. Finally we removed MOZ_OVERRIDE and MOZ_FINAL and we have 'override' and 'final' everywhere. @@ +843,4 @@ > bool mSendDataStoreInfos; > bool mIsForBrowser; > bool mIsNuwaProcess; > + bool mIsGamepadListener; Initialize this to false in the CTOR. ::: dom/ipc/PContent.ipdl @@ +341,5 @@ > void_t; > }; > > +struct GamepadAdded { > + nsString id; PContent.ipdl is a bit a mess from the indentation point of view, but I guess it should be 4 spaces.

(In reply to Andrea Marchesini (:baku) from comment #36) > @@ +61,5 @@ > > +RemoveGamepad(uint32_t aIndex) > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + MOZ_ASSERT(IsMainProcess()); > > + gIndexSet.Remove(aIndex); > > hold on! This means that we can reuse the same index? I guess the index > should be 'unique'. The spec allows (and endorses) reusing indices from content's perspective. Whether it makes sense to use those internally is a different question, I guess.

Yeah, I think I took the index reuse a bit /too/ far. We need to reuse indexes per content process/tab/whatever, but not internally. So we need to map external indexes to internal indexes in the content processes. Not a huge problem to fix.

(In reply to Andrea Marchesini (:baku) from comment #36) > ::: dom/gamepad/GamepadFunctions.cpp > @@ +10,5 @@ > > +#include "mozilla/unused.h" > > + > > +namespace mozilla { > > +namespace dom { > > +namespace GamepadFunctions { > > namespace { Ok, maybe I'm missing something here and in GamepadMonitoring, but I don't think I can make this an unnamed namespace. The namespace is used explicitly in the platform-specific files in later patches. Not to mention, using namespace { } seems weird in a header if I don't include the code there, since each unnamed namespace name will be unique and therefore won't keep declarations and definitions together. > @@ +15,5 @@ > > + > > +namespace GamepadMonitoring > > +{ > > +void MaybeStopGamepadMonitoring(); > > +void StartGamepadMonitoring(); > > where is it implemented? These are implemented in the platform specific patches, which I didn't include in this patch (they're in patch 4). They handle generation of the platform specific service for gamepad updates. I've added a comment to make this clearer. > @@ +553,5 @@ > > + const GamepadRemoved& a = aEvent.get_GamepadRemoved(); > > + RemoveGamepad(a.index()); > > + } else if (aEvent.type() == GamepadChangeEvent::TGamepadButtonInformation) { > > + const GamepadButtonInformation& a = aEvent.get_GamepadButtonInformation(); > > + // TODO: We currently have no way to get analog button values? > > What do you mean? Are you planning to do a follow-up for this? Wow, ok, I guess that's something that changed from between when I first wrote this patch in late 2013 and now? I don't actually remember why I made that comment. Anyways, this is wrong now, we can actually send analog button values over, even though we only do on windows at the moment. I filed a follow up at bug 1152967, but will fix this also, and add tests that make sure we do send over analog values when possible (all current mochis only send bool values).

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #39) > Wow, ok, I guess that's something that changed from between when I first > wrote this patch in late 2013 and now? I don't actually remember why I made > that comment. Anyways, this is wrong now, we can actually send analog button > values over, even though we only do on windows at the moment. I filed a > follow up at bug 1152967, but will fix this also, and add tests that make > sure we do send over analog values when possible (all current mochis only > send bool values). As I stated in the other bug, we also support analog buttons on MacOS.

Oh, one more thing on the review: > @@ +430,3 @@ > > return; > > } > > > > should this gamepad == aGamepad? Can we assert it? No, we can't assert this. aGamepad is a clone of gamepad, since each nsGlobalWindow has its own copies of the gamepad array.

> Ok, maybe I'm missing something here and in GamepadMonitoring, but I don't > think I can make this an unnamed namespace. The namespace is used explicitly > in the platform-specific files in later patches. Not to mention, using > namespace { } seems weird in a header if I don't include the code there, I didn't read the other patches, and yes, anonymous namespace was a suggestion just for the cpp files and just for IsMainProcess function. Good for the rest!

Gamepad indexing for content access now happens at the nsGlobalWindow level, meaning each inner window can have unique indexes for Gamepads if required. For internal consistency, I now have a monotonically increasing uint32_t sitting in GamepadFunctions, that we increment on every gamepad addition. So, if someone connects 4 billion gamepads after connecting their first one, we might have a problem, but otherwise we're fine. I also fixed the issue with not passing analog button values. They're now passed to content, and the TODO comment has been removed. I've updated the mochitests in Patch 5 to test both internal and external indexes, and everything seems to pass. baku, could you take a quick look over this again and re-r+ if it's ok? Just want to make sure my new indexing scheme is sane.

Updates to mochitests for testing new indexing scheme and passing of analog button values.

Haven't done a try run in a while. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f8f1c37d13

try round 2, fixing compilation error on OS X and hopefully without test bug we picked up from m-i earlier. https://treeherder.mozilla.org/#/jobs?repo=try&revision=166d79d8ee85

Not sure if this should block e10s uplift.

If we don't fix this before uplifting e10s the Gamepad API will stop working.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #48) > If we don't fix this before uplifting e10s the Gamepad API will stop working. I should clarify - by "uplift" I meant aurora uplift. How many people use the "gamepad" api? I seriously doubt there are that many uses out there. Obviously we have to fix our games apis under e10s, but does this block aurora uplift? I'm guessing that it shouldn't. Also, we were told that games would be doing their demos with e10s off for the foreseeable future due to perf issues that need to be worked out under e10s.

Comment on attachment 8568921 [details] [diff] [review] Patch 1 (v1) - Move HAL gamepad files to dom/gamepad Funny, this originally lived under dom/system but I moved it to hal. :)

Comment on attachment 8569515 [details] [diff] [review] Patch 4 (v3) - Update Gamepad Platform Specifics for IPC Review of attachment 8569515 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks nice, thanks! ::: dom/gamepad/android/AndroidGamepad.cpp @@ +8,3 @@ > namespace mozilla { > +namespace dom { > +namespace GamepadMonitoring { This namespace feels extraneous, especially given the fact that it's duplicated in the function names.

I assume you're going to fold some of those patches for landing?

Comment on attachment 8590977 [details] [diff] [review] Patch 5 (v4) - Mochitest updates for IPC Gamepad API Review of attachment 8590977 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/gamepad/test_check_timestamp.html @@ +12,5 @@ > <script class="testbody" type="text/javascript"> > SimpleTest.waitForExplicitFinish(); > + > +// We need to start the child-side gamepad service if it hasn't been started > +// already, so add event listener first. Can we not fix GamepadServiceTest to handle this internally? Feels a little crummy to have to munge tests for this. ::: dom/tests/mochitest/gamepad/test_navigator_gamepads.html @@ +36,5 @@ > } > window.addEventListener("gamepadconnected", connecthandler); > window.addEventListener("gamepaddisconnected", disconnecthandler); > // Add a gamepad > +var internal_index1 = GamepadService.addGamepad("test gamepad 1", // id Boy, that makes writing tests a PITA, doesn't it? @@ +42,5 @@ > + 4, // buttons > + 2);// axes > +var external_index1 = 0; > +var internal_index2; > +var external_index2 = 1; Maybe call these "content_index" instead of external_index?

Comment on attachment 8590975 [details] [diff] [review] Patch 3 (v4) - IPC Implementation of Gamepad API Review of attachment 8590975 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +13359,4 @@ > nsGlobalWindow::AddGamepad(uint32_t aIndex, Gamepad* aGamepad) > { > MOZ_ASSERT(IsInnerWindow()); > + // Create the index we will present to the user based on which indices are "to content" @@ +13359,5 @@ > nsGlobalWindow::AddGamepad(uint32_t aIndex, Gamepad* aGamepad) > { > MOZ_ASSERT(IsInnerWindow()); > + // Create the index we will present to the user based on which indices are > + // already taken. You could also put a link to the spec text here, since this is required per spec: https://w3c.github.io/gamepad/gamepad.html#widl-Gamepad-index ::: dom/base/nsGlobalWindow.h @@ +1593,1 @@ > nsRefPtrHashtable<nsUint32HashKey, mozilla::dom::Gamepad> mGamepads; I can't remember if there's a reason this is a hashtable and not just an nsTArray. It seems to have always been that way in my original patches, but maybe it could be changed now and you wouldn't need mGamepadIndexSet? I suspect it might have predated WebIDL DOM bindings and been due to some quirk of XPIDL nonsense. ::: dom/gamepad/GamepadService.h @@ +45,3 @@ > void RemoveListener(nsGlobalWindow* aWindow); > > // Add a gamepad to the list of known gamepads, and return its index. The comment here is wrong now.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #54) > ::: dom/base/nsGlobalWindow.h > @@ +1593,1 @@ > > nsRefPtrHashtable<nsUint32HashKey, mozilla::dom::Gamepad> mGamepads; > > I can't remember if there's a reason this is a hashtable and not just an > nsTArray. It seems to have always been that way in my original patches, but > maybe it could be changed now and you wouldn't need mGamepadIndexSet? > > I suspect it might have predated WebIDL DOM bindings and been due to some > quirk of XPIDL nonsense. For the sake of at least getting this landed and out of the way, I'm just filing a followup for this since it at least works right now. Bug 1156424

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #33) > (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment > #27) > > Right now, these tests are still only non-e10s. I've gotta figure out a way > > to get GamepadServiceTest calls back to the main process. May file a > > followup bug for that. > > I think the simplest way to do this would be to use > SpecialPowers.loadChromeScript, which loads a script for you in the chrome > process and gives you a proxy with a MessageManager API so you can > communicate with it. You should be able to do that and then have message > listeners in the chrome script to poke GamepadServiceTest for you. Here's an > example of loadChromeScript: > https://hg.mozilla.org/mozilla-central/annotate/dcc86f78d75e/b2g/chrome/ > content/test/mochitest/RecordingStatusHelper.js#l25 > > and the corresponding chrome script: > https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/test/ > mochitest/RecordingStatusChromeScript.js > > Thanks for getting back to this, I'll try to give these patches a once-over > tomorrow. Filed followup for this at Bug 1156957

Comment on attachment 8590975 [details] [diff] [review] Patch 3 (v4) - IPC Implementation of Gamepad API Review of attachment 8590975 [details] [diff] [review]: ----------------------------------------------------------------- lgtm! ::: dom/gamepad/GamepadFunctions.cpp @@ +42,5 @@ > + uint32_t aNumButtons, uint32_t aNumAxes) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default); > + extra spaces. ::: dom/gamepad/GamepadService.cpp @@ +115,4 @@ > } > > if (!mStarted && mEnabled) { > + if (XRE_GetProcessType() != GeckoProcessType_Default) { if (XRE_GetProcessType() == GeckoProcessType_Default) { StartGameMonitoring(); } else { ... } @@ +500,4 @@ > } > > if (self->mListeners.Length() == 0) { > + if (XRE_GetProcessType() != GeckoProcessType_Default) { if (XRE_GetProcessType() == GeckoProcessType_Default) { MaybeStopGamepadMonitoring(); } else { ... } ::: dom/gamepad/GamepadService.h @@ +6,4 @@ > #define mozilla_dom_GamepadService_h_ > > #include <stdint.h> > +#include "mozilla/dom/Gamepad.h" Can you do a forwarded declaration of Gamepad? @@ +57,4 @@ > // aPressed is used for digital buttons, aValue is for analog buttons. > void NewButtonEvent(uint32_t aIndex, uint32_t aButton, bool aPressed, > double aValue); > + remove these extra spaces. @@ +124,4 @@ > > // Gamepads connected to the system. Copies of these are handed out > // to each window. > + nsRefPtrHashtable<nsUint32HashKey, mozilla::dom::Gamepad> mGamepads; remove mozilla::dom::

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e32ef88878cf Push (merged all patches into 1): https://hg.mozilla.org/integration/mozilla-inbound/rev/45bbfe5ef9b2

Merged final patch

Thanks for making this happen, Kyle!

Verified Gamepad API support with e10s enabled by using latest Developer Edition 42, across platforms [1]. Used various test pages [2] with a Logitech Gamepad F310 and a Xbox 360 controller. Only 1 potential issue encountered during testing - under OS X starting with Nightly 2015-04-25 (e10s enabled/disabled), when I connect both controllers, only one of them is recognized (issue not reproducible with Nightly 2015-04-24, e10s on). Kyle, any idea? Is this issue already tracked by you? [1] Windows 7 x64, Ubuntu 14.04 x32 and Mac OS X 10.10.4 [2] http://luser.github.io/gamepadtest/; http://people.mozilla.com/~tmielczarek/combat/; http://html5gamepad.com/

Hmm, no, there's not a bug on this yet. I'll open a new one and take a look.