Closed Bug 1381378 Opened 7 years ago Closed 7 years ago

Isolate Gamepad index and GamepadServiceType from GamepadChangeEvent

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: cleu, Assigned: cleu)

Details

Attachments

(1 file)

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: nobody → cleu
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+
Keywords: checkin-needed
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)
Keywords: checkin-needed
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.