From c22547a4efb69a26f80ac13e800c154508f3cc8e Mon Sep 17 00:00:00 2001 From: Keegan McAllister Date: Mon, 26 Aug 2013 17:31:16 -0700 Subject: [PATCH] Embed layout data directly in Node This eliminates layout_data: Option<@mut ()> and the unsafe casting around it, which was causing crashes on exit. Fixes #762. --- src/components/main/css/node_util.rs | 28 ++++--- src/components/main/layout/aux.rs | 88 ++++----------------- src/components/main/layout/incremental.rs | 11 +++ src/components/main/layout/layout_task.rs | 77 +++++++++--------- src/components/script/dom/node.rs | 96 ++++++++++++++++------- 5 files changed, 152 insertions(+), 148 deletions(-) diff --git a/src/components/main/css/node_util.rs b/src/components/main/css/node_util.rs index 721b8ca9132..4cf8eda956f 100644 --- a/src/components/main/css/node_util.rs +++ b/src/components/main/css/node_util.rs @@ -2,10 +2,10 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use layout::aux::LayoutAuxMethods; use layout::incremental::RestyleDamage; -use std::cast::transmute; +use std::cast; +use std::cell::Cell; use newcss::complete::CompleteSelectResults; use script::dom::node::{AbstractNode, LayoutView}; @@ -31,15 +31,20 @@ impl<'self> NodeUtil<'self> for AbstractNode { fail!(~"style() called on a node without aux data!"); } - match self.layout_data().style { - None => fail!(~"style() called on node without a style!"), - Some(ref style) => unsafe { transmute(style) } + do self.read_layout_data |layout_data| { + match layout_data.style { + None => fail!(~"style() called on node without a style!"), + Some(ref style) => unsafe { cast::transmute_region(style) } + } } } /// Does this node have a computed style yet? fn have_css_select_results(self) -> bool { - self.has_layout_data() && self.layout_data().style.is_some() + if !self.has_layout_data() { + return false; + } + self.read_layout_data(|data| data.style.is_some()) } /// Update the computed style of an HTML element with a style specified by CSS. @@ -48,7 +53,8 @@ impl<'self> NodeUtil<'self> for AbstractNode { fail!(~"set_css_select_results() called on a node without aux data!"); } - self.layout_data().style = Some(decl); + let cell = Cell::new(decl); + self.write_layout_data(|data| data.style = Some(cell.take())); } /// Get the description of how to account for recent style changes. @@ -65,7 +71,11 @@ impl<'self> NodeUtil<'self> for AbstractNode { if !self.has_layout_data() { return default; } - self.layout_data().restyle_damage.unwrap_or_default(default) + do self.read_layout_data |layout_data| { + layout_data.restyle_damage + .map(|&x| RestyleDamage::from_int(x)) + .unwrap_or_default(default) + } } /// Set the restyle damage field. @@ -74,6 +84,6 @@ impl<'self> NodeUtil<'self> for AbstractNode { fail!(~"set_restyle_damage() called on a node without aux data!"); } - self.layout_data().restyle_damage = Some(damage); + self.write_layout_data(|data| data.restyle_damage = Some(damage.to_int())); } } diff --git a/src/components/main/layout/aux.rs b/src/components/main/layout/aux.rs index f891ce03389..6325c6de230 100644 --- a/src/components/main/layout/aux.rs +++ b/src/components/main/layout/aux.rs @@ -4,94 +4,34 @@ //! Code for managing the layout data in the DOM. -use layout::incremental::RestyleDamage; -use gfx::display_list::DisplayList; -use servo_util::range::Range; - -use extra::arc::Arc; -use newcss::complete::CompleteSelectResults; -use script::dom::node::{AbstractNode, LayoutView}; +use script::dom::node::{AbstractNode, LayoutView, LayoutData}; use servo_util::tree::TreeNodeRef; -pub struct DisplayBoxes { - display_list: Option>>>, - range: Option, -} - -/// Data that layout associates with a node. -pub struct LayoutData { - /// The results of CSS styling for this node. - style: Option, - - /// Description of how to account for recent style changes. - restyle_damage: Option, - - /// The boxes assosiated with this flow. - /// Used for getBoundingClientRect and friends. - boxes: DisplayBoxes, -} - -impl LayoutData { - /// Creates new layout data. - pub fn new() -> LayoutData { - LayoutData { - style: None, - restyle_damage: None, - boxes: DisplayBoxes { display_list: None, range: None }, - } - } -} - /// Functionality useful for querying the layout-specific data on DOM nodes. pub trait LayoutAuxMethods { - fn layout_data(self) -> @mut LayoutData; - fn has_layout_data(self) -> bool; - fn set_layout_data(self, data: @mut LayoutData); - - fn initialize_layout_data(self) -> Option<@mut LayoutData>; - fn initialize_style_for_subtree(self, refs: &mut ~[@mut LayoutData]); + fn initialize_layout_data(self); + fn initialize_style_for_subtree(self); } impl LayoutAuxMethods for AbstractNode { - fn layout_data(self) -> @mut LayoutData { - unsafe { - self.unsafe_layout_data() - } - } - fn has_layout_data(self) -> bool { - unsafe { - self.unsafe_has_layout_data() - } - } - fn set_layout_data(self, data: @mut LayoutData) { - unsafe { - self.unsafe_set_layout_data(data) - } - } - /// If none exists, creates empty layout data for the node (the reader-auxiliary /// box in the COW model) and populates it with an empty style object. - fn initialize_layout_data(self) -> Option<@mut LayoutData> { + fn initialize_layout_data(self) { if self.has_layout_data() { - self.layout_data().boxes.display_list = None; - self.layout_data().boxes.range = None; - None + do self.write_layout_data |data| { + data.boxes.display_list = None; + data.boxes.range = None; + } } else { - let data = @mut LayoutData::new(); - self.set_layout_data(data); - Some(data) + self.set_layout_data(LayoutData::new()); } } /// Initializes layout data and styles for a Node tree, if any nodes do not have - /// this data already. Append created layout data to the task's GC roots. - fn initialize_style_for_subtree(self, refs: &mut ~[@mut LayoutData]) { - let _ = for n in self.traverse_preorder() { - match n.initialize_layout_data() { - Some(r) => refs.push(r), - None => {} - } - }; + /// this data already. + fn initialize_style_for_subtree(self) { + for n in self.traverse_preorder() { + n.initialize_layout_data(); + } } } - diff --git a/src/components/main/layout/incremental.rs b/src/components/main/layout/incremental.rs index da26ddcfc2e..6beea28cf0c 100644 --- a/src/components/main/layout/incremental.rs +++ b/src/components/main/layout/incremental.rs @@ -54,6 +54,17 @@ impl RestyleDamage { RestyleDamage::all() } + /// Create a RestyleDamage from the underlying bit field. + /// We would rather not allow this, but some types in script + /// need to store RestyleDamage without depending on this crate. + pub fn from_int(n: int) -> RestyleDamage { + RestyleDamage { bits: n } + } + + pub fn to_int(self) -> int { + self.bits + } + pub fn is_empty(self) -> bool { self.bits == 0 } diff --git a/src/components/main/layout/layout_task.rs b/src/components/main/layout/layout_task.rs index 141df0e3f33..1ad44c4e01c 100644 --- a/src/components/main/layout/layout_task.rs +++ b/src/components/main/layout/layout_task.rs @@ -7,7 +7,7 @@ use css::matching::MatchMethods; use css::select::new_css_select_ctx; -use layout::aux::{LayoutData, LayoutAuxMethods}; +use layout::aux::LayoutAuxMethods; use layout::box_builder::LayoutTreeBuilder; use layout::context::LayoutContext; use layout::display_list_builder::{DisplayListBuilder}; @@ -59,9 +59,6 @@ struct LayoutTask { doc_url: Option, screen_size: Option>, - /// This is used to root reader data. - layout_refs: ~[@mut LayoutData], - display_list: Option>>>, css_select_ctx: @mut SelectCtx, @@ -123,7 +120,6 @@ impl LayoutTask { display_list: None, - layout_refs: ~[], css_select_ctx: @mut new_css_select_ctx(), profiler_chan: profiler_chan, } @@ -210,7 +206,7 @@ impl LayoutTask { // // FIXME: This is inefficient. We don't need an entire traversal to do this! do profile(time::LayoutAuxInitCategory, self.profiler_chan.clone()) { - node.initialize_style_for_subtree(&mut self.layout_refs); + node.initialize_style_for_subtree(); } // Perform CSS selector matching if necessary. @@ -328,13 +324,13 @@ impl LayoutTask { }; assert!(node.has_layout_data(), "Node has display item but no layout data"); - let layout_data = node.layout_data(); - layout_data.boxes.display_list = Some(display_list.clone()); + do node.write_layout_data |layout_data| { + layout_data.boxes.display_list = Some(display_list.clone()); - if layout_data.boxes.range.is_none() { - debug!("Creating initial range for node"); - layout_data.boxes.range = Some(Range::new(i,1)); - } else { + if layout_data.boxes.range.is_none() { + debug!("Creating initial range for node"); + layout_data.boxes.range = Some(Range::new(i,1)); + } else { debug!("Appending item to range"); unsafe { let old_node: AbstractNode<()> = transmute(node); @@ -343,6 +339,7 @@ impl LayoutTask { } layout_data.boxes.range.unwrap().extend_by(1); + } } } @@ -376,28 +373,30 @@ impl LayoutTask { transmute(node) }; - let response = match (node.layout_data().boxes.display_list.clone(), node.layout_data().boxes.range) { - (Some(display_list), Some(range)) => { - let mut rect: Option> = None; - for i in range.eachi() { - rect = match rect { - Some(acc) => Some(acc.union(&display_list.get().list[i].bounds())), - None => Some(display_list.get().list[i].bounds()) + let response = do node.read_layout_data |layout_data| { + match (layout_data.boxes.display_list.clone(), layout_data.boxes.range) { + (Some(display_list), Some(range)) => { + let mut rect: Option> = None; + for i in range.eachi() { + rect = match rect { + Some(acc) => Some(acc.union(&display_list.get().list[i].bounds())), + None => Some(display_list.get().list[i].bounds()) + } + } + + match rect { + None => { + error!("no boxes for node"); + Err(()) + } + Some(rect) => Ok(ContentBoxResponse(rect)) } } - - match rect { - None => { - error!("no boxes for node"); - Err(()) - } - Some(rect) => Ok(ContentBoxResponse(rect)) + _ => { + error!("no display list present"); + Err(()) } } - _ => { - error!("no display list present"); - Err(()) - } }; reply_chan.send(response) @@ -408,16 +407,18 @@ impl LayoutTask { transmute(node) }; - let response = match (node.layout_data().boxes.display_list.clone(), node.layout_data().boxes.range) { - (Some(display_list), Some(range)) => { - let mut boxes = ~[]; - for i in range.eachi() { - boxes.push(display_list.get().list[i].bounds()); - } + let response = do node.read_layout_data |layout_data| { + match (layout_data.boxes.display_list.clone(), layout_data.boxes.range) { + (Some(display_list), Some(range)) => { + let mut boxes = ~[]; + for i in range.eachi() { + boxes.push(display_list.get().list[i].bounds()); + } - Ok(ContentBoxesResponse(boxes)) + Ok(ContentBoxesResponse(boxes)) + } + _ => Err(()), } - _ => Err(()), }; reply_chan.send(response) diff --git a/src/components/script/dom/node.rs b/src/components/script/dom/node.rs index 0248f86de44..ad7b6135f83 100644 --- a/src/components/script/dom/node.rs +++ b/src/components/script/dom/node.rs @@ -16,12 +16,17 @@ use dom::htmliframeelement::HTMLIFrameElement; use dom::text::Text; use std::cast; +use std::cell::Cell; use std::cast::transmute; use std::libc::c_void; +use extra::arc::Arc; use js::jsapi::{JSObject, JSContext}; use js::rust::Compartment; use netsurfcss::util::VoidPtrLike; +use newcss::complete::CompleteSelectResults; use servo_util::tree::{TreeNode, TreeNodeRef}; +use servo_util::range::Range; +use gfx::display_list::DisplayList; // // The basic Node structure @@ -56,7 +61,7 @@ pub struct AbstractNodeChildrenIterator { /// /// `View` describes extra data associated with this node that this task has access to. For /// the script task, this is the unit type `()`. For the layout task, this is -/// `layout::aux::LayoutData`. +/// `LayoutData`. pub struct Node { /// The JavaScript wrapper for this node. wrapper: WrapperCache, @@ -85,7 +90,7 @@ pub struct Node { owner_doc: Option, /// Layout information. Only the layout task may touch this data. - priv layout_data: Option<@mut ()> + priv layout_data: Option, } /// The different types of nodes. @@ -172,31 +177,6 @@ impl<'self, View> AbstractNode { } } - /// Returns the layout data, unsafely cast to whatever type layout wishes. Only layout is - /// allowed to call this. This is wildly unsafe and is therefore marked as such. - pub unsafe fn unsafe_layout_data(self) -> @mut T { - do self.with_base |base| { - transmute(base.layout_data.unwrap()) - } - } - /// Returns true if this node has layout data and false otherwise. - pub unsafe fn unsafe_has_layout_data(self) -> bool { - do self.with_base |base| { - base.layout_data.is_some() - } - } - /// Sets the layout data, unsafely casting the type as layout wishes. Only layout is allowed - /// to call this. This is wildly unsafe and is therefore marked as such. - pub unsafe fn unsafe_set_layout_data(self, data: @mut T) { - // Don't decrement the refcount on data, since we're giving it to the - // base structure. - cast::forget(data); - - do self.with_mut_base |base| { - base.layout_data = Some(transmute(data)) - } - } - // Convenience accessors /// Returns the type ID of this node. Fails if this node is borrowed mutably. @@ -631,3 +611,65 @@ impl BindingObject for Node { } } +// This stuff is notionally private to layout, but we put it here because it needs +// to be stored in a Node, and we can't have cross-crate cyclic dependencies. + +pub struct DisplayBoxes { + display_list: Option>>>, + range: Option, +} + +/// Data that layout associates with a node. +pub struct LayoutData { + /// The results of CSS styling for this node. + style: Option, + + /// Description of how to account for recent style changes. + restyle_damage: Option, + + /// The boxes assosiated with this flow. + /// Used for getBoundingClientRect and friends. + boxes: DisplayBoxes, +} + +impl LayoutData { + /// Creates new layout data. + pub fn new() -> LayoutData { + LayoutData { + style: None, + restyle_damage: None, + boxes: DisplayBoxes { display_list: None, range: None }, + } + } +} + +impl AbstractNode { + pub fn set_layout_data(self, data: LayoutData) { + let cell = Cell::new(data); + do self.with_mut_base |b| { + b.layout_data = Some(cell.take()); + } + } + + pub fn has_layout_data(self) -> bool { + do self.with_base |b| { + b.layout_data.is_some() + } + } + + // These accessors take a continuation rather than returning a reference, because + // an AbstractNode doesn't have a lifetime parameter relating to the underlying + // Node. Also this makes it easier to switch to RWArc if we decide that is + // necessary. + pub fn read_layout_data(self, blk: &fn(data: &LayoutData) -> R) -> R { + do self.with_base |b| { + blk(b.layout_data.get_ref()) + } + } + + pub fn write_layout_data(self, blk: &fn(data: &mut LayoutData) -> R) -> R { + do self.with_mut_base |b| { + blk(b.layout_data.get_mut_ref()) + } + } +}