Closed
Bug 1381378
Opened 7 years ago
Closed 7 years ago
Isolate Gamepad index and GamepadServiceType from GamepadChangeEvent
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
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 | ||
Updated•7 years ago
|
Assignee: nobody → cleu
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 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 | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec0c2fbd73e169156bf7432e0f735f1dcc372bd Here is try result.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 6•7 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)
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/06e2da701e49
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•