[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. |