852944 - [e10s] Add content process support to Gamepad backend

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[(not currently active) Ted Mielczarek]

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.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 2

[(not currently active) Ted Mielczarek]

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

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) (WIP) - Gamepad OOP-izing

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

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v2) - Make Gamepad API work in content processes

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Update mochitests to reflect new Gameapi API signatures

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=0ece05923a3e

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v3) - Make Gamepad API work in content processes

Fixed platform includes, removed unneeded ipdl

Comment 10

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=9c27f9646799

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Update mochitests to reflect new Gameapi API signatures

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

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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...

Comment 14

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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. :|

Comment 15

[(not currently active) Ted Mielczarek]

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.

Comment 16

[Chris Peterson [:cpeterson]]

This should block e10s uplift to Aurora.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 18

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Move HAL gamepad files to dom/gamepad

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Remove gamepad functions from HAL

Comment 20

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v1) - WIP: IPC Implementation of Gamepad API

Comment 21

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v1) - WIP: Update Gamepad Platform Specifics for IPC

Comment 22

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 5 (v1) - WIP: Mochitest updates for IPC Gamepad API

Comment 23

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 24

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v2) - Remove gamepad functions from HAL

Fixed issue with windows not being able to find HAL_LOG

Comment 25

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v2) - IPC Implementation of Gamepad API

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

Comment 26

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v2) - Update Gamepad Platform Specifics for IPC

Comment 27

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 5 (v2) - Mochitest updates for IPC Gamepad API

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.

Comment 28

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v3) - Remove gamepad functions from HAL

Removed stub function additions from HalInternal.

Comment 29

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v3) - IPC Implementation of Gamepad API

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

Comment 30

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 4 (v3) - Update Gamepad Platform Specifics for IPC

Removed a bunch of unneeded includes

Comment 31

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 5 (v3) - Mochitest updates for IPC Gamepad API

Removed unneeded includes

Comment 32

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 33

[(not currently active) Ted Mielczarek]

(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.

Comment 34

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 35

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Any chance this can get a review soon?

Comment 36

[Andrea Marchesini [:baku]]

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.

Comment 37

[(not currently active) Ted Mielczarek]

(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.

Comment 38

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 39

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(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).

Comment 40

[Randall Barker [:rbarker]]

(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.

Comment 41

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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.

Comment 42

[Andrea Marchesini [:baku]]

> 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!

Comment 43

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 3 (v4) - IPC Implementation of Gamepad API

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.

Comment 44

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 5 (v4) - Mochitest updates for IPC Gamepad API

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

Comment 45

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Haven't done a try run in a while.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f8f1c37d13

Comment 46

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 47

[Jim Mathies [:jimm]]

Not sure if this should block e10s uplift.

Comment 48

[(not currently active) Ted Mielczarek]

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

Comment 49

[Jim Mathies [:jimm]]

(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 50

[(not currently active) Ted Mielczarek]

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 51

[(not currently active) Ted Mielczarek]

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.

Comment 52

[(not currently active) Ted Mielczarek]

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

Comment 53

[(not currently active) Ted Mielczarek]

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 54

[(not currently active) Ted Mielczarek]

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.

Comment 55

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(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

Comment 56

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(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 57

[Andrea Marchesini [:baku]]

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::

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/45bbfe5ef9b2

Comment 59

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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

Comment 60

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch (FINAL) - Gamepad OOP API; r=ted r=baku

Merged final patch

Comment 61

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45bbfe5ef9b2

Comment 62

[(not currently active) Ted Mielczarek]

Thanks for making this happen, Kyle!

Comment 63

[Ada [:adalucinet]]

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/

Comment 64

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

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