Expose a `GamepadProvider` and use `Responder` types for messages by atbrakhi · Pull Request #41568 · servo/servo

atbrakhi · 2025-12-29T14:36:54Z

Expose a GamepadProvider and use Responder types for messages

Testing:
Fixes: #41453

atbrakhi · 2025-12-29T14:37:03Z

run: https://github.com/atbrakhi/servo/actions/runs/20572535508

mrobinson

Nice start. This needs a bit of a rework to make it match the design of the other parts of the API:

components/servo/lib.rs

components/servo/responders.rs

ports/servoshell/egl/app.rs

ports/servoshell/egl/gamepad.rs

mrobinson · 2026-01-02T17:30:20Z

ports/servoshell/running_app_state.rs

+#[cfg(feature = "gamepad")]
+pub(crate) struct ServoshellGamepadProvider {
+    gamepad_support: Rc<RefCell<Option<GamepadSupport>>>,
+}


Instead of wrapping a GamepadSupport make GamepadSuport the actual implementation of GamepadProvider and just don't set it if it is None.

@mrobinson Just to confirm my understanding: GamepadSupport should directly implement GamepadProvider, and ServoBuilder::gamepad_provider() should only be called if GamepadSupport::maybe_new() returns Some. This would require adding RefCell internally to GamepadSupport for the haptic_effects field since the trait method takes &self. Is that correct?

ports/servoshell/desktop/gamepad.rs

atbrakhi · 2026-01-18T12:36:24Z

@mrobinson i have addressed rest of your feedback in bbdc172. Sorry for the delay, i was ooo until recently.

mrobinson

Nice. This is getting closer. Just a few suggestions:

components/servo/gamepad_provider.rs

components/servo/servo.rs

mrobinson · 2026-01-19T06:39:39Z

ports/servoshell/running_app_state.rs

+#[cfg(feature = "gamepad")]
+pub(crate) struct ServoshellGamepadProvider {
+    gamepad_support: Rc<RefCell<Option<GamepadSupport>>>,
+}


Yes, I think we should do that. There should only be GamepadSupport would be renamed ServoshellGamepadProvider and implement GamepadProvider directly.

@mrobinson done in 09461e8 , thanks. Also updated some comments!

Signed-off-by: atbrakhi <[email protected]>

mrobinson

Excellent work @atbrakhi. A few comments before landing:

mrobinson · 2026-01-28T09:10:26Z

components/servo/gamepad_provider.rs

+    pub fn effect_type(&self) -> Option<&GamepadHapticEffectType> {
+        match &self.request_type {
+            GamepadHapticEffectRequestType::Play(effect_type) => Some(effect_type),
+            GamepadHapticEffectRequestType::Stop => None,
+        }
+    }


I think you can just remove this as it's in the GamepadHapticEffectRequestType.

mrobinson · 2026-01-28T09:12:41Z

ports/servoshell/desktop/app.rs

+            #[cfg(feature = "gamepad")]
+            gamepad_provider,


I would just move the above line here, if possible:

Suggested change

#[cfg(feature = "gamepad")]

gamepad_provider,

#[cfg(feature = "gamepad")]

ServoshellGamepadProvider::maybe_new().map(Rc::new),

mrobinson · 2026-01-28T09:15:23Z

ports/servoshell/desktop/gamepad.rs

+            GamepadHapticEffectRequestType::Play(_) => {
+                self.play_haptic_effect(request);


Passing the effect type directly can make play_haptic_effect a little simpler:

Suggested change

GamepadHapticEffectRequestType::Play(_) => {

self.play_haptic_effect(request);

GamepadHapticEffectRequestType::Play(effect_type) => {

self.play_haptic_effect(request, effect_type);

mrobinson · 2026-01-28T09:16:26Z

ports/servoshell/egl/app.rs

+        let servo_builder = ServoBuilder::default()
            .opts(init.opts)
            .preferences(init.preferences.clone())
            .event_loop_waker(init.event_loop_waker.clone());
+


I think you'll want to revert these lines. This will break if WebXR is enabled, I believe.

mrobinson · 2026-01-28T09:18:45Z

ports/servoshell/running_app_state.rs

+        self.gamepad_provider
+            .as_ref()
+            .map(|p| p.clone() as Rc<dyn GamepadProvider>)


I think you should be able to return this directly using clone (or cloned, I can never remember).

Suggested change

self.gamepad_provider

.as_ref()

.map(|p| p.clone() as Rc<dyn GamepadProvider>)

self.gamepad_provider.clone()

@mrobinson We would need to change the type to Option<Rc<ServoshellGamepadProvider>> in that case. .clone() won't compile due to the type mismatch between Rc<ServoshellGamepadProvider> and Rc<dyn GamepadProvider>. I am not sure if we should do that just to make clone work.

@atbrakhi Well spotted. I think changing the type is appropriate here and simplifies the code. Additionally, other callers might want to do servoshell-specific things with this getter so we stay as specific as possible until inserting it into the WebViewDelegate.

…1568) Expose a `GamepadProvider` and use `Responder` types for messages Testing: Fixes: #41453 --------- Signed-off-by: atbrakhi <[email protected]>

Signed-off-by: atbrakhi <[email protected]>

atbrakhi force-pushed the gamepadprovider branch from c05ffe5 to b421b8c Compare December 30, 2025 08:02

mrobinson requested changes Jan 2, 2026

View reviewed changes

atbrakhi force-pushed the gamepadprovider branch 2 times, most recently from dae3040 to bbdc172 Compare January 18, 2026 12:35

atbrakhi force-pushed the gamepadprovider branch 2 times, most recently from 8531d4f to 4da9dee Compare January 18, 2026 12:57

mrobinson requested changes Jan 19, 2026

View reviewed changes

Expose a GamepadProvider and use Responder types for messages

95a89f4

Signed-off-by: atbrakhi <[email protected]>

atbrakhi force-pushed the gamepadprovider branch from 7873e31 to 09461e8 Compare January 27, 2026 14:29

atbrakhi marked this pull request as ready for review January 27, 2026 14:31

atbrakhi requested a review from jschwe as a code owner January 27, 2026 14:31

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 27, 2026

atbrakhi requested a review from mrobinson January 27, 2026 14:31

mrobinson approved these changes Jan 28, 2026

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Jan 28, 2026

atbrakhi force-pushed the gamepadprovider branch from 09461e8 to fd65ca0 Compare January 29, 2026 06:13

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 29, 2026

atbrakhi force-pushed the gamepadprovider branch from fd65ca0 to 5469db7 Compare January 29, 2026 14:25

atbrakhi added this pull request to the merge queue Jan 29, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jan 29, 2026

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2026

servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jan 29, 2026

review fix

6e6798e

Signed-off-by: atbrakhi <[email protected]>

atbrakhi force-pushed the gamepadprovider branch from 5469db7 to 6e6798e Compare February 1, 2026 13:16

servo-highfive removed the S-tests-failed The changes caused existing tests to fail. label Feb 1, 2026

atbrakhi added this pull request to the merge queue Feb 1, 2026

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 1, 2026

Merged via the queue into servo:main with commit b6a1761 Feb 1, 2026
29 checks passed

atbrakhi deleted the gamepadprovider branch February 1, 2026 13:58

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 1, 2026

This was referenced Mar 29, 2026

Expose a GamepadProvider and use Responder types for messages #41453

Closed

libservo: WebViewDelegate methods for gamepad haptic effects are never called #43743

Closed

		GamepadHapticEffectRequestType::Play(_) => {
		self.play_haptic_effect(request);

Uh oh!

Conversation

atbrakhi commented Dec 29, 2025

Uh oh!

atbrakhi commented Dec 29, 2025

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

atbrakhi commented Jan 18, 2026

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrobinson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants