servo: Track async webview focus operations (#38243)

https://github.com/servo/servo/pull/38160 added a webdriver-specific API
to support waiting on focus operations to complete. These changes
replace that with a generalized pattern, where a unique ID is created
for each focus operation and the embedder can receive notifications
about each focus operation when it is complete, regardless of whether
the focus was actually changed.

Testing: Existing test coverage from
https://github.com/servo/servo/pull/38160/ is unchanged.

---------

Signed-off-by: Josh Matthews <josh@joshmatthews.net>

components/constellation/constellation.rs

@@ -129,7 +129,7 @@ use devtools_traits::{
 use embedder_traits::resources::{self, Resource};
 use embedder_traits::user_content_manager::UserContentManager;
 use embedder_traits::{
-    AnimationState, CompositorHitTestResult, Cursor, EmbedderMsg, EmbedderProxy,
+    AnimationState, CompositorHitTestResult, Cursor, EmbedderMsg, EmbedderProxy, FocusId,
     FocusSequenceNumber, InputEvent, JSValue, JavaScriptEvaluationError, JavaScriptEvaluationId,
     KeyboardEvent, MediaSessionActionType, MediaSessionEvent, MediaSessionPlaybackState,
     MouseButton, MouseButtonAction, MouseButtonEvent, Theme, ViewportDetails, WebDriverCommandMsg,
@@ -1347,8 +1347,8 @@ where
                 }
                 self.handle_panic(webview_id, error, None);
             },
-            EmbedderToConstellationMessage::FocusWebView(webview_id, response_sender) => {
+            EmbedderToConstellationMessage::FocusWebView(webview_id, focus_id) => {
-                self.handle_focus_web_view(webview_id, response_sender);
+                self.handle_focus_web_view(webview_id, focus_id);
             },
             EmbedderToConstellationMessage::BlurWebView => {
                 self.webviews.unfocus();
@@ -2755,20 +2755,13 @@ where
     }
 
     #[servo_tracing::instrument(skip_all)]
-    fn handle_focus_web_view(
+    fn handle_focus_web_view(&mut self, webview_id: WebViewId, focus_id: FocusId) {
-        &mut self,
+        let focused = self.webviews.focus(webview_id).is_ok();
-        webview_id: WebViewId,
+        if !focused {
-        response_sender: Option<IpcSender<bool>>,
+            warn!("{webview_id}: FocusWebView on unknown top-level browsing context");
-    ) {
-        if self.webviews.get(webview_id).is_none() {
-            if let Some(response_sender) = response_sender {
-                let _ = response_sender.send(false);
-            }
-            return warn!("{webview_id}: FocusWebView on unknown top-level browsing context");
         }
-        self.webviews.focus(webview_id);
         self.embedder_proxy
-            .send(EmbedderMsg::WebViewFocused(webview_id, response_sender));
+            .send(EmbedderMsg::WebViewFocused(webview_id, focus_id, focused));
     }
 
     #[servo_tracing::instrument(skip_all)]
@@ -4121,9 +4114,12 @@ where
         }
 
         // Focus the top-level browsing context.
-        self.webviews.focus(webview_id);
+        let focused = self.webviews.focus(webview_id);
-        self.embedder_proxy
+        self.embedder_proxy.send(EmbedderMsg::WebViewFocused(
-            .send(EmbedderMsg::WebViewFocused(webview_id, None));
+            webview_id,
+            FocusId::new(),
+            focused.is_ok(),
+        ));
 
         // If a container with a non-null nested browsing context is focused,
         // the nested browsing context's active document becomes the focused

components/constellation/webview_manager.rs

@@ -67,11 +67,14 @@ impl<WebView> WebViewManager<WebView> {
         }
     }
 
-    pub fn focus(&mut self, webview_id: WebViewId) {
+    pub fn focus(&mut self, webview_id: WebViewId) -> Result<(), ()> {
-        debug_assert!(self.webviews.contains_key(&webview_id));
+        if !self.webviews.contains_key(&webview_id) {
+            return Err(());
+        }
         self.focus_order.retain(|b| *b != webview_id);
         self.focus_order.push(webview_id);
         self.is_focused = true;
+        Ok(())
     }
 
     pub fn unfocus(&mut self) {

components/servo/lib.rs

@@ -732,15 +732,17 @@ impl Servo {
                     webview.delegate().notify_closed(webview);
                 }
             },
-            EmbedderMsg::WebViewFocused(webview_id, response_sender) => {
+            EmbedderMsg::WebViewFocused(webview_id, focus_id, focus_result) => {
-                for id in self.webviews.borrow().keys() {
+                if focus_result {
-                    if let Some(webview) = self.get_webview_handle(*id) {
+                    for id in self.webviews.borrow().keys() {
-                        let focused = webview.id() == webview_id;
+                        if let Some(webview) = self.get_webview_handle(*id) {
-                        webview.set_focused(focused);
+                            let focused = webview.id() == webview_id;
+                            webview.set_focused(focused);
+                        }
                     }
                 }
-                if let Some(response_sender) = response_sender {
+                if let Some(webview) = self.get_webview_handle(webview_id) {
-                    let _ = response_sender.send(true);
+                    webview.complete_focus(focus_id);
                 }
             },
             EmbedderMsg::WebViewBlurred => {

components/servo/webview.rs

@@ -13,8 +13,8 @@ use compositing_traits::WebViewTrait;
 use constellation_traits::{EmbedderToConstellationMessage, TraversalDirection};
 use dpi::PhysicalSize;
 use embedder_traits::{
-    Cursor, InputEvent, JSValue, JavaScriptEvaluationError, LoadStatus, MediaSessionActionType,
+    Cursor, FocusId, InputEvent, JSValue, JavaScriptEvaluationError, LoadStatus,
-    ScreenGeometry, Theme, TraversalId, ViewportDetails,
+    MediaSessionActionType, ScreenGeometry, Theme, TraversalId, ViewportDetails,
 };
 use euclid::{Point2D, Scale, Size2D};
 use servo_geometry::DeviceIndependentPixel;
@@ -25,7 +25,7 @@ use webrender_api::units::{DeviceIntPoint, DevicePixel, DeviceRect};
 use crate::clipboard_delegate::{ClipboardDelegate, DefaultClipboardDelegate};
 use crate::javascript_evaluator::JavaScriptEvaluator;
 use crate::webview_delegate::{DefaultWebViewDelegate, WebViewDelegate};
-use crate::{ConstellationProxy, IpcSender, Servo, WebRenderDebugOption};
+use crate::{ConstellationProxy, Servo, WebRenderDebugOption};
 
 /// A handle to a Servo webview. If you clone this handle, it does not create a new webview,
 /// but instead creates a new handle to the webview. Once the last handle is dropped, Servo
@@ -293,6 +293,10 @@ impl WebView {
         self.delegate().notify_focus_changed(self, new_value);
     }
 
+    pub(crate) fn complete_focus(self, focus_id: FocusId) {
+        self.delegate().notify_focus_complete(self, focus_id);
+    }
+
     pub fn cursor(&self) -> Cursor {
         self.inner().cursor
     }
@@ -305,22 +309,15 @@ impl WebView {
         self.delegate().notify_cursor_changed(self, new_value);
     }
 
-    pub fn focus(&self) {
+    pub fn focus(&self) -> FocusId {
+        let focus_id = FocusId::new();
         self.inner()
             .constellation_proxy
             .send(EmbedderToConstellationMessage::FocusWebView(
                 self.id(),
-                None,
+                focus_id.clone(),
-            ));
-    }
-
-    pub fn focus_from_webdriver(&self, response_sender: IpcSender<bool>) {
-        self.inner()
-            .constellation_proxy
-            .send(EmbedderToConstellationMessage::FocusWebView(
-                self.id(),
-                Some(response_sender),
             ));
+        focus_id
     }
 
     pub fn blur(&self) {

components/servo/webview_delegate.rs

@@ -7,7 +7,7 @@ use std::path::PathBuf;
 use base::id::PipelineId;
 use constellation_traits::EmbedderToConstellationMessage;
 use embedder_traits::{
-    AllowOrDeny, AuthenticationResponse, ContextMenuResult, Cursor, FilterPattern,
+    AllowOrDeny, AuthenticationResponse, ContextMenuResult, Cursor, FilterPattern, FocusId,
     GamepadHapticEffectType, InputMethodType, KeyboardEvent, LoadStatus, MediaSessionEvent,
     Notification, PermissionFeature, RgbColor, ScreenGeometry, SelectElementOptionOrOptgroup,
     SimpleDialog, TraversalId, WebResourceRequest, WebResourceResponse, WebResourceResponseMsg,
@@ -418,6 +418,9 @@ pub trait WebViewDelegate {
     /// This [`WebView`] has either become focused or lost focus. Whether or not the
     /// [`WebView`] is focused can be accessed via [`WebView::focused`].
     fn notify_focus_changed(&self, _webview: WebView, _focused: bool) {}
+    /// A focus operation that was initiated by this webview has completed.
+    /// The current focus status of this [`WebView`] can be accessed via [`WebView::focused`].
+    fn notify_focus_complete(&self, _webview: WebView, _focus_id: FocusId) {}
     /// This [`WebView`] has either started to animate or stopped animating. When a
     /// [`WebView`] is animating, it is up to the embedding application ensure that
     /// `Servo::spin_event_loop` is called at regular intervals in order to update the

components/shared/constellation/lib.rs

@@ -19,8 +19,9 @@ use base::Epoch;
 use base::cross_process_instant::CrossProcessInstant;
 use base::id::{MessagePortId, PipelineId, WebViewId};
 use embedder_traits::{
-    CompositorHitTestResult, Cursor, InputEvent, JavaScriptEvaluationId, MediaSessionActionType,
+    CompositorHitTestResult, Cursor, FocusId, InputEvent, JavaScriptEvaluationId,
-    Theme, TraversalId, ViewportDetails, WebDriverCommandMsg, WebDriverCommandResponse,
+    MediaSessionActionType, Theme, TraversalId, ViewportDetails, WebDriverCommandMsg,
+    WebDriverCommandResponse,
 };
 pub use from_script_message::*;
 use ipc_channel::ipc::IpcSender;
@@ -68,9 +69,9 @@ pub enum EmbedderToConstellationMessage {
     CloseWebView(WebViewId),
     /// Panic a top level browsing context.
     SendError(Option<WebViewId>, String),
-    /// Make a webview focused. If sender is provided, it will be used to send back a
+    /// Make a webview focused. [EmbedderMsg::WebViewFocused] will be sent with
-    /// bool indicating whether the focus was successfully set in EmbedderMsg::WebViewFocused.
+    /// the result of this operation.
-    FocusWebView(WebViewId, Option<IpcSender<bool>>),
+    FocusWebView(WebViewId, FocusId),
     /// Make none of the webviews focused.
     BlurWebView,
     /// Forward an input event to an appropriate ScriptTask.

components/shared/embedder/lib.rs

@@ -327,8 +327,19 @@ pub struct ScreenMetrics {
     pub available_size: DeviceIndependentIntSize,
 }
 
+/// An opaque identifier for a single webview focus operation.
+#[derive(Clone, Deserialize, Eq, Hash, PartialEq, Serialize)]
+pub struct FocusId(String);
+
+impl FocusId {
+    #[allow(clippy::new_without_default)]
+    pub fn new() -> Self {
+        Self(Uuid::new_v4().to_string())
+    }
+}
+
 /// An opaque identifier for a single history traversal operation.
-#[derive(Clone, Deserialize, PartialEq, Serialize)]
+#[derive(Clone, Deserialize, Eq, Hash, PartialEq, Serialize)]
 pub struct TraversalId(String);
 
 impl TraversalId {
@@ -372,10 +383,10 @@ pub enum EmbedderMsg {
     AllowOpeningWebView(WebViewId, IpcSender<Option<(WebViewId, ViewportDetails)>>),
     /// A webview was destroyed.
     WebViewClosed(WebViewId),
-    /// A webview gained focus for keyboard events
+    /// A webview potentially gained focus for keyboard events, as initiated
-    /// If sender is provided, it will be used to send back a
+    /// by the provided focus id. If the boolean value is false, the webiew
-    /// bool indicating whether the focus was successfully set.
+    /// could not be focused.
-    WebViewFocused(WebViewId, Option<IpcSender<bool>>),
+    WebViewFocused(WebViewId, FocusId, bool),
     /// All webviews lost focus for keyboard events.
     WebViewBlurred,
     /// Wether or not to unload a document

ports/servoshell/desktop/app.rs

@@ -380,7 +380,8 @@ impl App {
                 },
                 WebDriverCommandMsg::FocusWebView(webview_id, response_sender) => {
                     if let Some(webview) = running_state.webview_by_id(webview_id) {
-                        webview.focus_from_webdriver(response_sender);
+                        let focus_id = webview.focus();
+                        running_state.set_pending_focus(focus_id, response_sender);
                     }
                 },
                 WebDriverCommandMsg::GetWindowRect(_webview_id, response_sender) => {
@@ -478,21 +479,13 @@ impl App {
                 WebDriverCommandMsg::GoBack(webview_id, load_status_sender) => {
                     if let Some(webview) = running_state.webview_by_id(webview_id) {
                         let traversal_id = webview.go_back(1);
-                        running_state.set_pending_traversal(
+                        running_state.set_pending_traversal(traversal_id, load_status_sender);
-                            webview_id,
-                            traversal_id,
-                            load_status_sender,
-                        );
                     }
                 },
                 WebDriverCommandMsg::GoForward(webview_id, load_status_sender) => {
                     if let Some(webview) = running_state.webview_by_id(webview_id) {
                         let traversal_id = webview.go_forward(1);
-                        running_state.set_pending_traversal(
+                        running_state.set_pending_traversal(traversal_id, load_status_sender);
-                            webview_id,
-                            traversal_id,
-                            load_status_sender,
-                        );
                     }
                 },
                 // Key events don't need hit test so can be forwarded to constellation for now

ports/servoshell/desktop/app_state.rs

@@ -18,10 +18,11 @@ use servo::ipc_channel::ipc::IpcSender;
 use servo::webrender_api::ScrollLocation;
 use servo::webrender_api::units::{DeviceIntPoint, DeviceIntSize};
 use servo::{
-    AllowOrDenyRequest, AuthenticationRequest, FilterPattern, FormControl, GamepadHapticEffectType,
+    AllowOrDenyRequest, AuthenticationRequest, FilterPattern, FocusId, FormControl,
-    KeyboardEvent, LoadStatus, PermissionRequest, Servo, ServoDelegate, ServoError, SimpleDialog,
+    GamepadHapticEffectType, KeyboardEvent, LoadStatus, PermissionRequest, Servo, ServoDelegate,
-    TraversalId, WebDriverCommandMsg, WebDriverJSResult, WebDriverJSValue, WebDriverLoadStatus,
+    ServoError, SimpleDialog, TraversalId, WebDriverCommandMsg, WebDriverJSResult,
-    WebDriverUserPrompt, WebView, WebViewBuilder, WebViewDelegate,
+    WebDriverJSValue, WebDriverLoadStatus, WebDriverUserPrompt, WebView, WebViewBuilder,
+    WebViewDelegate,
 };
 use url::Url;
 
@@ -45,7 +46,8 @@ pub(crate) enum AppState {
 struct WebDriverSenders {
     pub load_status_senders: HashMap<WebViewId, IpcSender<WebDriverLoadStatus>>,
     pub script_evaluation_interrupt_sender: Option<IpcSender<WebDriverJSResult>>,
-    pub pending_traversals: HashMap<WebViewId, (TraversalId, IpcSender<WebDriverLoadStatus>)>,
+    pub pending_traversals: HashMap<TraversalId, IpcSender<WebDriverLoadStatus>>,
+    pub pending_focus: HashMap<FocusId, IpcSender<bool>>,
 }
 
 pub(crate) struct RunningAppState {
@@ -267,7 +269,9 @@ impl RunningAppState {
             .and_then(|id| inner.webviews.get(id));
 
         match last_created {
-            Some(last_created_webview) => last_created_webview.focus(),
+            Some(last_created_webview) => {
+                last_created_webview.focus();
+            },
             None => self.servo.start_shutting_down(),
         }
     }
@@ -446,16 +450,22 @@ impl RunningAppState {
             });
     }
 
+    pub(crate) fn set_pending_focus(&self, focus_id: FocusId, sender: IpcSender<bool>) {
+        self.webdriver_senders
+            .borrow_mut()
+            .pending_focus
+            .insert(focus_id, sender);
+    }
+
     pub(crate) fn set_pending_traversal(
         &self,
-        webview_id: WebViewId,
         traversal_id: TraversalId,
         sender: IpcSender<WebDriverLoadStatus>,
     ) {
         self.webdriver_senders
             .borrow_mut()
             .pending_traversals
-            .insert(webview_id, (traversal_id, sender));
+            .insert(traversal_id, sender);
     }
 
     pub(crate) fn set_load_status_sender(
@@ -539,13 +549,11 @@ impl WebViewDelegate for RunningAppState {
         }
     }
 
-    fn notify_traversal_complete(&self, webview: servo::WebView, traversal_id: TraversalId) {
+    fn notify_traversal_complete(&self, _webview: servo::WebView, traversal_id: TraversalId) {
         let mut webdriver_state = self.webdriver_senders.borrow_mut();
-        if let Entry::Occupied(entry) = webdriver_state.pending_traversals.entry(webview.id()) {
+        if let Entry::Occupied(entry) = webdriver_state.pending_traversals.entry(traversal_id) {
-            if entry.get().0 == traversal_id {
+            let sender = entry.remove();
-                let (_, sender) = entry.remove();
+            let _ = sender.send(WebDriverLoadStatus::Complete);
-                let _ = sender.send(WebDriverLoadStatus::Complete);
-            }
         }
     }
 
@@ -638,6 +646,14 @@ impl WebViewDelegate for RunningAppState {
         self.close_webview(webview.id());
     }
 
+    fn notify_focus_complete(&self, webview: servo::WebView, focus_id: FocusId) {
+        let mut webdriver_state = self.webdriver_senders.borrow_mut();
+        if let Entry::Occupied(entry) = webdriver_state.pending_focus.entry(focus_id) {
+            let sender = entry.remove();
+            let _ = sender.send(webview.focused());
+        }
+    }
+
     fn notify_focus_changed(&self, webview: servo::WebView, focused: bool) {
         let mut inner_mut = self.inner_mut();
         if focused {