[Gamepad] Implement renderer-side dispatch of Raw Input Change Event API [chromium/src : main]
Sneha Agarwal (Gerrit)
Sneha Agarwal added 1 comment![]( "Open in Gerrit")
Hi Gabriel and Michael,
Please review this CL when you get a chance. I am working on fixing the CI errors on the unit test (this CL takes a dependency on the browser-side changes). If you could ignore that for now and just review the core renderer logic, I would really appreciate it.
Thanks
Related details
- Gabriel Brito
- Michael Tang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Tang (Gerrit)
Michael Tang added 13 comments![]( "Open in Gerrit")
Sorry for the delay! Here's a first pass, I'll take a close look at your tests tomorrow!
raw_ref(features::kWebIdentityDigitalCredentials), kDefault},nit: Is this change required?
return gin::Wrappable<GamepadControllerBindings>::GetObjectTemplateBuilder(
isolate)
.SetMethod("connect", &GamepadControllerBindings::Connect)
.SetMethod("dispatchConnected",
&GamepadControllerBindings::DispatchConnected)
.SetMethod("disconnect", &GamepadControllerBindings::Disconnect)
.SetMethod("setId", &GamepadControllerBindings::SetId)
.SetMethod("setButtonCount", &GamepadControllerBindings::SetButtonCount)
.SetMethod("setButtonData", &GamepadControllerBindings::SetButtonData)
.SetMethod("setAxisCount", &GamepadControllerBindings::SetAxisCount)
.SetMethod("setAxisData", &GamepadControllerBindings::SetAxisData)
.SetMethod("setDualRumbleVibrationActuator",
&GamepadControllerBindings::SetDualRumbleVibrationActuator)
.SetMethod("setTriggerRumbleVibrationActuator",
&GamepadControllerBindings::SetTriggerRumbleVibrationActuator)
.SetMethod("setTouchCount", &GamepadControllerBindings::SetTouchCount)
.SetMethod("setTouchData", &GamepadControllerBindings::SetTouchData)
.SetMethod("dispatchRawInputChanged",
&GamepadControllerBindings::DispatchRawInputChanged);```suggestion
return gin::Wrappable<GamepadControllerBindings>::GetObjectTemplateBuilder(
isolate)
.SetMethod("connect", &GamepadControllerBindings::Connect)
.SetMethod("dispatchConnected",
&GamepadControllerBindings::DispatchConnected)
.SetMethod("disconnect", &GamepadControllerBindings::Disconnect)
.SetMethod("dispatchRawInputChanged",
&GamepadControllerBindings::DispatchRawInputChanged)
.SetMethod("setId", &GamepadControllerBindings::SetId)
.SetMethod("setButtonCount", &GamepadControllerBindings::SetButtonCount)
.SetMethod("setButtonData", &GamepadControllerBindings::SetButtonData)
.SetMethod("setAxisCount", &GamepadControllerBindings::SetAxisCount)
.SetMethod("setAxisData", &GamepadControllerBindings::SetAxisData)
.SetMethod("setDualRumbleVibrationActuator",
&GamepadControllerBindings::SetDualRumbleVibrationActuator)
.SetMethod("setTriggerRumbleVibrationActuator",
&GamepadControllerBindings::SetTriggerRumbleVibrationActuator)
.SetMethod("setTouchCount", &GamepadControllerBindings::SetTouchCount)
.SetMethod("setTouchData", &GamepadControllerBindings::SetTouchData);
```
style nit: Consistent ordering
void GamepadControllerBindings::DispatchRawInputChanged(int index) {
if (controller_) {
controller_->DispatchRawInputChanged(index);
}
}
void GamepadControllerBindings::Disconnect(int index) {
if (controller_)
controller_->Disconnect(index);
}```suggestion
void GamepadControllerBindings::Disconnect(int index) {
if (controller_)
controller_->Disconnect(index);
}
void GamepadControllerBindings::DispatchRawInputChanged(int index) {
if (controller_) {
controller_->DispatchRawInputChanged(index);
}
}
```
style nit: Consistent ordering.void GamepadController::DispatchRawInputChanged(int index) {style nit: move above `GamepadController::SetId` for consistent ordering of methods
dictionary GamepadRawInputChangeEventInit : EventInit {Where do we use this type?
DidRemoveGamepadEventListeners();Is this effectively the same as `DidRemoveAllEventListeners`?
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {
compare_all_axes = has_input_changed_event_listener_;
compare_all_buttons = has_input_changed_event_listener_;
}
Should we compare all axes/buttons even when there is no raw input change event listener attached?
Vector<int> buttons_pressed = compare_result.GetButtonsPressed(index);
Vector<int> buttons_released = compare_result.GetButtonsReleased(index);Are these excluded from the early exit below because encode the change in `buttons_value_changed`, which is already captured by the latter?
If so, can we move them after the early exit and add a comment describe why?
{ ```suggestion
{
```
nit: trailing whitespace
nit: remove this line and clean up trailing whitespace in this file.
Related details
- Gabriel Brito
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 11 comments![]( "Open in Gerrit")
Did a quick first pass today. Will do another one tomorrow.
raw_ref(features::kWebIdentityDigitalCredentials), kDefault},nit: Is this change required?
Maybe `git cl format` did it when Sneha modified the file before. I think that, if reverted, `format` won't complain since we won't have touched this file.
bool GamepadStateCompareResult::CompareTouches(Gamepad* old_gamepad,We should probably test this in [gamepad_comparisons_test.cc](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc?q=gamepad_comparisons&ss=chromium%2Fchromium%2Fsrc)
NotifyControllers();I think that this isn't the mechanism we want to use to trigger the event dispatching, right? Should we add a TODO comment here linking to https://issues.chromium.org/issues/438906421?
class GamepadRawInputChangeEvent : public Event {Is there any reason for this to not inherit from `GamepadEvent`?
GamepadRawInputChangeEvent::~GamepadRawInputChangeEvent() = default;super nit: add blank line
dictionary GamepadRawInputChangeEventInit : EventInit {Where do we use this type?
Although we don't use it directly in the code, I think that this is exposed to pages via js if they want to create `GamepadRawInputChangeEvent`s themselves?
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i]) {This is a confusing condition. How can `is_connected` and `is_disconnected` be both false for us to fire the event? What is the purpose of checking `gamepads_back_[i]`?
UseCounter::Count(context, WebFeature::kGamepadRawInputChangeEventListener);Should we move this to where the event handler is added to avoid calling it unnecessarily every time an event is to be dispatched?
settable_from_internals: true,Why do we need `settable_from_internals` and `public`?
nit: delete the white spaces in this file (pink squares in Gerrit)
Related details
- Sneha Agarwal
Sneha Agarwal (Gerrit)
Sneha Agarwal added 21 comments![]( "Open in Gerrit")
Hi Michael and Gabriel,
I have addressed your feedback.
Thanks
raw_ref(features::kWebIdentityDigitalCredentials), kDefault},Gabriel Britonit: Is this change required?
Maybe `git cl format` did it when Sneha modified the file before. I think that, if reverted, `format` won't complain since we won't have touched this file.
Done
return gin::Wrappable<GamepadControllerBindings>::GetObjectTemplateBuilder(
isolate)
.SetMethod("connect", &GamepadControllerBindings::Connect)
.SetMethod("dispatchConnected",
&GamepadControllerBindings::DispatchConnected)
.SetMethod("disconnect", &GamepadControllerBindings::Disconnect)
.SetMethod("setId", &GamepadControllerBindings::SetId)
.SetMethod("setButtonCount", &GamepadControllerBindings::SetButtonCount)
.SetMethod("setButtonData", &GamepadControllerBindings::SetButtonData)
.SetMethod("setAxisCount", &GamepadControllerBindings::SetAxisCount)
.SetMethod("setAxisData", &GamepadControllerBindings::SetAxisData)
.SetMethod("setDualRumbleVibrationActuator",
&GamepadControllerBindings::SetDualRumbleVibrationActuator)
.SetMethod("setTriggerRumbleVibrationActuator",
&GamepadControllerBindings::SetTriggerRumbleVibrationActuator)
.SetMethod("setTouchCount", &GamepadControllerBindings::SetTouchCount)
.SetMethod("setTouchData", &GamepadControllerBindings::SetTouchData)
.SetMethod("dispatchRawInputChanged",
&GamepadControllerBindings::DispatchRawInputChanged);```suggestion
return gin::Wrappable<GamepadControllerBindings>::GetObjectTemplateBuilder(
isolate)
.SetMethod("connect", &GamepadControllerBindings::Connect)
.SetMethod("dispatchConnected",
&GamepadControllerBindings::DispatchConnected)
.SetMethod("disconnect", &GamepadControllerBindings::Disconnect)
.SetMethod("dispatchRawInputChanged",
&GamepadControllerBindings::DispatchRawInputChanged)
.SetMethod("setId", &GamepadControllerBindings::SetId)
.SetMethod("setButtonCount", &GamepadControllerBindings::SetButtonCount)
.SetMethod("setButtonData", &GamepadControllerBindings::SetButtonData)
.SetMethod("setAxisCount", &GamepadControllerBindings::SetAxisCount)
.SetMethod("setAxisData", &GamepadControllerBindings::SetAxisData)
.SetMethod("setDualRumbleVibrationActuator",
&GamepadControllerBindings::SetDualRumbleVibrationActuator)
.SetMethod("setTriggerRumbleVibrationActuator",
&GamepadControllerBindings::SetTriggerRumbleVibrationActuator)
.SetMethod("setTouchCount", &GamepadControllerBindings::SetTouchCount)
.SetMethod("setTouchData", &GamepadControllerBindings::SetTouchData);
```
style nit: Consistent ordering
Done
void GamepadControllerBindings::DispatchRawInputChanged(int index) {
if (controller_) {
controller_->DispatchRawInputChanged(index);
}
}
void GamepadControllerBindings::Disconnect(int index) {
if (controller_)
controller_->Disconnect(index);
}```suggestion
void GamepadControllerBindings::Disconnect(int index) {
if (controller_)
controller_->Disconnect(index);
}void GamepadControllerBindings::DispatchRawInputChanged(int index) {
if (controller_) {
controller_->DispatchRawInputChanged(index);
}
}
```
style nit: Consistent ordering.
Done
void GamepadController::DispatchRawInputChanged(int index) {style nit: move above `GamepadController::SetId` for consistent ordering of methods
Done
bool GamepadStateCompareResult::CompareTouches(Gamepad* old_gamepad,We should probably test this in [gamepad_comparisons_test.cc](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc?q=gamepad_comparisons&ss=chromium%2Fchromium%2Fsrc)
Done
I think that this isn't the mechanism we want to use to trigger the event dispatching, right? Should we add a TODO comment here linking to https://issues.chromium.org/issues/438906421?
Done
Is there any reason for this to not inherit from `GamepadEvent`?
We only expose the gamepadSnapshot property and do not use or expose the standard gamepad attribute from GamepadEvent. Since this event is focused on providing a snapshot of the gamepad state at the time of the event, inheriting from Event is more appropriate than inheriting from GamepadEvent
```suggestion
```nit: remove empty line
Done
GamepadRawInputChangeEvent::~GamepadRawInputChangeEvent() = default;super nit: add blank line
Done
Gabriel BritoWhere do we use this type?
Although we don't use it directly in the code, I think that this is exposed to pages via js if they want to create `GamepadRawInputChangeEvent`s themselves?
Yes, that is correct.
FrozenArray<long> in the dictionary is exposed to JavaScript, so web developers can create their own GamepadRawInputChangeEvent objects and pass arrays for these properties.
Is this effectively the same as `DidRemoveAllEventListeners`?
yes it is functionally equivalent to calling DidRemoveAllEventListeners.
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {
compare_all_axes = has_input_changed_event_listener_;
compare_all_buttons = has_input_changed_event_listener_;
}
Should we compare all axes/buttons even when there is no raw input change event listener attached?
No we should not compare all axes and buttons when no listener is attached. I have improved the name of the variables and added a comment.
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i]) {This is a confusing condition. How can `is_connected` and `is_disconnected` be both false for us to fire the event? What is the purpose of checking `gamepads_back_[i]`?
Yes, both is_connected and is_disconnected are false when the gamepad's connection state did not change. It's present in both samples. This condition ensures we only dispatch a raw input change event for input changes (not connection/disconnection). The comment above makes that clear. The checks for gamepads_[i] and gamepads_back_[i] confirm the gamepad exists in both the current and previous states, so we don't fire input change events for gamepads that just appeared or disappeared. I have improved the comment to make the intent clearer.
Vector<int> buttons_pressed = compare_result.GetButtonsPressed(index);
Vector<int> buttons_released = compare_result.GetButtonsReleased(index);Are these excluded from the early exit below because encode the change in `buttons_value_changed`, which is already captured by the latter?
If so, can we move them after the early exit and add a comment describe why?
Sure, thanks for the suggestion, I have moved it and added comment.
UseCounter::Count(context, WebFeature::kGamepadRawInputChangeEventListener);Should we move this to where the event handler is added to avoid calling it unnecessarily every time an event is to be dispatched?
I think It should stay here as the counter metrics should record the actual feature usage and not just feature availability. If we move it to the event handler registration, it will count even if no events are ever dispatched, which would inflate the metric and not reflect real usage.
{ Sneha Agarwal```suggestion
{
```
nit: trailing whitespace
Done
Why do we need `settable_from_internals` and `public`?
Removed Settable_from_internals, but keeping public: true, so the feature is accessible to web content
nit: remove this line and clean up trailing whitespace in this file.
Done
nit: delete the white spaces in this file (pink squares in Gerrit)
- Gabriel Brito
- Michael Tang
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 7 comments![]( "Open in Gerrit")
I had very little time to review it today. Hope to be able to give more meaningful feedback about the tests soon.
if (!new_touches || !old_touches) {
return true;
}Should we add a test for this condition?
if (!new_touches || !old_touches) {
return true;
}```suggestion
if (!new_touches != !old_touches) {
return true;
}
```
Compare(new_touch->position().Get(), old_touch->position().Get())) {Are we testing this condition?
// Tests that a touch disappearing (gamepad with touch vs empty gamepad)What do you mean with this? It's not clear to me.
class GamepadRawInputChangeEvent : public Event {Sneha AgarwalIs there any reason for this to not inherit from `GamepadEvent`?
We only expose the gamepadSnapshot property and do not use or expose the standard gamepad attribute from GamepadEvent. Since this event is focused on providing a snapshot of the gamepad state at the time of the event, inheriting from Event is more appropriate than inheriting from GamepadEvent
Marked as unresolved.
I don't see this "standard gamepad attribute" in [GamepadEvent](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_event.idl). I suggest leaving this unresolved for now until Matt also have a look.
At the time of writing the explainer, we went with inheriting directly from `Event` because the gamepad object liveness issue had not been set yet. Now that we know that the event contains a snapshot and that the event target is the window (just like `GamepadEvent`), I don't see a reason for not doing this.
UseCounter::Count(context, WebFeature::kGamepadRawInputChangeEventListener);Sneha AgarwalShould we move this to where the event handler is added to avoid calling it unnecessarily every time an event is to be dispatched?
I think It should stay here as the counter metrics should record the actual feature usage and not just feature availability. If we move it to the event handler registration, it will count even if no events are ever dispatched, which would inflate the metric and not reflect real usage.
Marked as unresolved.
UseCounters are not UMA histograms. We are just interested in recording whether a web page is adopting the Gamepad Raw events API. For this, if a web page voluntarily adds an event listener for `rawgamepadchange`, this means that feature is being adopted and accounted for. In this sense, only the first call to `UseCounter::Count` during a navigation matters and is the one that will be recorded. All the following ones are not counted and are additional overhead that should be avoided.
Since RawInput events are dispatched with high frequency, this could be impactful. Ideally, we should be recording this only when a new raw input event listener is added.
Related details
- Michael Tang
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal voted and added 7 comments![]( "Open in Gerrit")
Votes added by Sneha Agarwal
| Commit-Queue | +1 |
7 comments
Hi Gabriel,
I have addressed some comments, currently working on the remaining one related to counter.
Thanks
if (!new_touches || !old_touches) {
return true;
}Should we add a test for this condition?
the Touch "disappear" test is the one for this condition. checks the case where one gamepad has touches and the other does not. I have improved the name of the test.
if (!new_touches || !old_touches) {
return true;
}```suggestion
if (!new_touches != !old_touches) {
return true;
}
```
Done
Compare(new_touch->position().Get(), old_touch->position().Get())) {Are we testing this condition?
Yes there was a pre-existing test "CompareDifferentSurfaceTouch" for this condition.
// Tests that a touch disappearing (gamepad with touch vs empty gamepad)What do you mean with this? It's not clear to me.
Means the gamepad that previously reported one or more touch events now reports no touches at all. I have improved the comment.
class GamepadRawInputChangeEvent : public Event {Sneha AgarwalIs there any reason for this to not inherit from `GamepadEvent`?
Gabriel BritoWe only expose the gamepadSnapshot property and do not use or expose the standard gamepad attribute from GamepadEvent. Since this event is focused on providing a snapshot of the gamepad state at the time of the event, inheriting from Event is more appropriate than inheriting from GamepadEvent
Marked as unresolved.
I don't see this "standard gamepad attribute" in [GamepadEvent](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_event.idl). I suggest leaving this unresolved for now until Matt also have a look.
At the time of writing the explainer, we went with inheriting directly from `Event` because the gamepad object liveness issue had not been set yet. Now that we know that the event contains a snapshot and that the event target is the window (just like `GamepadEvent`), I don't see a reason for not doing this.
Sure I will leave this open for Matt to Chime in.
UseCounter::Count(context, WebFeature::kGamepadRawInputChangeEventListener);Sneha AgarwalShould we move this to where the event handler is added to avoid calling it unnecessarily every time an event is to be dispatched?
Gabriel BritoI think It should stay here as the counter metrics should record the actual feature usage and not just feature availability. If we move it to the event handler registration, it will count even if no events are ever dispatched, which would inflate the metric and not reflect real usage.
Marked as unresolved.
UseCounters are not UMA histograms. We are just interested in recording whether a web page is adopting the Gamepad Raw events API. For this, if a web page voluntarily adds an event listener for `rawgamepadchange`, this means that feature is being adopted and accounted for. In this sense, only the first call to `UseCounter::Count` during a navigation matters and is the one that will be recorded. All the following ones are not counted and are additional overhead that should be avoided.
Since RawInput events are dispatched with high frequency, this could be impactful. Ideally, we should be recording this only when a new raw input event listener is added.
- Gabriel Brito
- Michael Tang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 10 comments![]( "Open in Gerrit")
Left a couple more of comments. Need to look at the tests next.
if (!new_touches || !old_touches) {
return true;
}Sneha AgarwalShould we add a test for this condition?
the Touch "disappear" test is the one for this condition. checks the case where one gamepad has touches and the other does not. I have improved the name of the test.
Marked as unresolved.
Are you sure that we are not returning early in the check below? Maybe all the elements in the list created with `CreateEmptyGamepadList()` are `nullptr`? It looks like your test is simulating a disconnection instead of a touch-up. See [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc;l=321;drc=6b98c207facca73446d15164ad3f9a62f4b41d11;bpv=1;bpt=1?q=CreateEmptyGamepadList&ss=chromium%2Fchromium%2Fsrc) for an example.
```
bool GamepadStateCompareResult::CompareTouches(Gamepad* old_gamepad,
Gamepad* new_gamepad,
size_t gamepad_index) {
if (!new_gamepad) {
return false;
}
[...]
```
TEST_F(GamepadComparisonsTest, CompareTouchCountMismatch) {In all these tests, shouldn't we also check the comparison results via `GamepadStateCompareResult::GetChangedTouches`?
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {Shouldn't we check for `has_input_changed_event_listener_` instead?
```
if (has_input_changed_event_listener_) {
```
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {if we are not firing "raw input" and "connection/disconnection" events in the same update, shouldn't this be an `else`?
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i]) {There is one case that your conditional is not capturing: if the gamepad has just connected (`is_connected == true`) but there are no connection event listeners, shouldn't we fire a rawinputevent?
is_gamepads_back_exposed = true;This does not seem correct. We are not exposing the back buffer. Remove?
// Detect raw input changes for this gamepad index.nit: This comment seems redundant. Remove?
// Early exit if there are no axis, button value, or touch changes.nit: ditto
ExecutionContext* context = DomWindow();nit: Just call `DomWindow()` directly in `UseCounter::Count` and remove the extra variable `context`.
Related details
- Michael Tang
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal restored this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal added 11 comments![]( "Open in Gerrit")
Hi Gabriel,
Please take a look at the comments I addressed.
Thanks
if (!new_touches || !old_touches) {
return true;
}Sneha AgarwalShould we add a test for this condition?
Gabriel Britothe Touch "disappear" test is the one for this condition. checks the case where one gamepad has touches and the other does not. I have improved the name of the test.
Marked as unresolved.
Are you sure that we are not returning early in the check below? Maybe all the elements in the list created with `CreateEmptyGamepadList()` are `nullptr`? It looks like your test is simulating a disconnection instead of a touch-up. See [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_comparisons_test.cc;l=321;drc=6b98c207facca73446d15164ad3f9a62f4b41d11;bpv=1;bpt=1?q=CreateEmptyGamepadList&ss=chromium%2Fchromium%2Fsrc) for an example.
```
bool GamepadStateCompareResult::CompareTouches(Gamepad* old_gamepad,
Gamepad* new_gamepad,
size_t gamepad_index) {
if (!new_gamepad) {
return false;
}
[...]
```
thanks, updated the test to simulate touch up
TEST_F(GamepadComparisonsTest, CompareTouchCountMismatch) {In all these tests, shouldn't we also check the comparison results via `GamepadStateCompareResult::GetChangedTouches`?
Sure we can do that
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {Shouldn't we check for `has_input_changed_event_listener_` instead?
```
if (has_input_changed_event_listener_) {
```
Yes, we can do that.
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {if we are not firing "raw input" and "connection/disconnection" events in the same update, shouldn't this be an `else`?
Done
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i]) {There is one case that your conditional is not capturing: if the gamepad has just connected (`is_connected == true`) but there are no connection event listeners, shouldn't we fire a rawinputevent?
I am not sure if I understand this comment. Please clarify. The raw input change event should not depend on whether or not connection event listeners are there. the connection event listener should be checked by the logic that dispatches the connection event.
This does not seem correct. We are not exposing the back buffer. Remove?
Done
nit: This comment seems redundant. Remove?
Done
// Early exit if there are no axis, button value, or touch changes.Sneha Agarwalnit: ditto
Done
UseCounter::Count(context, WebFeature::kGamepadRawInputChangeEventListener);Sneha AgarwalShould we move this to where the event handler is added to avoid calling it unnecessarily every time an event is to be dispatched?
I think It should stay here as the counter metrics should record the actual feature usage and not just feature availability. If we move it to the event handler registration, it will count even if no events are ever dispatched, which would inflate the metric and not reflect real usage.
Marked as unresolved.
UseCounters are not UMA histograms. We are just interested in recording whether a web page is adopting the Gamepad Raw events API. For this, if a web page voluntarily adds an event listener for `rawgamepadchange`, this means that feature is being adopted and accounted for. In this sense, only the first call to `UseCounter::Count` during a navigation matters and is the one that will be recorded. All the following ones are not counted and are additional overhead that should be avoided.
Since RawInput events are dispatched with high frequency, this could be impactful. Ideally, we should be recording this only when a new raw input event listener is added.
Ok I see your point. I can try your suggestion. thanks
Done
nit: Just call `DomWindow()` directly in `UseCounter::Count` and remove the extra variable `context`.
- Gabriel Brito
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal restored this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Tang (Gerrit)
Michael Tang added 5 comments![]( "Open in Gerrit")
Hi! Sorry for the delay-- I only have minor nits on the implementation. I will review the tests more thoroughly tomorrow!
if (!new_touches != !old_touches) {nit: I think these are equivalent.
```suggestion
if (new_touches != old_touches) {
```
if (has_input_changed_event_listener_) {
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = has_input_changed_event_listener_;
should_compare_all_buttons = has_input_changed_event_listener_;
}Thanks for the comment.
nit: functionally identical
```suggestion
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = has_input_changed_event_listener_;
should_compare_all_buttons = has_input_changed_event_listener_;
```
or:
```suggestion
if (has_input_changed_event_listener_) {
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = true;
should_compare_all_buttons = true;
}
```
!is_disconnected && gamepads_[i] && gamepads_back_[i]) {Is this redundant with checking for `!is_connected && !is_disconnected`? IIUC, `is_connected == (!gamepads_back_[i] && gamepads_[i])`, while `is_disconnected == (gamepads_back_[i] && !gamepads_[i])`. Is this correct?
Related details
- Gabriel Brito
- Michael Tang
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 1 comment![]( "Open in Gerrit")
if (!new_touches != !old_touches) {nit: I think these are equivalent.
```suggestion
if (new_touches != old_touches) {
```
Quick comment. I don't think these are equivalent. The `operator!` is there to coerce a conversion to `bool` - i.e. to tell us whether the pointer is `nullptr` or not. The way it is written now is basically a xor operation where the operands are "is the pointer nullptr" so it will return `true` whenever one of the pointer is `nullptr` and the other isn't.
If we compare the pointers directly, won't we be comparing only the pointer addresses? I imagine it would return `true` if both pointers are valid but point to different memory addresses.
Related details
- Michael Tang
- Sneha Agarwal
Michael Tang (Gerrit)
Michael Tang added 1 comment![]( "Open in Gerrit")
if (!new_touches != !old_touches) {Gabriel Britonit: I think these are equivalent.
```suggestion
if (new_touches != old_touches) {
```
Quick comment. I don't think these are equivalent. The `operator!` is there to coerce a conversion to `bool` - i.e. to tell us whether the pointer is `nullptr` or not. The way it is written now is basically a xor operation where the operands are "is the pointer nullptr" so it will return `true` whenever one of the pointer is `nullptr` and the other isn't.
If we compare the pointers directly, won't we be comparing only the pointer addresses? I imagine it would return `true` if both pointers are valid but point to different memory addresses.
You're totally right. I wonder if, for clarity, we should have this (and the above condition) use `*_gamepad->HasTouchEvents()` instead and then follow up with `CHECK`s that `*_touches` are non-null.
What you have here is otherwise fine. Please feel free to close with no action.
Sneha Agarwal (Gerrit)
Sneha Agarwal added 5 comments![]( "Open in Gerrit")
Hi Michael,
I have addressed your comments. Thanks
nit: I think these are equivalent.
```suggestion
if (new_touches != old_touches) {
```
I think this change is not correct for this context.
!new_touches != !old_touches specifically checks if one pointer is null and the other is not, which means one gamepad has touches and the other does not.
new_touches != old_touches would also be true if both are non-null but point to different objects, which is not the intent here.
The original code is more precise for detecting the case where only one side has touches.
if (has_input_changed_event_listener_) {
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = has_input_changed_event_listener_;
should_compare_all_buttons = has_input_changed_event_listener_;
}Thanks for the comment.
nit: functionally identical
```suggestion
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = has_input_changed_event_listener_;
should_compare_all_buttons = has_input_changed_event_listener_;
```or:
```suggestion
if (has_input_changed_event_listener_) {
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = true;
should_compare_all_buttons = true;
}
```
Done
!is_disconnected && gamepads_[i] && gamepads_back_[i]) {Is this redundant with checking for `!is_connected && !is_disconnected`? IIUC, `is_connected == (!gamepads_back_[i] && gamepads_[i])`, while `is_disconnected == (gamepads_back_[i] && !gamepads_[i])`. Is this correct?
No, the check !is_connected && !is_disconnected && gamepads_[i] && gamepads_back_[i] is not redundant. This logic considers both connection state and ID changes, not just pointer presence. They are computed by comparing the connection state and ID of the gamepads in the previous and current samples.
The proposed equivalence:
is_connected == (!gamepads_back_[i] && gamepads_[i])
is_disconnected == (gamepads_back_[i] && !gamepads_[i])
is not correct.
The actual logic considers both connection state and ID changes, not just pointer presence.
The current logic is necessary to distinguish between connection/disconnection events and continuous input changes.
nit: Remove extra new line
Done
Related details
- Michael Tang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 6 comments![]( "Open in Gerrit")
Still need to review device/gamepad/gamepad_service_unittest.cc. Also please don't forget to dry run the latest patchset. Always upload a new one with `git cl upload -d`.
TEST_F(GamepadComparisonsTest, CompareTouchCountMismatch) {Sneha AgarwalIn all these tests, shouldn't we also check the comparison results via `GamepadStateCompareResult::GetChangedTouches`?
Sure we can do that
Marked as unresolved.
For all these tests, shouldn't we also check that what is being reported as changed is what we expect?
const auto& changed_touches = compareResult.GetChangedTouches(0);
EXPECT_FALSE(changed_touches.empty());```suggestion
EXPECT_FALSE(compareResult.GetChangedTouches(0).empty());
```
I don't think you need to create these variables. Please apply this to the other test cases too.
EXPECT_TRUE(changed_touches.empty());Shouldn't we expect to see at least one change?
bool should_compare_all_axes = false;
bool should_compare_all_buttons = false;
if (has_input_changed_event_listener_) {
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = true;
should_compare_all_buttons = true;
}
GamepadStateCompareResult compare_result = GamepadComparisons::Compare(
gamepads_, gamepads_back_, should_compare_all_axes,
should_compare_all_buttons);Do we need to have 2 different arguments for `GamepadComparisons::Compare`? Can't we merge them into one `should_compare_all_inputs`?
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {Sneha Agarwalif we are not firing "raw input" and "connection/disconnection" events in the same update, shouldn't this be an `else`?
Done
Marked as unresolved.
The `else if` does not feel correct to me. IIUC, if a connection/disconnection event is fired, we don't need to fire a rawinput event in the same cycle. Otherwise, we are firing an extra and unnecessary event as connection/disconnection event also contain a gamepad snapshot. What I mean is:
```
if should fire connect or disconnect event:
fire events
else if should fire raw input event:
fire raw input event
```
This should also simplify the conditions checked in
```
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i])
```
cc: @ta...@microsoft.com
Related details
- Michael Tang
- Sneha Agarwal
Sneha Agarwal (Gerrit)
Sneha Agarwal added 6 comments![]( "Open in Gerrit")
HI Gabriel, Please, take a look at the change/comments. Thanks
TEST_F(GamepadComparisonsTest, CompareTouchCountMismatch) {Sneha AgarwalIn all these tests, shouldn't we also check the comparison results via `GamepadStateCompareResult::GetChangedTouches`?
Gabriel BritoSure we can do that
Marked as unresolved.
For all these tests, shouldn't we also check that what is being reported as changed is what we expect?
I can only add that for the tests that have some property changed at an index not for the ones that detect the number of touches changed.
const auto& changed_touches = compareResult.GetChangedTouches(0);
EXPECT_FALSE(changed_touches.empty());```suggestion
EXPECT_FALSE(compareResult.GetChangedTouches(0).empty());
```
I don't think you need to create these variables. Please apply this to the other test cases too.
Done
Shouldn't we expect to see at least one change?
No, in this test, the position of the touch does not change. The test is about the removal of a touch event (the touch count goes from 1 to 0), not about a change in the properties (such as position) of an existing touch.
If the position were to change, both gamepad states would still have a touch at the same index, and GetChangedTouches(0) would contain that index, indicating a property change. In this test, since the touch is removed entirely, there is no touch in the second state to compare positions with, and GetChangedTouches(0) should be empty.
bool should_compare_all_axes = false;
bool should_compare_all_buttons = false;
if (has_input_changed_event_listener_) {
// Only compare all axes/buttons if a raw input change event listener is
// present. This avoids unnecessary comparisons when no listener is
// attached.
should_compare_all_axes = true;
should_compare_all_buttons = true;
}
GamepadStateCompareResult compare_result = GamepadComparisons::Compare(
gamepads_, gamepads_back_, should_compare_all_axes,
should_compare_all_buttons);Do we need to have 2 different arguments for `GamepadComparisons::Compare`? Can't we merge them into one `should_compare_all_inputs`?
I think we should keep it as it maintains clarity in the code. Passing a generic compare_all_inputs flag to CompareAxes and CompareButtons can be confusing, since those functions expect a flag specific to their domain (axes or buttons)
Also avoiding unnecessary refactoring in SampleAndCompareGamepadState method - as future event queuing change will be getting rid of this method.
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {Sneha Agarwalif we are not firing "raw input" and "connection/disconnection" events in the same update, shouldn't this be an `else`?
Gabriel BritoDone
Marked as unresolved.
The `else if` does not feel correct to me. IIUC, if a connection/disconnection event is fired, we don't need to fire a rawinput event in the same cycle. Otherwise, we are firing an extra and unnecessary event as connection/disconnection event also contain a gamepad snapshot. What I mean is:
```
if should fire connect or disconnect event:
fire events
else if should fire raw input event:
fire raw input event
```This should also simplify the conditions checked in
```
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i])
```
cc: @ta...@microsoft.com
Functionally, both statements cannot execute at the same time because the conditions being checked are mutually exclusive. Adding else does not change the behavior, since the explicit conditions already prevent double-firing. I prefer to check the conditions explicitly for clarity and safety. At this point, I think this is a matter of style rather than correctness.
Related details
- Gabriel Brito
- Michael Tang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 6 comments![]( "Open in Gerrit")
I think that it seems good to me overall % open questions
base::UnguessableToken SetupRawInputGamepad(You can make this method simpler like this:
```
base::UnguessableToken SetupRawInputGamepad(
MockGamepadConsumer* consumer,
SimulatedGamepadParams params) {
params.name = "Raw input test gamepad";
[...]
};
```
Then use it like:
```
SimulatedGamepadParams params = {
.button_bounds = {std::nullopt, std::nullopt},
.axis_bounds = {GamepadLogicalBounds(-1.0, 1.0)}
};
auto token = SetupRawInputGamepad(consumer, std::move(params));
```
// Helper to expect and verify a raw input change callback.
template <typename VerifyFunction>
auto ExpectRawInputChange(MockGamepadConsumer* consumer,
VerifyFunction verify_func) {
auto raw_input_future =
std::make_shared<TestFuture<uint32_t, const Gamepad&>>();
EXPECT_CALL(*consumer, OnGamepadRawInputChanged)
.WillOnce(InvokeFuture(*raw_input_future));
// Return a lambda that captures the shared_ptr by value
return [raw_input_future, verify_func]() {
EXPECT_EQ(raw_input_future->Get<0>(), 0u);
verify_func(raw_input_future->Get<1>());
};
}Is it being used?
EXPECT_EQ(disconnected_future.Get<0>(), 0u);Similarly to `SetupRawInputGamepad` should we also `EXPECT_FALSE(connected_future.Get<1>().connected)`?
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {Sneha Agarwalif we are not firing "raw input" and "connection/disconnection" events in the same update, shouldn't this be an `else`?
Gabriel BritoDone
Sneha AgarwalMarked as unresolved.
The `else if` does not feel correct to me. IIUC, if a connection/disconnection event is fired, we don't need to fire a rawinput event in the same cycle. Otherwise, we are firing an extra and unnecessary event as connection/disconnection event also contain a gamepad snapshot. What I mean is:
```
if should fire connect or disconnect event:
fire events
else if should fire raw input event:
fire raw input event
```This should also simplify the conditions checked in
```
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i])
```
cc: @ta...@microsoft.com
Functionally, both statements cannot execute at the same time because the conditions being checked are mutually exclusive. Adding else does not change the behavior, since the explicit conditions already prevent double-firing. I prefer to check the conditions explicitly for clarity and safety. At this point, I think this is a matter of style rather than correctness.
Although the conditions might be mutually exclusive and work as expected, I am trying to say that this `if` condition looks unnecessarily complicated and the mutual exclusivity is not trivial to perceive. For example, the checks below are mutually exclusive:
```
if (a && !b && !c && d):
Do TaskA
if (!a || b || c || !d):
Do TaskB
```
But they could be simplified by means of an `else`:
```
if (a && !b && !c && d):
Do TaskA
else:
Do TaskB
```
Can't we rewrite it in a clearer way? If it is true that "_if a connection/disconnection event is fired, we don't need to fire a rawinput event in the same cycle_", then can't we use this information to simplify the rawinput event check?
I suggest leaving this open for now so that Matt can give his opinion as an editor of the specification.
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i]) {Sneha AgarwalThere is one case that your conditional is not capturing: if the gamepad has just connected (`is_connected == true`) but there are no connection event listeners, shouldn't we fire a rawinputevent?
I am not sure if I understand this comment. Please clarify. The raw input change event should not depend on whether or not connection event listeners are there. the connection event listener should be checked by the logic that dispatches the connection event.
Sure. There must be a raw input event listener too. The case that I am referring to is:
- Gamepad has just connected; and
- `has_input_changed_event_listener_ = true`
- `has_connection_event_listener_ = false`
- `is_connected = true`
Let's sidetrack this for now since we haven't committed to a specification yet (also what I asked about might not be desired).
Related details
- Michael Tang
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal added 4 comments![]( "Open in Gerrit")
Hi Matt,
When you have a chance please take a look at this change. There are a couple open comments that could use your inputs.
Thanks
base::UnguessableToken SetupRawInputGamepad(You can make this method simpler like this:
```
base::UnguessableToken SetupRawInputGamepad(
MockGamepadConsumer* consumer,
SimulatedGamepadParams params) {
params.name = "Raw input test gamepad";
[...]
};
```
Then use it like:
```
SimulatedGamepadParams params = {
.button_bounds = {std::nullopt, std::nullopt},
.axis_bounds = {GamepadLogicalBounds(-1.0, 1.0)}
};
auto token = SetupRawInputGamepad(consumer, std::move(params));
```
I will try it out thanks
// Helper to expect and verify a raw input change callback.
template <typename VerifyFunction>
auto ExpectRawInputChange(MockGamepadConsumer* consumer,
VerifyFunction verify_func) {
auto raw_input_future =
std::make_shared<TestFuture<uint32_t, const Gamepad&>>();
EXPECT_CALL(*consumer, OnGamepadRawInputChanged)
.WillOnce(InvokeFuture(*raw_input_future));
// Return a lambda that captures the shared_ptr by value
return [raw_input_future, verify_func]() {
EXPECT_EQ(raw_input_future->Get<0>(), 0u);
verify_func(raw_input_future->Get<1>());
};
}Sneha AgarwalIs it being used?
Yes, the ExpectRawInputChange helper is used in the test
Similarly to `SetupRawInputGamepad` should we also `EXPECT_FALSE(connected_future.Get<1>().connected)`?
- Matt Reynolds
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Reynolds (Gerrit)
Matt Reynolds added 4 comments![]( "Open in Gerrit")
I think there should also be a new event listener attribute window.ongamepadrawinputchange. We recently added these for ongamepadconnected and ongamepaddisconnected but they haven't launched yet: https://crrev.com/c/6984681 ongamepadrawinputchange should also be added in window_gamepad.idl but we'll need to fix the RuntimeEnabled attributes so it isn't gated on GamepadWindowEventHandlers.
// Record that there wasn't an observer to get the GamepadRawInputChanged
// RPC so we can send it when SetObserver gets called.
missed_dispatches_.set(index);`missed_dispatches_` should only be set in DispatchConnected. I don't think we need similar behavior for GamepadRawInputChanged.
auto ExpectRawInputChange(MockGamepadConsumer* consumer,ExpectRawInputChange appears unused, please remove
base::RunLoop loop;
EXPECT_CALL(*consumer, OnGamepadRawInputChanged)
.WillOnce([&](uint32_t, const Gamepad& gamepad) {
EXPECT_EQ(gamepad.buttons_length, 1u);
EXPECT_EQ(gamepad.buttons[0].value, 0.5);
loop.Quit();
});Prefer using `base::test::TestFuture` to capture callback parameters instead of nesting expectations inside the callback.
Related details
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 1 comment![]( "Open in Gerrit")
name: "GamepadWindowRawInputChangeEventHandler",We should probably reuse the `GamepadRawInputChangeEvent` runtime flag instead of creating a new one.
Related details
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sneha Agarwal (Gerrit)
Sneha Agarwal added 8 comments![]( "Open in Gerrit")
I have addressed your comments.
Matt there is an open comment that needs your attention please.
FYI: Merge conflict is in the enum.xml file which updates frequently, I will do a rebase soon to fix that. Please ignore that for now.
Thanks
I think there should also be a new event listener attribute window.ongamepadrawinputchange. We recently added these for ongamepadconnected and ongamepaddisconnected but they haven't launched yet: https://crrev.com/c/6984681 ongamepadrawinputchange should also be added in window_gamepad.idl but we'll need to fix the RuntimeEnabled attributes so it isn't gated on GamepadWindowEventHandlers.
Done
// Record that there wasn't an observer to get the GamepadRawInputChanged
// RPC so we can send it when SetObserver gets called.
missed_dispatches_.set(index);`missed_dispatches_` should only be set in DispatchConnected. I don't think we need similar behavior for GamepadRawInputChanged.
Done
base::UnguessableToken SetupRawInputGamepad(Sneha AgarwalYou can make this method simpler like this:
```
base::UnguessableToken SetupRawInputGamepad(
MockGamepadConsumer* consumer,
SimulatedGamepadParams params) {
params.name = "Raw input test gamepad";
[...]
};
```
Then use it like:
```
SimulatedGamepadParams params = {
.button_bounds = {std::nullopt, std::nullopt},
.axis_bounds = {GamepadLogicalBounds(-1.0, 1.0)}
};
auto token = SetupRawInputGamepad(consumer, std::move(params));
```
I will try it out thanks
Done
auto ExpectRawInputChange(MockGamepadConsumer* consumer,ExpectRawInputChange appears unused, please remove
Done
// Helper to expect and verify a raw input change callback.
template <typename VerifyFunction>
auto ExpectRawInputChange(MockGamepadConsumer* consumer,
VerifyFunction verify_func) {
auto raw_input_future =
std::make_shared<TestFuture<uint32_t, const Gamepad&>>();
EXPECT_CALL(*consumer, OnGamepadRawInputChanged)
.WillOnce(InvokeFuture(*raw_input_future));
// Return a lambda that captures the shared_ptr by value
return [raw_input_future, verify_func]() {
EXPECT_EQ(raw_input_future->Get<0>(), 0u);
verify_func(raw_input_future->Get<1>());
};
}Sneha AgarwalIs it being used?
Yes, the ExpectRawInputChange helper is used in the test
Actually it was being used in an older iteration but not anymore, so I will delete it. Thanks
base::RunLoop loop;
EXPECT_CALL(*consumer, OnGamepadRawInputChanged)
.WillOnce([&](uint32_t, const Gamepad& gamepad) {
EXPECT_EQ(gamepad.buttons_length, 1u);
EXPECT_EQ(gamepad.buttons[0].value, 0.5);
loop.Quit();
});Prefer using `base::test::TestFuture` to capture callback parameters instead of nesting expectations inside the callback.
Done
class GamepadRawInputChangeEvent : public Event {Is there any reason for this to not inherit from `GamepadEvent`?
We only expose the gamepadSnapshot property and do not use or expose the standard gamepad attribute from GamepadEvent. Since this event is focused on providing a snapshot of the gamepad state at the time of the event, inheriting from Event is more appropriate than inheriting from GamepadEvent
Marked as unresolved.
I don't see this "standard gamepad attribute" in [GamepadEvent](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_event.idl). I suggest leaving this unresolved for now until Matt also have a look.
At the time of writing the explainer, we went with inheriting directly from `Event` because the gamepad object liveness issue had not been set yet. Now that we know that the event contains a snapshot and that the event target is the window (just like `GamepadEvent`), I don't see a reason for not doing this.
Sure I will leave this open for Matt to Chime in.
- Gabriel Brito
- Matt Reynolds
Matt Reynolds (Gerrit)
Matt Reynolds added 7 comments![]( "Open in Gerrit")
base::test::TestFuture<uint32_t, const Gamepad&> future;nit: this file has `using base::test::TestFuture` at the top so we can omit the `base::test::` prefix here and below.
auto [id, gamepad] = future.Take();nit: Prefer const auto& for structured bindings to avoid unnecessary copies
```suggestion
const auto& [id, gamepad] = future.Take();
```
DCHECK_LT(pad_index, device::Gamepads::kItemsLengthCap);
DCHECK_LT(touch_index, device::Gamepad::kTouchEventsLengthCap);In new code, prefer CHECK over DCHECK.
```suggestion
CHECK_LT(pad_index, device::Gamepads::kItemsLengthCap);
CHECK_LT(touch_index, device::Gamepad::kTouchEventsLengthCap);
```
virtual void ButtonOrAxisDidChange(uint32_t index,ButtonOrAxisDidChange is unused (implemented by GamepadDispatcher but never called) and was missed when I cleaned up the previous implementation. Please remove ButtonOrAxisDidChange in this CL to avoid confusion.
class GamepadRawInputChangeEvent : public Event {Sneha AgarwalIs there any reason for this to not inherit from `GamepadEvent`?
Gabriel BritoWe only expose the gamepadSnapshot property and do not use or expose the standard gamepad attribute from GamepadEvent. Since this event is focused on providing a snapshot of the gamepad state at the time of the event, inheriting from Event is more appropriate than inheriting from GamepadEvent
Sneha AgarwalMarked as unresolved.
I don't see this "standard gamepad attribute" in [GamepadEvent](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/gamepad/gamepad_event.idl). I suggest leaving this unresolved for now until Matt also have a look.
At the time of writing the explainer, we went with inheriting directly from `Event` because the gamepad object liveness issue had not been set yet. Now that we know that the event contains a snapshot and that the event target is the window (just like `GamepadEvent`), I don't see a reason for not doing this.
Sneha AgarwalSure I will leave this open for Matt to Chime in.
@mattre...@chromium.org Please share your input. Thanks
Sorry for the delay. If the .gamepadSnapshot is equivalent to .gamepad on other gamepad events (I think this is true) then it should be renamed to match. I agree with Gabriel, I don't see any reason not to inherit from GamepadEvent.
if (RuntimeEnabledFeatures::GamepadRawInputChangeEventEnabled()) {Sneha Agarwalif we are not firing "raw input" and "connection/disconnection" events in the same update, shouldn't this be an `else`?
Done
Marked as unresolved.
The `else if` does not feel correct to me. IIUC, if a connection/disconnection event is fired, we don't need to fire a rawinput event in the same cycle. Otherwise, we are firing an extra and unnecessary event as connection/disconnection event also contain a gamepad snapshot. What I mean is:
```
if should fire connect or disconnect event:
fire events
else if should fire raw input event:
fire raw input event
```This should also simplify the conditions checked in
```
if (has_input_changed_event_listener_ && !is_connected &&
!is_disconnected && gamepads_[i] && gamepads_back_[i])
```
cc: @ta...@microsoft.com
Gabriel BritoFunctionally, both statements cannot execute at the same time because the conditions being checked are mutually exclusive. Adding else does not change the behavior, since the explicit conditions already prevent double-firing. I prefer to check the conditions explicitly for clarity and safety. At this point, I think this is a matter of style rather than correctness.
Although the conditions might be mutually exclusive and work as expected, I am trying to say that this `if` condition looks unnecessarily complicated and the mutual exclusivity is not trivial to perceive. For example, the checks below are mutually exclusive:
```
if (a && !b && !c && d):
Do TaskA
if (!a || b || c || !d):
Do TaskB
```But they could be simplified by means of an `else`:
```
if (a && !b && !c && d):
Do TaskA
else:
Do TaskB
```Can't we rewrite it in a clearer way? If it is true that "_if a connection/disconnection event is fired, we don't need to fire a rawinput event in the same cycle_", then can't we use this information to simplify the rawinput event check?
I suggest leaving this open for now so that Matt can give his opinion as an editor of the specification.
My preference would be to factor out a method that processes changes for a single gamepad. The method could use early exits to guarantee that only one event is dispatched. (Chromium style generally prefers early exits over nested code blocks.)
attribute EventHandler ongamepadrawinputchanged;Instead of adding another partial interface, let's merge them into one partial interface and move the `RuntimeEnabled`s to the attribute level.
```
[
ImplementedAs=DOMWindowGamepad
] partial interface mixin WindowEventHandlers {
[RuntimeEnabled=GamepadWindowEventHandlers] attribute EventHandler ongamepadconnected;
[RuntimeEnabled=GamepadWindowEventHandlers] attribute EventHandler ongamepaddisconnected;
[RuntimeEnabled=GamepadRawInputChangeEvent] attribute EventHandler ongamepadrawinputchanged;
};
```
Related details
- Gabriel Brito
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Brito (Gerrit)
Gabriel Brito added 8 comments![]( "Open in Gerrit")
// auto token = SetupRawInputGamepad(consumer);nit: delete?
const auto [id, gamepad] = future.Take();Shouldn't it be `const auto&`? Same for the other occurrences
// Stores an immutable (frozen) snapshot of the gamepad state at the time the
// event was created.nit: seems out of place now. Remove?
#include "third_party/blink/renderer/modules/gamepad/gamepad.h"nit: not needed? You can get away with forward declaring `Gamepad`
#include "third_party/blink/renderer/bindings/modules/v8/v8_gamepad_raw_input_change_event_init.h"nit: not needed? You can get away with forward declaring `GamepadRawInputChangeEventInit`.
void NavigatorGamepad::DispatchGamepadEvent(I don't think that creating this new method helps with the confusing logic in the `if` statement. I would revert it. Also, it is not clear that this method is called only if `compare_result.IsDifferent()` is true.
gamepads_[index] && gamepads_back_[index]) {Is it really necessary to check for `gamepads_[index]` and `gamepads_back_[index]` in the `if` statement above since we know that `compare_result.IsDifferent()` is true?
Are there any other scenarios besides connection where this would be true? See how we are already not firing it if either `is_connected` or `is_disconnected` are true? maybe we can remove the `gamepads_[index] && gamepads_back_[index]` check altogether?
```
// The gamepad exists in both the current and previous samples
// This ensures we only report input changes for gamepads that are
// continuously present
```
is_gamepads_exposed_ = true;Where are we exposing `gamepads_back_` to set this to true?
Related details
- Sneha Agarwal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |