1221730 - Move gamepad API to PBackground

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

The gamepad API can be pretty chatty with events fired thru the main thread. It'd be nice if we could move all that I/O out to PBackground.

I'm listing this as a mentored bug 'cause I don't /think/ it will be a lot of work and might be a nice way to introduce someone else to PBackground usage, but I could be wrong. If anyone is interested in taking it, having at least a background in IPC/IPDL and how threads work is pretty much a requirement. ni? me if you think that's you. Otherwise I might pick this up when I have time myself.

Comment 1

[Michael Leu[:Lenzak](UTC+8)]

Hi Kyle

I am interested in this bug and PBackground usage.

I traced the current Gamepad API implementation, the IPDL is inside PContent.
The PContentChild calls PContentParent to invoke GamepadMonitoring and PContentParent calls PContentChild to forward gamepad events from OS to GamepadService.

To move the communication to PBackground, I have some preliminary thought.

1. Make an IPDL for gamepad api managed by PBackground, constructor is in parent side.
(I named it GamepadEventChannel) It will look like it.
parent:
   QueryStartMonitoring()
Child:
   NotifyGamepadChange(GamepadEvent e)

2. Make GamepadService additionally inherit nsIIPCBackgroundChildCreateCallback.

3. Call GetOrCreateForCurrentThread(this) when AddListener is first called in some nsGlobalWindow,
   and the original StartMonitoring() will be always invoked by SendQueryStartMonitoring() in GamepadEventChannelChild 

4. Inside ActorCreated(PBackgroundChild *child), create GamepadEventChannelChild and request PBackgroundParent to create GamepadEventChannelParent.

5. GamepadEventChannelParent will add itself in an additional array in GamepadFunction.cpp once it's constructed, the array will replace ContentParent::GetAll() for forwarding Gamepad event.

Do you have any suggestion?

Thank you:)

Comment 2

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

This sounds like a good plan. :D

Feel free to assign the bug to yourself, then ni? or r? me as needed for help or reviews.

Comment 3

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

BTW, you may want to merge the patch from bug 1156957 into the branch you're working on. It's the fixes to get gamepad tests working under e10s. It's been waiting for review for months now, I'm going to poke ted about it one more time, but if I don't get anything back from him I may just have you review it, since you're going to be familiar with the code anyways. :)

Comment 4

[Michael Leu[:Lenzak](UTC+8)]

Thanks for your information, I will survey it :D

Moving gamepad IPDL to PBackground is almost done, but I found that the original design of HAL OSX part can only run properly in main thread, so I am finding some workaround to make the "CocoaGamepad.cpp" support runnung in background thread. Explicitly calling CFRunLoopRun() seems to be a good idea, but it is a blocking function, which will cause problem if we are shutting it down :(

Comment 5

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

Oh, wow, this is actually a problem I'm having with WebMIDI too. I need a CFRunLoopRun() call to detect device hotplugging. Still haven't figured out how I was going to handle that. :/

Comment 6

[Michael Leu[:Lenzak](UTC+8)]

I found a workaround and it works in gamepad API.

When device monitoring started, I create a dedicate thread to run CFRunLoop so it will not block background parent. And I additionally record its CFRunLoopRef.
When shutdown is invoked, I can use this CFRunLoopRef to ask the thread to stop by CFRunLoopStop() in background parent.

It may be a clumsy approach, and Apple claims it is "generally"(not always) thread safe, but I haven't come accross any problem so far.

Comment 7

[Michael Leu[:Lenzak](UTC+8)]

I forked gecko-dev on github to push my branch, you can see my code here :)
https://github.com/subsevenx2001/gecko-dev/tree/gamepad-pbackground

I am now testing it in other platforms

Comment 8

[Michael Leu[:Lenzak](UTC+8)]

Uhm....
The HAL Windows part can only run in main thread, otherwise it will just crash immediately.
I have removed the part that crashes firefox, now I am trying to make it work again :/

Comment 9

[Michael Leu[:Lenzak](UTC+8)]

After some survey on Windows API(DirectInput and XInput),
the current HAL Windows part needs major change to run in non-main thread.
It not only utilizes nsIObserver which can only run in main thread but also depend on some Windows API like CreateWindowW which is not designed to run in non-main thread.

I'm not sure whether it's feasible to change this part.....

Any suggestion?

Comment 10

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

Agh, sorry about this, I had no idea the windows side was going to be this nasty to get off the main thread. I /think/ it's possible to move to a pure off-main-thread XInput system, get rid of the CreateWindow call by polling for devices through XInput, and find some other timer that doesn't use nsIObserver. That said, that sounds like a decent sized project in itself. 

I'm ni?'ing Ted on this to see if he has any thoughts, even though it's been almost 3 years since he wrote the XInput stuff. Ted, any input about moving the windows gamepad backend off the main thread? Don't know if you'd thought about this at all. If not or you don't have time, that's fine, just let me know.

Comment 11

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify WindowsGamepad.cpp to support running in non-main thread.

This patch removed all nsITimer and nsIObserver things since they either refuse to work or just crash if we are not in main thread :/

I used nsRunnable and Sleep to emulate original behaviour

Both XInput and RawInput are tested in Windows 10 with my PBackground branch

Any suggestion? :)

Comment 12

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug1221730: Move gamepad API to PBackground

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #10)
> I'm ni?'ing Ted on this to see if he has any thoughts, even though it's been
> almost 3 years since he wrote the XInput stuff. Ted, any input about moving
> the windows gamepad backend off the main thread? Don't know if you'd thought
> about this at all. If not or you don't have time, that's fine, just let me
> know.

The XInput calls should be fine on any thread, IIRC, it's already on a background thread because there's no event-based API for it (which sucks). From some brief Googling it looks like the raw input calls should be OK on a background thread, we're already creating our own window to handle the messages.

Comment 14

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

Comment on attachment 8713083 [details] [diff] [review]
Modify WindowsGamepad.cpp to support running in non-main thread.

Review of attachment 8713083 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/gamepad/windows/WindowsGamepad.cpp
@@ +436,5 @@
>  }
>  
> +class XInputMonitoringRunnable : public nsRunnable {
> +public:
> +	XInputMonitoringRunnable() {}

Nit: looks like your indentation width is off here?

@@ +442,5 @@
> +
> +	NS_IMETHOD Run() {
> +		while (gService->isXInputMonitoring) {
> +			gService->PollXInput();
> +			Sleep(kXInputPollInterval);

Instead of sleeping here, just post a new runnable to the current thread, delaying the task by the idle time.

@@ +464,5 @@
> +		isXInputMonitoring = true;
> +		if (!mXInputMonitorThread) {
> +			NS_NewThread(getter_AddRefs(mXInputMonitorThread), new XInputMonitoringRunnable());
> +		}
> +		else {

nit: } else {

Comment 15

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

Ok, I made a few comments on the patch, but overall the idea looks good! Feel free to put it in for review alongside the PBackground patch.

Comment 16

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Move Gamepad API to PBackground (de-nitted) v1

Comment 17

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify WindowsGamepad.cpp to support running in non-main thread (de-nitted and modify XInput Poll loop) v1

Comment 18

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Move Gamepad API to PBackground (de-nitted) v1

Comment 19

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify WindowsGamepad.cpp to support running in non-main thread (de-nitted and modify XInput Poll loop) v1

Comment 20

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug1221730: Move gamepad API to PBackground (remove redundant files) v2

Comment 21

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify WindowsGamepad.cpp to support running in non-main thread (remove redundant files )

Comment 22

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730: Move Gamepad API to PBackground (de-nitted)

Comment 23

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify RawInput and XInput to support non-main thread (de-nitted)

Comment 24

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8716632 [details] [diff] [review]
Bug 1221730: Move Gamepad API to PBackground (de-nitted)

Review of attachment 8716632 [details] [diff] [review]:
-----------------------------------------------------------------

The patch is finally done.
De-nitting is quite hard :/

Comment 25

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

(In reply to Chih-Yi Leu[:lenzak800] from comment #24)
> The patch is finally done.
> De-nitting is quite hard :/

Thanks for getting the patches done! And if if you get nits that seem like they're harder to fix than just a nit, say something in the bug. That's why we mark those that way, 'cause it /seems/ easy to the reviewer but they may be missing something. :)

Comment 26

[Michael Leu[:Lenzak](UTC+8)]

Those nits I've mentioned is actually nits generate by myself when I am modifying codes.
(I should not trust code format function in Eclipse. :/)

When I make the first patches, I found that lots of original codes are messed up because of my carelessness :(

So the "de-nit" is to recover those messed up codes while keep my changes consistent with original coding style.

Comment 27

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

Comment on attachment 8716632 [details] [diff] [review]
Bug 1221730: Move Gamepad API to PBackground (de-nitted)

Review of attachment 8716632 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, looking good so far! I'm r-'ing right now because due to changes that happened in the past week or so, this won't compile anymore when rebased to the head of central (see ipdl function comments). Should be quick to fix up though. Right now, it's that and a whole bunch of code style nits. I'm going to be reviewing the technical changes more while you get these fixed up. :)

::: dom/gamepad/GamepadFunctions.cpp
@@ +15,5 @@
>  namespace mozilla {
>  namespace dom {
>  namespace GamepadFunctions {
>  
> +SendGamepadUpdateRunnable::SendGamepadUpdateRunnable(GamepadEventChannelParent *aParent, GamepadChangeEvent aEvent){

Nit: { on separate line in member function definitions, here and below.

@@ +21,5 @@
> +  mEvent = aEvent;
> +}
> +
> +SendGamepadUpdateRunnable::~SendGamepadUpdateRunnable(){
> +

Nit: remove extra line

@@ +24,5 @@
> +SendGamepadUpdateRunnable::~SendGamepadUpdateRunnable(){
> +
> +}
> +
> +NS_IMETHODIMP SendGamepadUpdateRunnable::Run(){

nit: return type on own line

@@ +48,2 @@
>    }
> +

Nit: remove extra space

::: dom/gamepad/GamepadFunctions.h
@@ +22,5 @@
> +  virtual ~SendGamepadUpdateRunnable();
> +private:
> +  GamepadEventChannelParent *mParent;
> +  GamepadChangeEvent mEvent;
> +};

Neither this nor the variables below need to be in the header. You should be able to move them to the cpp file, just to keep things a little cleaner.

@@ +52,5 @@
>  // indexes.
>  void ResetGamepadIndexes();
>  
> +nsTArray<GamepadEventChannelParent*> mParents;
> +nsCOMPtr<nsIThread> backgroundThread;

Nit: When moving these to the cpp file, both should have a "g" prefix (i.e. gParents, gBackgroundThread), which just means they're global at compilation unit namespace scope.

::: dom/gamepad/GamepadService.cpp
@@ +102,4 @@
>  }
>  
>  void
> +

nit: remove extra line

::: dom/gamepad/cocoa/CocoaGamepad.cpp
@@ +230,5 @@
> +public:
> +  DarwinGamepadServiceStartupRunnable(DarwinGamepadService *service);
> +  NS_IMETHOD
> +  Run() override;
> +  private:

nit: indentation

::: dom/gamepad/ipc/GamepadEventChannelChild.cpp
@@ +1,1 @@
> +#include "GamepadEventChannelChild.h"

nit: Needs a license header

@@ +3,5 @@
> +
> +using namespace mozilla;
> +using namespace dom;
> +
> +GamepadUpdateRunnable::GamepadUpdateRunnable(const GamepadChangeEvent& aGamepadEvent) {

Nit: { on own line, here and below

@@ +7,5 @@
> +GamepadUpdateRunnable::GamepadUpdateRunnable(const GamepadChangeEvent& aGamepadEvent) {
> +  mEvent = aGamepadEvent;
> +}
> +
> +NS_IMETHODIMP GamepadUpdateRunnable::Run() {

nit: return type on own line

::: dom/gamepad/ipc/GamepadEventChannelChild.h
@@ +1,1 @@
> +#include "mozilla/dom/PGamepadEventChannelChild.h"

Nit: needs a license header

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp
@@ +1,1 @@
> +#include "GamepadEventChannelParent.h"

nit: needs a license header

@@ +6,5 @@
> +
> +using namespace mozilla;
> +using namespace dom;
> +
> +GamepadEventChannelParent::GamepadEventChannelParent(){

nit: { on own line, here and below

@@ +18,5 @@
> +
> +bool
> +GamepadEventChannelParent::RecvGamepadListenerAdded()
> +{
> +    if (mHasGamepadListener) {

nit: 2 space indentation, here and below

::: dom/gamepad/ipc/GamepadEventChannelParent.h
@@ +1,1 @@
> +#include "mozilla/dom/PGamepadEventChannelParent.h"

nit: Needs a license header

::: dom/gamepad/ipc/PGamepadEventChannel.ipdl
@@ +1,1 @@
> +include protocol PBackground;

Nit: Needs a license header

@@ +2,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +	struct GamepadAdded {

Nit: Indentation should be 2 spaces, here and below.

@@ +37,5 @@
> +
> +	protocol PGamepadEventChannel{
> +		manager PBackground;
> +		parent:
> +			GamepadListenerAdded();

This won't compile now, due to changes from last week or so. We no longer default to "async" when no specifier is used to denote the synchronicity of an IPC function, so you need to prefix all your IPC messages with "async" now. (I gotta go do this on WebMIDI too :) )

::: dom/ipc/ContentParent.cpp
@@ +269,4 @@
>  
>  #include "VRManagerParent.h"            // for VRManagerParent
>  
> +

nit: remote extra line

::: dom/ipc/ContentParent.h
@@ +475,4 @@
>  
>    void MaybeInvokeDragSession(TabParent* aParent);
>  
> +

nit: remove extra lines, here and below

::: dom/ipc/PContent.ipdl
@@ +670,4 @@
>       */
>      async UpdateWindow(uintptr_t aChildId);
>  
> +    

nit: remove extra whitespace, here and below

::: ipc/glue/BackgroundChildImpl.cpp
@@ +451,5 @@
> +/*
> + * Gamepad API Background IPC
> + */
> +dom::PGamepadEventChannelChild*
> +BackgroundChildImpl::AllocPGamepadEventChannelChild(){

nit: { on own line

::: ipc/glue/BackgroundParentImpl.cpp
@@ +742,5 @@
> +/*
> + * Gamepad API Background IPC
> + */
> +dom::PGamepadEventChannelParent*
> +BackgroundParentImpl::AllocPGamepadEventChannelParent(){

nit: { on own line, here and below

@@ +757,4 @@
>  } // namespace ipc
>  } // namespace mozilla
>  
> +

nit: remove added whitespace

::: ipc/glue/PBackground.ipdl
@@ +67,4 @@
>    PBackgroundIDBFactory(LoggingInfo loggingInfo);
>  
>    PBackgroundIndexedDBUtils();
> +  

nit: remove whitespace

Comment 28

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

Also: This patch is going to break all of the current mochitests (run "mach mochitest dom/tests/mochitest/gamepad" and watch it just time out :) ), so we're going to have to land fixed mochitests alongside it. I'm going to see what I can do about getting these tests fixed up, as I already had a partial solution over in bug 1156957, just need to see if I can make it work with this.

Comment 29

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

Comment on attachment 8716633 [details] [diff] [review]
Modify RawInput and XInput to support non-main thread (de-nitted)

Review of attachment 8716633 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed and question about sleep answered

::: dom/gamepad/windows/WindowsGamepad.cpp
@@ +94,4 @@
>  
>  class WindowsGamepadService;
>  WindowsGamepadService* gService = nullptr;
> +nsCOMPtr<nsIThread> monitorThread;

nit: prefix variable name with g.

@@ +94,5 @@
>  
>  class WindowsGamepadService;
>  WindowsGamepadService* gService = nullptr;
> +nsCOMPtr<nsIThread> monitorThread;
> +bool isEnd = false;

nit: Should be static, prefix variable with s (i.e. sIsEnd)

@@ +426,5 @@
> +class XInputMonitoringRunnable : public nsRunnable {
> +public:
> +  XInputMonitoringRunnable() {}
> +  ~XInputMonitoringRunnable() {}
> +  

nit: remove extra whitespace

@@ +835,2 @@
>  {
> +  Sleep(kDevicesChangedStableDelay);

Is blocking here ok, versus just scheduling a runnable at this time? Didn't know if it's interfere with raw input.

@@ +984,5 @@
> +  isEnd = true;
> +  monitorThread->Dispatch(new StopWindowsGamepadServiceRunnable(), NS_DISPATCH_NORMAL);
> +  monitorThread->Shutdown();
> +  monitorThread = nullptr;
> +  

nit: remove extra whitespace

Comment 30

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

Comment on attachment 8716633 [details] [diff] [review]
Modify RawInput and XInput to support non-main thread (de-nitted)

Review of attachment 8716633 [details] [diff] [review]:
-----------------------------------------------------------------

Forgot to actually mark it, heh.

Comment 31

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground (de-nitted v2)

Oh... Sorry about so many nits.....:(

I rechecked my patch and rewrote some of the runnable declarations to make code looks cleaner.

ps. For the Sleep issue in WindowsGamepad.cpp, I currently have no Windows mahchines to test so I don't know whether scheduling a runnable to current thread is feasible or not. I will test it when I return to Office.

Comment 32

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground (de-nitted v2)

Comment 33

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify RawInput and XInput to support non-main thread (de-nitted v2) r=qdot

About the Sleep part, the thread runs a message loop in StartWindowsGamepadServiceRunnable which never return until the whole gamepad service ends, any pending runnables will not be executed as expect.

Comment 34

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

Just wanted to let you know, I'm buried under some other bugs right now but I'm trying to get back to this review ASAP.

Comment 35

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground v3

Minor changes:

1. Add namespace comment in GamepadEventChannelChild.h
2. Remove redundant whitespace in GamepadService::ActorCreated()
3. Add static_cast to DeallocPGamepadEventChannelParent in BackgroundParentImpl.cpp
4. GamepadEventChannelParent will remove itself from gParent when deallocated

Comment 36

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

Comment on attachment 8723375 [details] [diff] [review]
Bug 1221730 Move Gamepad API to PBackground v3

Review of attachment 8723375 [details] [diff] [review]:
-----------------------------------------------------------------

First off, even more apologies for taking so long on this. Never again will I say "Oh I'll just finish this NPAPI thing then I'll do reviews". :(

Anyways, on to the review! This is looking pretty good contentwise, most of my comments are mainly about organization/refactoring to make things a little more clear. And I swear I will get to reviews faster on this next round. :)

::: dom/gamepad/GamepadFunctions.cpp
@@ +16,5 @@
>  namespace dom {
>  namespace GamepadFunctions {
>  
> +nsTArray<GamepadEventChannelParent*> gParents;
> +nsCOMPtr<nsIThread> gBackgroundThread;

I really only had this as a namespace in the first place because there wasn't state other than the gamepad index. This is gaining enough state that I think I might be more comfortable if we actually turn this into a the usual singleton-ish class, the same way we do the GamepadService? I realize that's mostly the same as what is already here, but that way we can ensure initialization, have shutdown observers to make sure the thread is cleaned up correctly no matter what, etc, versus relying on the gamepad service shutdown chain. I think it will also clean up how references happen elsewhere in the API.

@@ +31,5 @@
> +                            mEvent(aEvent) {}
> +  virtual ~SendGamepadUpdateRunnable() {}
> +  NS_IMETHOD Run()
> +  {
> +    Unused << mParent->SendGamepadUpdate(mEvent);

Since we don't own the GamepadEventChannelParent pointer here, what happens if mParent is cleaned up before this runnable runs? We probably need some sort of validity check, otherwise we'll be accessing a destroyed parent, which would be bad.

::: dom/gamepad/GamepadService.cpp
@@ +159,1 @@
>      StartCleanupTimer();

I disabled the cleanup timer on e10s to get the tests working. I think we can potentially remove it altogether and just shut down directly, now that we're moving to PBackground.

::: dom/gamepad/GamepadService.h
@@ +119,5 @@
>    // true when shutdown has begun
>    bool mShuttingDown;
>  
> +  //Gamepad IPDL child
> +  static GamepadEventChannelChild *gChild;

Nit: This should probably be sChild, not gChild. 

Also, does this need to be static? We should only have one gamepadservice per process (either the one in the parent process for non-e10s, or one per content process for e10s), and one event child per content process, so the lifetime of the gamepad service will dictate the lifetime of the channel, right? I may be misunderstanding this, so please correct me if I am.

::: dom/gamepad/cocoa/CocoaGamepad.cpp
@@ +508,5 @@
> +
> +void DarwinGamepadService::Startup()
> +{
> +  Unused << NS_NewThread(getter_AddRefs(mMonitorThread),
> +          new DarwinGamepadServiceStartupRunnable(this));

nit: indentation

::: dom/gamepad/ipc/GamepadEventChannelChild.cpp
@@ +25,5 @@
> +};
> +
> +GamepadEventChannelChild::GamepadEventChannelChild() {}
> +
> +GamepadEventChannelChild::~GamepadEventChannelChild() {}

nit: You can just put nop constructor/destructors in the header.

::: dom/gamepad/ipc/PGamepadEventChannel.ipdl
@@ +12,5 @@
> +  uint32_t mapping;
> +  uint32_t num_buttons;
> +  uint32_t num_axes;
> +};
> +  

nit: remove extra whitespace

::: dom/ipc/PContent.ipdl
@@ +669,4 @@
>       * a Windows specific call.
>       */
>      async UpdateWindow(uintptr_t aChildId);
> +    

nit: remove extra whitespace, here and below

::: ipc/glue/PBackground.ipdl
@@ +66,4 @@
>  
>    PBackgroundIDBFactory(LoggingInfo loggingInfo);
>  
> +  PBackgroundIndexedDBUtils();  

nit: remove extra whitespace

Comment 37

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground v4

1. Change GamepadFunctions into a singleton class.

2. Remove whitespaces

ps. nsIObserver can only run in main thread, so I think it still needs the shutdown chain

Comment 38

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8732876 [details] [diff] [review]
Bug 1221730 Move Gamepad API to PBackground v4

Review of attachment 8732876 [details] [diff] [review]:
-----------------------------------------------------------------

1. Change GamepadFunctions into a singleton class.

2. Remove redundant whitespaces

3. Remove cleanup timer in GamepadService

ps. nsIObserver can only run in main thread, so I think it still needs the shutdown chain

Comment 39

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground v5

Minor Changes:

1. Completely remove nsITimer in GamepadService while last version didn't remove it in header

2. Some coding style nit fix

Comment 40

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

Comment on attachment 8734231 [details] [diff] [review]
Bug 1221730 Move Gamepad API to PBackground v5

Review of attachment 8734231 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! Just got a few more changes.

::: dom/gamepad/GamepadFunctions.cpp
@@ +39,5 @@
> +
> +// static
> +GamepadFunctions*
> +GamepadFunctions::GetService()
> +{

Add a check that we're in the ParentProcess here. Otherwise we could create services in content processes.

Also, do we possibly want to check whether we're currently shutting down the browser here? We probably would just want to return null in that case, but I'm not sure if that'd be a useful check since this is out on the PBackground thread as is.

@@ +72,3 @@
>    GamepadAdded a(NS_ConvertUTF8toUTF16(nsDependentCString(aID)), index,
>                   (uint32_t)aMapping, aNumButtons, aNumAxes);
> +  mGamepadIndex++;

The double increment here was an accident that somehow slipped into the code (I thought I took this out in my testing patch, when was the last time you rebased this patch?), so one of these increments can be removed.

::: dom/gamepad/GamepadFunctions.h
@@ +8,4 @@
>  #define mozilla_dom_GamepadFunctions_h_
>  
>  #include "mozilla/dom/GamepadBinding.h"
> +#include "mozilla/dom/GamepadEventChannelParent.h"

Nit: You're only holding/referencing pointers to this, right? So you can just declare it up here and move this include to the cpp file.

@@ +52,5 @@
> +  void RemoveChannelParent(GamepadEventChannelParent* aParent);
> +
> +  void SetBackgroundThread(nsCOMPtr<nsIThread> aThread);
> +
> +  bool StillHasGamepadListener();

Nit: You can just call this "HasGamepadListener"

@@ +61,5 @@
> +  virtual ~GamepadFunctions() {}
> +  template<class T> void NotifyGamepadChange(const T& aInfo);
> +
> + private:
> +  static GamepadFunctions *sGamepadFunctionsSingleton;

So right now, this could possibly live past shutdown, since there's no shutdown observers and this is a raw static pointer. Might want to change this to a StaticAutoPtr<> and then we can use ClearOnShutdown() when it's created?

::: dom/gamepad/GamepadMonitoring.cpp
@@ +17,2 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
> +  GamepadFunctions *functions = GamepadFunctions::GetService();

I realize that currently we can't have a gamepad functions services that's a nullptr, but we should do a validity check here and elsewhere anyways just in case.

::: dom/gamepad/GamepadService.h
@@ +20,3 @@
>  // Needed for GamepadMappingType
>  #include "mozilla/dom/GamepadBinding.h"
> +#include "GamepadEventChannelChild.h"

Nit: You're only holding/referencing pointers to this, right? So you can just declare it up here and move this include to the cpp file.

::: dom/gamepad/GamepadServiceTest.cpp
@@ +46,4 @@
>                                 uint32_t aNumAxes,
>                                 uint32_t* aGamepadIndex)
>  {
> +  GamepadFunctions* functions = GamepadFunctions::GetService();

Test pointer validity, here and below.

::: dom/gamepad/cocoa/CocoaGamepad.cpp
@@ +228,5 @@
> +class DarwinGamepadServiceStartupRunnable : public nsRunnable {
> + private:
> +  DarwinGamepadService *mService;
> + public:
> +  DarwinGamepadServiceStartupRunnable(DarwinGamepadService *service) :

MOZ_ASSERT on service pointer validity

@@ +270,4 @@
>                       sizeof(product_name), kCFStringEncodingASCII);
>    char buffer[256];
>    sprintf(buffer, "%x-%x-%s", vendorId, productId, product_name);
> +  GamepadFunctions *functions = GamepadFunctions::GetService();

Check service validity, here and below

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp
@@ +10,5 @@
> +
> +GamepadEventChannelParent::GamepadEventChannelParent() :
> +                           mHasGamepadListener(false)
> +{
> +  GamepadFunctions* functions = GamepadFunctions::GetService();

Check validity, here and below

::: dom/gamepad/linux/LinuxGamepad.cpp
@@ +162,4 @@
>    if (!devpath) {
>      return;
>    }
> +  GamepadFunctions *functions = GamepadFunctions::GetService();

Check validity, here and below

::: dom/gamepad/windows/WindowsGamepad.cpp
@@ +403,4 @@
>      }
>  
>      // Not already present, add it.
> +    GamepadFunctions* functions = GamepadFunctions::GetService();

Check validity, here and below

Comment 41

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground v6

1. Rebase to revision 21d0ee2e6c6a2384e250026444cf36bdec95a768
2. Add parent process check in GamepadFunctions::GetService()
3. Remove redundant increment in GamepadFunctions::AddGamepad()
4. Move GamepadEventChannelParent.h include to GamepadFunctions.cpp with forward declaration
5. Move GamepadEventChannelChild.h include to GamepadService.cpp with forward declaration
6. Change GamepadFunctions::StillHasGamepad() to GamepadFunctions::HasGamepad()
7. Add assertions to check validity of GamepadFunctions*
8. Add assertion to check DarwinGamepadService in DarwinGamepadServiceStartupRunnable::Run()
9. Change sGamepadFunctionsSingleton from raw pointer to StaticAutoPtr<>
10. Change GamepadService to delete its IPC channel in BeginShutdown() to prevent leak

Comment 42

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground v6.1

1. Rebase to revision 21d0ee2e6c6a2384e250026444cf36bdec95a768
2. Add parent process check in GamepadFunctions::GetService()
3. Remove redundant increment in GamepadFunctions::AddGamepad()
4. Move GamepadEventChannelParent.h include to GamepadFunctions.cpp with forward declaration
5. Move GamepadEventChannelChild.h include to GamepadService.cpp with forward declaration
6. Change GamepadFunctions::StillHasGamepad() to GamepadFunctions::HasGamepad()
7. Add assertions to check validity of GamepadFunctions*
8. Add assertion to check DarwinGamepadService in DarwinGamepadServiceStartupRunnable::Run()
9. Change sGamepadFunctionsSingleton from raw pointer to StaticAutoPtr<>
10. Add check validity of GamepadEventChannelChild in GamepadService::BeginShutdown()

Comment 43

[Michael Leu[:Lenzak](UTC+8)]

The patch has been already revised.
However, I found something that may be a leak issue.

I use raw pointers for GamepadEventChannelParent/Child in GamepadFunction and GamepadService,
should I explicitly use Send__delete__() and delete to remove them after shutdown(GamepadService::BeginShutdown())?

If it is not an issue, my patch is actually reviewable now :)

Comment 44

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

(In reply to Michael Leu[:lenzak800] from comment #43)
> The patch has been already revised.
> However, I found something that may be a leak issue.
> 
> I use raw pointers for GamepadEventChannelParent/Child in GamepadFunction
> and GamepadService,
> should I explicitly use Send__delete__() and delete to remove them after
> shutdown(GamepadService::BeginShutdown())?
> 
> If it is not an issue, my patch is actually reviewable now :)

Oh, yes, you should! Glad you caught that. I have to do something similar in webmidi:

https://github.com/qdot/gecko-hg/blob/836897-webmidi/dom/midi/ipc/MIDIPortChild.cpp#L33

(The check for mShuttingDown there is to make sure destruction order is correct)

Comment 45

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move Gamepad API to PBackground v6.2

1. Add Shutdown() in GamepadEventChannelChild to perform Send__delete__()
2. GamepadService will destruct GamepadEventChannelParent/Child on Shutdown

Comment 46

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

Comment on attachment 8737110 [details] [diff] [review]
Bug 1221730 Move Gamepad API to PBackground v6.2

Review of attachment 8737110 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this looks good! I'm r+'ing with one tiny nit. Next up:

- I'm gonna bring in a DOM peer to look at this real quick, since it does involve adding threads and what not.
- We need to update the mochitests before this can land. We can just add another patch to this bug. I'm looking at what that will take right now and will write a followup comment.

Thanks again for doing this!

::: ipc/glue/PBackground.ipdl
@@ +97,4 @@
>  
>    async PQuota();
>  
> +  async PGamepadEventChannel();  

nit: extra whitespace

Comment 47

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

Ok, well, I figured I would go look at WebMIDI to see what I did about testing and see if that works here. Unfortunately that's not going to be the case.

With WebMIDI, we have 2 way communication, so I just set up a "Control" device to take commands that would simulate adding/removing devices, as well as other events. Gamepad is a read-only API right now though (that could change with Rumble, but hasn't even been discussed yet), so we don't have any channels we can use to talk back to gamepad.

To make this work, we could add an extra method somewhere that only works when something like a gamepad.testing pref is true, and use that to request tests. We'd probably need to put this on Navigator, but we can pref block the method in the .webidl file. It can just take a json block to specify what we want to do (add/remove gamepad, press button, etc), as well as the parameters we'll need. Then we'll need to thread it through ipdl and all that so it'll access the GamepadServiceTest the same way we do currently, though now GamepadServiceTest will live on the PBackground thread. 

Does that make sense, and sound reasonable?

Comment 48

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

Comment on attachment 8737110 [details] [diff] [review]
Bug 1221730 Move Gamepad API to PBackground v6.2

Baku, could you possibly take a look at this patch and see if it's ok? I've done a few review rounds before this to try and make it as smooth as possible, but it'd be nice to have a DOM Peer look at it.

If you don't have time, just unassign and I can find someone else.

Comment 49

[Michael Leu[:Lenzak](UTC+8)]

I think it's reasonable to make gamepad mochitest works like webmidi's since they are both APIs about hardware events, but I'm not familiar with mochitest, so my understanding is through your WebMidi mochitest implementation, please correct me if I'm wrong :)

1. We will have a method like "queryGamepadServiceTest" in Navigator, we pass our test content as JavaScript callbacks into SpecialPowers.

2. In implementation of "queryGamepadServiceTest", we will provide corresponding test commands and have GamepadServiceTest run it in background thread through IPDL.

So we will have to add a method in Navigator and a new IPDL for gamepad testing.


BTW, since the patch has a new r?, should I denit the patch now or wait until it get reviewed?

Comment 50

[Andrea Marchesini [:baku]]

Comment on attachment 8737110 [details] [diff] [review]
Bug 1221730 Move Gamepad API to PBackground v6.2

Review of attachment 8737110 [details] [diff] [review]:
-----------------------------------------------------------------

The IPDL part must be updated to be 100% sure that we keep the actors alive correctly. I'm ok to review it again.

::: dom/gamepad/GamepadFunctions.cpp
@@ +10,5 @@
>  #include "mozilla/unused.h"
> +#include "nsIThread.h"
> +#include "mozilla/dom/GamepadEventChannelParent.h"
> +#include "mozilla/ipc/BackgroundParent.h"
> +#include "mozilla/ClearOnShutdown.h"

alphabetic order.

@@ +18,2 @@
>  
> +class SendGamepadUpdateRunnable : public nsRunnable

final?

@@ +24,5 @@
> + public:
> +  SendGamepadUpdateRunnable(GamepadEventChannelParent *aParent,
> +                            GamepadChangeEvent aEvent) :
> +                            mParent(aParent),
> +                            mEvent(aEvent) {}

assert that aParent exists.

@@ +25,5 @@
> +  SendGamepadUpdateRunnable(GamepadEventChannelParent *aParent,
> +                            GamepadChangeEvent aEvent) :
> +                            mParent(aParent),
> +                            mEvent(aEvent) {}
> +  virtual ~SendGamepadUpdateRunnable() {}

move it to private and remove virtual.

@@ +26,5 @@
> +                            GamepadChangeEvent aEvent) :
> +                            mParent(aParent),
> +                            mEvent(aEvent) {}
> +  virtual ~SendGamepadUpdateRunnable() {}
> +  NS_IMETHOD Run()

virtual

@@ +29,5 @@
> +  virtual ~SendGamepadUpdateRunnable() {}
> +  NS_IMETHOD Run()
> +  {
> +    if(mParent) {
> +      Unused << mParent->SendGamepadUpdate(mEvent);

What's prevent mParent to have already been destroyed?
It seems that if you want to use runnables, you must have it refcounted.
And in case ActorDestroyed() or Removed() has been called, you must check it and do not send a Update().

::: dom/gamepad/GamepadMonitoring.cpp
@@ +17,4 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
> +  GamepadFunctions *functions = GamepadFunctions::GetService();
> +  MOZ_ASSERT(functions);
> +  if(functions->HasGamepadListener()) {

If we can have more listeners, make it plural.

::: dom/gamepad/GamepadService.cpp
@@ +11,4 @@
>  #include "mozilla/dom/GamepadButtonEvent.h"
>  #include "mozilla/dom/GamepadEvent.h"
>  #include "mozilla/dom/GamepadMonitoring.h"
> +#include "mozilla/dom/GamepadEventChannelChild.h"

alphabetic order.

@@ +86,5 @@
>  {
>    mShuttingDown = true;
> +  if(mChild) {
> +    mChild->SendGamepadListenerRemoved();
> +    mChild->Shutdown();

This is wrong. Here you are sending a GamepadListenerRemoved() and immediately after a __delete__. What about if the parent actor is sending updates in the meantime?
you call __delete__, the child is deleted, the sending of the message fails and we crash.

What you want to do is something a bit more complex. You want this:

child->SendGamepadListenerRemoved()

then in parent->RecvGamepadListenerRemoved() you call: parent->Send__delete__();

child->ActorDestroyed() is called, and we are happy.
Just to improve this logic, add a boolean mRemoved or something better to know that we are in this intermediate state.

@@ +120,5 @@
> +        ActorCreated(actor);
> +      } else {
> +        MOZ_ASSERT(NS_IsMainThread());
> +        bool ok = BackgroundChild::GetOrCreateForCurrentThread(this);
> +        if (NS_WARN_IF(!ok)) {

MOZ_ASSERT(ok) directly.

@@ +129,3 @@
>      } else {
> +      //If the IPDL child is created, use it immediately
> +      mChild->SendGamepadListenerAdded();

Yeah, it's cleaner and more easy to follow if we have 1 child only when we have 1 or more listeners. We cannot be in this case then.

@@ +152,4 @@
>  
>    mListeners.RemoveElement(aWindow);
>  
> +  if (mListeners.Length() == 0 && !mShuttingDown && mChild) {

IsEmpty() ?

@@ +155,5 @@
> +  if (mListeners.Length() == 0 && !mShuttingDown && mChild) {
> +    if (mShuttingDown) {
> +     return;
> +    }
> +    if (mListeners.Length() == 0) {

IsEmpty()

@@ +156,5 @@
> +    if (mShuttingDown) {
> +     return;
> +    }
> +    if (mListeners.Length() == 0) {
> +      mChild->SendGamepadListenerRemoved();

Don't we want to shutdown the child here?
I think it's cleaner to have 1 child only when we have > 0 listeners.

::: dom/gamepad/GamepadService.h
@@ +77,5 @@
>  
>    // Receive GamepadChangeEvent messages from parent process to fire DOM events
>    void Update(const GamepadChangeEvent& aGamepadEvent);
> +
> +  //Override from nsIIPCBackgroundChildCreateCallback

NS_DECL_NSIIPCBACKGROUNDCHILDCREATECALLBACK ?

::: dom/gamepad/cocoa/CocoaGamepad.cpp
@@ +224,5 @@
>    void Shutdown();
> +  friend class DarwinGamepadServiceStartupRunnable;
> +};
> +
> +class DarwinGamepadServiceStartupRunnable : public nsRunnable {

1. { in a new line.
2. final?

@@ +226,5 @@
> +};
> +
> +class DarwinGamepadServiceStartupRunnable : public nsRunnable {
> + private:
> +  DarwinGamepadService *mService;

MOZ_NON_OWNING_REF ?

Write a comment saying that this runnable is scheduled to run in a new thread and DarwingGamepadService is in charge of it. So it's ok to have a raw pointer here... something like that.

::: dom/gamepad/ipc/GamepadEventChannelChild.cpp
@@ +10,5 @@
> +class GamepadUpdateRunnable : public nsRunnable
> +{
> + public:
> +  GamepadUpdateRunnable(const GamepadChangeEvent& aGamepadEvent) :
> +                        mEvent(aGamepadEvent) {}

GemepadUpdateRunnable(const GamepadChangeEvent& aGamepadEvent)
  : mEvent(aGamepadEvent)
{}

@@ +11,5 @@
> +{
> + public:
> +  GamepadUpdateRunnable(const GamepadChangeEvent& aGamepadEvent) :
> +                        mEvent(aGamepadEvent) {}
> +  NS_IMETHOD Run()

override

::: dom/gamepad/ipc/GamepadEventChannelChild.h
@@ +12,5 @@
> +class GamepadEventChannelChild final : public PGamepadEventChannelChild
> +{
> + public:
> +  GamepadEventChannelChild() {}
> +  virtual ~GamepadEventChannelChild() {}

no virtual for final classes.

@@ +13,5 @@
> +{
> + public:
> +  GamepadEventChannelChild() {}
> +  virtual ~GamepadEventChannelChild() {}
> +  virtual bool RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent) override;

80chars max.

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp
@@ +7,5 @@
> +
> +using namespace mozilla;
> +using namespace dom;
> +
> +GamepadEventChannelParent::GamepadEventChannelParent() :

GamepadEventChannelParent::GamepadEventChannelParent()
  : mHasGamepadListener(false)
{
  ...

@@ +16,5 @@
> +  functions->AddChannelParent(this);
> +}
> +
> +GamepadEventChannelParent::~GamepadEventChannelParent()
> +{

MOZ_ASSERT(!mHasGamepadListener);

@@ +56,5 @@
> +GamepadEventChannelParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  GamepadFunctions* functions = GamepadFunctions::GetService();
> +  MOZ_ASSERT(functions);
> +  functions->RemoveChannelParent(this);

hold on... this can be called if the child process crashes.
Here you want to:

1. set mHasGamepadListener to false.
2. Call MaybeStopGamepadMonitoring()

@@ +60,5 @@
> +  functions->RemoveChannelParent(this);
> +}
> +
> +bool
> +GamepadEventChannelParent::HasGamepadListener()

move it inline.

::: dom/gamepad/ipc/GamepadEventChannelParent.h
@@ +12,5 @@
> +class GamepadEventChannelParent final : public PGamepadEventChannelParent
> +{
> + public:
> +  GamepadEventChannelParent();
> +  virtual ~GamepadEventChannelParent();

no virtual for final DTOR.

@@ +16,5 @@
> +  virtual ~GamepadEventChannelParent();
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> +  virtual bool RecvGamepadListenerAdded() override;
> +  virtual bool RecvGamepadListenerRemoved() override;
> +  bool HasGamepadListener();

bool HasGamepadListener() const;

::: ipc/glue/BackgroundChildImpl.cpp
@@ +26,4 @@
>  #include "mozilla/layout/VsyncChild.h"
>  #include "mozilla/net/PUDPSocketChild.h"
>  #include "mozilla/dom/network/UDPSocketChild.h"
> +#include "mozilla/dom/GamepadEventChannelChild.h"

alphabetic order.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +37,4 @@
>  #include "nsTraceRefcnt.h"
>  #include "nsXULAppAPI.h"
>  #include "ServiceWorkerManagerParent.h"
> +#include "mozilla/dom/PGamepadEventChannelParent.h"

alphabetic order in these headers.

::: ipc/glue/BackgroundParentImpl.h
@@ +186,4 @@
>  
>    virtual bool
>    DeallocPQuotaParent(PQuotaParent* aActor) override;
> +  /*

add an empty line between the pquota block and yours.

::: ipc/glue/PBackground.ipdl
@@ +19,4 @@
>  include protocol PServiceWorkerManager;
>  include protocol PUDPSocket;
>  include protocol PVsync;
> +include protocol PGamepadEventChannel;

alphabetic order here.

@@ +58,4 @@
>    manages PServiceWorkerManager;
>    manages PUDPSocket;
>    manages PVsync;
> +  manages PGamepadEventChannel;

... and here.

Comment 51

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

(In reply to Michael Leu[:lenzak800] from comment #49)
> 1. We will have a method like "queryGamepadServiceTest" in Navigator, we
> pass our test content as JavaScript callbacks into SpecialPowers.
> 
> 2. In implementation of "queryGamepadServiceTest", we will provide
> corresponding test commands and have GamepadServiceTest run it in background
> thread through IPDL.
> 
> So we will have to add a method in Navigator and a new IPDL for gamepad
> testing.

Yup, this is the right idea. After that's in place, we'll just have the tests call that queryGamepadServiceTest function instead of using the chrome script stuff we were doing before.

Comment 52

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v7

Comment 53

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v8

1. Change shutdown order of GamepadEventChannel
2. Remove redundant if-elses in GamepadService
3. Make GamepadChannelParent RefConted
4. Rename GamepadFunctions::HasGamepadListener() to GamepadFunctions::HasGamepadListeners()
5. Denit

ps. Since there is at most one GamepadEventChannelChild in GamepadService, I think it is OK to just shut it down once no DOM listener listening to it. 
So we don't need to add a boolean in GamepadListenerRemoved to specify whether we'll shutdown or not.

Comment 54

[Michael Leu[:Lenzak](UTC+8)]

BTW, what do you think about Kyle's plan to add method for gamepad testing in Navigator in Comment 47?

I think it's quite reasonable but I didn't see others perform tests like this.

Do you have any suggestions? :)

Comment 55

[Andrea Marchesini [:baku]]

> I think it's quite reasonable but I didn't see others perform tests like
> this.


Oh yes! Please do it.
Here an example: HTMLInputElement.webidl has mozSetFileNameArray(). This is chromeOnly.
Write good comments :) and then use it like this: SpecialPowers.wrap(fileList).mozSetFileNameArray(foo);

Navigator seems a good place to have this chromeOnly extra method.

Comment 56

[Michael Leu[:Lenzak](UTC+8)]

I am now trying to implement the test query method in Navigator,
but I'm confused about the design of the method.

According to WebMidi mochitest implementation, I think the webidl should like this:

partial interface Navigator {
  [Pref="dom.gamepad.test.enabled", ChromeOnly]
  Promise<GamepadTester> queryGamepadServiceTest();
};

GamepadTester is a new webidl object which is a Content Process side proxy of GamepadServiceTest that send testing commands to Chrome Process.

And we can use .then()s to perform our test cases as callback functions.

It seems to be different with your plan that put a JSON for test cases in Comment 47, is my understanding correct?

If not, what will be the format of this JSON?

Comment 57

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

Oh, that works too! The JSON idea was just something I was throwing out to make sure my solution description was at least somewhat complete, but doing it through IPDL like you recommend seems easier since we're bridging JS/C++ here.

Only reason I can get away with what I do in WebMIDI is because I'm abusing sysex messages, so I've already got a protocol I can piggyback on. :)

Comment 58

[Michael Leu[:Lenzak](UTC+8)]

Attachment: WIP - Add ContentProcess Proxy for Gamepad mochitest

Comment 59

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v9

I found there are some problems occurs when we are suspending GamepadService under Mochitest, this update changes GamepadService::BeginShutdown() to restart properly.

Comment 60

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Revise GamepadServiceTest as proxy of our test commands in Content Process

Sorry for taking so long.

I found that we don't need a new method in Navigator, we can just add an IPDL which is reverse version of GamepadEventChannel for testing, and make GamepadServiceTest as the DOM side delegate so we can send test commands without heavily modifying current mochitest code while keep away those chrome scripts.

Is this approach OK? :)

Comment 61

[Andrea Marchesini [:baku]]

Comment on attachment 8744212 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v9

Review of attachment 8744212 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/gamepad/GamepadFunctions.cpp
@@ +26,5 @@
> +  RefPtr<GamepadEventChannelParent> mParent;
> +  GamepadChangeEvent mEvent;
> + public:
> +  SendGamepadUpdateRunnable(GamepadEventChannelParent* aParent,
> +                            GamepadChangeEvent aEvent) :

Usually we do:
<newline>
  : mParent(aParent)
  , mEvent(aEvent)
{
  MOZ_ASSERT(aParent);
}

@@ +34,5 @@
> +    mParent = aParent;
> +  }
> +  NS_IMETHOD Run() override
> +  {
> +    if(mParent && mParent->HasGamepadListener()) {

how can mparent be null?

@@ +71,1 @@
>    MOZ_ASSERT(XRE_IsParentProcess());

In which thread is this called? Any?
If yes, what protects mParents to be modified in another thread in the meantime?

@@ +71,4 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
>    GamepadChangeEvent e(aInfo);
> +
> +  for(uint32_t i = 0; i < mParents.Length(); ++i) {

mParents is a super confusing name.

@@ +71,5 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
>    GamepadChangeEvent e(aInfo);
> +
> +  for(uint32_t i = 0; i < mParents.Length(); ++i) {
> +    nsCOMPtr<nsIRunnable> r(new SendGamepadUpdateRunnable(mParents[i].get(), e));

remove .get()

@@ +72,5 @@
>    GamepadChangeEvent e(aInfo);
> +
> +  for(uint32_t i = 0; i < mParents.Length(); ++i) {
> +    nsCOMPtr<nsIRunnable> r(new SendGamepadUpdateRunnable(mParents[i].get(), e));
> +    mBackgroundThread->Dispatch(r, NS_DISPATCH_NORMAL);

What about if you do here:

mParents[i]->DispatchGamepadChangeEvent(e);

and in the parent implementation you do:

mBackgroundThread->Dispatch(...);
You can move mBackgoundThread from here to the Parent class and populate it in the CTOR.

@@ +85,2 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
> +  int index = mGamepadIndex;

same issue as mParent. If we can call this from anythread, what protects mGamepadIndex?

@@ +134,5 @@
> +}
> +
> +void
> +GamepadFunctions::AddChannelParent(GamepadEventChannelParent* aParent)
> +{

1. mParents -> mChannelParents ?
2. in which thread can this method be called?

::: dom/gamepad/GamepadFunctions.h
@@ +17,4 @@
>  
>  // Functions for building and transmitting IPDL messages through the HAL
>  // sandbox. Used by platform specific Gamepad implementations
> +class GamepadFunctions

final?

@@ +19,5 @@
>  // sandbox. Used by platform specific Gamepad implementations
> +class GamepadFunctions
> +{
> + public:
> +  virtual ~GamepadFunctions();

1. if final, remove virtual.
2. can it be private?

::: dom/gamepad/GamepadMonitoring.cpp
@@ +17,1 @@
>    MOZ_ASSERT(XRE_IsParentProcess());

thread?

@@ +17,2 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
> +  GamepadFunctions *functions = GamepadFunctions::GetService();

I suspect that all of this is main-thread only, but I need a confirmation from you.
If it's not, GamepadFunctions doesn't seem to be thread-safe at all.

::: dom/gamepad/GamepadService.cpp
@@ +123,5 @@
> +      //Try to get the PBackground Child actor
> +      if (actor) {
> +        ActorCreated(actor);
> +      } else {
> +        MOZ_ASSERT(NS_IsMainThread());

This is ok, but it's confusing. It seems that only this block is main-thread, the rest can be called on any thread.

::: dom/gamepad/GamepadService.h
@@ +116,5 @@
>    // true when shutdown has begun
>    bool mShuttingDown;
>  
> +  //Gamepad IPDL child
> +  GamepadEventChannelChild *mChild;

1. MOZ_NON_OWNING_REF
2. write a comment about why this is a raw pointer.

::: dom/gamepad/cocoa/CocoaGamepad.cpp
@@ +276,4 @@
>                       sizeof(product_name), kCFStringEncodingASCII);
>    char buffer[256];
>    sprintf(buffer, "%x-%x-%s", vendorId, productId, product_name);
> +  GamepadFunctions *functions = GamepadFunctions::GetService();

GamepadFunctions* functions = ..

@@ +287,5 @@
>  
>  void
>  DarwinGamepadService::DeviceRemoved(IOHIDDeviceRef device)
>  {
> +  GamepadFunctions *functions = GamepadFunctions::GetService();

GamepadFunctions* functions = ..

::: dom/gamepad/ipc/GamepadEventChannelChild.cpp
@@ +26,5 @@
> +
> +bool
> +GamepadEventChannelChild::RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
> +{
> +  NS_DispatchToMainThread(new GamepadUpdateRunnable(aGamepadEvent));

nsresult rv = ...
NS_WARN_IF(NS_FAILED(rv));

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp
@@ +21,5 @@
> +{
> +  mozilla::ipc::AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(!mHasGamepadListener);
> +  mHasGamepadListener = true;
> +  GamepadFunctions *functions = GamepadFunctions::GetService();

* to the left.

@@ +23,5 @@
> +  MOZ_ASSERT(!mHasGamepadListener);
> +  mHasGamepadListener = true;
> +  GamepadFunctions *functions = GamepadFunctions::GetService();
> +  MOZ_ASSERT(functions);
> +  functions->SetBackgroundThread(NS_GetCurrentThread());

Different approach:

instead storing this in the GamepadFunctions singleton, what about if you store it in the GamepadEventChannelParent, in the CTOR.
Then the singleton asks it to dispatch the runnable.

Comment 62

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

Comment on attachment 8744232 [details] [diff] [review]
Revise GamepadServiceTest as proxy of our test commands in Content Process

Review of attachment 8744232 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 63

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify RawInput and XInput to support non-main thread v3

I found that we can set up a Windows message loop in non-blocking manner out of main-thread, so nsITimer will work and we don't need to abuse sleep and multiple runnables anymore.

This patch utilizes PeekMessage instead of GetMessage to achieve an interruptible message loop, so we can schedule XInput polling and Device change stablizer with nsITimer in same thread now.

Since the change is quite big, I think it should be reviewed again.

Comment 64

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v10

1. Correct class ctor format.
2. Add comments and assertion about method in GamepadFunctions.
3. Change mParents to mChannelParents
4. Change GamepadFunctions to final class
    (ps. Its destructor can't be private because it uses StaticAutoPointer)
5. Move SendGamepadUpdateRunnable to GamepadEventChannelParent

Comment 65

[Andrea Marchesini [:baku]]

Comment on attachment 8745852 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v10

Review of attachment 8745852 [details] [diff] [review]:
-----------------------------------------------------------------

good. Some additional fixes to apply.

::: dom/gamepad/GamepadFunctions.cpp
@@ +30,5 @@
> +GamepadFunctions*
> +GamepadFunctions::GetService()
> +{
> +  if(!XRE_IsParentProcess()) {
> +    NS_WARNING("GamepadFunctions can only be used in Parent Process");

MOZ_ASSERT(XRE_IsParentProcess()); ?

@@ +51,2 @@
>    GamepadChangeEvent e(aInfo);
> +

What keeps alive these mChannelParents? What prevents that the Background thread calls RemoveChannelParent() ?

What about if:

1. ChannelParent is refcounted.
2. Any time you touch mChannelParents you use a mutex

@@ +65,3 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  int index = mGamepadIndex;

uint32_t

@@ +65,4 @@
>    MOZ_ASSERT(XRE_IsParentProcess());
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  int index = mGamepadIndex;
> +  mGamepadIndex++;

Probably it's better to avoid index 0. But maybe I'm wrong. What do you think?

@@ +133,5 @@
> +GamepadFunctions::AddChannelParent(GamepadEventChannelParent* aParent)
> +{
> +  // mChannelParents can only be modified once GamepadEventChannelParent
> +  // is created or removed in Background thread
> +  mozilla::ipc::AssertIsOnBackgroundThread();

MOZ_ASSERT(aParent);
MOZ_ASSERT(!mChannelParents.Contains(aParent));

@@ +143,5 @@
> +{
> +  // mChannelParents can only be modified once GamepadEventChannelParent
> +  // is created or removed in Background thread
> +  mozilla::ipc::AssertIsOnBackgroundThread();
> +  mChannelParents.RemoveElement(aParent);

MOZ_ASSERT(aParent);
MOZ_ASSERT(mChannelParent.Contains(aParent);

@@ +147,5 @@
> +  mChannelParents.RemoveElement(aParent);
> +}
> +
> +bool
> +GamepadFunctions::HasGamepadListeners()

should it be: bool GamepadFunctions::HasGamepadListeners() const

@@ +148,5 @@
> +}
> +
> +bool
> +GamepadFunctions::HasGamepadListeners()
> +{

when is it called? Add some assertion :)

@@ +159,5 @@
> +}
> +
> +void
> +GamepadFunctions::Cleanup()
> +{

assertions.

::: dom/gamepad/GamepadFunctions.h
@@ +17,4 @@
>  
>  // Functions for building and transmitting IPDL messages through the HAL
>  // sandbox. Used by platform specific Gamepad implementations
> +class GamepadFunctions final

Ok... more I read the code, and more I think that this class should be called 'GamepadBackendService' or 'GamepadParentService'.
Or... maybe, rename GamepadService to GamepadManager and this to GamepadService.

@@ +21,5 @@
> +{
> + public:
> +  ~GamepadFunctions();
> +  //Get the singleton service
> +  static GamepadFunctions* GetService();

this is definitely a service. GamepadParentService seems a better name.

@@ +56,5 @@
> +
> +  bool HasGamepadListeners();
> +
> + private:
> +  GamepadFunctions() : mGamepadIndex(0) {}

move it in the CPP file.

@@ +61,5 @@
> +  template<class T> void NotifyGamepadChange(const T& aInfo);
> +  void Cleanup();
> +
> +  static StaticAutoPtr<GamepadFunctions> sGamepadFunctionsSingleton;
> +  uint32_t mGamepadIndex;

This GamepadFunctions class is used in multiple threads. I would like to read for each member a comment about in which thread it's touched. mGamepadIndex only on the backend-thread. mChannelParent everywhere (and it must be protected by mutex).

::: dom/gamepad/GamepadService.cpp
@@ +524,5 @@
> +{
> +  MOZ_ASSERT(aActor);
> +  GamepadEventChannelChild *child = new GamepadEventChannelChild();
> +  PGamepadEventChannelChild *initedChild =
> +      aActor->SendPGamepadEventChannelConstructor(child);

2 spaces of indentation.

::: dom/gamepad/GamepadService.h
@@ +13,3 @@
>  #include "nsGlobalWindow.h"
>  #include "nsIFocusManager.h"
> +#include "nsIGamepadServiceTest.h"

why this include?

@@ +24,5 @@
>  namespace dom {
>  
>  class EventTarget;
>  class GamepadChangeEvent;
>  class Gamepad;

alphabetic order.

::: dom/gamepad/ipc/GamepadEventChannelChild.cpp
@@ +6,5 @@
> +
> +using namespace mozilla;
> +using namespace dom;
> +
> +class GamepadUpdateRunnable final : public nsRunnable

rebase your patches. This is called 'Runnable'.

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp
@@ +8,5 @@
> +
> +using namespace mozilla;
> +using namespace dom;
> +
> +class SendGamepadUpdateRunnable final : public nsRunnable

anonymous namespace?

@@ +61,5 @@
> +}
> +
> +void
> +GamepadEventChannelParent::ActorDestroy(ActorDestroyReason aWhy)
> +{

mozilla::ipc::AssertIsOnBackgroundThread();
plus, add namespace mozilla::ipc;

Comment 66

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v11

1. Rename GamepadFunctions to GamepadPlatformService
2. Rename GamepadService to GamepadManager
3. Add more assertions and comments in GamepadPlatformService
4. Add Mutex to protect mChannelParents in GamepadPlatformService
5. Change gamepad start index from 0 to 1
6. Remove redundant includes and clean up code

Comment 67

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

Hoping to get to the windows reviews before the end of the week, hopefully tomorrow.

Comment 68

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v12

Comment 69

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest backend update part1 - Add new IPDL for Gamepad Mochitest.

Comment 70

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest backend update part2 - Make GamepadServiceTest be webidl

Comment 71

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

Comment on attachment 8745849 [details] [diff] [review]
Modify RawInput and XInput to support non-main thread v3

Review of attachment 8745849 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good, just got some issues with the thread init/shutdown.

::: dom/gamepad/windows/WindowsGamepad.cpp
@@ +95,5 @@
>  
>  class WindowsGamepadService;
>  WindowsGamepadService* gService = nullptr;
> +nsCOMPtr<nsIThread> gMonitorThread;
> +static bool sIsEnd = false;

Nit: A better name for this might be sIsShutdown, End sounds a bit final for a service that can come up and down multiple times during a session.

@@ +326,5 @@
>    // Parse gamepad input from a WM_INPUT message.
>    bool HandleRawInput(HRAWINPUT handle);
>  
> +  static void XInputMessageLoopOnceCallback(nsITimer *aTimer, void* aClosure);
> +  static void DevicesChangeCallback(nsITimer *aTimer, void* aClosure);

Nit: These are just services, not closures, right? Should probably just be called aService.

@@ +337,4 @@
>    bool ScanForXInputDevices();
>    bool HaveXInputGamepad(int userIndex);
>  
> +  bool isXInputMonitoring;

nit: mIsXInputMonitoring

@@ +384,5 @@
>  
> +// static
> +void
> +WindowsGamepadService::XInputMessageLoopOnceCallback(nsITimer *aTimer,
> +	                                                   void* aClosure)

nit: indent

@@ +386,5 @@
> +void
> +WindowsGamepadService::XInputMessageLoopOnceCallback(nsITimer *aTimer,
> +	                                                   void* aClosure)
> +{
> +  WindowsGamepadService* self = static_cast<WindowsGamepadService*>(aClosure);

Should probably assert aClosure is not null, here and below

@@ +934,5 @@
> +  ~StartWindowsGamepadServiceRunnable() {}
> +
> +  NS_IMETHOD Run() override
> +  {
> +    gService = new WindowsGamepadService();

Should probably assert that gService is null here (I realize we do this in StartGamepadMonitoring but the assert won't hurt for finding weird threading issues).

@@ +995,2 @@
>  {
>    if (gService) {

We should check for both existence of gService and gMonitorThread, otherwise someone could possibly call this twice in quick succession before the monitor thread has had a chance to create the service, and that would end up in weirdness.

@@ +1006,2 @@
>  {
>    if (!gService) {

Might actually want to check sIsEnd == true here? Otherwise this could get called twice but gMonitorThread may not have had a chance to clean up the service yet, which would once again lead to weirdness.

Comment 72

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Change GamepadServiceTest into WebIDL for mochitest

Comment 73

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Update Mochitest

Comment 74

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify RawInput and XInput to support non-main thread v4

1. Change sIsEnd to sIsShutdown
2. Add AssertIsOnBackgroundThread() in StartGamepadMonitoring and StopGamepadMonitoring
3. Change aClosure to aService
4. Make all runnable classes final
5. Move all dtors of runnables to private scope
6. minor denit

Comment 75

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v13

Fix leak problem in mochiest

Comment 76

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v1

Comment 77

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part2 - Update Mochitest html files

Comment 78

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v13

Comment 79

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v1

Comment 80

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part4: Update Mochitest html files

Comment 81

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8751149 [details] [diff] [review]
Part4: Update Mochitest html files

Review of attachment 8751149 [details] [diff] [review]:
-----------------------------------------------------------------

These 2 patches update the mochitest backend so we can get an GamepadServiceTest instance from Navigator so we can run mochitest without chrmescript

Comment 82

[Andrea Marchesini [:baku]]

Comment on attachment 8751147 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v13

Review of attachment 8751147 [details] [diff] [review]:
-----------------------------------------------------------------

It looks better! But it seems that you didn't apply all my previous comments. Check all of them please.
Here something more. I'll be quicker the next time!

::: dom/gamepad/GamepadManager.cpp
@@ +42,5 @@
> +  "dom.gamepad.non_standard_events.enabled";
> +// Amount of time to wait before cleaning up gamepad resources
> +// when no pages are listening for events.
> +const int kCleanupDelayMS = 2000;
> +const nsTArray<RefPtr<nsGlobalWindow> >::index_type NoIndex =

no space between > and > here and everywhere else.

@@ +43,5 @@
> +// Amount of time to wait before cleaning up gamepad resources
> +// when no pages are listening for events.
> +const int kCleanupDelayMS = 2000;
> +const nsTArray<RefPtr<nsGlobalWindow> >::index_type NoIndex =
> +    nsTArray<RefPtr<nsGlobalWindow> >::NoIndex;

2 spaces.

@@ +49,5 @@
> +StaticRefPtr<GamepadManager> gGamepadManagerSingleton;
> +
> +} // namespace
> +
> +bool GamepadManager::sShutdown = false;

why do you need this? can it be a static variable in the anonymous namespace?

@@ +61,5 @@
> +  mEnabled = IsAPIEnabled();
> +  mNonstandardEventsEnabled =
> +    Preferences::GetBool(kGamepadEventsEnabledPref, false);
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();

this can return null.

@@ +62,5 @@
> +  mNonstandardEventsEnabled =
> +    Preferences::GetBool(kGamepadEventsEnabledPref, false);
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();
> +  observerService->AddObserver(this,

this could fail.

@@ +64,5 @@
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();
> +  observerService->AddObserver(this,
> +                               NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID,
> +                               false);

move all of this in a ::Init() and propagate the error code.

@@ +73,5 @@
> +                        const char* aTopic,
> +                        const char16_t* aData)
> +{
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();

this can return null.

@@ +196,5 @@
> +    NS_WARNING("Trying to delete gamepad with invalid index");
> +    return;
> +  }
> +  gamepad->SetConnected(false);
> +    NewConnectionEvent(aIndex, false);

indent.

@@ +216,5 @@
> +  // can mutate the mListeners array.
> +  nsTArray<RefPtr<nsGlobalWindow>> listeners(mListeners);
> +
> +  for (uint32_t i = listeners.Length(); i > 0 ; ) {
> +    --i;

why here? move it into the for()

MOZ_ASSERT(!listeners.IsEmpty());

for (uint32_t i = listeners.Length() -1; i >= 0; --i) {
 ...

@@ +284,5 @@
> +  // can mutate the mListeners array.
> +  nsTArray<RefPtr<nsGlobalWindow> > listeners(mListeners);
> +
> +  for (uint32_t i = listeners.Length(); i > 0 ; ) {
> +    --i;

ditto.

@@ +354,5 @@
> +  nsTArray<RefPtr<nsGlobalWindow> > listeners(mListeners);
> +
> +  if (aConnected) {
> +    for (uint32_t i = listeners.Length(); i > 0 ; ) {
> +      --i;

ditto.

@@ +382,5 @@
> +  } else {
> +    // For disconnection events, fire one at every window that has received
> +    // data from this gamepad.
> +    for (uint32_t i = listeners.Length(); i > 0 ; ) {
> +      --i;

ditto.

@@ +502,5 @@
> +  if (aEvent.type() == GamepadChangeEvent::TGamepadAdded) {
> +    const GamepadAdded& a = aEvent.get_GamepadAdded();
> +    AddGamepad(a.index(), a.id(),
> +               static_cast<GamepadMappingType>(a.mapping()),
> +               a.num_buttons(), a.num_axes());

I prefer:

if (aEvent.type() == ...) {
  const ...
  AddGamepad(...);
  return;
}

if ( ...

::: dom/gamepad/GamepadManager.h
@@ +8,5 @@
> +#define mozilla_dom_GamepadManager_h_
> +
> +#include <stdint.h>
> +#include "nsAutoPtr.h"
> +#include "nsCOMArray.h"

remove this.

@@ +9,5 @@
> +
> +#include <stdint.h>
> +#include "nsAutoPtr.h"
> +#include "nsCOMArray.h"
> +#include "nsGlobalWindow.h"

remove and forward declaration.

@@ +10,5 @@
> +#include <stdint.h>
> +#include "nsAutoPtr.h"
> +#include "nsCOMArray.h"
> +#include "nsGlobalWindow.h"
> +#include "nsIFocusManager.h"

remove

@@ +13,5 @@
> +#include "nsGlobalWindow.h"
> +#include "nsIFocusManager.h"
> +#include "nsIIPCBackgroundChildCreateCallback.h"
> +#include "nsIObserver.h"
> +#include "nsITimer.h"

remove

@@ +51,5 @@
> +  void RemoveListener(nsGlobalWindow* aWindow);
> +
> +  // Add a gamepad to the list of known gamepads.
> +  void AddGamepad(uint32_t aIndex, const nsAString& aID, GamepadMappingType aMapping,
> +                      uint32_t aNumButtons, uint32_t aNumAxes);

identation.

@@ +137,5 @@
> +  // to each window.
> +  nsRefPtrHashtable<nsUint32HashKey, Gamepad> mGamepads;
> +  // Inner windows that are listening for gamepad events.
> +  // has been sent to that window.
> +  nsTArray<RefPtr<nsGlobalWindow> > mListeners;

nsTArray<RefPtr<nsGlobalWindow>> mListeners;

::: dom/gamepad/GamepadPlatformService.cpp
@@ +46,5 @@
> +  if(!sGamepadPlatformServiceSingleton) {
> +    sGamepadPlatformServiceSingleton = new GamepadPlatformService();
> +  }
> +  if(!sMutex) {
> +    sMutex = new Mutex("mozilla::dom::GamepadPlatformService");

remove this. and make it member of GampadPlatformService. In its CTOR do:

 ...
 , mMutex("mozilla::dom::GamepadPlatformService")
{}

@@ +65,5 @@
> +  GamepadChangeEvent e(aInfo);
> +
> +  // mChannelParents may be accessed by background thread in the
> +  // same time, we use mutex to prevent possible race condtion
> +  MutexAutoLock autoLock(*sMutex);

we must have 1 mutex for each platformService.

@@ +73,5 @@
> +}
> +
> +uint32_t
> +GamepadPlatformService::AddGamepad(const char* aID,
> +                             GamepadMappingType aMapping,

indentation.

@@ +103,5 @@
> +}
> +
> +void
> +GamepadPlatformService::NewButtonEvent(uint32_t aIndex, uint32_t aButton,
> +                                 bool aPressed, double aValue)

indentation everywhere.

Comment 83

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14

Comment 84

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14

Comment 85

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14

Comment 86

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14

Comment 87

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14

Comment 88

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14

Comment 89

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8754201 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v14

Review of attachment 8754201 [details] [diff] [review]:
-----------------------------------------------------------------

1.   Change GamepadPlatformService::GetService() to GamepadPlatformService::GetParentService()
2.   Change <> > to <>> in GamepadManager.cpp/h
3.   Fix indentation error in GamepadManager.cpp and GamepadPlatformService.cpp
4.   Change gGamepadManagerSingleton to sGamepadManagerSingleton and make it a private static member like the singleton in GamepadPlatformService.
5.   Move GamepadManager::sShutdown to static variable in anonymous namespace
6.   Add MOZ_ASSERT() to ensure that mozilla::services::GetObserverService() returns valid service.
7.   Move GamepadManager constructor to Init() and forward the return value from AddObserver().
8.   Change "for(uint32_t i = listeners.Length(); i > 0 ; ) { --i;...}" to "for(uint32_t i = 0; i < listeners.Length(); i++) {...}"
     (ps. the --i; cannot put in the for(;;) statement or it will cause array out of bound, I think make i start from 0 is more readable)
9.   Modify GamepadManager::Update() to use return instead of "else if"
10.  Remove unused headers in GamepadManager.h
     (ps. nsIIPCBackgroundChildCreateCallback.h and nsIObserver.h is necessary so we can use the NS_DECL_... macros)
11.  Change sMutex in mMutex as a member variable in GamepadPlatformService
     (ps. To make this change we have to remove the const modifier on HasGamepadListeners())

Comment 90

[Andrea Marchesini [:baku]]

Comment on attachment 8754201 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v14

Review of attachment 8754201 [details] [diff] [review]:
-----------------------------------------------------------------

This is almost perfect!! And I don't want to see the full patch anymore :)
Can you send me an interdiff with these comments applied? The main reason why I give you r- is because the GamepadEventParent life-time management is not correct yet.
But it's getting closer!

Rename 'functions' to 'service' everywhere.

::: dom/base/nsGlobalWindow.cpp
@@ +9674,4 @@
>  
>    if (mHasGamepad) {
>  #ifdef MOZ_GAMEPAD
> +    RefPtr<GamepadManager> gamepadsvc(GamepadManager::GetService());

s/gamepadsvc/gamepadManager/g

::: dom/gamepad/GamepadManager.cpp
@@ +45,5 @@
> +
> +const nsTArray<RefPtr<nsGlobalWindow>>::index_type NoIndex =
> +  nsTArray<RefPtr<nsGlobalWindow>>::NoIndex;
> +
> +static bool sShutdown = false;

no static in anonymous namespace.

@@ +50,5 @@
> +
> +} // namespace
> +
> +StaticRefPtr<GamepadManager>
> +GamepadManager::sGamepadManagerSingleton;

This can go in the anonymous namespace as well as:

StaticRef<GamepadManager> sGamepadManagerSingleton;

@@ +58,5 @@
> +GamepadManager::GamepadManager()
> +  : mShuttingDown(false),
> +    mChild(nullptr)
> +{
> +  nsresult rv = Init();

Remove the init from here... move it to GetService().

@@ +70,5 @@
> +  mNonstandardEventsEnabled =
> +    Preferences::GetBool(kGamepadEventsEnabledPref, false);
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();
> +  MOZ_ASSERT(observerService);

This is really aggressive :)
Do:

if (NS_WARN_IF(!observerService)) {
  return NS_ERROR_FAILURE;
}

@@ +75,5 @@
> +
> +  nsresult rv;
> +  rv = observerService->AddObserver(this,
> +                                    NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID,
> +                                    false);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

return NS_OK;

@@ +85,5 @@
> +                        const char* aTopic,
> +                        const char16_t* aData)
> +{
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();

if (observerService) {
  observerService->RemoveObserver(...)
}

@@ +128,5 @@
> +
> +  if (mListeners.IndexOf(aWindow) != NoIndex) {
> +    return; // already exists
> +  }
> +

can you do:

mListeners.AppendElement(aWindow);

if (!mEnabled) {
  return;
}

if (mChild) {
  return;
}

PBackgroundChild *actor = ...

@@ +169,5 @@
> +  }
> +}
> +
> +already_AddRefed<Gamepad>
> +GamepadManager::GetGamepad(uint32_t aIndex)

GamepadManager::GetGamepad(uint32_t aIndex) const

@@ +196,5 @@
> +                aNumButtons,
> +                aNumAxes);
> +
> +  // We store the gamepad related to its index given by the parent process.
> +  mGamepads.Put(aIndex, gamepad);

What about if aIndex is already set? Do we care to handle this corner case?

@@ +216,5 @@
> +
> +void
> +GamepadManager::NewButtonEvent(uint32_t aIndex, uint32_t aButton, bool aPressed,
> +                               double aValue)
> +{

Check mShuttingDown before calling GetGamepad(aIndex).

@@ +240,5 @@
> +      continue;
> +    }
> +
> +    bool first_time = false;
> +    if (!WindowHasSeenGamepad(listeners[i], aIndex)) {

Create a helper method called:

bool MaybeWindowHasSeenGamepad(listener, index) with this block of code and call it here:

bool firstTime = MaybeWindowHasSeenGamepad(listeners[i], aIndex);

@@ +267,5 @@
> +                                Gamepad* aGamepad,
> +                                uint32_t aButton,
> +                                double aValue)
> +{
> +  nsString name = aValue == 1.0L ? NS_LITERAL_STRING("gamepadbuttondown") :

nsAutoString name = ...

@@ +287,5 @@
> +void
> +GamepadManager::NewAxisMoveEvent(uint32_t aIndex, uint32_t aAxis, double aValue)
> +{
> +  RefPtr<Gamepad> gamepad = GetGamepad(aIndex);
> +  if (mShuttingDown || !gamepad) {

same here.

@@ +308,5 @@
> +      continue;
> +    }
> +
> +    bool first_time = false;
> +    if (!WindowHasSeenGamepad(listeners[i], aIndex)) {

bool firstTime = MaybeWindowHasSeenGamepad(listeners[i], aIndex);

@@ +357,5 @@
> +GamepadManager::NewConnectionEvent(uint32_t aIndex, bool aConnected)
> +{
> +  RefPtr<Gamepad> gamepad = GetGamepad(aIndex);
> +
> +  if (mShuttingDown || !gamepad) {

ditto.

@@ +436,5 @@
> +void
> +GamepadManager::SyncGamepadState(uint32_t aIndex, Gamepad* aGamepad)
> +{
> +  RefPtr<Gamepad> gamepad = GetGamepad(aIndex);
> +  if (mShuttingDown || !mEnabled || !gamepad) {

ditto.

@@ +459,5 @@
> +    return nullptr;
> +  }
> +
> +  if (!sGamepadManagerSingleton) {
> +    sGamepadManagerSingleton = new GamepadManager();

Here you should do:


RefPtr<GamepadManager> manager = new GamepadManager();
nsresult rv = manager->Init();
if (NS_WARN_IF(NS_FAILED(rv))) {
  return nullptr;
}

sGamepadManagerSingleton = manager;
ClearOnShutdown(&sGamepadManagerSingleton);

@@ +473,5 @@
> +  return Preferences::GetBool(kGamepadEnabledPref, false);
> +}
> +
> +bool
> +GamepadManager::WindowHasSeenGamepad(nsGlobalWindow* aWindow, uint32_t aIndex)

GamepadManager::WindowHasSeenGamepad(nsGlobalWindow* aWindow, uint32_t aIndex) const

@@ +496,5 @@
> +  if (aHasSeen) {
> +    aWindow->SetHasSeenGamepadInput(true);
> +    nsCOMPtr<nsISupports> window = ToSupports(aWindow);
> +    RefPtr<Gamepad> gamepad = GetGamepad(aIndex);
> +    MOZ_ASSERT(gamepad);

remove this assertion or remove if(!gamepad) return;

::: dom/gamepad/GamepadManager.h
@@ +21,5 @@
> +class Gamepad;
> +class GamepadChangeEvent;
> +class GamepadEventChannelChild;
> +
> +class GamepadManager : public nsIObserver,

final

@@ +74,5 @@
> +  void Update(const GamepadChangeEvent& aGamepadEvent);
> +
> + protected:
> +  GamepadManager();
> +  virtual ~GamepadManager() {};

remove virtual if this class is final.

@@ +107,5 @@
> +  bool mNonstandardEventsEnabled;
> +  // true if the platform-specific backend has started work
> +  bool mStarted;
> +  // true when shutdown has begun
> +  bool mShuttingDown;

All of these must be initialized in the CTOR and they are not.

::: dom/gamepad/GamepadMonitoring.cpp
@@ +16,5 @@
>  void
>  MaybeStopGamepadMonitoring()
>  {
> +  AssertIsOnBackgroundThread();
> +  GamepadPlatformService* functions =

s/functions/service/g

::: dom/gamepad/GamepadPlatformService.cpp
@@ +20,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +StaticAutoPtr<GamepadPlatformService>
> +GamepadPlatformService::sGamepadPlatformServiceSingleton;

anonymous namespace as the GamepadManager one.

@@ +75,5 @@
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  MOZ_ASSERT(mGamepadIndex > 0);
> +
> +  uint32_t index = mGamepadIndex;
> +  mGamepadIndex++;

Instead doing this, what about if mGamepadIndex is 0 by default. And then you do:

uint32_t index = ++mGamepadIndex; ?

@@ +136,5 @@
> +  // This method is called by monitor thread populated in
> +  // platform-dependent backends
> +  MOZ_ASSERT(XRE_IsParentProcess());
> +  MOZ_ASSERT(!NS_IsMainThread());
> +  mGamepadIndex = 1;

0 :)

@@ +190,5 @@
> +{
> +  // This method is invoked in MaybeStopGamepadMonitoring when
> +  // an IPDL channel is going to be destroyed
> +  AssertIsOnBackgroundThread();
> +

mutex here!

@@ +203,5 @@
> +  // This method is called when GamepadPlatformService is
> +  // successfully shutdown in background thread
> +  AssertIsOnBackgroundThread();
> +
> +  mChannelParents.Clear();

mutex here!

::: dom/gamepad/GamepadPlatformService.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_GamepadFunctions_h_

Functions?

@@ +16,5 @@
> +namespace dom {
> +
> +class GamepadEventChannelParent;
> +
> +// Functions for building and transmitting IPDL messages through the HAL

Functions?

::: dom/gamepad/GamepadServiceTest.cpp
@@ +46,4 @@
>                                 uint32_t aNumAxes,
>                                 uint32_t* aGamepadIndex)
>  {
> +  GamepadFunctions* functions = GamepadFunctions::GetService();

Functions? :)

::: dom/gamepad/cocoa/CocoaGamepad.cpp
@@ +276,4 @@
>                       sizeof(product_name), kCFStringEncodingASCII);
>    char buffer[256];
>    sprintf(buffer, "%x-%x-%s", vendorId, productId, product_name);
> +  GamepadPlatformService* functions =

functions?

::: dom/gamepad/ipc/GamepadEventChannelChild.cpp
@@ +11,5 @@
> +
> +class GamepadUpdateRunnable final : public Runnable
> +{
> + public:
> +  GamepadUpdateRunnable(const GamepadChangeEvent& aGamepadEvent)

explicit

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp
@@ +62,5 @@
> +bool
> +GamepadEventChannelParent::RecvGamepadListenerRemoved()
> +{
> +  AssertIsOnBackgroundThread();
> +  MOZ_ASSERT(mHasGamepadListener);

You should call RemoveChannelParent(this) here. Otherwise here a race condition:

1. child->SendGamepadListenerRemoved()
2. Parent->RecvGamepadListenerRemoved();
3. parent->Send_some_update();
4. child crashes.

So what you have to do here is:

bool GamepadEventChannelParent::RecvGamepadListenerRemoved()
{
...
mHasGamepadListener = false;
service->RemoveChannelParent(this);
Send__delete__();
}

Read my next comment in ActorDestroy()

@@ +74,5 @@
> +  mHasGamepadListener = false;
> +  GamepadPlatformService* functions =
> +    GamepadPlatformService::GetParentService();
> +  MOZ_ASSERT(functions);
> +  functions->RemoveChannelParent(this);

It can happen that the child crashes. In this case, you don't receive the RecvGamepadListenerRemoved().
Herer you have to do:

if (!mDisabled) {
  mHasGamepadListener = false;
  service->RemoveChannelParent(this);
}

There is another cornercase! What about if ActorDestroy() or RecvGamepadListenerRemoved() has been called but we already have a runnable dispatched?
Because you set mHasGamepadListener to false, the SendUpdateRunnable will work correctly! So we are happy. Do you agree?

::: dom/gamepad/windows/WindowsGamepad.cpp
@@ +442,4 @@
>      }
>  
>      // Not already present, add it.
> +    GamepadPlatformService* functions =

functions?

Comment 91

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

Comment on attachment 8751138 [details] [diff] [review]
Modify RawInput and XInput to support non-main thread v4

Review of attachment 8751138 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with comments addressed.

::: dom/gamepad/windows/WindowsGamepad.cpp
@@ +940,3 @@
>  
> +  NS_IMETHOD Run() override
> +  {

Might put an assert in here to make sure current thread is gMonitorThread.

@@ +975,5 @@
> + public:
> +  StopWindowsGamepadServiceRunnable() {}
> +
> +  NS_IMETHOD Run() override
> +  {

Might put an assert in here to make sure current thread is gMonitorThread.

@@ +1004,5 @@
> +StartGamepadMonitoring()
> +{
> +  AssertIsOnBackgroundThread();
> +
> +  if (gMonitorThread && gService) {

I think this should probably be an ||, not a &&? Could have the thread created without having created the service yet?

Comment 92

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

Comment on attachment 8751148 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v1

Review of attachment 8751148 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but this will also need a DOM peer review since it adds new webidl. Poking baku for that.

Comment 93

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14 to 15 interdiff

Comment 94

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14 to 15 interdiff

Comment 95

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14 to 15 interdiff

Comment 96

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8754740 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v14 to 15 interdiff

Review of attachment 8754740 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsGlobalWindow.cpp
@@ +9674,5 @@
>  
>    if (mHasGamepad) {
>  #ifdef MOZ_GAMEPAD
> +    RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());
> +    if (gamepadManager) {

gamepadsvc --> gamepadManager

@@ +9689,4 @@
>  
>    if (mHasGamepad) {
>  #ifdef MOZ_GAMEPAD
> +    RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());

gamepadsvc --> gamepadManager

@@ +13172,4 @@
>  {
>    MOZ_ASSERT(IsInnerWindow());
>    if (mHasSeenGamepadInput) {
> +    RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());

gamepadsvc --> gamepadManager

::: dom/gamepad/GamepadManager.cpp
@@ +50,2 @@
>  
> +StaticRefPtr<GamepadManager> gGamepadManagerSingleton;

Remove static modifier and move singleton in anonymous namespace

@@ +54,5 @@
>  
>  NS_IMPL_ISUPPORTS(GamepadManager, nsIObserver)
>  
>  GamepadManager::GamepadManager()
> +  : mEnabled(false),

Complete the initialization list

@@ +70,5 @@
>    nsCOMPtr<nsIObserverService> observerService =
>      mozilla::services::GetObserverService();
> +
> +  if (NS_WARN_IF(!observerService)) {
> +    return NS_ERROR_FAILURE;

Use return error code instead of a MOZ_ASSERT failure

@@ +79,5 @@
>                                      NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID,
>                                      false);
> +
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;

Add warning if we failed

@@ +93,5 @@
>  {
>    nsCOMPtr<nsIObserverService> observerService =
>      mozilla::services::GetObserverService();
> +  if (observerService) {
> +    observerService->RemoveObserver(this, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID);

Simply check if observerService is valid instead of trigger an assertion fail

@@ +129,5 @@
>    MOZ_ASSERT(aWindow);
>    MOZ_ASSERT(aWindow->IsInnerWindow());
>    MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!mEnabled || mShuttingDown) {

Bail out if we're shutting down or the API is not enabled

@@ +141,5 @@
>    mListeners.AppendElement(aWindow);
> +
> +  // IPDL child has been created
> +  if (mChild) {
> +    return;

Change the order of IPDL creation and listener adding

@@ +178,4 @@
>  }
>  
>  already_AddRefed<Gamepad>
> +GamepadManager::GetGamepad(uint32_t aIndex) const

Add const modifier

@@ +205,4 @@
>                  aNumAxes);
>  
>    // We store the gamepad related to its index given by the parent process.
> +  MOZ_ASSERT(!mGamepads.Get(aIndex, nullptr));

Assert that the destination index must be unset

@@ +227,5 @@
>  GamepadManager::NewButtonEvent(uint32_t aIndex, uint32_t aButton, bool aPressed,
>                                 double aValue)
>  {
> +  if (mShuttingDown) {
> +    return;

Return before call GetGamepad

@@ +253,4 @@
>        continue;
>      }
>  
> +    bool firstTime = MaybeWindowHasSeenGamepad(listeners[i], aIndex);

Use helper method here

@@ -243,5 @@
> -    bool first_time = false;
> -    if (!WindowHasSeenGamepad(listeners[i], aIndex)) {
> -      // This window hasn't seen this gamepad before, so
> -      // send a connection event first.
> -      SetWindowHasSeenGamepad(listeners[i], aIndex);

This block of code has been move to a helper method MaybeWindowsHasSeenGamepad()

@@ +295,5 @@
>  GamepadManager::NewAxisMoveEvent(uint32_t aIndex, uint32_t aAxis, double aValue)
>  {
> +  if (mShuttingDown) {
> +    return;
> +  }

Return before call GetGamepad

@@ +319,4 @@
>        continue;
>      }
>  
> +    bool firstTime = MaybeWindowHasSeenGamepad(listeners[i], aIndex);

Use helper method here

@@ -312,5 @@
> -    if (!WindowHasSeenGamepad(listeners[i], aIndex)) {
> -      // This window hasn't seen this gamepad before, so
> -      // send a connection event first.
> -      SetWindowHasSeenGamepad(listeners[i], aIndex);
> -      first_time = true;

This part will become a helper method

@@ +444,4 @@
>  void
>  GamepadManager::SyncGamepadState(uint32_t aIndex, Gamepad* aGamepad)
>  {
> +  if (mShuttingDown || !mEnabled) {

Return before call GetGamepad

@@ +472,5 @@
>    }
>  
> +  if (!gGamepadManagerSingleton) {
> +    RefPtr<GamepadManager> manager = new GamepadManager();
> +    nsresult rv = manager->Init();

Call Init() in GetService() instead of in constructor so we can return nullptr if we failed

@@ +500,5 @@
> +    SetWindowHasSeenGamepad(aWindow, aIndex);
> +    return true;
> +  }
> +  return false;
> +}

We move it as a helper method to determine whether the gamepad is first seen or not

@@ -497,4 @@
>      aWindow->SetHasSeenGamepadInput(true);
>      nsCOMPtr<nsISupports> window = ToSupports(aWindow);
>      RefPtr<Gamepad> gamepad = GetGamepad(aIndex);
> -    MOZ_ASSERT(gamepad);

Remove redundant assertion

::: dom/gamepad/GamepadManager.h
@@ +22,5 @@
>  class GamepadChangeEvent;
>  class GamepadEventChannelChild;
>  
> +class GamepadManager final : public nsIObserver,
> +                             public nsIIPCBackgroundChildCreateCallback

Add final modifier

@@ +68,4 @@
>    void SyncGamepadState(uint32_t aIndex, Gamepad* aGamepad);
>  
>    // Returns gamepad object if index exists, null otherwise
> +  already_AddRefed<Gamepad> GetGamepad(uint32_t aIndex) const;

Add const modifier

@@ +74,5 @@
>    void Update(const GamepadChangeEvent& aGamepadEvent);
>  
>   protected:
>    GamepadManager();
> +  ~GamepadManager() {};

Remove virtual since it's a final class now

@@ -105,5 @@
>    bool mEnabled;
>    // true if non-standard events are enabled in preferences
>    bool mNonstandardEventsEnabled;
> -  // true if the platform-specific backend has started work
> -  bool mStarted;

This variable is not used anymore after we move gamepad API backend to PBackground

::: dom/gamepad/GamepadMonitoring.cpp
@@ +17,4 @@
>  MaybeStopGamepadMonitoring()
>  {
>    AssertIsOnBackgroundThread();
> +  GamepadPlatformService* service =

functions --> service

::: dom/gamepad/GamepadPlatformService.cpp
@@ +27,2 @@
>  StaticAutoPtr<GamepadPlatformService>
> +gGamepadPlatformServiceSingleton;

Move singleton into anonymous namespace

@@ +82,2 @@
>  
> +  uint32_t index = ++mGamepadIndex;

Change mGamepadIndex's initial value 0, but increase before we assign it to a gamepad

@@ +199,5 @@
> +  bool isChannelParentEmpty;
> +  {
> +    MutexAutoLock autoLock(mMutex);
> +    isChannelParentEmpty = mChannelParents.IsEmpty();
> +  }

Mutex lock
ps. we cannot lock when we are release the singleton or we will get deadlocked

@@ +214,3 @@
>    AssertIsOnBackgroundThread();
>  
> +  MutexAutoLock autoLock(mMutex);

This is called when we release the singleton, we get deadlocked if we have locked it before calling

::: dom/gamepad/GamepadPlatformService.h
@@ +4,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef mozilla_dom_GamepadPlatformService_h_

Rename unrenamed header guard

@@ +17,4 @@
>  
>  class GamepadEventChannelParent;
>  
> +// Platform Service for building and transmitting IPDL messages

Rename unrenamed comment

::: dom/gamepad/GamepadServiceTest.cpp
@@ +47,5 @@
>                                 uint32_t* aGamepadIndex)
>  {
> +  Gamepadservice* service = Gamepadservice::GetService();
> +  MOZ_ASSERT(service);
> +  *aGamepadIndex = service->AddGamepad(aID,

functions  --> service

::: dom/gamepad/cocoa/CocoaGamepad.cpp
@@ +276,4 @@
>                       sizeof(product_name), kCFStringEncodingASCII);
>    char buffer[256];
>    sprintf(buffer, "%x-%x-%s", vendorId, productId, product_name);
> +  GamepadPlatformService* service =

functions --> service

::: dom/gamepad/ipc/GamepadEventChannelChild.cpp
@@ +12,4 @@
>  class GamepadUpdateRunnable final : public Runnable
>  {
>   public:
> +  explicit GamepadUpdateRunnable(const GamepadChangeEvent& aGamepadEvent)

Add explicit modifier

::: dom/gamepad/ipc/GamepadEventChannelParent.cpp
@@ +42,4 @@
>  GamepadEventChannelParent::GamepadEventChannelParent()
>    : mHasGamepadListener(false)
>  {
> +  GamepadPlatformService* service =

functions --> service

@@ +67,5 @@
> +  mHasGamepadListener = false;
> +  GamepadPlatformService* service =
> +    GamepadPlatformService::GetParentService();
> +  MOZ_ASSERT(service);
> +  service->RemoveChannelParent(this);

We remove IPDL parent here to prevent potential race condition

@@ +79,5 @@
> +  AssertIsOnBackgroundThread();
> +
> +  // It may be called because IPDL child side crashed, we'll
> +  // not receive RecvGamepadListenerRemoved in that case
> +  if (mHasGamepadListener) {

Add case thar if it's called because IPDL child is crashed

::: dom/gamepad/linux/LinuxGamepad.cpp
@@ +134,4 @@
>             name);
>  
>    char numAxes = 0, numButtons = 0;
> +  GamepadPlatformService* service =

functions --> service

::: dom/gamepad/windows/WindowsGamepad.cpp
@@ +442,4 @@
>      }
>  
>      // Not already present, add it.
> +    GamepadPlatformService* service =

functions --> service

Comment 97

[Andrea Marchesini [:baku]]

Comment on attachment 8751148 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v1

Review of attachment 8751148 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Bindings.conf
@@ +506,5 @@
>      'wrapperCache': False,
>  },
>  
> +'GamepadServiceTest': {
> +    'headerFile': 'GamepadServiceTest.h'

move it to mozilla/dom/ and remove this block.

::: dom/gamepad/GamepadServiceTest.cpp
@@ +56,5 @@
> +
> +GamepadServiceTest::~GamepadServiceTest() {}
> +
> +void
> +GamepadServiceTest::InitGamepadTestChannel()

you are not initialize the GamepadTestChannel. Rename this to InitPBackgroundActor ?

@@ +71,4 @@
>  }
>  
> +void
> +GamepadServiceTest::DestroyGamepadTestChannel()

same here

@@ +83,5 @@
>                                 uint32_t aNumButtons,
>                                 uint32_t aNumAxes,
> +                               ErrorResult& aRv)
> +{
> +  if(!mChild) {

if<space>(

@@ +87,5 @@
> +  if(!mChild) {
> +    InitGamepadTestChannel();
> +  }
> +  const nsString str(aID);
> +  GamepadAdded a(str, 0,

GamepadAdded(nsString(aID), ..

@@ +97,5 @@
> +  if(aRv.Failed()) {
> +    MOZ_CRASH("Promise is failed!");
> +    return nullptr;
> +  }
> +  mChild->SetPromise(p);

this is wrong. Why do you assume that mChild should exist here?

@@ +109,5 @@
> +{
> +  MOZ_ASSERT(mChild);
> +  GamepadRemoved a(aIndex);
> +  GamepadChangeEvent e(a);
> +  mChild->SendGamepadTestEvent(e);

Why mChild should exist here?

@@ +111,5 @@
> +  GamepadRemoved a(aIndex);
> +  GamepadChangeEvent e(a);
> +  mChild->SendGamepadTestEvent(e);
> +  mGamepadCount--;
> +  if(mGamepadCount==0) {

if<space>(mGamepadCount<space>==<space>0) {

@@ +124,5 @@
> +{
> +  MOZ_ASSERT(mChild);
> +  GamepadButtonInformation a(aIndex, aButton, aPressed, aPressed ? 1.0 : 0);
> +  GamepadChangeEvent e(a);
> +  mChild->SendGamepadTestEvent(e);

ditto.

@@ +136,5 @@
>  {
> +  MOZ_ASSERT(mChild);
> +  GamepadButtonInformation a(aIndex, aButton, aPressed, aValue);
> +  GamepadChangeEvent e(a);
> +  mChild->SendGamepadTestEvent(e);

ditto.

@@ +147,5 @@
>  {
> +  MOZ_ASSERT(mChild);
> +  GamepadAxisInformation a(aIndex, aAxis, aValue);
> +  GamepadChangeEvent e(a);
> +  mChild->SendGamepadTestEvent(e);

ditto.

::: dom/gamepad/GamepadServiceTest.h
@@ +25,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(GamepadServiceTest)
> +  NS_DECL_NSIIPCBACKGROUNDCHILDCREATECALLBACK
> +
> +  uint32_t NoMapping() { return 0; }

const

@@ +26,5 @@
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(GamepadServiceTest)
> +  NS_DECL_NSIIPCBACKGROUNDCHILDCREATECALLBACK
> +
> +  uint32_t NoMapping() { return 0; }
> +  uint32_t StandardMapping() { return 1; }

const

::: dom/gamepad/ipc/GamepadTestChannelChild.cpp
@@ +6,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +GamepadTestChannelChild::GamepadTestChannelChild()
> +  : mPromise(nullptr) {}

remove this. by default RefPtr is null.

::: dom/gamepad/ipc/GamepadTestChannelChild.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

add an empty line after this comment.

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "mozilla/dom/PGamepadTestChannelChild.h"
> +#include "mozilla/dom/Promise.h"

forward declaration.

@@ +7,5 @@
> +#ifndef mozilla_dom_GamepadTestChannelChild_h_
> +#define mozilla_dom_GamepadTestChannelChild_h_
> +
> +namespace mozilla{
> +namespace dom{

<space>{

@@ +14,5 @@
> +{
> + public:
> +  GamepadTestChannelChild();
> +  ~GamepadTestChannelChild() {}
> +  void Shutdown()

remove this and call SendShutdownChannel() directly.

@@ +18,5 @@
> +  void Shutdown()
> +  {
> +    SendShutdownChannel();
> +  }
> +  void SetPromise(Promise *aPromise) { mPromise = aPromise; }

MOZ_ASSERT(!mPromise);

::: dom/gamepad/ipc/GamepadTestChannelParent.cpp
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "GamepadTestChannelParent.h"

emtpy line after the comment.

@@ +13,5 @@
> +GamepadTestChannelParent::RecvGamepadTestEvent(
> +	                                   const GamepadChangeEvent& aEvent)
> +{
> +  mozilla::ipc::AssertIsOnBackgroundThread();
> +  GamepadPlatformService*  functions = GamepadPlatformService::GetService();

functions? :)

@@ +22,5 @@
> +                                           (GamepadMappingType)a.mapping(),
> +                                           a.num_buttons(),
> +                                           a.num_axes());
> +    if(!mShuttingdown) {
> +      Unused << this->SendReplyGamepadIndex(index);

remove 'this->'

@@ +23,5 @@
> +                                           a.num_buttons(),
> +                                           a.num_axes());
> +    if(!mShuttingdown) {
> +      Unused << this->SendReplyGamepadIndex(index);
> +    }

return;

@@ +24,5 @@
> +                                           a.num_axes());
> +    if(!mShuttingdown) {
> +      Unused << this->SendReplyGamepadIndex(index);
> +    }
> +  } else if (aEvent.type() == GamepadChangeEvent::TGamepadRemoved) {

if ( ... ) {
  ...
  return;
}

::: dom/gamepad/ipc/GamepadTestChannelParent.h
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

empty line.

Comment 98

[Michael Leu[:Lenzak](UTC+8)]

Attachment: 0001-Bug-1221730-Move-gamepad-API-to-PBackground-v14-to-v.patch

Comment 99

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v14 to 15 interdiff

Comment 100

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8755355 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v14 to 15 interdiff

Review of attachment 8755355 [details] [diff] [review]:
-----------------------------------------------------------------

1. functions ----> service
2. gamepadsvc ----> gamepadManager
3. Change singleton declaration from private static member to anonymous class in GamepadManager and GamepadPlatformService
4. Revise several methods in GamepadManager
   (ps. It seems that we can't assign NS_LITERAL_STRING to nsAutoString, compiler will complain about no viable constructors)
5. Complete the initialization list in constructor of GamepadManager
6. Remove unused member variables in GamepadManager
7. Change the gamepad index increment logic in GamepadPlatformService
8. Add mutex in GamepadPlatformService
   (ps. lock "gGamepadPlatformServiceSingleton = nullptr" will result in deadlock)
9. Move RemoveChannelParents from ActorDestroy to RecvGamepadListenerRemoved
   (ps. I use mHasGamepadListener to judge whether RecvGamepadListenerRemoved is called or not)

Comment 101

[Andrea Marchesini [:baku]]

Comment on attachment 8755355 [details] [diff] [review]
Bug 1221730 Move gamepad API to PBackground v14 to 15 interdiff

Review of attachment 8755355 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/gamepad/GamepadPlatformService.cpp
@@ +27,2 @@
>  StaticAutoPtr<GamepadPlatformService>
> +gGamepadPlatformServiceSingleton;

indentation here.

Comment 102

[Michael Leu[:Lenzak](UTC+8)]

(In reply to Andrea Marchesini (:baku) from comment #97)
> @@ +97,5 @@
> > +  if(aRv.Failed()) {
> > +    MOZ_CRASH("Promise is failed!");
> > +    return nullptr;
> > +  }
> > +  mChild->SetPromise(p);
> 
> this is wrong. Why do you assume that mChild should exist here?

About this, I plan to initialize IPDL in GetService(), setting a flag in ActorCreated()
so I can make sure mChild is valid.

To achieve this, I need an observer or something to notify me the program is shutting down to
destruct IPDL connection in right time.

But it seems that I cannot inherit 2 nsISupports-derived classes (nsIObserver and nsIIPCBackgroundChildCreateCallback) with cycle collection macros or
compiler will complain about diamond inheritance

Is there any example of similar condition or alternative approach?

Comment 103

[Andrea Marchesini [:baku]]

> To achieve this, I need an observer or something to notify me the program is
> shutting down to
> destruct IPDL connection in right time.

I don't understand what you mean.

> But it seems that I cannot inherit 2 nsISupports-derived classes
> (nsIObserver and nsIIPCBackgroundChildCreateCallback) with cycle collection
> macros or
> compiler will complain about diamond inheritance

Yes you can. Check MessagePort.h for instance.

Comment 104

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v2

Comment 105

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v1 to v2 interdiff

Comment 106

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8755707 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v1 to v2 interdiff

Review of attachment 8755707 [details] [diff] [review]:
-----------------------------------------------------------------

1. Remove redundant block in binding.conf
2. Move GamepadServiceTest::sSingleton to anonymous namespace
3. Change method name Init/DestroyGamepadTestChannel to Init/DestroyPBackgroundActor
4. fix if( nits
5. Remove immediate nsString in GamepadServiceTest::AddGamepad
6. Change IPDL initialization flow
   (ps. We initialize the IPDL connection when we first attempt to get the service via GetService,
    and it will be destroy when XPCOM is going to shutdown)
7. Add const in some functions in GamepadServiceTest.h
8. Remove redundant initialization list in GamepadTestChannelChild.h
   (ps. When I attempted to forward declare Promise, the compiler complains about
        incomplete type when compiling the nsCOMPtr declaration)
9. fix /** Licence Header **/ <white line> nits
10. fix <namespace><space>{ nits
11. functions ----> service in GamepadTestChannelParent.cpp
12. if(){...}else if(){...} into if(){return;} if(){return;} in GamepadTestChannelParent.cpp
13. Remove this-> in GamepadTestChannelParent.cpp
14. Add assertion in GamepadTestChannelChild::SetPromise()
15. Remove GamepadTestChannelChild::Shutdown()

This patch is interdiff of the current update and previous patch, full patch is on the attachment list, too :)

Comment 107

[Andrea Marchesini [:baku]]

Comment on attachment 8755707 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v1 to v2 interdiff

Review of attachment 8755707 [details] [diff] [review]:
-----------------------------------------------------------------

I'll continue reviewing the full patch.

::: dom/gamepad/GamepadServiceTest.cpp
@@ +82,5 @@
> +
> +  if (!observerService) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  observerService->AddObserver(this,

this can fail.

@@ +172,5 @@
>    MOZ_ASSERT(mChild);
>    GamepadRemoved a(aIndex);
>    GamepadChangeEvent e(a);
>    mChild->SendGamepadTestEvent(e);
>    mGamepadCount--;

what about if we go to 0 here?

Comment 108

[Andrea Marchesini [:baku]]

Comment on attachment 8755706 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v2

Review of attachment 8755706 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there :) Tell me more about multiple AddPromise() calls. Is it possible? If not, tell me why. If yes, this code is buggy.
Can you write a test for it?

::: dom/gamepad/GamepadServiceTest.cpp
@@ +62,2 @@
>  {
> +  if (gSingleton == nullptr) {

if (!gSingleton) ...

@@ +119,5 @@
> +  MOZ_ASSERT(!mChild);
> +  PBackgroundChild *actor = BackgroundChild::GetForCurrentThread();
> +  //Try to get the PBackground Child actor
> +  if (actor) {
> +    ActorCreated(actor);

So, this is not correct yet. What about if we have this scenario:

1. the service is created
2. the actor creation is still pending
3. AddGamepad is called.

mChild doesn't exist... and we crash. Right? You must implement some kind of pending operation array, and process when ActorCreated() is called.

@@ +144,2 @@
>  {
> +  if (!mIsIPDLActorAvailable) {

This seems way too aggressive... What about:

GamepadAdded a(...);
GamepadChangeEvent e(a);
RefPtr<Promise> p;
...
p = Promise::Create(go, aRv);
...

if (mChild) {
  mChild->SetPromise(e);
  mChild->SendGamepadTestEvent(e);
  ..
} else {
  PendingOperation op(p, e);
  mPendingOperations.AppendElement(op);
}

return p.forget();

@@ +148,5 @@
> +  MOZ_ASSERT(mChild);
> +  GamepadAdded a(nsString(aID), 0,
> +                (uint32_t)aMapping, aNumButtons, aNumAxes);
> +  GamepadChangeEvent e(a);
> +  nsCOMPtr<Promise> p;

RefPtr<Promise>

::: dom/gamepad/ipc/GamepadTestChannelChild.cpp
@@ +7,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +void
> +GamepadTestChannelChild::SetPromise(Promise* aPromise)

Can we receive multiple AddGamepad() ? In this case, this code doesn't work... right? Because we overwrite the promise.

::: dom/gamepad/ipc/GamepadTestChannelParent.cpp
@@ +20,5 @@
> +  MOZ_ASSERT(service);
> +  if (aEvent.type() == GamepadChangeEvent::TGamepadAdded) {
> +    const GamepadAdded& a = aEvent.get_GamepadAdded();
> +    uint32_t index = service->AddGamepad(ToNewCString(a.id()),
> +                                           (GamepadMappingType)a.mapping(),

indentation.

@@ +44,5 @@
> +    service->NewAxisMoveEvent(a.index(), a.axis(), a.value());
> +    return true;
> +  }
> +
> +  MOZ_CRASH("We shouldn't be here!");

NS_WARNING("Unknown event type.");

@@ +45,5 @@
> +    return true;
> +  }
> +
> +  MOZ_CRASH("We shouldn't be here!");
> +  return true;

return false;

Comment 109

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v3

Comment 110

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v2 to v3 interdiff

Comment 111

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8755780 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v3

Review of attachment 8755780 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I didn't notice the case that multiple addGamepad commands are called, GamepadTestChannelChild indeed needs multiple Promises supports.

1. Remove mGamepadCount and mIsIPDLActorAvailable since we don't need it anymore.
2. Add an ID along with all test events to identify different Promises for upcoming resolves.
3. GamepadServiceTest will now buffer all commands until IPDL channel is created.
4. GamepadTestChannelChild will now support multiple Promises.
5. Add failure handling in GamepadServiceTest::RegisterObserver
5. Various denits.

This is full patch, interdiff is also submitted like last time :)

Comment 112

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v3.1

Remove MOZ_ASSERT(mChild) in GamepadServiceTest.cpp

Comment 113

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v2 to v3.1 interdiff

Comment 114

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v2 to v3.1 interdiff

Comment 115

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify RawInput and XInput to support non-main thread v4.1f r=qdot

Comment 116

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v15f r=qdot, r=baku

Comment 117

[Andrea Marchesini [:baku]]

Comment on attachment 8756207 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v3.1

Review of attachment 8756207 [details] [diff] [review]:
-----------------------------------------------------------------

Almost done! Just the shutting down is still a bit unclear.

::: dom/gamepad/GamepadServiceTest.cpp
@@ +62,2 @@
>  {
> +  if (gSingleton == nullptr) {

if (!gSingleton) {

@@ +62,5 @@
>  {
> +  if (gSingleton == nullptr) {
> +    gSingleton = new GamepadServiceTest(aWindow);
> +    nsresult rv = gSingleton->RegisterObserver();
> +    if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +79,5 @@
> +{
> +  nsCOMPtr<nsIObserverService> observerService =
> +    mozilla::services::GetObserverService();
> +
> +  if (!observerService) {

NS_WARN_IF

@@ +86,5 @@
> +  nsresult rv =
> +    observerService->AddObserver(this,
> +                                 NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID,
> +                                 false);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +132,5 @@
>  }
>  
> +void
> +GamepadServiceTest::DestroyPBackgroundActor()
> +{

this is still wrong. What about if PBackground is so slow that this method is called before ActorCreated?
You must change the code here so that:

1. if mChild exist, well, mPendingOperations should be empty (assert!) and you call SendShutdownChannel.
2. if it doesn't, you clear the pendingOperations array, then you set a flag (mShuttingDown?) and when ActorCreated is called, you use that flag to send ShutdownChannel().

@@ +147,5 @@
>  {
> +  GamepadAdded a(nsString(aID), 0,
> +                (uint32_t)aMapping, aNumButtons, aNumAxes);
> +  GamepadChangeEvent e(a);
> +  RefPtr<Promise> p;

move this when you create the promise.

RefPtr<Promise> p = Promise::Create(...

@@ +152,5 @@
> +  nsCOMPtr<nsIGlobalObject> go = do_QueryInterface(mWindow);
> +
> +  p = Promise::Create(go, aRv);
> +  if (aRv.Failed()) {
> +    MOZ_CRASH("Promise is failed!");

it can happen :) remove this assertion.

@@ +160,5 @@
> +  uint32_t id = ++mEventNumber;
> +  if (mChild) {
> +    mChild->AddPromise(id, p);
> +    mChild->SendGamepadTestEvent(id, e);
> +  } else {

here and everywhere else, you should check if mShuttingDown is true and in case, ignore the operation.

@@ +237,5 @@
> +
> +void
> +GamepadServiceTest::FlushPendingOperations()
> +{
> +  for (uint32_t i=0; i<mPendingOperations.Length(); i++) {

for (uint32_t i = 0; i < mPending... ; ++i) {
spaces!

::: dom/gamepad/ipc/GamepadTestChannelChild.cpp
@@ +26,5 @@
> +  return true;
> +}
> +
> +}
> +}

comments about these dom/mozilla namespaces

::: dom/gamepad/ipc/GamepadTestChannelParent.cpp
@@ +11,5 @@
> +namespace dom {
> +
> +bool
> +GamepadTestChannelParent::RecvGamepadTestEvent(const uint32_t& aID,
> +	                                   const GamepadChangeEvent& aEvent)

indentation

Comment 118

[Michael Leu[:Lenzak](UTC+8)]

Attachment: 8756207: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v4

Comment 119

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v4.1

Comment 120

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8758129 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v4.1

Review of attachment 8758129 [details] [diff] [review]:
-----------------------------------------------------------------

1. if (gSingleton == nullptr) --> if (!gSingleton)
2. Add NS_WARN_IF() in fail handling code
3. Revise DestroyPBackgroundActor()
   (ps.) If mChild exists, we assert no pending operations and shutdown the IPDL channel.
         If not, we just cancel all pending operations, and we will set a flag indicating we're shutting down so any operation after DestroyPBackgroundActor() including ActorCreated() will be ignored.
4. Rearrange code in AddGamepad()
5. Various denits

Comment 121

[Andrea Marchesini [:baku]]

Comment on attachment 8758129 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v4.1

Review of attachment 8758129 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry! I didn't catch this issue before. The rest is OK!

::: dom/base/Navigator.cpp
@@ +1904,5 @@
> +
> +already_AddRefed<GamepadServiceTest>
> +Navigator::RequestGamepadServiceTest()
> +{
> +  RefPtr<GamepadServiceTest> test = GamepadServiceTest::GetService(mWindow);

See the comment in GamePadServiceTest. Here this code must change in:

if (!mGamepadServiceTest) {
  mGamepadServiceTest = GamepadServiceTest::CreateService(mWindow);
}

And it can be:
	
GamepadServiceTest* Navigator::RequestGamepadServiceTest()

::: dom/gamepad/GamepadServiceTest.cpp
@@ +62,3 @@
>  {
> +  if (!gSingleton) {
> +    gSingleton = new GamepadServiceTest(aWindow);

oh oh! I didn't catch this bug before!
This is wrong... What about if we have 2 windows? we should have 1 GamepadServiceTest per window.

Make this service a member of Navigator and move the creation in the getter. I'll write a comment there.
Remove gSingleton and rename this method: 'CreateService'.

Don't use ClearOnShutdown but clean the service when the navigator is CC/GCed.

@@ +146,5 @@
> +    mChild->SendShutdownChannel();
> +    mChild = nullptr;
> +  } else {
> +    // If the IPDL channel has not been created and we
> +    // want destroy it now, just cancel all pending

want to destroy it

@@ +284,5 @@
> +void
> +GamepadServiceTest::ActorCreated(PBackgroundChild* aActor)
> +{
> +  MOZ_ASSERT(aActor);
> +  if (mShuttingDown) {

MOZ_ASSERT(mPendingOperations.IsEmpty());

::: dom/gamepad/ipc/GamepadEventTypes.ipdlh
@@ +36,5 @@
> +  GamepadAxisInformation;
> +  GamepadButtonInformation;
> +};
> +
> +}

comments here.

::: dom/gamepad/ipc/GamepadTestChannelChild.cpp
@@ +18,5 @@
> +GamepadTestChannelChild::RecvReplyGamepadIndex(const uint32_t& aID,
> +                                               const uint32_t& aIndex)
> +{
> +  RefPtr<Promise> p;
> +  mPromiseList.Get(aID, getter_AddRefs(p));

Better if you do:

if (!mPromiseList.Get(aID, getter_AddRefs(p))) {
  MOZ_CRASH("We should always have a promise.");
}

p->MaybeResolve(aIndx);
...

Comment 122

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v5

Comment 123

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v4.1 to v5 interdiff

Comment 124

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8758168 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v5

Review of attachment 8758168 [details] [diff] [review]:
-----------------------------------------------------------------

1. Revise Navigator::RequestGamepadServiceTest
   ps. It seems that other similar functions throws exceptions so I added !mWindow error handling as well.
2. Make GamepadServiceTest instance member of Navigator instead of singleton.
3. GetService ----> CreateService in GamepadServiceTest.
4. Fix comment in GamepadServiceTest
5. Add assertion in GamepadServiceTest::ActorCreated()
6. Add namespace comments in GamepadEventTypes.ipdlh
7. Revise GamepadTestChannelChild::RecvReplyGamepadIndex.

ps. This time I submit interdiff, too :)

Comment 125

[Andrea Marchesini [:baku]]

Comment on attachment 8758168 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v5

Review of attachment 8758168 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Navigator.cpp
@@ +1909,5 @@
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +  if (!mGamepadServiceTest) {
> +    mGamepadServiceTest = GamepadServiceTest::CreateService(mWindow);

Error must be propagated, right?

@@ +1911,5 @@
> +  }
> +  if (!mGamepadServiceTest) {
> +    mGamepadServiceTest = GamepadServiceTest::CreateService(mWindow);
> +  }
> +  return mGamepadServiceTest;

Then, you must TRAVERSE, UNLINK it. In unlink, you want to call some Shutdown() method in this service to shutdown the IPC actor.

::: dom/gamepad/GamepadServiceTest.cpp
@@ +56,5 @@
>  {
> +  RefPtr<GamepadServiceTest> service = new GamepadServiceTest(aWindow);
> +  nsresult rv = service->RegisterObserver();
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return nullptr;

This method should receive ErrorResult& aRv, and use it here.

@@ +73,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsresult rv =
> +    observerService->AddObserver(this,
> +                                 NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID,

remove all of this :) No registration is needed.

@@ +91,1 @@
>  {

All of this should be removed. This object depends on the navigator now.
So yeah, no observer here.

@@ +124,5 @@
> +
> +void
> +GamepadServiceTest::DestroyPBackgroundActor()
> +{
> +  mShuttingDown = true;

mShuttingDown should go away as well, because Navigator should take care of it.

Comment 126

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v5 to v6 interdiff

Comment 127

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v6

Comment 128

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8758531 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v6

Review of attachment 8758531 [details] [diff] [review]:
-----------------------------------------------------------------

1. Add NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mGamepadServiceTest) in Navigator.cpp
2. Unlink mGamepadServiceTest in Navigator::Invalidate()
3. Add #ifdef MOZ_GAMEPAD like original gamepad api implementation
4. Remove nsIObserver inheritance in GamepadServiceTest

ps. I think we should keep mShuttingDown because ActorCreated() may be called after Navigator is destroyed since it's an asynchronous call.

Comment 129

[Andrea Marchesini [:baku]]

Comment on attachment 8758531 [details] [diff] [review]
Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v6

Review of attachment 8758531 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/gamepad/GamepadServiceTest.cpp
@@ +56,2 @@
>  {
> +  if (!aWindow) {

We use aRv only for the window?

1. MOZ_ASSERT(aWindow);
2. remove ErrorResult from CreateService
3. Remove ErrorResult from navigator::RequestGamepadServiceTest
4. remove [Throws] from the webidl.

@@ +64,5 @@
>  }
>  
> +void
> +GamepadServiceTest::Shutdown()
> +{

MOZ_ASSERT(!mShuttingDown);

::: dom/gamepad/GamepadServiceTest.h
@@ +43,2 @@
>  
> +  GamepadServiceTest(nsPIDOMWindowInner* aWindow);

make this private.

Comment 130

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Modify RawInput and XInput to support non-main thread v4.1f r=qdot

Comment 131

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v15f r=qdot, r=baku

Comment 132

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Mochitest Part1 - Change GamepadServiceTest into WebIDL for mochitest v6.1f r=qdot, r=baku

Comment 133

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1221730 Move gamepad API to PBackground v15.1f r=qdot, r=baku

Rebase to commit fdfe455fca53903c47d7494f880b067ad9bdbdbf

Comment 134

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part5: Fix false error in mochitest

Some of the mochitests fail after moving gamepad backends to PBackground.
I found that it is because some of the tests assumes that gamepad state changes before mochitest assertion so racing condition happens when we move gamepad api to PBackground.

This patch change SpecialPowers.executionSoon to callbacks trigger by corresponding events which ensures the execution order of state change and assertion.

Comment 135

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part1: Modify RawInput and XInput to support non-main thread v4.2f r=qdot

Rebase to commit fdfe455fca53903c47d7494f880b067ad9bdbdbf

Comment 136

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part2: Move gamepad API to PBackground v15.2f r=qdot, r=baku

Remove MOZ_ASSERT that triggers Werror in Try Server in GamepadManager.cpp

Comment 137

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part3: Change GamepadServiceTest into WebIDL for mochitest v6.1f r=qdot, r=baku

Remove MOZ_ASSERT that triggers -Werror in try server in GamepadServiceTest.cpp

Comment 138

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part2: Move gamepad API to PBackground v15.2f r=qdot, r=baku

Add explicit modifier in DarwinGamepadServiceRunnable in cocoagamepad.cpp to prevent build error in try server

Comment 139

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part3: Change GamepadServiceTest into WebIDL for mochitest v6.1f r=qdot, r=baku

Add explicit modifier for PendingOperation and GamepadServiceTest in GamepadServiceTest.h to prevent try server complaining.

Comment 140

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part2: Move gamepad API to PBackground v15.3f r=qdot, r=baku

Update nsAppShell.cpp in android build.

Comment 141

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part3: Change GamepadServiceTest into WebIDL for mochitest v6.2f r=qdot, r=baku

Fix a leak bug in GamepadTestChannelParent::RecvGamepadTestEvent detected in tryserver linux x64 asan

Comment 142

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part6: Fix gamepad analog button logic

In original gamepad api design, only value 1 triggers button down event, I think it should be always button down event as long as button value is not zero.

This causes problem when we want to test button down event with value in interval (0,1) with event callback in Mochitest

I found this bug when I am dealing with some intermittent mochitest failures in tryserver, I dont know whether it should be a separate issue or not.

Comment 143

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Fix false mochitest test error under windows

This is similar issue in patch part5, original design may cause race condition between mochitest assertions and DOM events.

Comment 144

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Postpone singleton release in GamepadPlatformService::MaybeShutdown

I found there is intermittent mochitest crashes under windows platform in tryserver.

One of the crash happens in GamepadPlatformService::MaybeShutdown.
It crashes in RtlEnterCriticalSection which is inside the mutex lock.

I think it is an issue of wrong release sequence, so I use RefPtr to postpone the release outside of the lock while making the singleton null inside the lock.

Comment 145

[(not currently active) Ted Mielczarek]

Comment on attachment 8764289 [details] [diff] [review]
Part6: Fix gamepad analog button logic

Review of attachment 8764289 [details] [diff] [review]:
-----------------------------------------------------------------

The button events aren't in the spec, so we have flexibility here, but I don't want it to work this way. `FireButtonEvent` is only called from one place, `NewButtonEvent`, and that method takes an `aPressed` parameter. You should just pass that parameter down to `FireButtonEvent`, and use that to determine whether to send `gamepadbutton{down,up}`.

If you need events for "the button's value has changed but the pressed status has not changed" that's totally reasonable, feel free to implement something like a `gamepadbuttonchanged` event for that case. (That's useful to know to inform what we ought to be writing in the spec when we actually get around to spec'ing events!)

Comment 146

[(not currently active) Ted Mielczarek]

Also, I don't know if you've seen, but I wrote a few web platform tests for the Gamepad API a while ago:
http://w3c-test.org/gamepad/

Most of them require manual activity to run (connect a gamepad, press a button, etc). You can run the whole set in a semi-automated fashion (the harness will run them each in turn but you'll still have to press buttons on a gamepad) by loading the test runner URL and clicking Start:
http://w3c-test.org/tools/runner/index.html?path=/gamepad

Comment 147

[Michael Leu[:Lenzak](UTC+8)]

Thanks for your information Ted. :D

I will try this platform test :)

Sorry about taking so long, I am now fighting with some strange tryserver mochitest crash failures under Windows platform which I cannot reproduce in my local machine.

Comment 148

[Andrea Marchesini [:baku]]

Comment on attachment 8764295 [details] [diff] [review]
Postpone singleton release in GamepadPlatformService::MaybeShutdown

Review of attachment 8764295 [details] [diff] [review]:
-----------------------------------------------------------------

I want to see how you change the creation of the service.

::: dom/gamepad/GamepadPlatformService.cpp
@@ +200,5 @@
> +  // the mutex as well as making upcoming GetParentService() call
> +  // recreate new singleton, so we use this RefPtr to temporarily
> +  // hold the reference, postponing the release process until this
> +  // method ends.
> +  RefPtr<GamepadPlatformService> tmpGrab;

call tmpGrab: kungFuDeathGrip

@@ +208,5 @@
>      MutexAutoLock autoLock(mMutex);
>      isChannelParentEmpty = mChannelParents.IsEmpty();
> +    if(isChannelParentEmpty) {
> +      tmpGrab = gGamepadPlatformServiceSingleton;
> +      gGamepadPlatformServiceSingleton = nullptr;

This is nice. but... do you use a mutex when you create the service? Or when you call GetParentService?

Comment 149

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Postpone singleton release in GamepadPlatformService::MaybeShutdown

Change tmpGrab to kungFuDeathGrip

Comment 150

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Postpone singleton release in GamepadPlatformService::MaybeShutdown

This patch limits only background thread can create new GamepadPlatformService singleton if it's null, which prevents monitor thread from accidentally create the singleton after we shutdown the whole service.

With this change, obtaining null GamepadPlatformService in monitor thread will not be treat as a fatal error, we just ignore the operation.

Comment 151

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Fix false mochitest test error under windows

Fix test_gamepad.html mochitest to prevent possible race condition false error under windows.

About the button logic problem, I don't want to change the event model here since it is out of this bug's scope, maybe we can open another bug to do it.

Comment 152

[Michael Leu[:Lenzak](UTC+8)]

I think the patch is almost OK to land, but there is a strange problem.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5976abbc5fb&selectedJob=22772482
In Try Server, mochitest "test_gamepad_connect_events.html" encounters intermittent assertion failure, which indicates that the monitor thread is not correct in windowsgamepad.cpp under windows platform.

Here is the error log:
Assertion failure: NS_GetCurrentThread() == gMonitorThread, at c:/builds/moz2_slave/try-w32-d-00000000000000000000/build/src/dom/gamepad/windows/WindowsGamepad.cpp:974
PROCESS-CRASH | dom/tests/mochitest/gamepad/test_gamepad_connect_events.html | application crashed [@ `anonymous namespace'::StartWindowsGamepadServiceRunnable::Run]

This assertion should always holds since the only code which triggers StartWindowsGamepadServiceRunnable is NS_NewThread(getter_AddRefs(gMonitorThread), new StartWindowsGamepadServiceRunnable()).

And I can't reproduce this failure in my local windows machine so I have no idea why this happen.

Have you ever seen similar issue like this?

Comment 153

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Postpone singleton release in GamepadPlatformService::MaybeShutdown r=baku

Add android service change, I missed it in last patch.

Comment 154

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

I've got the patches building locally now and also hit some retriggers on the treeherder to see how often this happens. Will let you know if I manage to repro.

Comment 155

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Fix assertion failure in windowsgamepad.cpp

I found why the assertion failure happens, the cause is in NS_NewThread()

The execution order of NS_NewThread() is described below.

1. Create a temporary thread
2. Dispatch the runnable to this temporary thread
3. Assign the temporary thread to our input nsCOMPtr<nsIThread>

Our StartWindowsGamepadServiceRunnable asserts NS_GetCurrentThread() == gMonitorThread in the first line of Run().

So it may happen that the assertion is faster than 3. so the assertion sees gMonitorThread == nullptr and we fail.

The solution is to split thread assign and runnable dispatch into 2 steps.

Sorry for taking so long, I think this bug is ready to check-in after solving this problem :)

Comment 156

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part4: Postpone singleton release in GamepadPlatformService::MaybeShutdown r=baku

Fix build failure in android.
(forget GamepadPlatformService* ----> RefPtr<GamepadPlatformService> previous patch)

Comment 157

[Michael Leu[:Lenzak](UTC+8)]

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

Comment 158

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part1: Modify RawInput and XInput to support non-main thread v4.3f r=qdot

Squash "8765515: Fix assertion failure in windowsgamepad.cpp" into this patch

Comment 159

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part5: Update Mochitest html files r=qdot

Squash other mochitest fix to this patch

Comment 160

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part1: Modify RawInput and XInput to support non-main thread v4.3f r=qdot

Rebase

Comment 161

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part2: Move gamepad API to PBackground v15.3f r=qdot, r=baku

Rebase

Comment 162

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part3: Change GamepadServiceTest into WebIDL for mochitest v6.2f r=qdot, r=baku

Rebase

Comment 163

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part4: Postpone singleton release in GamepadPlatformService::MaybeShutdown r=baku

Rebase

Comment 164

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Part5: Update Mochitest html files r=qdot

Rebase

Comment 165

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe54272cd8ff
Remove Observers in Windows gamepad backend. r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/342068569153
Move gamepad API to PBackground. r=qdot, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/050df9c81c1e
Change GamepadServiceTest into webidl. r=qdot, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8875dbdc71
Postpone singleton release in GamepadPlatformService::MaybeShutdown. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c79d6cf318c4
Update mochitest. r=qdot

Comment 166

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe54272cd8ff
https://hg.mozilla.org/mozilla-central/rev/342068569153
https://hg.mozilla.org/mozilla-central/rev/050df9c81c1e
https://hg.mozilla.org/mozilla-central/rev/8f8875dbdc71
https://hg.mozilla.org/mozilla-central/rev/c79d6cf318c4