libservo: Rework and clarify the rendering model of the WebView (#35522)

Make the rendering model of the `WebView` clearer:

1. `WebViewDelegate::notify_new_frame_ready()` indicates that the
   WebView has become dirty and needs to be repainted.
2. `WebView::paint()` asks Servo to paint the contents of the `WebView`
   into the `RenderingContext`.
3. `RenderingContext::present()` does a buffer swap if the
   `RenderingContext` is actually double-buffered.

This is documented and all in-tree embedders are updated to work with
this new model.

Signed-off-by: Martin Robinson <mrobinson@igalia.com>
Co-authored-by: Ngo Iok Ui (Wu Yu Wei) <yuweiwu@pm.me>
Co-authored-by: Mukilan Thiyagarajan <mukilan@igalia.com>

Cargo.lock

@@ -1035,6 +1035,7 @@ name = "compositing"
 version = "0.0.1"
 dependencies = [
  "base",
+ "bitflags 2.8.0",
  "compositing_traits",
  "crossbeam-channel",
  "embedder_traits",

components/compositing/Cargo.toml

@@ -19,6 +19,7 @@ webxr = ["dep:webxr"]
 
 [dependencies]
 base = { workspace = true }
+bitflags = { workspace = true }
 compositing_traits = { workspace = true }
 crossbeam-channel = { workspace = true }
 embedder_traits = { workspace = true }

components/compositing/compositor.rs

@@ -2,7 +2,7 @@
  * 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 std::collections::hash_set::Iter;
+use std::cell::Cell;
 use std::collections::HashMap;
 use std::env;
 use std::fs::{create_dir_all, File};
@@ -15,6 +15,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH};
 use base::cross_process_instant::CrossProcessInstant;
 use base::id::{PipelineId, TopLevelBrowsingContextId, WebViewId};
 use base::{Epoch, WebRenderEpochToU16};
+use bitflags::bitflags;
 use compositing_traits::{
     CompositionPipeline, CompositorMsg, CompositorReceiver, ConstellationMsg, SendableFrameTree,
 };
@@ -122,8 +123,8 @@ pub struct IOCompositor {
     /// The type of composition to perform
     composite_target: CompositeTarget,
 
-    /// Tracks whether we should composite this frame.
+    /// Tracks whether or not the view needs to be repainted.
-    composition_request: CompositionRequest,
+    needs_repaint: Cell<RepaintReason>,
 
     /// Tracks whether we are in the process of shutting down, or have shut down and should close
     /// the compositor.
@@ -198,10 +199,6 @@ pub struct IOCompositor {
     /// The number of frames pending to receive from WebRender.
     pending_frames: usize,
 
-    /// A list of [`WebViewId`]s of WebViews that have new frames ready that are waiting to
-    /// be presented.
-    webviews_waiting_on_present: FnvHashSet<WebViewId>,
-
     /// The [`Instant`] of the last animation tick, used to avoid flooding the Constellation and
     /// ScriptThread with a deluge of animation ticks.
     last_animation_tick: Instant,
@@ -230,27 +227,21 @@ enum ScrollZoomEvent {
     Scroll(ScrollEvent),
 }
 
-/// Why we performed a composite. This is used for debugging.
+/// Why we need to be repainted. This is used for debugging.
-///
+#[derive(Clone, Copy, Default)]
-/// TODO: It would be good to have a bit more precision here about why a composite
+struct RepaintReason(u8);
-/// was originally triggered, but that would require tracking the reason when a
-/// frame is queued in WebRender and then remembering when the frame is ready.
-#[derive(Clone, Copy, Debug, PartialEq)]
-enum CompositingReason {
-    /// We're performing the single composite in headless mode.
-    Headless,
-    /// We're performing a composite to run an animation.
-    Animation,
-    /// A new WebRender frame has arrived.
-    NewWebRenderFrame,
-    /// The window has been resized and will need to be synchronously repainted.
-    Resize,
-}
 
-#[derive(Debug, PartialEq)]
+bitflags! {
-enum CompositionRequest {
+    impl RepaintReason: u8 {
-    NoCompositingNecessary,
+        /// We're performing the single repaint in headless mode.
-    CompositeNow(CompositingReason),
+        const ReadyForScreenshot = 1 << 0;
+        /// We're performing a repaint to run an animation.
+        const ChangedAnimationState = 1 << 1;
+        /// A new WebRender frame has arrived.
+        const NewWebRenderFrame = 1 << 2;
+        /// The window has been resized and will need to be synchronously repainted.
+        const Resize = 1 << 3;
+    }
 }
 
 #[derive(Clone, Copy, Debug, PartialEq)]
@@ -355,7 +346,7 @@ impl IOCompositor {
             port: state.receiver,
             webviews: WebViewManager::default(),
             pipeline_details: HashMap::new(),
-            composition_request: CompositionRequest::NoCompositingNecessary,
+            needs_repaint: Cell::default(),
             touch_handler: TouchHandler::new(),
             pending_scroll_zoom_events: Vec::new(),
             composite_target,
@@ -383,7 +374,6 @@ impl IOCompositor {
             exit_after_load,
             convert_mouse_to_touch,
             pending_frames: 0,
-            webviews_waiting_on_present: Default::default(),
             last_animation_tick: Instant::now(),
             version_string,
         };
@@ -404,8 +394,14 @@ impl IOCompositor {
         }
     }
 
-    pub fn webviews_waiting_on_present(&self) -> Iter<'_, WebViewId> {
+    fn set_needs_repaint(&self, reason: RepaintReason) {
-        self.webviews_waiting_on_present.iter()
+        let mut needs_repaint = self.needs_repaint.get();
+        needs_repaint.insert(reason);
+        self.needs_repaint.set(needs_repaint);
+    }
+
+    pub fn needs_repaint(&self) -> bool {
+        !self.needs_repaint.get().is_empty()
     }
 
     fn update_cursor(&mut self, result: &CompositorHitTestResult) {
@@ -433,14 +429,12 @@ impl IOCompositor {
         }
     }
 
-    pub fn maybe_start_shutting_down(&mut self) {
+    pub fn start_shutting_down(&mut self) {
-        if self.shutdown_state == ShutdownState::NotShuttingDown {
+        if self.shutdown_state != ShutdownState::NotShuttingDown {
-            debug!("Shutting down the constellation for WindowEvent::Quit");
+            warn!("Requested shutdown while already shutting down");
-            self.start_shutting_down();
+            return;
         }
-    }
 
-    fn start_shutting_down(&mut self) {
         debug!("Compositor sending Exit message to Constellation");
         if let Err(e) = self.constellation_chan.send(ConstellationMsg::Exit) {
             warn!("Sending exit message to constellation failed ({:?}).", e);
@@ -525,7 +519,7 @@ impl IOCompositor {
                 } else {
                     self.ready_to_save_state = ReadyState::Unknown;
                 }
-                self.composite_if_necessary(CompositingReason::Headless);
+                self.set_needs_repaint(RepaintReason::ReadyForScreenshot);
             },
 
             CompositorMsg::SetThrottled(pipeline_id, throttled) => {
@@ -549,7 +543,7 @@ impl IOCompositor {
                 }
 
                 if recomposite_needed || self.animation_callbacks_active() {
-                    self.composite_if_necessary(CompositingReason::NewWebRenderFrame)
+                    self.set_needs_repaint(RepaintReason::NewWebRenderFrame);
                 }
             },
 
@@ -558,7 +552,7 @@ impl IOCompositor {
                 if matches!(self.composite_target, CompositeTarget::PngFile(_)) ||
                     self.exit_after_load
                 {
-                    self.composite_if_necessary(CompositingReason::Headless);
+                    self.set_needs_repaint(RepaintReason::ReadyForScreenshot);
                 }
             },
 
@@ -903,7 +897,7 @@ impl IOCompositor {
                 let throttled = self.pipeline_details(pipeline_id).throttled;
                 self.pipeline_details(pipeline_id).animations_running = true;
                 if !throttled {
-                    self.composite_if_necessary(CompositingReason::Animation);
+                    self.set_needs_repaint(RepaintReason::ChangedAnimationState);
                 }
             },
             AnimationState::AnimationCallbacksPresent => {
@@ -1297,7 +1291,7 @@ impl IOCompositor {
         }
 
         self.update_after_zoom_or_hidpi_change();
-        self.composite_if_necessary(CompositingReason::Resize);
+        self.set_needs_repaint(RepaintReason::Resize);
         true
     }
 
@@ -1953,18 +1947,21 @@ impl IOCompositor {
     }
 
     pub fn composite(&mut self) {
-        match self.composite_specific_target(self.composite_target.clone(), None) {
+        if let Err(error) = self.composite_specific_target(self.composite_target.clone(), None) {
-            Ok(_) => {
+            warn!("Unable to composite: {error:?}");
-                if matches!(self.composite_target, CompositeTarget::PngFile(_)) ||
+            return;
-                    self.exit_after_load
+        }
-                {
+
-                    println!("Shutting down the Constellation after generating an output file or exit flag specified");
+        // We've painted the default target, which means that from the embedder's perspective,
-                    self.start_shutting_down();
+        // the scene no longer needs to be repainted.
-                }
+        self.needs_repaint.set(RepaintReason::empty());
-            },
+
-            Err(error) => {
+        // Queue up any subsequent paints for animations.
-                trace!("Unable to composite: {error:?}");
+        self.process_animations(true);
-            },
+
+        if matches!(self.composite_target, CompositeTarget::PngFile(_)) || self.exit_after_load {
+            println!("Shutting down the Constellation after generating an output file or exit flag specified");
+            self.start_shutting_down();
         }
     }
 
@@ -2090,19 +2087,6 @@ impl IOCompositor {
             },
         };
 
-        #[cfg(feature = "tracing")]
-        let _span =
-            tracing::trace_span!("ConstellationMsg::ReadyToPresent", servo_profiling = true)
-                .entered();
-
-        // Notify embedder that servo is ready to present.
-        // Embedder should call `present` to tell compositor to continue rendering.
-        self.webviews_waiting_on_present
-            .extend(self.webviews.painting_order().map(|(&id, _)| id));
-        self.composition_request = CompositionRequest::NoCompositingNecessary;
-
-        self.process_animations(true);
-
         Ok(rv)
     }
 
@@ -2172,31 +2156,13 @@ impl IOCompositor {
         }
     }
 
-    #[cfg_attr(
-        feature = "tracing",
-        tracing::instrument(skip_all, fields(servo_profiling = true), level = "trace")
-    )]
-    pub fn present(&mut self) {
-        #[cfg(feature = "tracing")]
-        let _span =
-            tracing::trace_span!("Compositor Present Surface", servo_profiling = true).entered();
-        self.rendering_context.present();
-        self.webviews_waiting_on_present.clear();
-    }
-
-    fn composite_if_necessary(&mut self, reason: CompositingReason) {
-        trace!(
-            "Will schedule a composite {reason:?}. Previously was {:?}",
-            self.composition_request
-        );
-        self.composition_request = CompositionRequest::CompositeNow(reason)
-    }
-
     fn clear_background(&self) {
         let gl = &self.webrender_gl;
         self.assert_gl_framebuffer_complete();
 
-        // Set the viewport background based on prefs.
+        // Always clear the entire RenderingContext, regardless of how many WebViews there are
+        // or where they are positioned. This is so WebView actually clears even before the
+        // first WebView is ready.
         let color = servo_config::pref!(shell_background_color_rgba);
         gl.clear_color(
             color[0] as f32,
@@ -2204,22 +2170,7 @@ impl IOCompositor {
             color[2] as f32,
             color[3] as f32,
         );
-
+        gl.clear(gleam::gl::COLOR_BUFFER_BIT);
-        // Clear the viewport rect of each top-level browsing context.
-        for (_, webview) in self.webviews.painting_order() {
-            let rect = self.embedder_coordinates.flip_rect(&webview.rect.to_i32());
-            gl.scissor(
-                rect.min.x,
-                rect.min.y,
-                rect.size().width,
-                rect.size().height,
-            );
-            gl.enable(gleam::gl::SCISSOR_TEST);
-            gl.clear(gleam::gl::COLOR_BUFFER_BIT);
-            gl.disable(gleam::gl::SCISSOR_TEST);
-        }
-
-        self.assert_gl_framebuffer_complete();
     }
 
     #[track_caller]
@@ -2288,11 +2239,6 @@ impl IOCompositor {
             self.zoom_action = false;
         }
 
-        match self.composition_request {
-            CompositionRequest::NoCompositingNecessary => {},
-            CompositionRequest::CompositeNow(_) => self.composite(),
-        }
-
         #[cfg(feature = "webxr")]
         // Run the WebXR main thread
         self.webxr_main_thread.run_one_frame();

components/servo/examples/winit_minimal.rs

@@ -8,7 +8,7 @@ use std::rc::Rc;
 
 use compositing::windowing::{AnimationState, EmbedderMethods, WindowMethods};
 use euclid::{Point2D, Scale, Size2D};
-use servo::{RenderingContext, Servo, TouchEventAction, WebView, WindowRenderingContext};
+use servo::{RenderingContext, Servo, TouchEventType, WebView, WindowRenderingContext};
 use servo_geometry::DeviceIndependentPixel;
 use tracing::warn;
 use url::Url;
@@ -44,6 +44,7 @@ fn main() -> Result<(), Box<dyn Error>> {
 struct AppState {
     window_delegate: Rc<WindowDelegate>,
     servo: Servo,
+    rendering_context: Rc<WindowRenderingContext>,
     webviews: RefCell<Vec<WebView>>,
 }
 
@@ -93,9 +94,10 @@ impl ApplicationHandler<WakerEvent> for App {
                 .expect("Failed to create winit Window");
             let window_handle = window.window_handle().expect("Failed to get window handle");
 
-            let rendering_context =
+            let rendering_context = Rc::new(
                 WindowRenderingContext::new(display_handle, window_handle, &window.inner_size())
-                    .expect("Could not create RenderingContext for window.");
+                    .expect("Could not create RenderingContext for window."),
+            );
             let window_delegate = Rc::new(WindowDelegate::new(window));
 
             let _ = rendering_context.make_current();
@@ -103,7 +105,7 @@ impl ApplicationHandler<WakerEvent> for App {
             let servo = Servo::new(
                 Default::default(),
                 Default::default(),
-                Rc::new(rendering_context),
+                rendering_context.clone(),
                 Box::new(EmbedderDelegate {
                     waker: waker.clone(),
                 }),
@@ -116,6 +118,7 @@ impl ApplicationHandler<WakerEvent> for App {
             let app_state = Rc::new(AppState {
                 window_delegate,
                 servo,
+                rendering_context,
                 webviews: Default::default(),
             });
 
@@ -152,8 +155,8 @@ impl ApplicationHandler<WakerEvent> for App {
             },
             WindowEvent::RedrawRequested => {
                 if let Self::Running(state) = self {
-                    state.webviews.borrow().last().unwrap().paint_immediately();
+                    state.webviews.borrow().last().unwrap().paint();
-                    state.servo.present();
+                    state.rendering_context.present();
                 }
             },
             WindowEvent::MouseWheel { delta, .. } => {
@@ -170,7 +173,7 @@ impl ApplicationHandler<WakerEvent> for App {
                         webview.notify_scroll_event(
                             ScrollLocation::Delta(moved_by),
                             DeviceIntPoint::new(10, 10),
-                            TouchEventAction::Down,
+                            TouchEventType::Down,
                         );
                     }
                 }

components/servo/lib.rs

@@ -658,11 +658,15 @@ impl Servo {
     }
 
     fn send_new_frame_ready_messages(&self) {
+        if !self.compositor.borrow().needs_repaint() {
+            return;
+        }
+
         for webview in self
-            .compositor
+            .webviews
             .borrow()
-            .webviews_waiting_on_present()
+            .values()
-            .filter_map(|id| self.get_webview_handle(*id))
+            .filter_map(WebView::from_weak_handle)
         {
             webview.delegate().notify_new_frame_ready(webview);
         }
@@ -686,7 +690,7 @@ impl Servo {
     }
 
     pub fn start_shutting_down(&self) {
-        self.compositor.borrow_mut().maybe_start_shutting_down();
+        self.compositor.borrow_mut().start_shutting_down();
     }
 
     pub fn allow_navigation_response(&self, pipeline_id: PipelineId, allow: bool) {
@@ -703,10 +707,6 @@ impl Servo {
         self.compositor.borrow_mut().deinit();
     }
 
-    pub fn present(&self) {
-        self.compositor.borrow_mut().present();
-    }
-
     pub fn new_webview(&self, url: url::Url) -> WebView {
         let webview = WebView::new(&self.constellation_proxy, self.compositor.clone());
         self.webviews

components/servo/webview.rs

@@ -23,6 +23,33 @@ use crate::clipboard_delegate::{ClipboardDelegate, DefaultClipboardDelegate};
 use crate::webview_delegate::{DefaultWebViewDelegate, WebViewDelegate};
 use crate::ConstellationProxy;
 
+/// 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
+/// considers that the webview has closed and will clean up all associated resources related
+/// to this webview.
+///
+/// ## Rendering Model
+///
+/// Every [`WebView`] has a [`RenderingContext`](crate::RenderingContext). The embedder manages when
+/// the contents of the [`WebView`] paint to the [`RenderingContext`](crate::RenderingContext). When
+/// a [`WebView`] needs to be painted, for instance, because its contents have changed, Servo will
+/// call [`WebViewDelegate::notify_new_frame_ready`] in order to signal that it is time to repaint
+/// the [`WebView`] using [`WebView::paint`].
+///
+/// An example of how this flow might work is:
+///
+/// 1. [`WebViewDelegate::notify_new_frame_ready`] is called. The applications triggers a request
+///    to repaint the window that contains this [`WebView`].
+/// 2. During window repainting, the application calls [`WebView::paint`] and the contents of the
+///    [`RenderingContext`][crate::RenderingContext] are updated.
+/// 3. If the [`RenderingContext`][crate::RenderingContext] is double-buffered, the
+///    application then calls [`crate::RenderingContext::present()`] in order to swap the back buffer
+///    to the front, finally displaying the updated [`WebView`] contents.
+///
+/// In cases where the [`WebView`] contents have not been updated, but a repaint is necessary, for
+/// instance when repainting a window due to damage, an application may simply perform the final two
+/// steps and Servo will repaint even without first calling the
+/// [`WebViewDelegate::notify_new_frame_ready`] method.
 #[derive(Clone)]
 pub struct WebView(Rc<RefCell<WebViewInner>>);
 
@@ -63,12 +90,6 @@ impl Drop for WebViewInner {
     }
 }
 
-/// Handle for a webview.
-///
-/// - The webview exists for exactly as long as there are WebView handles
-///   (FIXME: this is not true yet; webviews can still close of their own volition)
-/// - All methods are infallible; if the constellation dies, the embedder finds out when calling
-///   [Servo::handle_events](crate::Servo::handle_events)
 impl WebView {
     pub(crate) fn new(
         constellation_proxy: &ConstellationProxy,
@@ -415,7 +436,7 @@ impl WebView {
             .send(ConstellationMsg::SendError(Some(self.id()), message));
     }
 
-    pub fn paint_immediately(&self) {
+    pub fn paint(&self) {
         self.inner().compositor.borrow_mut().composite();
     }
 }

ports/servoshell/desktop/app.rs

@@ -58,7 +58,6 @@ pub(crate) enum PumpResult {
     Shutdown,
     Continue {
         need_update: bool,
-        new_servo_frame: bool,
         need_window_redraw: bool,
     },
 }
@@ -195,23 +194,15 @@ impl App {
             },
             PumpResult::Continue {
                 need_update: update,
-                new_servo_frame,
                 need_window_redraw,
             } => {
-                // A new Servo frame is ready, so swap the buffer on our `RenderingContext`. In headed mode
-                // this won't immediately update the widget surface, because we render to an offscreen
-                // `RenderingContext`.
-                if new_servo_frame {
-                    state.servo().present();
-                }
-
                 let updated = match (update, &mut self.minibrowser) {
                     (true, Some(minibrowser)) => minibrowser.update_webview_data(state),
                     _ => false,
                 };
 
                 // If in headed mode, request a winit redraw event, so we can paint the minibrowser.
-                if updated || need_window_redraw || new_servo_frame {
+                if updated || need_window_redraw {
                     if let Some(window) = window.winit_window() {
                         window.request_redraw();
                     }
@@ -247,14 +238,7 @@ impl App {
                 state.shutdown();
                 self.state = AppState::ShuttingDown;
             },
-            PumpResult::Continue {
+            PumpResult::Continue { .. } => state.repaint_servo_if_necessary(),
-                new_servo_frame, ..
-            } => {
-                if new_servo_frame {
-                    // In headless mode, we present directly.
-                    state.servo().present();
-                }
-            },
         }
 
         !matches!(self.state, AppState::ShuttingDown)

ports/servoshell/desktop/app_state.rs

@@ -73,8 +73,9 @@ pub struct RunningAppStateInner {
     /// Whether or not the application interface needs to be updated.
     need_update: bool,
 
-    /// Whether or not the application needs to be redrawn.
+    /// Whether or not Servo needs to repaint its display. Currently this is global
-    new_servo_frame_ready: bool,
+    /// because every `WebView` shares a `RenderingContext`.
+    need_repaint: bool,
 }
 
 impl Drop for RunningAppState {
@@ -101,7 +102,7 @@ impl RunningAppState {
                 window,
                 gamepad_support: GamepadSupport::maybe_new(),
                 need_update: false,
-                new_servo_frame_ready: false,
+                need_repaint: false,
             }),
         }
     }
@@ -124,6 +125,19 @@ impl RunningAppState {
         &self.servo
     }
 
+    pub(crate) fn repaint_servo_if_necessary(&self) {
+        if !self.inner().need_repaint {
+            return;
+        }
+        let Some(webview) = self.focused_webview() else {
+            return;
+        };
+
+        webview.paint();
+        self.inner().window.rendering_context().present();
+        self.inner_mut().need_repaint = false;
+    }
+
     /// Spins the internal application event loop.
     ///
     /// - Notifies Servo about incoming gamepad events
@@ -138,15 +152,12 @@ impl RunningAppState {
         }
 
         // Delegate handlers may have asked us to present or update compositor contents.
-        let new_servo_frame = std::mem::replace(&mut self.inner_mut().new_servo_frame_ready, false);
-        let need_update = std::mem::replace(&mut self.inner_mut().need_update, false);
-
         // Currently, egui-file-dialog dialogs need to be constantly redrawn or animations aren't fluid.
-        let need_window_redraw = new_servo_frame || self.has_active_dialog();
+        let need_window_redraw = self.inner().need_repaint || self.has_active_dialog();
+        let need_update = std::mem::replace(&mut self.inner_mut().need_update, false);
 
         PumpResult::Continue {
             need_update,
-            new_servo_frame,
             need_window_redraw,
         }
     }
@@ -484,7 +495,7 @@ impl WebViewDelegate for RunningAppState {
     }
 
     fn notify_new_frame_ready(&self, _webview: servo::WebView) {
-        self.inner_mut().new_servo_frame_ready = true;
+        self.inner_mut().need_repaint = true;
     }
 
     fn play_gamepad_haptic_effect(

ports/servoshell/desktop/minibrowser.rs

@@ -403,6 +403,8 @@ impl Minibrowser {
                     );
                 }
 
+                state.repaint_servo_if_necessary();
+
                 if let Some(render_to_parent) = rendering_context.render_to_parent_callback() {
                     ui.painter().add(PaintCallback {
                         rect,

ports/servoshell/egl/app_state.rs

@@ -680,8 +680,8 @@ impl RunningAppState {
     pub fn present_if_needed(&self) {
         if self.inner().need_present {
             self.inner_mut().need_present = false;
-            self.active_webview().paint_immediately();
+            self.active_webview().paint();
-            self.servo.present();
+            self.rendering_context.present();
         }
     }
 }