1358064 - Refactor fire event functions in GamepadManager

Status: RESOLVED FIXED

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

Description

[Daosheng Mu[:daoshengmu]]

I notice there are some duplicate code when asking global window fire events and set attributes to the gamepads. We should do some refactor at NewButtonEvent/NewAxisMoveEvent/NewPoseEvent/NewHandChangeEvent.

Comment 1

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1358064 - Part1: Combine MaybeWindowHasSeenGamepad and FireConnectionEvent; .mielczarek

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

Comment 2

[Michael Leu[:Lenzak](UTC+8)]

Attachment: Bug 1358064 - Simplify Gamepad Event processing;

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

Comment 3

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8859949 [details]
Bug 1358064 - Part1: Combine MaybeWindowHasSeenGamepad and FireConnectionEvent; .mielczarek

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

Comment 4

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8859949 [details]
Bug 1358064 - Part1: Combine MaybeWindowHasSeenGamepad and FireConnectionEvent; .mielczarek

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

Comment 5

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8860285 [details]
Bug 1358064 - Simplify Gamepad Event processing;

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

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8860285 [details]
Bug 1358064 - Simplify Gamepad Event processing;

https://reviewboard.mozilla.org/r/132308/#review135218

r=me, after fixing some nits.

::: dom/gamepad/GamepadManager.cpp:529
(Diff revision 2)
> -    NewAxisMoveEvent(a.index(), a.service_type(), a.axis(), a.value());
> +  // can mutate the mListeners array.
> +  nsTArray<RefPtr<nsGlobalWindow>> listeners(mListeners);
> +  MOZ_ASSERT(!listeners.IsEmpty());
> +
> +  for (uint32_t i = 0; i < listeners.Length(); i++) {
> +

please remove this new line

::: dom/gamepad/GamepadManager.cpp:557
(Diff revision 2)
> -  if (aEvent.type() == GamepadChangeEvent::TGamepadPoseInformation) {
> +
> +  RefPtr<Gamepad> gamepad;
> +
> +  switch (aEvent.type()) {
> +    case GamepadChangeEvent::TGamepadButtonInformation:
> +      {

You can choose to remove {} scope or let {} align the letter 'c' of case.

::: dom/gamepad/GamepadManager.cpp:588
(Diff revision 2)
> +  RefPtr<Gamepad> gamepad;
> +  bool ret = false;
> +
> +  switch (aEvent.type()) {
> +    case GamepadChangeEvent::TGamepadButtonInformation:
> +      {

Same as {} align issue

Comment 7

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8859949 [details]
Bug 1358064 - Part1: Combine MaybeWindowHasSeenGamepad and FireConnectionEvent; .mielczarek

https://reviewboard.mozilla.org/r/131998/#review135226

LGTM

Comment 8

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8860285 [details]
Bug 1358064 - Simplify Gamepad Event processing;

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

Comment 9

[Michael Leu[:Lenzak](UTC+8)]

The scopes in the switch-case code is need because each case has its own variation of GamepadChangeEvent, without them will result in a redefinition variable compile error.

Comment 10

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8860285 [details]
Bug 1358064 - Simplify Gamepad Event processing;

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

Comment 11

[Michael Leu[:Lenzak](UTC+8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e515efa6205

Try looks good.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2981a5649a3a
Simplify Gamepad Event processing; r=daoshengmu

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2981a5649a3a