From 53c0726ef431d32c6f71a1a1eee23019782bd68b Mon Sep 17 00:00:00 2001 From: Martin Robinson Date: Mon, 20 May 2024 15:04:32 +0200 Subject: [PATCH] script: Have `Document` own `Layout` (#32316) Have `Document` own `Layout`. This makes it impossible to have a `Document` without `Layout`, which was true, but now the compiler checks it. In addition, `Layout` is now released when the `Document` is, avoiding leaking the entire `Layout`. --- components/script/dom/document.rs | 37 +++----- components/script/dom/documentorshadowroot.rs | 4 +- components/script/dom/htmlelement.rs | 4 +- components/script/dom/htmlimageelement.rs | 6 +- components/script/dom/mediaquerylist.rs | 4 +- components/script/dom/window.rs | 93 +++++++++---------- components/script/script_thread.rs | 73 +++++---------- 7 files changed, 88 insertions(+), 133 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index f58f9472958..d2c97bdc5de 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -58,7 +58,6 @@ use servo_url::{ImmutableOrigin, MutableOrigin, ServoUrl}; use style::attr::AttrValue; use style::context::QuirksMode; use style::invalidation::element::restyle_hints::RestyleHint; -use style::media_queries::Device; use style::selector_parser::Snapshot; use style::shared_lock::SharedRwLock as StyleSharedRwLock; use style::str::{split_html_space_chars, str_join}; @@ -851,9 +850,7 @@ impl Document { let old_mode = self.quirks_mode.replace(new_mode); if old_mode != new_mode { - let _ = self - .window - .with_layout(move |layout| layout.set_quirks_mode(new_mode)); + self.window.layout_mut().set_quirks_mode(new_mode); } } @@ -3526,16 +3523,6 @@ impl Document { have_changed } - /// Runs the given closure using the Stylo `Device` suitable for media query evaluation. - /// - /// TODO: This can just become a getter when each Layout is more strongly associated with - /// its given Document and Window. - pub fn with_device(&self, call: impl FnOnce(&Device) -> T) -> T { - self.window - .with_layout(move |layout| call(layout.device())) - .unwrap() - } - pub fn salvageable(&self) -> bool { self.salvageable.get() } @@ -3899,12 +3886,10 @@ impl Document { let cloned_stylesheet = sheet.clone(); let insertion_point2 = insertion_point.clone(); - let _ = self.window.with_layout(move |layout| { - layout.add_stylesheet( - cloned_stylesheet, - insertion_point2.as_ref().map(|s| s.sheet.clone()), - ); - }); + self.window.layout_mut().add_stylesheet( + cloned_stylesheet, + insertion_point2.as_ref().map(|s| s.sheet.clone()), + ); DocumentOrShadowRoot::add_stylesheet( owner, @@ -3917,18 +3902,18 @@ impl Document { /// Given a stylesheet, load all web fonts from it in Layout. pub fn load_web_fonts_from_stylesheet(&self, stylesheet: Arc) { - let _ = self.window.with_layout(move |layout| { - layout.load_web_fonts_from_stylesheet(stylesheet); - }); + self.window + .layout() + .load_web_fonts_from_stylesheet(stylesheet); } /// Remove a stylesheet owned by `owner` from the list of document sheets. #[allow(crown::unrooted_must_root)] // Owner needs to be rooted already necessarily. pub fn remove_stylesheet(&self, owner: &Element, stylesheet: &Arc) { let cloned_stylesheet = stylesheet.clone(); - let _ = self - .window - .with_layout(|layout| layout.remove_stylesheet(cloned_stylesheet)); + self.window + .layout_mut() + .remove_stylesheet(cloned_stylesheet); DocumentOrShadowRoot::remove_stylesheet( owner, diff --git a/components/script/dom/documentorshadowroot.rs b/components/script/dom/documentorshadowroot.rs index 25ada1eddae..560f8c43e3d 100644 --- a/components/script/dom/documentorshadowroot.rs +++ b/components/script/dom/documentorshadowroot.rs @@ -91,8 +91,8 @@ impl DocumentOrShadowRoot { }; self.window - .with_layout(|layout| layout.query_nodes_from_point(*client_point, query_type)) - .unwrap_or_default() + .layout() + .query_nodes_from_point(*client_point, query_type) } #[allow(unsafe_code)] diff --git a/components/script/dom/htmlelement.rs b/components/script/dom/htmlelement.rs index 63143a7d2fa..9e4cc20ccf9 100644 --- a/components/script/dom/htmlelement.rs +++ b/components/script/dom/htmlelement.rs @@ -462,8 +462,8 @@ impl HTMLElementMethods for HTMLElement { window.layout_reflow(QueryMsg::ElementInnerTextQuery); let text = window - .with_layout(|layout| layout.query_element_inner_text(node.to_trusted_node_address())) - .unwrap_or_default(); + .layout() + .query_element_inner_text(node.to_trusted_node_address()); DOMString::from(text) } diff --git a/components/script/dom/htmlimageelement.rs b/components/script/dom/htmlimageelement.rs index 301c4e17b22..80c016b6b9d 100644 --- a/components/script/dom/htmlimageelement.rs +++ b/components/script/dom/htmlimageelement.rs @@ -670,7 +670,8 @@ impl HTMLImageElement { ) -> Au { let document = document_from_node(self); let quirks_mode = document.quirks_mode(); - document.with_device(move |device| source_size_list.evaluate(device, quirks_mode)) + let result = source_size_list.evaluate(document.window().layout().device(), quirks_mode); + result } /// @@ -696,7 +697,8 @@ impl HTMLImageElement { let mut parserInput = ParserInput::new(&media_query); let mut parser = Parser::new(&mut parserInput); let media_list = MediaList::parse(&context, &mut parser); - document.with_device(move |device| media_list.evaluate(device, quirks_mode)) + let result = media_list.evaluate(document.window().layout().device(), quirks_mode); + result } /// diff --git a/components/script/dom/mediaquerylist.rs b/components/script/dom/mediaquerylist.rs index 685e66f0390..16675e1ed98 100644 --- a/components/script/dom/mediaquerylist.rs +++ b/components/script/dom/mediaquerylist.rs @@ -73,8 +73,8 @@ impl MediaQueryList { pub fn evaluate(&self) -> bool { let quirks_mode = self.document.quirks_mode(); - self.document - .with_device(move |device| self.media_query_list.evaluate(device, quirks_mode)) + self.media_query_list + .evaluate(self.document.window().layout().device(), quirks_mode) } } diff --git a/components/script/dom/window.rs b/components/script/dom/window.rs index fcba7ffcbeb..5dbbc3f2377 100644 --- a/components/script/dom/window.rs +++ b/components/script/dom/window.rs @@ -3,7 +3,7 @@ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ use std::borrow::{Cow, ToOwned}; -use std::cell::Cell; +use std::cell::{Cell, RefCell, RefMut}; use std::collections::hash_map::Entry; use std::collections::{HashMap, HashSet}; use std::default::Default; @@ -193,6 +193,9 @@ pub struct Window { #[ignore_malloc_size_of = "trait objects are hard"] script_chan: MainThreadScriptChan, task_manager: TaskManager, + #[no_trace] + #[ignore_malloc_size_of = "TODO: Add MallocSizeOf support to layout"] + layout: RefCell>, navigator: MutNullableDom, #[ignore_malloc_size_of = "Arc"] #[no_trace] @@ -365,6 +368,14 @@ impl Window { &self.task_manager } + pub fn layout(&self) -> Ref> { + self.layout.borrow() + } + + pub fn layout_mut(&self) -> RefMut> { + self.layout.borrow_mut() + } + pub fn get_exists_mut_observer(&self) -> bool { self.exists_mut_observer.get() } @@ -1877,7 +1888,7 @@ impl Window { animations: document.animations().sets.clone(), }; - let _ = self.with_layout(move |layout| layout.reflow(reflow)); + self.layout.borrow_mut().reflow(reflow); let complete = match join_port.try_recv() { Err(TryRecvError::Empty) => { @@ -1988,10 +1999,7 @@ impl Window { elem.has_class(&atom!("reftest-wait"), CaseSensitivity::CaseSensitive) }); - let pending_web_fonts = self - .with_layout(move |layout| layout.waiting_for_web_fonts_to_load()) - .unwrap(); - + let pending_web_fonts = self.layout.borrow().waiting_for_web_fonts_to_load(); let has_sent_idle_message = self.has_sent_idle_message.get(); let is_ready_state_complete = document.ReadyState() == DocumentReadyState::Complete; let pending_images = !self.pending_layout_images.borrow().is_empty(); @@ -2022,10 +2030,7 @@ impl Window { return; } - let epoch = self - .with_layout(move |layout| layout.current_epoch()) - .unwrap(); - + let epoch = self.layout.borrow().current_epoch(); debug!( "{:?}: Updating constellation epoch: {epoch:?}", self.pipeline_id() @@ -2049,39 +2054,34 @@ impl Window { } let document = self.Document(); - self.with_layout(|layout| { - layout.query_resolved_font_style( - node.to_trusted_node_address(), - &value, - document.animations().sets.clone(), - document.current_animation_timeline_value(), - ) - }) - .unwrap() + let animations = document.animations().sets.clone(); + self.layout.borrow().query_resolved_font_style( + node.to_trusted_node_address(), + &value, + animations, + document.current_animation_timeline_value(), + ) } pub fn content_box_query(&self, node: &Node) -> Option> { if !self.layout_reflow(QueryMsg::ContentBox) { return None; } - self.with_layout(|layout| layout.query_content_box(node.to_opaque())) - .unwrap_or(None) + self.layout.borrow().query_content_box(node.to_opaque()) } pub fn content_boxes_query(&self, node: &Node) -> Vec> { if !self.layout_reflow(QueryMsg::ContentBoxes) { return vec![]; } - self.with_layout(|layout| layout.query_content_boxes(node.to_opaque())) - .unwrap_or_default() + self.layout.borrow().query_content_boxes(node.to_opaque()) } pub fn client_rect_query(&self, node: &Node) -> UntypedRect { if !self.layout_reflow(QueryMsg::ClientRectQuery) { return Rect::zero(); } - self.with_layout(|layout| layout.query_client_rect(node.to_opaque())) - .unwrap_or_default() + self.layout.borrow().query_client_rect(node.to_opaque()) } /// Find the scroll area of the given node, if it is not None. If the node @@ -2091,8 +2091,7 @@ impl Window { if !self.layout_reflow(QueryMsg::ScrollingAreaQuery) { return Rect::zero(); } - self.with_layout(|layout| layout.query_scrolling_area(opaque)) - .unwrap_or_default() + self.layout.borrow().query_scrolling_area(opaque) } pub fn scroll_offset_query(&self, node: &Node) -> Vector2D { @@ -2136,18 +2135,14 @@ impl Window { } let document = self.Document(); - DOMString::from( - self.with_layout(|layout| { - layout.query_resolved_style( - element, - pseudo, - property, - document.animations().sets.clone(), - document.current_animation_timeline_value(), - ) - }) - .unwrap(), - ) + let animations = document.animations().sets.clone(); + DOMString::from(self.layout.borrow().query_resolved_style( + element, + pseudo, + property, + animations, + document.current_animation_timeline_value(), + )) } pub fn inner_window_dimensions_query( @@ -2157,8 +2152,9 @@ impl Window { if !self.layout_reflow(QueryMsg::InnerWindowDimensionsQuery) { return None; } - self.with_layout(|layout| layout.query_inner_window_dimension(browsing_context)) - .unwrap() + self.layout + .borrow() + .query_inner_window_dimension(browsing_context) } #[allow(unsafe_code)] @@ -2167,9 +2163,7 @@ impl Window { return (None, Rect::zero()); } - let response = self - .with_layout(|layout| layout.query_offset_parent(node.to_opaque())) - .unwrap(); + let response = self.layout.borrow().query_offset_parent(node.to_opaque()); let element = response.node_address.and_then(|parent_node_address| { let node = unsafe { from_untrusted_node_address(parent_node_address) }; DomRoot::downcast(node) @@ -2185,8 +2179,9 @@ impl Window { if !self.layout_reflow(QueryMsg::TextIndexQuery) { return None; } - self.with_layout(|layout| layout.query_text_indext(node.to_opaque(), point_in_node)) - .unwrap() + self.layout + .borrow() + .query_text_indext(node.to_opaque(), point_in_node) } #[allow(unsafe_code)] @@ -2309,10 +2304,6 @@ impl Window { self.Document().url() } - pub fn with_layout(&self, call: impl FnOnce(&mut dyn Layout) -> T) -> Result { - ScriptThread::with_layout(self.pipeline_id(), call) - } - pub fn windowproxy_handler(&self) -> WindowProxyHandler { WindowProxyHandler(self.dom_static.windowproxy_handler.0) } @@ -2517,6 +2508,7 @@ impl Window { runtime: Rc, script_chan: MainThreadScriptChan, task_manager: TaskManager, + layout: Box, image_cache_chan: Sender, image_cache: Arc, resource_threads: ResourceThreads, @@ -2580,6 +2572,7 @@ impl Window { ), script_chan, task_manager, + layout: RefCell::new(layout), image_cache_chan, image_cache, navigator: Default::default(), diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 4c67071a7b0..7a3401a3eac 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -73,9 +73,7 @@ use parking_lot::Mutex; use percent_encoding::percent_decode; use profile_traits::mem::{self as profile_mem, OpaqueSender, ReportsChan}; use profile_traits::time::{self as profile_time, profile, ProfilerCategory}; -use script_layout_interface::{ - Layout, LayoutConfig, LayoutFactory, ReflowGoal, ScriptThreadFactory, -}; +use script_layout_interface::{LayoutConfig, LayoutFactory, ReflowGoal, ScriptThreadFactory}; use script_traits::webdriver_msg::WebDriverScriptCommand; use script_traits::{ CompositorEvent, ConstellationControlMsg, DiscardBrowsingContext, DocumentActivity, @@ -731,10 +729,6 @@ pub struct ScriptThread { // Secure context inherited_secure_context: Option, - /// The layouts that we control. - #[no_trace] - layouts: RefCell>>, - /// A factory for making new layouts. This allows layout to depend on script. #[no_trace] layout_factory: Arc, @@ -864,22 +858,6 @@ impl ScriptThreadFactory for ScriptThread { } impl ScriptThread { - pub fn with_layout( - pipeline_id: PipelineId, - call: impl FnOnce(&mut dyn Layout) -> T, - ) -> Result { - SCRIPT_THREAD_ROOT.with(|root| { - let script_thread = unsafe { &*root.get().unwrap() }; - let mut layouts = script_thread.layouts.borrow_mut(); - if let Some(ref mut layout) = layouts.get_mut(&pipeline_id) { - Ok(call(&mut ***layout)) - } else { - warn!("No layout found for {}", pipeline_id); - Err(()) - } - }) - } - pub fn runtime_handle() -> ParentRuntime { SCRIPT_THREAD_ROOT.with(|root| { let script_thread = unsafe { &*root.get().unwrap() }; @@ -1204,19 +1182,14 @@ impl ScriptThread { properties: Vec, painter: Box, ) { - let window = self.documents.borrow().find_window(pipeline_id); - let window = match window { - Some(window) => window, - None => { - return warn!( - "Paint worklet registered after pipeline {} closed.", - pipeline_id - ); - }, + let Some(window) = self.documents.borrow().find_window(pipeline_id) else { + warn!("Paint worklet registered after pipeline {pipeline_id} closed."); + return; }; - let _ = window - .with_layout(|layout| layout.register_paint_worklet_modules(name, properties, painter)); + window + .layout_mut() + .register_paint_worklet_modules(name, properties, painter); } pub fn push_new_element_queue() { @@ -1441,7 +1414,6 @@ impl ScriptThread { gpu_id_hub: Arc::new(Mutex::new(Identities::new())), webgpu_port: RefCell::new(None), inherited_secure_context: state.inherited_secure_context, - layouts: Default::default(), layout_factory, } } @@ -2395,11 +2367,19 @@ impl ScriptThread { } fn handle_layout_message(&self, msg: LayoutControlMsg, pipeline_id: PipelineId) { - let _ = Self::with_layout(pipeline_id, |layout| layout.handle_constellation_msg(msg)); + let Some(window) = self.documents.borrow().find_window(pipeline_id) else { + warn!("Received layout message pipeline {pipeline_id} closed: {msg:?}."); + return; + }; + window.layout_mut().handle_constellation_msg(msg); } fn handle_font_cache(&self, pipeline_id: PipelineId) { - let _ = Self::with_layout(pipeline_id, |layout| layout.handle_font_cache_msg()); + let Some(window) = self.documents.borrow().find_window(pipeline_id) else { + warn!("Received font cache message pipeline {pipeline_id} closed."); + return; + }; + window.layout_mut().handle_font_cache_msg(); } fn handle_msg_from_webgpu_server(&self, msg: WebGPUMsg) { @@ -2857,13 +2837,11 @@ impl ScriptThread { let mut reports = vec![]; reports.extend(unsafe { get_reports(*self.get_cx(), path_seg) }); - SCRIPT_THREAD_ROOT.with(|root| { - let script_thread = unsafe { &*root.get().unwrap() }; - let layouts = script_thread.layouts.borrow(); - for layout in layouts.values() { - layout.collect_reports(&mut reports); - } - }); + + for (_, document) in documents.iter() { + document.window().layout().collect_reports(&mut reports); + } + reports_chan.send(reports); } @@ -3223,7 +3201,7 @@ impl ScriptThread { } debug!("{id}: Shutting down layout"); - let _ = document.window().with_layout(|layout| layout.exit_now()); + document.window().layout_mut().exit_now(); debug!("{id}: Sending PipelineExited message to constellation"); self.script_sender @@ -3563,16 +3541,13 @@ impl ScriptThread { paint_time_metrics, window_size: incomplete.window_size, }; - self.layouts.borrow_mut().insert( - incomplete.pipeline_id, - self.layout_factory.create(layout_config), - ); // Create the window and document objects. let window = Window::new( self.js_runtime.clone(), MainThreadScriptChan(sender.clone()), task_manager, + self.layout_factory.create(layout_config), self.image_cache_channel.clone(), self.image_cache.clone(), self.resource_threads.clone(),