1315896 - Remove MOZ_GAMEPAD and adjust for other non gamepad support platforms build.

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Daosheng Mu[:daoshengmu]]

As Bug 1310904 mentioned, there are some tier 3 platforms do not implement Gamepad API. However, currently, Gamepad is a part of our VR module. Therefore, it will generate build errors when including gamepad header files from VR related source files.

I think we should make VR to be disabled for these non MOZ_GAMEPAD platforms to solve this dependency problem.

Comment 1

[Daosheng Mu[:daoshengmu]]

Ted, I am considering to remove the dependency of non MOZ_GAMEPAD and VR. But it would have lots of works to do. Before starting this task, I would like to get your opinion about could we make the MOZ_GAMEPAD flag to be always enabled? Is there any other platform has to use --disable-gamepad? AFAIK, BSD already has a patch to avoid this dependency issue. Thanks.

Comment 2

[(not currently active) Ted Mielczarek]

Like I said in bug 1311460 comment 2, I think it would be better to just make `--disable-gamepad` set `MOZ_GAMEPAD_BACKEND=stub` and remove the `MOZ_GAMEPAD` build variable and define entirely. With the stub backend all the includes and such should work fine, we just don't have a backend to actually generate any gamepad input. That seems harmless, and it'd avoid breaking things.

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1315896 - Part 1: Remove MOZ_GAMEPAD in DOM API;

Review commit: https://reviewboard.mozilla.org/r/97738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97738/

Comment 4

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1315896 - Part 2: Remove MOZ_GAMEPAD in Gamepad module; .mielczarek

Review commit: https://reviewboard.mozilla.org/r/97740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97740/

Comment 5

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1315896 - Part 3: Remove MOZ_GAMEPAD in VR module;

Review commit: https://reviewboard.mozilla.org/r/97742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97742/

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817496 [details]
Bug 1315896 - Part 1: Remove MOZ_GAMEPAD in DOM API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97738/diff/1-2/

Comment 7

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817497 [details]
Bug 1315896 - Part 2: Remove MOZ_GAMEPAD in Gamepad module; .mielczarek

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97740/diff/1-2/

Comment 8

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817498 [details]
Bug 1315896 - Part 3: Remove MOZ_GAMEPAD in VR module;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97742/diff/1-2/

Comment 9

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817497 [details]
Bug 1315896 - Part 2: Remove MOZ_GAMEPAD in Gamepad module; .mielczarek

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97740/diff/2-3/

Comment 10

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817498 [details]
Bug 1315896 - Part 3: Remove MOZ_GAMEPAD in VR module;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97742/diff/2-3/

Comment 11

[Daosheng Mu[:daoshengmu]]

I refer Comment 2 to make sure 'MOZ_GAMEPAD_BACKEND=stub' when '--disable-gamepad' and its backend is FallbackGamepad. So, I remove MOZ_GAMEPAD in all define and build variable.

Comment 12

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817496 [details]
Bug 1315896 - Part 1: Remove MOZ_GAMEPAD in DOM API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97738/diff/2-3/

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817497 [details]
Bug 1315896 - Part 2: Remove MOZ_GAMEPAD in Gamepad module; .mielczarek

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97740/diff/3-4/

Comment 14

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817498 [details]
Bug 1315896 - Part 3: Remove MOZ_GAMEPAD in VR module;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97742/diff/3-4/

Comment 15

[:kip (Kearwood Gilbert)]

Comment on attachment 8817498 [details]
Bug 1315896 - Part 3: Remove MOZ_GAMEPAD in VR module;

https://reviewboard.mozilla.org/r/97742/#review98908

Looks good, nice cleanup, thanks!

Comment 16

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

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Like I said in bug 1311460 comment 2, I think it would be better to just
> make `--disable-gamepad` set `MOZ_GAMEPAD_BACKEND=stub` and remove the
> `MOZ_GAMEPAD` build variable and define entirely. With the stub backend all
> the includes and such should work fine, we just don't have a backend to
> actually generate any gamepad input. That seems harmless, and it'd avoid
> breaking things.

So I'm gonna guess it's way, way too late to make navigator.getGamepads() [throws]?

Comment 17

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

Comment on attachment 8817496 [details]
Bug 1315896 - Part 1: Remove MOZ_GAMEPAD in DOM API;

https://reviewboard.mozilla.org/r/97738/#review98934

Comment 18

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

I guess I'm ok removing the define guards, but can we at least like, print message to the console or something when we're calling getGamepads() and returning stub? Having it just be opaque looks like we actually support the platform.

Comment 19

[Daosheng Mu[:daoshengmu]]

(In reply to Kyle Machulis [:qdot] from comment #18)
> I guess I'm ok removing the define guards, but can we at least like, print
> message to the console or something when we're calling getGamepads() and
> returning stub? Having it just be opaque looks like we actually support the
> platform.

I am not sure if I understand it correctly. Do you suggest me to do like this?

void
nsGlobalWindow::GetGamepads(nsTArray<RefPtr<Gamepad> >& aGamepads)
{
#if !defined(XP_WIN) && !defined(XP_MACOSX)
    && !defined(XP_LINUX) && !defined(ANDROID)
      nsContentUtils::ReportToConsole(nsIScriptError::infoFlag,
                                NS_LITERAL_CSTRING("DOM"),
                                nullptr,
                                nsContentUtils::eDOM_PROPERTIES,
                                "This platform does not support Gamepad.");
      return;
#else
  MOZ_ASSERT(IsInnerWindow());
  aGamepads.Clear();
  // mGamepads.Count() may not be sufficient, but it's not harmful.
  aGamepads.SetCapacity(mGamepads.Count());
  for (auto iter = mGamepads.Iter(); !iter.Done(); iter.Next()) {
    Gamepad* gamepad = iter.UserData();
    aGamepads.EnsureLengthAtLeast(gamepad->Index() + 1);
    aGamepads[gamepad->Index()] = gamepad;
  }
#endif
}

If it is correct, nsGlobalWindow::GetGamepad() needs to add this message as well.

Comment 20

[(not currently active) Ted Mielczarek]

(In reply to Kyle Machulis [:qdot] from comment #16)
> So I'm gonna guess it's way, way too late to make navigator.getGamepads()
> [throws]?

Yeah, that ship has sailed.

(In reply to Kyle Machulis [:qdot] from comment #18)
> I guess I'm ok removing the define guards, but can we at least like, print
> message to the console or something when we're calling getGamepads() and
> returning stub? Having it just be opaque looks like we actually support the
> platform.

A console message would be fine, but I'm not sure it matters much. From the perspective of a webpage there's not much useful distinction between "this browser doesn't support the Gamepad API" and "this browser doesn't have any gamepads connected". I don't expect us to ship a Firefox release on any platform that doesn't support it, so this would really only be something that people using custom builds or non-Tier-1 platforms would hit. Making sure their *builds* continue to work seems more important than allowing a webpage to determine that the browser won't be able to provide any actual gamepad input.

Comment 21

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

Ah, ok, I didn't know if we'd want to warn on anything lower than Tier 1 and tend to default to being more cautious, hence thinking about the console message. If all we care about is Tier 1, then I guess we can skip putting in the message. So just r+ with no other comments from me then.

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 8817497 [details]
Bug 1315896 - Part 2: Remove MOZ_GAMEPAD in Gamepad module; .mielczarek

https://reviewboard.mozilla.org/r/97740/#review99412

::: old-configure.in
(Diff revision 4)
>  dnl Gamepad support
>  dnl ========================================================
> -MOZ_GAMEPAD=1
>  MOZ_GAMEPAD_BACKEND=stub
>  
> -MOZ_ARG_DISABLE_BOOL(gamepad,

I'm a little torn about this, because I think there might be situations where people still want to build without the native gamepad support, but maybe it isn't really an issue. I guess we can always put it back if someone complains.

::: old-configure.in:3458
(Diff revision 4)
> -[  --disable-gamepad   Disable gamepad support],
> -    MOZ_GAMEPAD=,
> -    MOZ_GAMEPAD=1)
> -
> -if test "$MOZ_GAMEPAD"; then
>  case "$OS_TARGET" in

I don't want to scope bloat, but this would be really easy to move to moz.configure now. Alternately, since the result is only used in one moz.build file, you could really just move this all to `dom/gamepad/moz.build` (except the header check, I suppose). Maybe we'll just do this in a followup bug.

::: old-configure.in:3470
(Diff revision 4)
>      MOZ_GAMEPAD_BACKEND=windows
>      ;;
>  Linux)
>      MOZ_CHECK_HEADER([linux/joystick.h])
>      if test "$ac_cv_header_linux_joystick_h" != "yes"; then
>        AC_MSG_ERROR([Can't find header linux/joystick.h, needed for gamepad support. Please install Linux kernel headers or reconfigure with --disable-gamepad to disable gamepad support.])

If you're going to remove --disable-gamepad you need to change this error message as well.

Comment 23

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817496 [details]
Bug 1315896 - Part 1: Remove MOZ_GAMEPAD in DOM API;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97738/diff/3-4/

Comment 24

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817497 [details]
Bug 1315896 - Part 2: Remove MOZ_GAMEPAD in Gamepad module; .mielczarek

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97740/diff/4-5/

Comment 25

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8817498 [details]
Bug 1315896 - Part 3: Remove MOZ_GAMEPAD in VR module;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97742/diff/4-5/

Comment 26

[Daosheng Mu[:daoshengmu]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> Comment on attachment 8817497 [details]
> Bug 1315896 - Part 2: Remove MOZ_GAMEPAD in Gamepad module; .mielczarek
> 
> https://reviewboard.mozilla.org/r/97740/#review99412
> 
> ::: old-configure.in:3458
> (Diff revision 4)
> > -[  --disable-gamepad   Disable gamepad support],
> > -    MOZ_GAMEPAD=,
> > -    MOZ_GAMEPAD=1)
> > -
> > -if test "$MOZ_GAMEPAD"; then
> >  case "$OS_TARGET" in
> 
> I don't want to scope bloat, but this would be really easy to move to
> moz.configure now. Alternately, since the result is only used in one
> moz.build file, you could really just move this all to
> `dom/gamepad/moz.build` (except the header check, I suppose). Maybe we'll
> just do this in a followup bug.
> 
I file a followup bug at Bug 1324147. Let's see what things we could do better.

> ::: old-configure.in:3470
> (Diff revision 4)
> >      MOZ_GAMEPAD_BACKEND=windows
> >      ;;
> >  Linux)
> >      MOZ_CHECK_HEADER([linux/joystick.h])
> >      if test "$ac_cv_header_linux_joystick_h" != "yes"; then
> >        AC_MSG_ERROR([Can't find header linux/joystick.h, needed for gamepad support. Please install Linux kernel headers or reconfigure with --disable-gamepad to disable gamepad support.])
> 
> If you're going to remove --disable-gamepad you need to change this error
> message as well.

I remove 'reconfigure with --disable-gamepad to disable gamepad support.' message. Thanks.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7b96029eeaa
Part 1: Remove MOZ_GAMEPAD in DOM API; r=qdot
https://hg.mozilla.org/integration/autoland/rev/8003accbf0a8
Part 2: Remove MOZ_GAMEPAD in Gamepad module; r=ted.mielczarek
https://hg.mozilla.org/integration/autoland/rev/937058a57a0f
Part 3: Remove MOZ_GAMEPAD in VR module; r=kip

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c7b96029eeaa
https://hg.mozilla.org/mozilla-central/rev/8003accbf0a8
https://hg.mozilla.org/mozilla-central/rev/937058a57a0f