libservo: Starting removing the multi-document interface API #40923
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
|
🔨 Triggering try run (#19739311995) for Linux (WPT), Android, OpenHarmony |
0f3653c to
99a9882
Compare
|
🔨 Triggering try run (#19739625918) for Linux (WPT), Android, OpenHarmony |
Bencher Report|
Test results for linux-wpt from try job (#19739625918): Flaky unexpected result (31)
Stable unexpected results that are known to be intermittent (28)
Stable unexpected results (2)
|
|
|
99a9882 to
d11f20c
Compare
|
🔨 Triggering try run (#19742569573) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19742569573): Flaky unexpected result (45)
Stable unexpected results that are known to be intermittent (35)
Stable unexpected results (2)
|
|
|
d11f20c to
8618235
Compare
|
🔨 Triggering try run (#19745176688) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19745176688): Flaky unexpected result (38)
Stable unexpected results that are known to be intermittent (28)
|
|
✨ Try run (#19745176688) succeeded. |
mukilan
reviewed
Nov 28, 2025
components/compositing/painter.rs
Outdated
| if self.webview_renderers.remove(webview_id).is_err() { | ||
| warn!("{webview_id}: Removing unknown webview"); | ||
| if self.webview_renderers.remove(&webview_id).is_none() { | ||
| warn!("Tried removing uknown WebView: {webview_id:?}"); |
Member
There was a problem hiding this comment.
Suggested change
| warn!("Tried removing uknown WebView: {webview_id:?}"); | |
| warn!("Tried removing unknown WebView: {webview_id:?}"); |
| /// The HiDPI scale factor for the `WebView` associated with this renderer. This is controlled | ||
| /// by the embedding layer. | ||
| hidpi_scale_factor: Scale<f32, DeviceIndependentPixel, DevicePixel>, | ||
| /// Whether or not this [`WebViewRenderer`] is hidden or not. |
Member
There was a problem hiding this comment.
Suggested change
| /// Whether or not this [`WebViewRenderer`] is hidden or not. | |
| /// Whether or not this [`WebViewRenderer`] is hidden. |
| .map(|webview| webview.focused_browsing_context_id); | ||
| focused_browsing_context_id.is_some_and(|focused_browsing_context_id| { | ||
| focused_browsing_context_id == change.browsing_context_id || | ||
| self.fully_active_descendant_browsing_contexts_iter(change.browsing_context_id) |
Member
There was a problem hiding this comment.
The name of the method change_browsing_context_is_descendant_of_focused implies that we are checking if the change BC is a descendant of focused BC. But this call seems to imply the reverse - it iterates over descendants of the change BC to see if focused BC is one of them. Is this correct?
Member
Author
There was a problem hiding this comment.
You're right. I've restored the original method name. I suspect that this code can be removed entirely. I don't know why loads should change focus.
| /// If the removed webview was focused, clears the focus. | ||
| /// Returns the removed webview, if it existed. | ||
| /// Removes a webview from the collection by [`WebViewId`]. If the removed [`WebView`] was the active | ||
| /// [`WebView`] then the next newest [`Webview`] will be activated. |
Member
There was a problem hiding this comment.
Suggested change
| /// [`WebView`] then the next newest [`Webview`] will be activated. | |
| /// [`WebView`] then the next newest [`WebView`] will be activated. |
| self.creation_order.retain(|&webview_id| webview_id != id); | ||
| if self.focused_webview_id == Some(id) { | ||
| self.focused_webview_id = None; | ||
| let removed_webivew = self.webviews.remove(&id); |
Member
There was a problem hiding this comment.
Suggested change
| let removed_webivew = self.webviews.remove(&id); | |
| let removed_webview = self.webviews.remove(&id); |
| self.window.request_redraw(); | ||
| } | ||
|
|
||
| fn request_open_auxiliary_webview(&self, parent_webview: WebView) -> Option<WebView> { |
Member
Author
There was a problem hiding this comment.
Yes, in order for this to work with the new API we'd need to iterate the other WebViews and hide them. I think it's better just to remove support for popups from winit_minimal, which is supposed to be a very minimal embedding example. I'm not sure that popups are necessary. We originally added this to test the popup API, but we have plenty of other tests now.
Member
|
I missed it during my testing for #40883, but on NixOS+Wayland, |
For a while Servo has while had a somewhat unfinished multi-document interface API that was meant to implement a kind of window manager inside a single `RenderingContext`. This came with some focus handling and a paint order implementation. This code was never really finished and the `WebView` API is moving toward a design where compositing different `WebView`s together is up to the embedder. This is finally possible now that rendering to multiple `RenderingContext`s is supported. In addition, the MDI API interferes with focus handling when `WebView`s are distributed across windows. Given these two points, this change starts to remove the MDI API. - All `WebView`s can still be hidden / unhidden and start in an unhidden state. - All `WebView`s always have system focus. System focus never properly interfaced with Servo's internal focus system anyway. A followup change will add proper system focus integration. - There are still some leftovers from the MDI interface (such as `WebView`s having their own size). These will be cleaned up in a followup change. Only the changes necessary to get multi-window support working are included here. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
8618235 to
091d1bd
Compare
Member
Author
mukilan
approved these changes
Nov 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For a while Servo has while had a somewhat unfinished multi-document
interface API that was meant to implement a kind of window manager
inside a single
RenderingContext. This came with some focus handlingand a paint order implementation. This code was never really finished
and the
WebViewAPI is moving toward a design where compositingdifferent
WebViews together is up to the embedder.This is finally possible now that rendering to multiple
RenderingContexts is supported. In addition, the MDI API interfereswith focus handling when
WebViews are distributed across windows.Given these two points, this change starts to remove the MDI API.
WebViews can still be hidden / unhidden and start in an unhiddenstate.
WebViews always have system focus. System focus never properlyinterfaced with Servo's internal focus system anyway. A followup
change will add proper system focus integration.
WebViews having their own size). These will be cleaned up in afollowup change. Only the changes necessary to get multi-window
support working are included here.
Testing: This should not change observable behavior and tests are updated for the
new APIs.