Isolate Gamepad index and GamepadServiceType from GamepadChangeEvent

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: cleu, Assigned: cleu)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
I current design, every type of GamepadChangeEvent has these 2 attributes, so we must open the union to access them, which results in duplicate code in GamepadManager.cpp, and the condition will get worse if we add new Gamepad or VR controller event in the future.
Assignee

Updated

2 years ago
Assignee: nobody → cleu
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8886911 [details]
Bug 1381378: Refactor - Isolate gamepad index and GamepadServiceType from GamepadChangeEvent;

https://reviewboard.mozilla.org/r/157644/#review163264

r=me after some nits to be fixed.

::: dom/gamepad/GamepadManager.cpp:506
(Diff revision 2)
>  {
>    if (!mEnabled || mShuttingDown || nsContentUtils::ShouldResistFingerprinting()) {
>      return;
>    }
>  
> -  if (aEvent.type() == GamepadChangeEvent::TGamepadAdded) {
> +  uint32_t index = aEvent.index();

const uint32_t would be better

::: dom/gamepad/GamepadManager.cpp:507
(Diff revision 2)
>    if (!mEnabled || mShuttingDown || nsContentUtils::ShouldResistFingerprinting()) {
>      return;
>    }
>  
> -  if (aEvent.type() == GamepadChangeEvent::TGamepadAdded) {
> -    const GamepadAdded& a = aEvent.get_GamepadAdded();
> +  uint32_t index = aEvent.index();
> +  GamepadServiceType type = aEvent.service_type();

Replace `type` with `serviceType` would be more clear because you have aEvent.type() following.

::: dom/gamepad/GamepadManager.cpp:564
(Diff revision 2)
> -        const GamepadButtonInformation& a = aEvent.get_GamepadButtonInformation();
> -        gamepad = aWindow->GetGamepad(a.index());
> -        if (gamepad) {
> +  if (gamepad) {
> +    switch (body.type()) {
> +      case GamepadChangeEventBody::TGamepadButtonInformation:
> +        {

wrong indentation. please align with case and put `break` inside the scope would be better

::: dom/gamepad/GamepadManager.cpp:570
(Diff revision 2)
>            FireButtonEvent(aWindow, gamepad, a.button(), a.value());
>          }
> -      }
> -      break;
> +        break;
> -    case GamepadChangeEvent::TGamepadAxisInformation:
> +      case GamepadChangeEventBody::TGamepadAxisInformation:
> -      {
> +        {

As the comment above.

::: dom/gamepad/GamepadManager.cpp:587
(Diff revision 2)
> -  uint32_t index;
> -  RefPtr<Gamepad> gamepad;
>    bool ret = false;
>    bool firstTime = false;
>  
> -  switch (aEvent.type()) {
> +  uint32_t index = GetGamepadIndexWithServiceType(aEvent.index(),

const uint32_t would be better.

::: dom/gamepad/GamepadManager.cpp:602
(Diff revision 2)
> +      {
> +        const GamepadButtonInformation& a = body.get_GamepadButtonInformation();
>          gamepad->SetButton(a.button(), a.pressed(), a.touched(), a.value());
> -        ret = true;
> -      }
> -    } break;
> +      } break;

Please put the `break` inside the scope.

::: dom/gamepad/GamepadManager.cpp:612
(Diff revision 2)
> -      gamepad = aWindow ? aWindow->GetGamepad(index) : GetGamepad(index);
> -      if (gamepad) {
>          gamepad->SetPose(a.pose_state());
> -         ret = true;
> -      }
> -    } break;
> +      } break;

please put `break` inside the scope

::: dom/gamepad/GamepadManager.cpp:617
(Diff revision 2)
> -        firstTime = MaybeWindowHasSeenGamepad(aWindow, index);
> -      }
> -      gamepad = aWindow ? aWindow->GetGamepad(index) : GetGamepad(index);
> -      if (gamepad) {
>          gamepad->SetHand(a.hand());
> -        ret = true;
> +      }  break;

please put `break` inside the scope

::: dom/gamepad/ipc/GamepadTestChannelParent.cpp:21
(Diff revision 2)
>  {
>    mozilla::ipc::AssertIsOnBackgroundThread();
>    RefPtr<GamepadPlatformService>  service =
>      GamepadPlatformService::GetParentService();
>    MOZ_ASSERT(service);
> -  if (aEvent.type() == GamepadChangeEvent::TGamepadAdded) {
> +  uint32_t index = aEvent.index();

const uint32_t would be better
Attachment #8886911 - Flags: review?(dmu) → review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 6

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 28bd2d578bf8 -d 4ff29997348c: rebasing 407954:28bd2d578bf8 "Bug 1381378: Refactor - Isolate gamepad index and GamepadServiceType from GamepadChangeEvent; r=daoshengmu" (tip)
merging dom/gamepad/GamepadManager.cpp
merging dom/gamepad/GamepadPlatformService.cpp
merging dom/gamepad/GamepadServiceTest.cpp
merging dom/gamepad/ipc/GamepadEventTypes.ipdlh
merging gfx/vr/VRManager.h
merging gfx/vr/gfxVR.cpp
warning: conflicts while merging dom/gamepad/GamepadManager.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/gamepad/GamepadPlatformService.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/gamepad/GamepadServiceTest.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/gamepad/ipc/GamepadEventTypes.ipdlh! (edit, then use 'hg resolve --mark')
warning: conflicts while merging gfx/vr/gfxVR.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/06e2da701e49
Refactor - Isolate gamepad index and GamepadServiceType from GamepadChangeEvent; r=daoshengmu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06e2da701e49
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.