Closed Bug 1315896 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

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.
QA Whiteboard: 1310904
QA Whiteboard: 1310904
See Also: → 1310904
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.
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
Summary: [webvr] Remove the dependency between VR module and non MOZ_GAMEPAD. → Remove MOZ_GAMEPAD and adjust for other non gamepad support platforms build.
Assignee: nobody → dmu
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 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!
Attachment #8817498 - Flags: review?(kgilbert) → review+
(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 on attachment 8817496 [details]
Bug 1315896 - Part 1: Remove MOZ_GAMEPAD in DOM API;

https://reviewboard.mozilla.org/r/97738/#review98934
Attachment #8817496 - Flags: review?(kyle) → review+
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.
(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.
Flags: needinfo?(kyle)
(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.
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.
Flags: needinfo?(kyle)
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.
Attachment #8817497 - Flags: review?(ted) → review+
Component: Graphics → General
See Also: → 1324147
Keywords: checkin-needed
(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.
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.