Gamepad: Remove GamepadList and fix dropped connection event on startup (#31684)

* Replace GamepadList

* Fix initial gamepad connection event from gilrs getting dropped

* Fix gamepad reconnection issues, use MutNullableDom

* Reduce some repetition in handle_gamepad_events

* Address feedback, move some steps to navigator methods

* Refactor internal navigator gamepad methods

* Add note re: unused gilrs index, adjust navigator gamepad methods

components/script/dom/gamepad.rs

@@ -199,6 +199,10 @@ impl Gamepad {
         }
     }
 
+    pub fn index(&self) -> i32 {
+        self.index.get()
+    }
+
     pub fn update_index(&self, index: i32) {
         self.index.set(index);
     }

components/script/dom/gamepadlist.rs

@@ -1,79 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
-
-use dom_struct::dom_struct;
-
-use crate::dom::bindings::cell::DomRefCell;
-use crate::dom::bindings::codegen::Bindings::GamepadListBinding::GamepadListMethods;
-use crate::dom::bindings::reflector::{reflect_dom_object, Reflector};
-use crate::dom::bindings::root::{Dom, DomRoot};
-use crate::dom::gamepad::Gamepad;
-use crate::dom::globalscope::GlobalScope;
-
-// https://www.w3.org/TR/gamepad/
-#[dom_struct]
-pub struct GamepadList {
-    reflector_: Reflector,
-    list: DomRefCell<Vec<Dom<Gamepad>>>,
-}
-
-impl GamepadList {
-    fn new_inherited(list: &[&Gamepad]) -> GamepadList {
-        GamepadList {
-            reflector_: Reflector::new(),
-            list: DomRefCell::new(list.iter().map(|g| Dom::from_ref(&**g)).collect()),
-        }
-    }
-
-    pub fn new(global: &GlobalScope, list: &[&Gamepad]) -> DomRoot<GamepadList> {
-        reflect_dom_object(Box::new(GamepadList::new_inherited(list)), global)
-    }
-
-    pub fn add_if_not_exists(&self, gamepads: &[DomRoot<Gamepad>]) {
-        for gamepad in gamepads {
-            if !self
-                .list
-                .borrow()
-                .iter()
-                .any(|g| g.gamepad_id() == gamepad.gamepad_id())
-            {
-                self.list.borrow_mut().push(Dom::from_ref(gamepad));
-                // Ensure that the gamepad has the correct index
-                gamepad.update_index(self.list.borrow().len() as i32 - 1);
-            }
-        }
-    }
-
-    pub fn remove_gamepad(&self, index: usize) {
-        self.list.borrow_mut().remove(index);
-    }
-
-    pub fn list(&self) -> Vec<Option<DomRoot<Gamepad>>> {
-        self.list
-            .borrow()
-            .iter()
-            .map(|gamepad| Some(DomRoot::from_ref(&**gamepad)))
-            .collect()
-    }
-}
-
-impl GamepadListMethods for GamepadList {
-    // https://w3c.github.io/gamepad/#dom-navigator-getgamepads
-    fn Length(&self) -> u32 {
-        self.list.borrow().len() as u32
-    }
-
-    // https://w3c.github.io/gamepad/#dom-navigator-getgamepads
-    fn Item(&self, index: u32) -> Option<DomRoot<Gamepad>> {
-        self.list
-            .borrow()
-            .get(index as usize)
-            .map(|gamepad| DomRoot::from_ref(&**gamepad))
-    }
-
-    // https://w3c.github.io/gamepad/#dom-navigator-getgamepads
-    fn IndexedGetter(&self, index: u32) -> Option<DomRoot<Gamepad>> {
-        self.Item(index)
-    }
-}

components/script/dom/globalscope.rs

@@ -63,10 +63,10 @@ use super::bindings::trace::HashMapTracedValues;
 use crate::dom::bindings::cell::{DomRefCell, RefMut};
 use crate::dom::bindings::codegen::Bindings::BroadcastChannelBinding::BroadcastChannelMethods;
 use crate::dom::bindings::codegen::Bindings::EventSourceBinding::EventSource_Binding::EventSourceMethods;
-use crate::dom::bindings::codegen::Bindings::GamepadListBinding::GamepadList_Binding::GamepadListMethods;
 use crate::dom::bindings::codegen::Bindings::ImageBitmapBinding::{
     ImageBitmapOptions, ImageBitmapSource,
 };
+use crate::dom::bindings::codegen::Bindings::NavigatorBinding::NavigatorMethods;
 use crate::dom::bindings::codegen::Bindings::PerformanceBinding::Performance_Binding::PerformanceMethods;
 use crate::dom::bindings::codegen::Bindings::PermissionStatusBinding::PermissionState;
 use crate::dom::bindings::codegen::Bindings::VoidFunctionBinding::VoidFunction;
@@ -3133,7 +3133,10 @@ impl GlobalScope {
     /// <https://www.w3.org/TR/gamepad/#dfn-gamepadconnected>
     pub fn handle_gamepad_connect(
         &self,
-        index: usize,
+        // As the spec actually defines how to set the gamepad index, the GilRs index
+        // is currently unused, though in practice it will almost always be the same.
+        // More infra is currently needed to track gamepads across windows.
+        _index: usize,
         name: String,
         axis_bounds: (f64, f64),
         button_bounds: (f64, f64),
@@ -3144,19 +3147,12 @@ impl GlobalScope {
         self.gamepad_task_source().queue_with_canceller(
             task!(gamepad_connected: move || {
                 let global = this.root();
-                let gamepad = Gamepad::new(&global, index as u32, name, axis_bounds, button_bounds);
 
                 if let Some(window) = global.downcast::<Window>() {
-                    let has_gesture = window.Navigator().has_gamepad_gesture();
+                    let navigator = window.Navigator();
-                    if has_gesture {
+                    let selected_index = navigator.select_gamepad_index();
-                        gamepad.set_exposed(true);
+                    let gamepad = Gamepad::new(&global, selected_index, name, axis_bounds, button_bounds);
-                        if window.Document().is_fully_active() {
+                    navigator.set_gamepad(selected_index as usize, &*gamepad);
-                            gamepad.update_connected(true, has_gesture);
-                        }
-                    }
-                    let gamepad_list = window.Navigator().gamepads();
-                    let gamepad_arr: [DomRoot<Gamepad>; 1] = [gamepad.clone()];
-                    gamepad_list.add_if_not_exists(&gamepad_arr);
                 }
             }),
             &self.task_canceller(TaskSourceName::Gamepad)
@@ -3172,18 +3168,11 @@ impl GlobalScope {
                 task!(gamepad_disconnected: move || {
                     let global = this.root();
                     if let Some(window) = global.downcast::<Window>() {
-                        let gamepad_list = window.Navigator().gamepads();
+                        let navigator = window.Navigator();
-                        if let Some(gamepad) = gamepad_list.Item(index as u32) {
+                        if let Some(gamepad) = navigator.get_gamepad(index) {
                             if window.Document().is_fully_active() {
                                 gamepad.update_connected(false, gamepad.exposed());
-                                gamepad_list.remove_gamepad(index);
+                                navigator.remove_gamepad(index);
-                            }
-                        }
-                        for i in (0..gamepad_list.Length()).rev() {
-                            if gamepad_list.Item(i).is_none() {
-                                gamepad_list.remove_gamepad(i as usize);
-                            } else {
-                                break;
                             }
                         }
                     }
@@ -3203,11 +3192,10 @@ impl GlobalScope {
                 task!(update_gamepad_state: move || {
                     let global = this.root();
                     if let Some(window) = global.downcast::<Window>() {
-                        let gamepad_list = window.Navigator().gamepads();
+                        let navigator = window.Navigator();
-                        if let Some(gamepad) = gamepad_list.Item(index as u32) {
+                        if let Some(gamepad) = navigator.get_gamepad(index) {
                             let current_time = global.performance().Now();
                             gamepad.update_timestamp(*current_time);
-
                             match update_type {
                                 GamepadUpdateType::Axis(index, value) => {
                                     gamepad.map_and_normalize_axes(index, value);
@@ -3216,26 +3204,27 @@ impl GlobalScope {
                                     gamepad.map_and_normalize_buttons(index, value);
                                 }
                             };
-
+                            if !navigator.has_gamepad_gesture() && contains_user_gesture(update_type) {
-                            if !window.Navigator().has_gamepad_gesture() && contains_user_gesture(update_type) {
+                                navigator.set_has_gamepad_gesture(true);
-                                window.Navigator().set_has_gamepad_gesture(true);
+                                navigator.GetGamepads()
-                                for i in 0..gamepad_list.Length() {
+                                    .iter()
-                                    if let Some(gamepad) = gamepad_list.Item(i) {
+                                    .filter_map(|g| g.as_ref())
+                                    .for_each(|gamepad| {
                                         gamepad.set_exposed(true);
                                         gamepad.update_timestamp(*current_time);
-                                        let new_gamepad = Trusted::new(&*gamepad);
+                                        let new_gamepad = Trusted::new(&**gamepad);
                                         if window.Document().is_fully_active() {
                                             window.task_manager().gamepad_task_source().queue_with_canceller(
                                                 task!(update_gamepad_connect: move || {
                                                     let gamepad = new_gamepad.root();
                                                     gamepad.notify_event(GamepadEventType::Connected);
                                                 }),
-                                                &window.upcast::<GlobalScope>().task_canceller(TaskSourceName::Gamepad),
+                                                &window.upcast::<GlobalScope>()
+                                                    .task_canceller(TaskSourceName::Gamepad),
                                             )
                                             .expect("Failed to queue update gamepad connect task.");
                                         }
-                                    }
+                                });
-                                }
                             }
                         }
                     }

components/script/dom/mod.rs

@@ -319,7 +319,6 @@ pub mod gamepad;
 pub mod gamepadbutton;
 pub mod gamepadbuttonlist;
 pub mod gamepadevent;
-pub mod gamepadlist;
 pub mod gamepadpose;
 pub mod globalscope;
 pub mod gpu;

components/script/dom/navigator.rs

@@ -9,6 +9,7 @@ use dom_struct::dom_struct;
 use js::jsval::JSVal;
 use lazy_static::lazy_static;
 
+use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::codegen::Bindings::NavigatorBinding::NavigatorMethods;
 use crate::dom::bindings::codegen::Bindings::WindowBinding::Window_Binding::WindowMethods;
 use crate::dom::bindings::reflector::{reflect_dom_object, DomObject, Reflector};
@@ -17,7 +18,7 @@ use crate::dom::bindings::str::DOMString;
 use crate::dom::bindings::utils::to_frozen_array;
 use crate::dom::bluetooth::Bluetooth;
 use crate::dom::gamepad::Gamepad;
-use crate::dom::gamepadlist::GamepadList;
+use crate::dom::gamepadevent::GamepadEventType;
 use crate::dom::gpu::GPU;
 use crate::dom::mediadevices::MediaDevices;
 use crate::dom::mediasession::MediaSession;
@@ -47,7 +48,7 @@ pub struct Navigator {
     xr: MutNullableDom<XRSystem>,
     mediadevices: MutNullableDom<MediaDevices>,
     /// <https://www.w3.org/TR/gamepad/#dfn-gamepads>
-    gamepads: MutNullableDom<GamepadList>,
+    gamepads: DomRefCell<Vec<MutNullableDom<Gamepad>>>,
     permissions: MutNullableDom<Permissions>,
     mediasession: MutNullableDom<MediaSession>,
     gpu: MutNullableDom<GPU>,
@@ -81,9 +82,50 @@ impl Navigator {
         self.xr.get()
     }
 
-    pub fn gamepads(&self) -> DomRoot<GamepadList> {
+    pub fn get_gamepad(&self, index: usize) -> Option<DomRoot<Gamepad>> {
-        self.gamepads
+        self.gamepads.borrow().get(index).and_then(|g| g.get())
-            .or_init(|| GamepadList::new(&self.global(), &[]))
+    }
+
+    pub fn set_gamepad(&self, index: usize, gamepad: &Gamepad) {
+        if let Some(gamepad_to_set) = self.gamepads.borrow().get(index) {
+            gamepad_to_set.set(Some(gamepad));
+        }
+        if self.has_gamepad_gesture.get() {
+            gamepad.set_exposed(true);
+            if self.global().as_window().Document().is_fully_active() {
+                gamepad.notify_event(GamepadEventType::Connected);
+            }
+        }
+    }
+
+    pub fn remove_gamepad(&self, index: usize) {
+        if let Some(gamepad_to_remove) = self.gamepads.borrow_mut().get(index) {
+            gamepad_to_remove.set(None);
+        }
+        self.shrink_gamepads_list();
+    }
+
+    /// <https://www.w3.org/TR/gamepad/#dfn-selecting-an-unused-gamepad-index>
+    pub fn select_gamepad_index(&self) -> u32 {
+        let mut gamepad_list = self.gamepads.borrow_mut();
+        if let Some(index) = gamepad_list.iter().position(|g| g.get().is_none()) {
+            index as u32
+        } else {
+            let len = gamepad_list.len();
+            gamepad_list.resize_with(len + 1, Default::default);
+            len as u32
+        }
+    }
+
+    fn shrink_gamepads_list(&self) {
+        let mut gamepad_list = self.gamepads.borrow_mut();
+        for i in (0..gamepad_list.len()).rev() {
+            if gamepad_list.get(i as usize).is_none() {
+                gamepad_list.remove(i as usize);
+            } else {
+                break;
+            }
+        }
     }
 
     pub fn has_gamepad_gesture(&self) -> bool {
@@ -200,9 +242,7 @@ impl NavigatorMethods for Navigator {
             return Vec::new();
         }
 
-        let root = self.gamepads.or_init(|| GamepadList::new(&global, &[]));
+        self.gamepads.borrow().iter().map(|g| g.get()).collect()
-
-        root.list()
     }
     // https://w3c.github.io/permissions/#navigator-and-workernavigator-extension
     fn Permissions(&self) -> DomRoot<Permissions> {

components/script/dom/webidls/GamepadList.webidl

@@ -1,10 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
- * License, v. 2.0. If a copy of the MPL was not distributed with this
- * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
-
-// https://w3c.github.io/gamepad/#navigator-interface-extension
-[Exposed=Window, Pref="dom.gamepad.enabled"]
-interface GamepadList {
-  getter Gamepad? item(unsigned long index);
-  readonly attribute unsigned long length;
-};

ports/servoshell/app.rs

@@ -407,7 +407,10 @@ impl App {
 
         // Catch some keyboard events, and push the rest onto the WebViewManager event queue.
         webviews.handle_window_events(embedder_events);
-        if pref!(dom.gamepad.enabled) && webviews.webview_id().is_some() {
+
+        // If the Gamepad API is enabled, handle gamepad events from GilRs.
+        // Checking for current_url_string should ensure we'll have a valid browsing context.
+        if pref!(dom.gamepad.enabled) && webviews.current_url_string().is_some() {
             webviews.handle_gamepad_events();
         }
 

ports/servoshell/webview.rs

@@ -155,14 +155,14 @@ where
                 let gamepad = gilrs.gamepad(event.id);
                 let name = gamepad.name();
                 let index = GamepadIndex(event.id.into());
+                let mut gamepad_event: Option<GamepadEvent> = None;
                 match event.event {
                     EventType::ButtonPressed(button, _) => {
                         let mapped_index = Self::map_gamepad_button(button);
                         // We only want to send this for a valid digital button, aka on/off only
                         if !matches!(mapped_index, 6 | 7 | 17) {
                             let update_type = GamepadUpdateType::Button(mapped_index, 1.0);
-                            let event = GamepadEvent::Updated(index, update_type);
+                            gamepad_event = Some(GamepadEvent::Updated(index, update_type));
-                            self.event_queue.push(EmbedderEvent::Gamepad(event));
                         }
                     },
                     EventType::ButtonReleased(button, _) => {
@@ -170,8 +170,7 @@ where
                         // We only want to send this for a valid digital button, aka on/off only
                         if !matches!(mapped_index, 6 | 7 | 17) {
                             let update_type = GamepadUpdateType::Button(mapped_index, 0.0);
-                            let event = GamepadEvent::Updated(index, update_type);
+                            gamepad_event = Some(GamepadEvent::Updated(index, update_type));
-                            self.event_queue.push(EmbedderEvent::Gamepad(event));
                         }
                     },
                     EventType::ButtonChanged(button, value, _) => {
@@ -179,8 +178,7 @@ where
                         // We only want to send this for a valid non-digital button, aka the triggers
                         if matches!(mapped_index, 6 | 7) {
                             let update_type = GamepadUpdateType::Button(mapped_index, value as f64);
-                            let event = GamepadEvent::Updated(index, update_type);
+                            gamepad_event = Some(GamepadEvent::Updated(index, update_type));
-                            self.event_queue.push(EmbedderEvent::Gamepad(event));
                         }
                     },
                     EventType::AxisChanged(axis, value, _) => {
@@ -203,8 +201,7 @@ where
                             };
                             let update_type =
                                 GamepadUpdateType::Axis(mapped_axis, axis_value as f64);
-                            let event = GamepadEvent::Updated(index, update_type);
+                            gamepad_event = Some(GamepadEvent::Updated(index, update_type));
-                            self.event_queue.push(EmbedderEvent::Gamepad(event));
                         }
                     },
                     EventType::Connected => {
@@ -213,15 +210,17 @@ where
                             axis_bounds: (-1.0, 1.0),
                             button_bounds: (0.0, 1.0),
                         };
-                        let event = GamepadEvent::Connected(index, name, bounds);
+                        gamepad_event = Some(GamepadEvent::Connected(index, name, bounds));
-                        self.event_queue.push(EmbedderEvent::Gamepad(event));
                     },
                     EventType::Disconnected => {
-                        let event = GamepadEvent::Disconnected(index);
+                        gamepad_event = Some(GamepadEvent::Disconnected(index));
-                        self.event_queue.push(EmbedderEvent::Gamepad(event));
                     },
                     _ => {},
                 }
+
+                if let Some(event) = gamepad_event {
+                    self.event_queue.push(EmbedderEvent::Gamepad(event));
+                }
             }
         }
     }