From 239f1ae1a7f01b8acdea6cbe327a6b47b775e9cb Mon Sep 17 00:00:00 2001 From: Arnaud Marant Date: Sat, 12 Sep 2015 13:09:41 +0200 Subject: [PATCH 1/3] fix for Layout memory reporter uses pre-redirect url #6872 --- components/layout/layout_task.rs | 28 +++++++++++++++++++-------- components/script/layout_interface.rs | 3 +++ components/script/script_task.rs | 5 +++++ 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index cf7ca56da00..f0d64398337 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -54,6 +54,7 @@ use script_traits::{ConstellationControlMsg, LayoutControlMsg, OpaqueScriptLayou use sequential; use serde_json; use std::borrow::ToOwned; +use std::cell::{Cell, RefCell}; use std::collections::HashMap; use std::collections::hash_state::DefaultState; use std::mem::transmute; @@ -119,7 +120,7 @@ pub struct LayoutTask { id: PipelineId, /// The URL of the pipeline that we belong to. - url: Url, + url: RefCell>, /// Is the current reflow of an iframe, as opposed to a root window? is_iframe: bool, @@ -404,7 +405,7 @@ impl LayoutTask { LayoutTask { id: id, - url: url, + url: RefCell::new(Some(url)), is_iframe: is_iframe, port: port, pipeline_port: pipeline_receiver, @@ -625,6 +626,10 @@ impl LayoutTask { Msg::CreateLayoutTask(info) => { self.create_layout_task(info) } + Msg::SetFinalUrl(final_url) => { + let mut url_ref_cell = self.url.borrow_mut(); + *url_ref_cell = Some(final_url); + }, Msg::PrepareToExit(response_chan) => { self.prepare_to_exit(response_chan); return false @@ -647,15 +652,17 @@ impl LayoutTask { // FIXME(njn): Just measuring the display tree for now. let rw_data = possibly_locked_rw_data.lock(); let stacking_context = rw_data.stacking_context.as_ref(); + let ref formatted_url = *self.url.borrow().as_ref().map_or("url(None)".to_owned(), + |url| format!("url({})", url)); reports.push(Report { - path: path![format!("url({})", self.url), "layout-task", "display-list"], + path: path![formatted_url, "layout-task", "display-list"], kind: ReportKind::ExplicitJemallocHeapSize, size: stacking_context.map_or(0, |sc| sc.heap_size_of_children()), }); // The LayoutTask has a context in TLS... reports.push(Report { - path: path![format!("url({})", self.url), "layout-task", "local-context"], + path: path![formatted_url, "layout-task", "local-context"], kind: ReportKind::ExplicitJemallocHeapSize, size: heap_size_of_local_context(), }); @@ -665,7 +672,7 @@ impl LayoutTask { let sizes = traversal.heap_size_of_tls(heap_size_of_local_context); for (i, size) in sizes.iter().enumerate() { reports.push(Report { - path: path![format!("url({})", self.url), + path: path![formatted_url, format!("layout-worker-{}-local-context", i)], kind: ReportKind::ExplicitJemallocHeapSize, size: *size, @@ -943,6 +950,8 @@ impl LayoutTask { Some(x) => x, }; + debug!("layout: received layout request for: {}", + self.url.borrow().as_ref().map_or("None".to_owned(), |url| url.serialize())); if log_enabled!(log::LogLevel::Debug) { node.dump(); } @@ -1005,9 +1014,10 @@ impl LayoutTask { } // Create a layout context for use throughout the following passes. + let url_clone = &self.url.borrow().as_ref().unwrap().clone(); let mut shared_layout_context = self.build_shared_layout_context(&*rw_data, viewport_size_changed, - &self.url, + url_clone, data.reflow_info.goal); if node.is_dirty() || node.has_dirty_descendants() { @@ -1118,9 +1128,10 @@ impl LayoutTask { page_clip_rect: MAX_RECT, }; + let url_clone = &self.url.borrow().as_ref().unwrap().clone(); let mut layout_context = self.build_shared_layout_context(&*rw_data, false, - &self.url, + url_clone, reflow_info.goal); self.perform_post_main_layout_passes(&reflow_info, &mut *rw_data, &mut layout_context); @@ -1142,9 +1153,10 @@ impl LayoutTask { page_clip_rect: MAX_RECT, }; + let url_clone = &self.url.borrow().as_ref().unwrap().clone(); let mut layout_context = self.build_shared_layout_context(&*rw_data, false, - &self.url, + url_clone, reflow_info.goal); if let Some(mut root_flow) = self.root_flow.clone() { diff --git a/components/script/layout_interface.rs b/components/script/layout_interface.rs index 4a78e23e23c..f18c850b077 100644 --- a/components/script/layout_interface.rs +++ b/components/script/layout_interface.rs @@ -86,6 +86,9 @@ pub enum Msg { /// /// This basically exists to keep the script-layout dependency one-way. CreateLayoutTask(NewLayoutTaskInfo), + + /// Set the final Url. + SetFinalUrl(Url), } /// Synchronous messages that script can send to layout. diff --git a/components/script/script_task.rs b/components/script/script_task.rs index f0ea6bd1354..22da6c81a98 100644 --- a/components/script/script_task.rs +++ b/components/script/script_task.rs @@ -1549,6 +1549,11 @@ impl ScriptTask { /// objects, parses HTML and CSS, and kicks off initial layout. fn load(&self, metadata: Metadata, incomplete: InProgressLoad) -> ParserRoot { let final_url = metadata.final_url.clone(); + { + // send the final url to the layout task. + let LayoutChan(ref chan) = incomplete.layout_chan; + chan.send(layout_interface::Msg::SetFinalUrl(final_url.clone())).unwrap(); + } debug!("ScriptTask: loading {} on page {:?}", incomplete.url.serialize(), incomplete.pipeline_id); // We should either be initializing a root page or loading a child page of an From 8cdafe8ffcad0c8fd303147d5a5781b44c3e47fd Mon Sep 17 00:00:00 2001 From: Arnaud Marant Date: Sun, 13 Sep 2015 11:45:47 +0200 Subject: [PATCH 2/3] remove Option because it is never used as None --- components/layout/layout_task.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index f0d64398337..453483efe7c 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -120,7 +120,7 @@ pub struct LayoutTask { id: PipelineId, /// The URL of the pipeline that we belong to. - url: RefCell>, + url: RefCell, /// Is the current reflow of an iframe, as opposed to a root window? is_iframe: bool, @@ -405,7 +405,7 @@ impl LayoutTask { LayoutTask { id: id, - url: RefCell::new(Some(url)), + url: RefCell::new(url), is_iframe: is_iframe, port: port, pipeline_port: pipeline_receiver, @@ -568,7 +568,7 @@ impl LayoutTask { }; let mut layout_context = self.build_shared_layout_context(&*rw_data, false, - &self.url, + &self.url.borrow(), reflow_info.goal); self.perform_post_style_recalc_layout_passes(&reflow_info, @@ -628,7 +628,7 @@ impl LayoutTask { } Msg::SetFinalUrl(final_url) => { let mut url_ref_cell = self.url.borrow_mut(); - *url_ref_cell = Some(final_url); + *url_ref_cell = final_url; }, Msg::PrepareToExit(response_chan) => { self.prepare_to_exit(response_chan); @@ -652,8 +652,7 @@ impl LayoutTask { // FIXME(njn): Just measuring the display tree for now. let rw_data = possibly_locked_rw_data.lock(); let stacking_context = rw_data.stacking_context.as_ref(); - let ref formatted_url = *self.url.borrow().as_ref().map_or("url(None)".to_owned(), - |url| format!("url({})", url)); + let ref formatted_url = format!("url({})", *self.url.borrow()); reports.push(Report { path: path![formatted_url, "layout-task", "display-list"], kind: ReportKind::ExplicitJemallocHeapSize, @@ -951,7 +950,7 @@ impl LayoutTask { }; debug!("layout: received layout request for: {}", - self.url.borrow().as_ref().map_or("None".to_owned(), |url| url.serialize())); + self.url.borrow().serialize()); if log_enabled!(log::LogLevel::Debug) { node.dump(); } @@ -1014,10 +1013,9 @@ impl LayoutTask { } // Create a layout context for use throughout the following passes. - let url_clone = &self.url.borrow().as_ref().unwrap().clone(); let mut shared_layout_context = self.build_shared_layout_context(&*rw_data, viewport_size_changed, - url_clone, + &self.url.borrow(), data.reflow_info.goal); if node.is_dirty() || node.has_dirty_descendants() { @@ -1128,7 +1126,6 @@ impl LayoutTask { page_clip_rect: MAX_RECT, }; - let url_clone = &self.url.borrow().as_ref().unwrap().clone(); let mut layout_context = self.build_shared_layout_context(&*rw_data, false, url_clone, @@ -1153,10 +1150,9 @@ impl LayoutTask { page_clip_rect: MAX_RECT, }; - let url_clone = &self.url.borrow().as_ref().unwrap().clone(); let mut layout_context = self.build_shared_layout_context(&*rw_data, false, - url_clone, + &self.url.borrow(), reflow_info.goal); if let Some(mut root_flow) = self.root_flow.clone() { From 2ee446de5d987c911f91dafa499d8a9f7df61995 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Wed, 9 Dec 2015 02:25:13 -0800 Subject: [PATCH 3/3] Fix build errors after rebasing and address review comments --- components/layout/layout_task.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index 453483efe7c..4eb0dd4b7de 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -54,7 +54,7 @@ use script_traits::{ConstellationControlMsg, LayoutControlMsg, OpaqueScriptLayou use sequential; use serde_json; use std::borrow::ToOwned; -use std::cell::{Cell, RefCell}; +use std::cell::RefCell; use std::collections::HashMap; use std::collections::hash_state::DefaultState; use std::mem::transmute; @@ -627,8 +627,7 @@ impl LayoutTask { self.create_layout_task(info) } Msg::SetFinalUrl(final_url) => { - let mut url_ref_cell = self.url.borrow_mut(); - *url_ref_cell = final_url; + *self.url.borrow_mut() = final_url; }, Msg::PrepareToExit(response_chan) => { self.prepare_to_exit(response_chan); @@ -918,7 +917,7 @@ impl LayoutTask { let document = unsafe { ServoLayoutNode::new(&data.document) }; let document = document.as_document().unwrap(); - debug!("layout: received layout request for: {}", self.url.serialize()); + debug!("layout: received layout request for: {}", self.url.borrow().serialize()); let mut rw_data = possibly_locked_rw_data.lock(); @@ -1128,7 +1127,7 @@ impl LayoutTask { let mut layout_context = self.build_shared_layout_context(&*rw_data, false, - url_clone, + &self.url.borrow(), reflow_info.goal); self.perform_post_main_layout_passes(&reflow_info, &mut *rw_data, &mut layout_context); @@ -1183,7 +1182,7 @@ impl LayoutTask { let mut layout_context = self.build_shared_layout_context(&*rw_data, false, - &self.url, + &self.url.borrow(), reflow_info.goal); // No need to do a style recalc here. @@ -1307,7 +1306,7 @@ impl LayoutTask { /// Returns profiling information which is passed to the time profiler. fn profiler_metadata(&self) -> Option { Some(TimerMetadata { - url: self.url.serialize(), + url: self.url.borrow().serialize(), iframe: if self.is_iframe { TimerMetadataFrameType::IFrame } else {