[Gamepad] Prototype GamepadButton.type for extended buttons (flagged) [chromium/src : main]
Hongchan Choi (Gerrit)
Hongchan Choi added 1 comment![]( "Open in Gerrit")
Also adding robpitkin@ as a second reviewer on the gamepad API.
Related details
- Matt Reynolds
- Rob Pitkin
- Sun Shin US
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rob Pitkin (Gerrit)
Rob Pitkin added 4 comments![]( "Open in Gerrit")
Overall looks good! Just some nits. Will also wait for @mattre...@chromium.org to review since he has the context from the original spec issue.
GAMEPAD_FEATURES_EXPORT BASE_DECLARE_FEATURE(kEnableGamepadButtonTypes);This flag isn't used anywhere in the device service, is this intentional/needed?
switch (*product) {Can we pull these out to constexpr variables the way we do with `kSonyVendorId`?
```suggestion
switch (*product) {
case kDualShock4Pid:
case kDualShock4V2Pid:
return true;
default:
return false;
}
```
case 0x09cc: // DS4 v2 / DS4 variantsShould we also be checking for DualSense or DualSense Edge gamepads?
Related details
- Matt Reynolds
- Sun Shin US
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. |
Nate Chapin (Gerrit)
Nate Chapin added 3 comments![]( "Open in Gerrit")
Drive-by nitpicks on IDL and v8 bindings
V8GamepadButtonType type_ =It's a little more efficient to store this as a `V8GamepadButtonType::Enum` and only wrap it in the `V8GamepadButtonType` object when returning in `type()` (the object is a helper for the converstion from C++ enum to v8::String).
Operating on `V8GamepadButtonType::Enum` all the way through will probably remove a bunch of boilerplate as well 😊
readonly attribute GamepadButtonType type;You need an entry in runtime_enabled_features.json5 and `[RuntimeEnabled=GamepadButtonTypes]` here to ensure this API surface isn't shipped by default.
(you can have that generate a feature flag for you that is automatically tied to the RuntimeEnabledFeature).
Related details
- Matt Reynolds
- Sun Shin US
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 3 comments![]( "Open in Gerrit")
Spec: w3c/gamepad#196nit: Let's use the actual URL here so it's easier to get to the PR
```suggestion
Spec: https://github.com/w3c/gamepad/pull/196
```
GamepadIdentityHint hint = InferIdentityHintFromIdString(id_);I think we shouldn't try to parse the vendor and product IDs from the ID string.
Instead, let's do this detection in the backend where we already have the vendor and product ID values, and set the button's type as a new field in `GamepadButton`.
enum GamepadButtonType {Please add some comments documenting the enum and each value.
Related details
- Sun Shin US
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. |
Sun Shin US (Gerrit)
Sun Shin US added 5 comments![]( "Open in Gerrit")
nit: Let's use the actual URL here so it's easier to get to the PR
```suggestion
Spec: https://github.com/w3c/gamepad/pull/196
```
@Matt, thank for the review.
GAMEPAD_FEATURES_EXPORT BASE_DECLARE_FEATURE(kEnableGamepadButtonTypes);This flag isn't used anywhere in the device service, is this intentional/needed?
@Rob,thanks for the review. removed redundant codes.
GamepadIdentityHint hint = InferIdentityHintFromIdString(id_);I think we shouldn't try to parse the vendor and product IDs from the ID string.
Instead, let's do this detection in the backend where we already have the vendor and product ID values, and set the button's type as a new field in `GamepadButton`.
Marked as resolved.
Please add some comments documenting the enum and each value.
@Matt, thanks for the review. Updated.
You need an entry in runtime_enabled_features.json5 and `[RuntimeEnabled=GamepadButtonTypes]` here to ensure this API surface isn't shipped by default.
(you can have that generate a feature flag for you that is automatically tied to the RuntimeEnabledFeature).
@Nate, thanks for the review. And updated the codes by your recommendation.
Related details
- Matt Reynolds
- Nate Chapin
- Rob Pitkin
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. |
Rob Pitkin (Gerrit)
Rob Pitkin voted and added 1 comment![]( "Open in Gerrit")
- Matt Reynolds
- Nate Chapin
- Sun Shin US
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. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
readonly attribute GamepadButtonType type;Sun Shin USYou need an entry in runtime_enabled_features.json5 and `[RuntimeEnabled=GamepadButtonTypes]` here to ensure this API surface isn't shipped by default.
(you can have that generate a feature flag for you that is automatically tied to the RuntimeEnabledFeature).
@Nate, thanks for the review. And updated the codes by your recommendation.
Thanks! As expected, you now have test failures for
`webexposed/global-interface-listing.html` but NOT
`virtual/stable/webexposed/global-interface-listing.html`. This shows that you have added a feature that is available in experimental builds but not in stable. You can update the test expectations for `webexposed/global-interface-listing.html` to match the new output.
In terms of process, you should create a [chromestatus](https://chromestatus.com/) entry if you haven't done so already, and send an `Intent to Prototype` before committing this CL.
Related details
- Matt Reynolds
- Sun Shin US
Matt Reynolds (Gerrit)
Matt Reynolds added 1 comment![]( "Open in Gerrit")
const size_t length =
std::min<size_t>(gamepad->buttons_length, BUTTON_INDEX_COUNT);
for (size_t i = 0; i < length; ++i) {
gamepad->buttons[i].type = GamepadButtonType::kStandard;
}This will set the button type to `"standard"` for any button, even buttons that are known to not exist on the gamepad. For instance, MapperDjiFpv sets the primary, secondary, tertiary, quaternary buttons to `NullButton()` because there are no corresponding buttons on the device:
I don't remember whether we intended this feature to expose whether a button is present on the gamepad. The spec PR says the `type` should only be set to `"standard"` if the button "represents a Standard gamepad button" which requires that the button input exists, so I think it is intended. If we follow the spec then we should make sure `type` is set to `"non-standard"` when the button is set to `NullButton()`.
Related details
- Sun Shin US
Sun Shin US (Gerrit)
Sun Shin US added 2 comments![]( "Open in Gerrit")
const size_t length =
std::min<size_t>(gamepad->buttons_length, BUTTON_INDEX_COUNT);
for (size_t i = 0; i < length; ++i) {
gamepad->buttons[i].type = GamepadButtonType::kStandard;
}This will set the button type to `"standard"` for any button, even buttons that are known to not exist on the gamepad. For instance, MapperDjiFpv sets the primary, secondary, tertiary, quaternary buttons to `NullButton()` because there are no corresponding buttons on the device:
I don't remember whether we intended this feature to expose whether a button is present on the gamepad. The spec PR says the `type` should only be set to `"standard"` if the button "represents a Standard gamepad button" which requires that the button input exists, so I think it is intended. If we follow the spec then we should make sure `type` is set to `"non-standard"` when the button is set to `NullButton()`.
@Matt thanks for the review and feedback. Updated the code by the review.
readonly attribute GamepadButtonType type;Sun Shin USYou need an entry in runtime_enabled_features.json5 and `[RuntimeEnabled=GamepadButtonTypes]` here to ensure this API surface isn't shipped by default.
(you can have that generate a feature flag for you that is automatically tied to the RuntimeEnabledFeature).
Nate Chapin@Nate, thanks for the review. And updated the codes by your recommendation.
Sun Shin USThanks! As expected, you now have test failures for
`webexposed/global-interface-listing.html` but NOT
`virtual/stable/webexposed/global-interface-listing.html`. This shows that you have added a feature that is available in experimental builds but not in stable. You can update the test expectations for `webexposed/global-interface-listing.html` to match the new output.In terms of process, you should create a [chromestatus](https://chromestatus.com/) entry if you haven't done so already, and send an `Intent to Prototype` before committing this CL.
@Nate, thanks again for the insightful guide. Will work this on as your suggestion.
Related details
- Matt Reynolds
- Nate Chapin
- Rob Pitkin
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. |
Sun Shin US (Gerrit)
Sun Shin US added 1 comment![]( "Open in Gerrit")
readonly attribute GamepadButtonType type;Sun Shin USYou need an entry in runtime_enabled_features.json5 and `[RuntimeEnabled=GamepadButtonTypes]` here to ensure this API surface isn't shipped by default.
(you can have that generate a feature flag for you that is automatically tied to the RuntimeEnabledFeature).
Nate Chapin@Nate, thanks for the review. And updated the codes by your recommendation.
Sun Shin USThanks! As expected, you now have test failures for
`webexposed/global-interface-listing.html` but NOT
`virtual/stable/webexposed/global-interface-listing.html`. This shows that you have added a feature that is available in experimental builds but not in stable. You can update the test expectations for `webexposed/global-interface-listing.html` to match the new output.In terms of process, you should create a [chromestatus](https://chromestatus.com/) entry if you haven't done so already, and send an `Intent to Prototype` before committing this CL.
@Nate, thanks again for the insightful guide. Will work this on as your suggestion.
@Nate, please refer to the following ChromeStatus link: https://chromestatus.com/feature/5075054393163776
Related details
- Matt Reynolds
- Nate Chapin
- Rob Pitkin
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. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
readonly attribute GamepadButtonType type;Sun Shin USYou need an entry in runtime_enabled_features.json5 and `[RuntimeEnabled=GamepadButtonTypes]` here to ensure this API surface isn't shipped by default.
(you can have that generate a feature flag for you that is automatically tied to the RuntimeEnabledFeature).
Nate Chapin@Nate, thanks for the review. And updated the codes by your recommendation.
Sun Shin USThanks! As expected, you now have test failures for
`webexposed/global-interface-listing.html` but NOT
`virtual/stable/webexposed/global-interface-listing.html`. This shows that you have added a feature that is available in experimental builds but not in stable. You can update the test expectations for `webexposed/global-interface-listing.html` to match the new output.In terms of process, you should create a [chromestatus](https://chromestatus.com/) entry if you haven't done so already, and send an `Intent to Prototype` before committing this CL.
Sun Shin US@Nate, thanks again for the insightful guide. Will work this on as your suggestion.
@Nate, please refer to the following ChromeStatus link: https://chromestatus.com/feature/5075054393163776
- Matt Reynolds
- Rob Pitkin
- Sun Shin US
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. |