From ebd0a62eec432f58c6744f1d70d566eee80f7010 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 13:07:27 +0200 Subject: [PATCH 01/13] Remove an unused import from layout_task. --- components/layout/layout_task.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index 1cad8855750..f4533dbaad5 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -21,7 +21,7 @@ use layout_debug; use opaque_node::OpaqueNodeMethods; use parallel::{self, UnsafeFlow}; use sequential; -use wrapper::{LayoutNode, TLayoutNode}; +use wrapper::LayoutNode; use azure::azure::AzColor; use canvas_traits::CanvasMsg; From a217ffb00a844c4396f5abb1d3ead2f2bb520a6c Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 11:49:58 +0200 Subject: [PATCH 02/13] Add assertions to LayoutDataRef accessors. --- components/script/dom/node.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index 851140e3929..af957f2beca 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -53,6 +53,7 @@ use script_traits::UntrustedNodeAddress; use util::geometry::Au; use util::namespace; use util::str::DOMString; +use util::task_state; use selectors::parser::{Selector, AttrSelector, NamespaceConstraint}; use selectors::parser::parse_author_origin_selector_list_from_str; use selectors::matching::matches; @@ -222,7 +223,7 @@ pub struct LayoutData { unsafe impl Send for LayoutData {} pub struct LayoutDataRef { - pub data_cell: RefCell>, + data_cell: RefCell>, } no_jsmanaged_fields!(LayoutDataRef); @@ -236,7 +237,8 @@ impl LayoutDataRef { /// Sends layout data, if any, back to the layout task to be destroyed. pub fn dispose(&self) { - if let Some(mut layout_data) = mem::replace(&mut *self.borrow_mut(), None) { + debug_assert!(task_state::get().is_script()); + if let Some(mut layout_data) = mem::replace(&mut *self.data_cell.borrow_mut(), None) { let layout_chan = layout_data.chan.take(); match layout_chan { None => {} @@ -248,18 +250,20 @@ impl LayoutDataRef { } } - /// Borrows the layout data immutably, *asserting that there are no mutators*. Bad things will + /// Borrows the layout data immutably, *assuming that there are no mutators*. Bad things will /// happen if you try to mutate the layout data while this is held. This is the only thread- /// safe layout data accessor. #[inline] #[allow(unsafe_code)] pub unsafe fn borrow_unchecked(&self) -> *const Option { + debug_assert!(task_state::get().is_layout()); self.data_cell.as_unsafe_cell().get() as *const _ } /// Borrows the layout data immutably. This function is *not* thread-safe. #[inline] pub fn borrow<'a>(&'a self) -> Ref<'a,Option> { + debug_assert!(task_state::get().is_layout()); self.data_cell.borrow() } @@ -270,6 +274,7 @@ impl LayoutDataRef { /// on it. This has already resulted in one bug! #[inline] pub fn borrow_mut<'a>(&'a self) -> RefMut<'a,Option> { + debug_assert!(task_state::get().is_layout()); self.data_cell.borrow_mut() } } From eb2c508df0ca8a286db9d30d5fb2ba8e47b41fa2 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 13:06:20 +0200 Subject: [PATCH 03/13] Implement the LayoutData getters on LayoutJS rather than Node itself. --- components/layout/data.rs | 8 +++---- components/layout/wrapper.rs | 12 ++++------ components/script/dom/node.rs | 41 +++++++++++++++++++++-------------- 3 files changed, 33 insertions(+), 28 deletions(-) diff --git a/components/layout/data.rs b/components/layout/data.rs index 9146e0110c4..f679435a91f 100644 --- a/components/layout/data.rs +++ b/components/layout/data.rs @@ -8,7 +8,7 @@ use construct::{ConstructionItem, ConstructionResult}; use incremental::RestyleDamage; use msg::constellation_msg::ConstellationChan; use parallel::DomParallelInfo; -use script::dom::node::SharedLayoutData; +use script::dom::node::{LayoutNodeHelpers, SharedLayoutData}; use script::layout_interface::LayoutChan; use std::cell::{Ref, RefMut}; use std::mem; @@ -116,20 +116,20 @@ pub trait LayoutDataAccess { impl<'ln> LayoutDataAccess for LayoutNode<'ln> { #[inline(always)] unsafe fn borrow_layout_data_unchecked(&self) -> *const Option { - mem::transmute(self.get().layout_data_unchecked()) + mem::transmute(self.get_jsmanaged().layout_data_unchecked()) } #[inline(always)] fn borrow_layout_data<'a>(&'a self) -> Ref<'a,Option> { unsafe { - mem::transmute(self.get().layout_data()) + mem::transmute(self.get_jsmanaged().layout_data()) } } #[inline(always)] fn mutate_layout_data<'a>(&'a self) -> RefMut<'a,Option> { unsafe { - mem::transmute(self.get().layout_data_mut()) + mem::transmute(self.get_jsmanaged().layout_data_mut()) } } } diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index b45162772d3..6db0552d2df 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -889,11 +889,11 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { layout_data_wrapper_ref.data.after_style.is_some() } - /// Borrows the layout data without checking. Fails on a conflicting borrow. + /// Borrows the layout data without checking. #[inline(always)] fn borrow_layout_data_unchecked<'a>(&'a self) -> *const Option { unsafe { - mem::transmute(self.get().layout_data_unchecked()) + self.node.borrow_layout_data_unchecked() } } @@ -902,9 +902,7 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { /// TODO(pcwalton): Make this private. It will let us avoid borrow flag checks in some cases. #[inline(always)] pub fn borrow_layout_data<'a>(&'a self) -> Ref<'a,Option> { - unsafe { - mem::transmute(self.get().layout_data()) - } + self.node.borrow_layout_data() } /// Borrows the layout data mutably. Fails on a conflicting borrow. @@ -912,9 +910,7 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { /// TODO(pcwalton): Make this private. It will let us avoid borrow flag checks in some cases. #[inline(always)] pub fn mutate_layout_data<'a>(&'a self) -> RefMut<'a,Option> { - unsafe { - mem::transmute(self.get().layout_data_mut()) - } + self.node.mutate_layout_data() } /// Traverses the tree in postorder. diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index af957f2beca..919136a4afd 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -1091,6 +1091,13 @@ pub trait LayoutNodeHelpers { unsafe fn get_flag(&self, flag: NodeFlags) -> bool; #[allow(unsafe_code)] unsafe fn set_flag(&self, flag: NodeFlags, value: bool); + + #[allow(unsafe_code)] + unsafe fn layout_data(&self) -> Ref>; + #[allow(unsafe_code)] + unsafe fn layout_data_mut(&self) -> RefMut>; + #[allow(unsafe_code)] + unsafe fn layout_data_unchecked(&self) -> *const Option; } impl LayoutNodeHelpers for LayoutJS { @@ -1162,6 +1169,24 @@ impl LayoutNodeHelpers for LayoutJS { (*this).flags.set(flags); } + + #[inline] + #[allow(unsafe_code)] + unsafe fn layout_data(&self) -> Ref> { + (*self.unsafe_get()).layout_data.borrow() + } + + #[inline] + #[allow(unsafe_code)] + unsafe fn layout_data_mut(&self) -> RefMut> { + (*self.unsafe_get()).layout_data.borrow_mut() + } + + #[inline] + #[allow(unsafe_code)] + unsafe fn layout_data_unchecked(&self) -> *const Option { + (*self.unsafe_get()).layout_data.borrow_unchecked() + } } pub trait RawLayoutNodeHelpers { @@ -1461,22 +1486,6 @@ impl Node { } } - #[inline] - pub fn layout_data(&self) -> Ref> { - self.layout_data.borrow() - } - - #[inline] - pub fn layout_data_mut(&self) -> RefMut> { - self.layout_data.borrow_mut() - } - - #[inline] - #[allow(unsafe_code)] - pub unsafe fn layout_data_unchecked(&self) -> *const Option { - self.layout_data.borrow_unchecked() - } - // https://dom.spec.whatwg.org/#concept-node-adopt pub fn adopt(node: &Node, document: &Document) { // Step 1. From ae5191275aa551e7d72b6dc839699e9cbedfbc0d Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 13:07:09 +0200 Subject: [PATCH 04/13] Remove TLayoutNode::get. It is highly unsafe and unused. --- components/layout/wrapper.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 6db0552d2df..a3bcd78ae82 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -21,8 +21,6 @@ //! //! Rules of the road for this file: //! -//! * You must not use `.get()`; instead, use `.unsafe_get()`. -//! //! * Do not call any methods on DOM nodes without checking to see whether they use borrow flags. //! //! o Instead of `get_attr()`, use `.get_attr_val_for_layout()`. @@ -91,12 +89,6 @@ pub trait TLayoutNode { /// call and as such is marked `unsafe`. unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS; - /// Returns the interior of this node as a `Node`. This is highly unsafe for layout to call - /// and as such is marked `unsafe`. - unsafe fn get<'a>(&'a self) -> &'a Node { - &*self.get_jsmanaged().unsafe_get() - } - fn node_is_element(&self) -> bool { match self.type_id() { Some(NodeTypeId::Element(..)) => true, @@ -740,10 +732,6 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> { self.node.get_jsmanaged() } - unsafe fn get<'a>(&'a self) -> &'a Node { // this change. - &*self.get_jsmanaged().unsafe_get() - } - fn first_child(&self) -> Option> { if self.pseudo != PseudoElementType::Normal { return None From 1398616ec1e184e2d7940ceb42de0b00b13f280e Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 13:07:53 +0200 Subject: [PATCH 05/13] Remove incorrect claim from a comment. --- components/layout/wrapper.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index a3bcd78ae82..5f74c187bec 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -1121,7 +1121,7 @@ pub trait PostorderNodeMutTraversal { } /// Opaque type stored in type-unsafe work queues for parallel layout. -/// Must be transmutable to and from LayoutNode/ThreadSafeLayoutNode. +/// Must be transmutable to and from LayoutNode. pub type UnsafeLayoutNode = (usize, usize); pub fn layout_node_to_unsafe_layout_node(node: &LayoutNode) -> UnsafeLayoutNode { From 54b9dc6563db5692f67946fcf49782b42fe195f9 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 14:54:21 +0200 Subject: [PATCH 06/13] Inline LayoutNode::text_content into its only caller, ThreadSafeLayoutNode::text_content. --- components/layout/wrapper.rs | 88 +++++++++++++++++------------------- 1 file changed, 41 insertions(+), 47 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 5f74c187bec..3fc7a3aa0b3 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -52,7 +52,7 @@ use script::dom::htmlcanvaselement::{HTMLCanvasElement, LayoutHTMLCanvasElementH use script::dom::htmliframeelement::HTMLIFrameElement; use script::dom::htmlimageelement::LayoutHTMLImageElementHelpers; use script::dom::htmlinputelement::{HTMLInputElement, LayoutHTMLInputElementHelpers}; -use script::dom::htmltextareaelement::{HTMLTextAreaElement, LayoutHTMLTextAreaElementHelpers}; +use script::dom::htmltextareaelement::LayoutHTMLTextAreaElementHelpers; use script::dom::node::{Node, NodeTypeId}; use script::dom::node::{LayoutNodeHelpers, RawLayoutNodeHelpers, SharedLayoutData}; use script::dom::node::{HAS_CHANGED, IS_DIRTY, HAS_DIRTY_SIBLINGS, HAS_DIRTY_DESCENDANTS}; @@ -153,12 +153,6 @@ pub trait TLayoutNode { } } - /// If this is a text node or generated content, copies out its content. If this is not a text - /// node, fails. - /// - /// FIXME(pcwalton): This might have too much copying and/or allocation. Profile this. - fn text_content(&self) -> Vec; - /// Returns the first child of this node. fn first_child(&self) -> Option; } @@ -204,30 +198,6 @@ impl<'ln> TLayoutNode for LayoutNode<'ln> { self.get_jsmanaged().first_child_ref().map(|node| self.new_with_this_lifetime(&node)) } } - - fn text_content(&self) -> Vec { - unsafe { - let text: Option> = TextCast::to_layout_js(self.get_jsmanaged()); - if let Some(text) = text { - return vec![ - ContentItem::String( - CharacterDataCast::from_layout_js(&text).data_for_layout().to_owned()) - ]; - } - let input: Option> = - HTMLInputElementCast::to_layout_js(self.get_jsmanaged()); - if let Some(input) = input { - return vec![ContentItem::String(input.get_value_for_layout())]; - } - let area: Option> = - HTMLTextAreaElementCast::to_layout_js(self.get_jsmanaged()); - if let Some(area) = area { - return vec![ContentItem::String(area.get_value_for_layout())]; - } - - panic!("not text!") - } - } } impl<'ln> LayoutNode<'ln> { @@ -757,22 +727,6 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> { self.get_jsmanaged().first_child_ref().map(|node| self.new_with_this_lifetime(&node)) } } - - fn text_content(&self) -> Vec { - if self.pseudo != PseudoElementType::Normal { - let layout_data_ref = self.borrow_layout_data(); - let node_layout_data_wrapper = layout_data_ref.as_ref().unwrap(); - - if self.pseudo.is_before() { - let before_style = node_layout_data_wrapper.data.before_style.as_ref().unwrap(); - return get_content(&before_style.get_box().content) - } else { - let after_style = node_layout_data_wrapper.data.after_style.as_ref().unwrap(); - return get_content(&after_style.get_box().content) - } - } - self.node.text_content() - } } impl<'ln> ThreadSafeLayoutNode<'ln> { @@ -1036,6 +990,46 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { _ => false } } + + /// If this is a text node, generated content, or a form element, copies out + /// its content. Otherwise, panics. + /// + /// FIXME(pcwalton): This might have too much copying and/or allocation. Profile this. + pub fn text_content(&self) -> Vec { + if self.pseudo != PseudoElementType::Normal { + let layout_data_ref = self.borrow_layout_data(); + let node_layout_data_wrapper = layout_data_ref.as_ref().unwrap(); + + if self.pseudo.is_before() { + let before_style = node_layout_data_wrapper.data.before_style.as_ref().unwrap(); + return get_content(&before_style.get_box().content) + } else { + let after_style = node_layout_data_wrapper.data.after_style.as_ref().unwrap(); + return get_content(&after_style.get_box().content) + } + } + + let this = unsafe { self.get_jsmanaged() }; + let text = TextCast::to_layout_js(this); + if let Some(text) = text { + let data = unsafe { + CharacterDataCast::from_layout_js(&text).data_for_layout().to_owned() + }; + return vec![ContentItem::String(data)]; + } + let input = HTMLInputElementCast::to_layout_js(this); + if let Some(input) = input { + let data = unsafe { input.get_value_for_layout() }; + return vec![ContentItem::String(data)]; + } + let area = HTMLTextAreaElementCast::to_layout_js(this); + if let Some(area) = area { + let data = unsafe { area.get_value_for_layout() }; + return vec![ContentItem::String(data)]; + } + + panic!("not text!") + } } pub struct ThreadSafeLayoutNodeChildrenIterator<'a> { From 8f58dafbd6e6d0b714a00bb0e3285b4617ecf111 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 15:20:47 +0200 Subject: [PATCH 07/13] Inline node_is_element and node_is_document into their only callers. --- components/layout/wrapper.rs | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 3fc7a3aa0b3..341816840d8 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -89,20 +89,6 @@ pub trait TLayoutNode { /// call and as such is marked `unsafe`. unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS; - fn node_is_element(&self) -> bool { - match self.type_id() { - Some(NodeTypeId::Element(..)) => true, - _ => false - } - } - - fn node_is_document(&self) -> bool { - match self.type_id() { - Some(NodeTypeId::Document(..)) => true, - _ => false - } - } - /// If this is an image element, returns its URL. If this is not an image element, fails. /// /// FIXME(pcwalton): Don't copy URLs. @@ -358,11 +344,17 @@ impl<'ln> TNode for LayoutNode<'ln> { } fn is_element(&self) -> bool { - self.node_is_element() + match self.type_id() { + Some(NodeTypeId::Element(..)) => true, + _ => false + } } fn is_document(&self) -> bool { - self.node_is_document() + match self.type_id() { + Some(NodeTypeId::Document(..)) => true, + _ => false + } } fn match_attr(&self, attr: &AttrSelector, test: F) -> bool where F: Fn(&str) -> bool { From 13a07a4ed2f6cf755279ca63a7a3b9291e01c1a9 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 15:40:52 +0200 Subject: [PATCH 08/13] Move some TLayoutNode methods to ThreadSafeLayoutNode. They are unused on LayoutNode. --- components/layout/fragment.rs | 2 +- components/layout/wrapper.rs | 96 ++++++++++++++++------------------- 2 files changed, 45 insertions(+), 53 deletions(-) diff --git a/components/layout/fragment.rs b/components/layout/fragment.rs index 36225040c4e..11dd6406b0a 100644 --- a/components/layout/fragment.rs +++ b/components/layout/fragment.rs @@ -19,7 +19,7 @@ use layout_debug; use model::{self, IntrinsicISizes, IntrinsicISizesContribution, MaybeAuto, specified}; use text; use opaque_node::OpaqueNodeMethods; -use wrapper::{TLayoutNode, ThreadSafeLayoutNode}; +use wrapper::ThreadSafeLayoutNode; use euclid::{Point2D, Rect, Size2D}; use gfx::display_list::{BLUR_INFLATION_FACTOR, OpaqueNode}; diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 341816840d8..dd061380241 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -48,8 +48,7 @@ use script::dom::characterdata::{CharacterDataTypeId, LayoutCharacterDataHelpers use script::dom::element::{Element, ElementTypeId}; use script::dom::element::{LayoutElementHelpers, RawLayoutElementHelpers}; use script::dom::htmlelement::HTMLElementTypeId; -use script::dom::htmlcanvaselement::{HTMLCanvasElement, LayoutHTMLCanvasElementHelpers}; -use script::dom::htmliframeelement::HTMLIFrameElement; +use script::dom::htmlcanvaselement::LayoutHTMLCanvasElementHelpers; use script::dom::htmlimageelement::LayoutHTMLImageElementHelpers; use script::dom::htmlinputelement::{HTMLInputElement, LayoutHTMLInputElementHelpers}; use script::dom::htmltextareaelement::LayoutHTMLTextAreaElementHelpers; @@ -89,56 +88,6 @@ pub trait TLayoutNode { /// call and as such is marked `unsafe`. unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS; - /// If this is an image element, returns its URL. If this is not an image element, fails. - /// - /// FIXME(pcwalton): Don't copy URLs. - fn image_url(&self) -> Option { - unsafe { - match HTMLImageElementCast::to_layout_js(self.get_jsmanaged()) { - Some(elem) => elem.image_url().as_ref().map(|url| (*url).clone()), - None => panic!("not an image!") - } - } - } - - fn renderer(&self) -> Option> { - unsafe { - let canvas_element: Option> = - HTMLCanvasElementCast::to_layout_js(self.get_jsmanaged()); - canvas_element.and_then(|elem| elem.get_renderer()) - } - } - - fn canvas_width(&self) -> u32 { - unsafe { - let canvas_element: Option> = - HTMLCanvasElementCast::to_layout_js(self.get_jsmanaged()); - canvas_element.unwrap().get_canvas_width() - } - } - - fn canvas_height(&self) -> u32 { - unsafe { - let canvas_element: Option> = - HTMLCanvasElementCast::to_layout_js(self.get_jsmanaged()); - canvas_element.unwrap().get_canvas_height() - } - } - - /// If this node is an iframe element, returns its pipeline and subpage IDs. If this node is - /// not an iframe element, fails. - fn iframe_pipeline_and_subpage_ids(&self) -> (PipelineId, SubpageId) { - unsafe { - let iframe_element: LayoutJS = - match HTMLIFrameElementCast::to_layout_js(self.get_jsmanaged()) { - Some(elem) => elem, - None => panic!("not an iframe element!") - }; - ((*iframe_element.unsafe_get()).containing_page_pipeline_id().unwrap(), - (*iframe_element.unsafe_get()).subpage_id().unwrap()) - } - } - /// Returns the first child of this node. fn first_child(&self) -> Option; } @@ -1022,6 +971,49 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { panic!("not text!") } + + /// If this is an image element, returns its URL. If this is not an image element, fails. + /// + /// FIXME(pcwalton): Don't copy URLs. + pub fn image_url(&self) -> Option { + unsafe { + HTMLImageElementCast::to_layout_js(self.get_jsmanaged()) + .expect("not an image!") + .image_url() + } + } + + pub fn renderer(&self) -> Option> { + unsafe { + let canvas_element = HTMLCanvasElementCast::to_layout_js(self.get_jsmanaged()); + canvas_element.and_then(|elem| elem.get_renderer()) + } + } + + pub fn canvas_width(&self) -> u32 { + unsafe { + let canvas_element = HTMLCanvasElementCast::to_layout_js(self.get_jsmanaged()); + canvas_element.unwrap().get_canvas_width() + } + } + + pub fn canvas_height(&self) -> u32 { + unsafe { + let canvas_element = HTMLCanvasElementCast::to_layout_js(self.get_jsmanaged()); + canvas_element.unwrap().get_canvas_height() + } + } + + /// If this node is an iframe element, returns its pipeline and subpage IDs. If this node is + /// not an iframe element, fails. + pub fn iframe_pipeline_and_subpage_ids(&self) -> (PipelineId, SubpageId) { + unsafe { + let iframe_element = HTMLIFrameElementCast::to_layout_js(self.get_jsmanaged()) + .expect("not an iframe element!"); + ((*iframe_element.unsafe_get()).containing_page_pipeline_id().unwrap(), + (*iframe_element.unsafe_get()).subpage_id().unwrap()) + } + } } pub struct ThreadSafeLayoutNodeChildrenIterator<'a> { From f0034b4ac9a7d56303647fa9e965fec0b923fb8c Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 15:46:06 +0200 Subject: [PATCH 09/13] Remove some dead code from ThreadSafeLayoutNode::first_child. self.pseudo is always PseudoElementType::Normal at this point. --- components/layout/wrapper.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index dd061380241..5a0b9ccd55b 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -649,19 +649,7 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> { } if self.has_before_pseudo() { - // FIXME(pcwalton): This logic looks weird. Is it right? - match self.pseudo { - PseudoElementType::Normal => { - let pseudo_before_node = self.with_pseudo(PseudoElementType::Before(self.get_before_display())); - return Some(pseudo_before_node) - } - PseudoElementType::Before(display::T::inline) => {} - PseudoElementType::Before(_) => { - let pseudo_before_node = self.with_pseudo(PseudoElementType::Before(display::T::inline)); - return Some(pseudo_before_node) - } - _ => {} - } + return Some(self.with_pseudo(PseudoElementType::Before(self.get_before_display()))); } unsafe { From dc167ca3435143665b147a48d1cd7172e8144ac5 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 16:36:03 +0200 Subject: [PATCH 10/13] Implement type_id as inherent methods. This implies LayoutNode no longer needs to return an Option, as it never represents a pseudo-element. Also, fixes lies in the documentation. --- components/layout/construct.rs | 2 +- components/layout/css/matching.rs | 4 ++-- components/layout/wrapper.rs | 40 +++++++++++++++---------------- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/components/layout/construct.rs b/components/layout/construct.rs index d3d21007242..27181f34e52 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -41,7 +41,7 @@ use table_row::TableRowFlow; use table_rowgroup::TableRowGroupFlow; use table_wrapper::TableWrapperFlow; use text::TextRunScanner; -use wrapper::{PostorderNodeMutTraversal, PseudoElementType, TLayoutNode, ThreadSafeLayoutNode}; +use wrapper::{PostorderNodeMutTraversal, PseudoElementType, ThreadSafeLayoutNode}; use gfx::display_list::OpaqueNode; use script::dom::characterdata::CharacterDataTypeId; diff --git a/components/layout/css/matching.rs b/components/layout/css/matching.rs index 8256d2b72f7..24356af6be8 100644 --- a/components/layout/css/matching.rs +++ b/components/layout/css/matching.rs @@ -13,7 +13,7 @@ use data::{LayoutDataAccess, LayoutDataWrapper}; use incremental::{self, RestyleDamage}; use opaque_node::OpaqueNodeMethods; use smallvec::SmallVec16; -use wrapper::{LayoutElement, LayoutNode, TLayoutNode}; +use wrapper::{LayoutElement, LayoutNode}; use script::dom::characterdata::CharacterDataTypeId; use script::dom::node::NodeTypeId; @@ -677,7 +677,7 @@ impl<'ln> MatchMethods for LayoutNode<'ln> { &mut None => panic!("no layout data"), &mut Some(ref mut layout_data) => { match self.type_id() { - Some(NodeTypeId::CharacterData(CharacterDataTypeId::Text)) => { + NodeTypeId::CharacterData(CharacterDataTypeId::Text) => { // Text nodes get a copy of the parent style. This ensures // that during fragment construction any non-inherited // CSS properties (such as vertical-align) are correctly diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 5a0b9ccd55b..be19fbc5b26 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -80,10 +80,6 @@ pub trait TLayoutNode { /// Creates a new layout node with the same lifetime as this layout node. unsafe fn new_with_this_lifetime(&self, node: &LayoutJS) -> Self; - /// Returns the type ID of this node. Fails if this node is borrowed mutably. Returns `None` - /// if this is a pseudo-element; otherwise, returns `Some`. - fn type_id(&self) -> Option; - /// Returns the interior of this node as a `LayoutJS`. This is highly unsafe for layout to /// call and as such is marked `unsafe`. unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS; @@ -118,12 +114,6 @@ impl<'ln> TLayoutNode for LayoutNode<'ln> { } } - fn type_id(&self) -> Option { - unsafe { - Some(self.node.type_id_for_layout()) - } - } - unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { &self.node } @@ -136,6 +126,13 @@ impl<'ln> TLayoutNode for LayoutNode<'ln> { } impl<'ln> LayoutNode<'ln> { + /// Returns the type ID of this node. + pub fn type_id(&self) -> NodeTypeId { + unsafe { + self.node.type_id_for_layout() + } + } + pub fn dump(self) { self.dump_indent(0); } @@ -294,14 +291,14 @@ impl<'ln> TNode for LayoutNode<'ln> { fn is_element(&self) -> bool { match self.type_id() { - Some(NodeTypeId::Element(..)) => true, + NodeTypeId::Element(..) => true, _ => false } } fn is_document(&self) -> bool { match self.type_id() { - Some(NodeTypeId::Document(..)) => true, + NodeTypeId::Document(..) => true, _ => false } } @@ -630,15 +627,6 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> { } } - /// Returns `None` if this is a pseudo-element. - fn type_id(&self) -> Option { - if self.pseudo != PseudoElementType::Normal { - return None - } - - self.node.type_id() - } - unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { self.node.get_jsmanaged() } @@ -676,6 +664,16 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { } } + /// Returns the type ID of this node. + /// Returns `None` if this is a pseudo-element; otherwise, returns `Some`. + pub fn type_id(&self) -> Option { + if self.pseudo != PseudoElementType::Normal { + return None + } + + Some(self.node.type_id()) + } + pub fn debug_id(self) -> usize { self.node.debug_id() } From 50d4084e9ab26cbfe587bc84cb03bf205e2291de Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 17:16:18 +0200 Subject: [PATCH 11/13] Remove TLayoutNode::first_child. It is replaced by the TNode implementation for LayoutNode and an inherent implementation for ThreadSafeLayoutNode. --- components/layout/wrapper.rs | 47 ++++++++++++------------------------ 1 file changed, 16 insertions(+), 31 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index be19fbc5b26..895c80ba678 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -83,9 +83,6 @@ pub trait TLayoutNode { /// Returns the interior of this node as a `LayoutJS`. This is highly unsafe for layout to /// call and as such is marked `unsafe`. unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS; - - /// Returns the first child of this node. - fn first_child(&self) -> Option; } /// A wrapper so that layout can access only the methods that it should have access to. Layout must @@ -117,12 +114,6 @@ impl<'ln> TLayoutNode for LayoutNode<'ln> { unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { &self.node } - - fn first_child(&self) -> Option> { - unsafe { - self.get_jsmanaged().first_child_ref().map(|node| self.new_with_this_lifetime(&node)) - } - } } impl<'ln> LayoutNode<'ln> { @@ -176,14 +167,8 @@ impl<'ln> LayoutNode<'ln> { /// Returns an iterator over this node's children. pub fn children(self) -> LayoutNodeChildrenIterator<'ln> { - // FIXME(zwarich): Remove this when UFCS lands and there is a better way - // of disambiguating methods. - fn first_child(this: T) -> Option { - this.first_child() - } - LayoutNodeChildrenIterator { - current: first_child(self), + current: self.first_child(), } } @@ -216,7 +201,7 @@ impl<'ln> LayoutNode<'ln> { } pub fn has_children(self) -> bool { - TLayoutNode::first_child(&self).is_some() + self.first_child().is_some() } /// While doing a reflow, the node at the root has no parent, as far as we're @@ -630,20 +615,6 @@ impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> { unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { self.node.get_jsmanaged() } - - fn first_child(&self) -> Option> { - if self.pseudo != PseudoElementType::Normal { - return None - } - - if self.has_before_pseudo() { - return Some(self.with_pseudo(PseudoElementType::Before(self.get_before_display()))); - } - - unsafe { - self.get_jsmanaged().first_child_ref().map(|node| self.new_with_this_lifetime(&node)) - } - } } impl<'ln> ThreadSafeLayoutNode<'ln> { @@ -682,6 +653,20 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { self.node.flow_debug_id() } + fn first_child(&self) -> Option> { + if self.pseudo != PseudoElementType::Normal { + return None + } + + if self.has_before_pseudo() { + return Some(self.with_pseudo(PseudoElementType::Before(self.get_before_display()))); + } + + unsafe { + self.get_jsmanaged().first_child_ref().map(|node| self.new_with_this_lifetime(&node)) + } + } + /// Returns the next sibling of this node. Unsafe and private because this can lead to races. unsafe fn next_sibling(&self) -> Option> { if self.pseudo.is_before() { From 167a396293ef161223f47fe14563048e1e81f5be Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Sun, 21 Jun 2015 17:29:08 +0200 Subject: [PATCH 12/13] Replace TLayoutNode by inherent methods. There is no reason for this trait to exist. --- components/layout/data.rs | 2 +- components/layout/opaque_node.rs | 2 +- components/layout/wrapper.rs | 39 +++++++++++--------------------- 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/components/layout/data.rs b/components/layout/data.rs index f679435a91f..6bee6916039 100644 --- a/components/layout/data.rs +++ b/components/layout/data.rs @@ -14,7 +14,7 @@ use std::cell::{Ref, RefMut}; use std::mem; use std::sync::Arc; use style::properties::ComputedValues; -use wrapper::{LayoutNode, TLayoutNode}; +use wrapper::LayoutNode; /// Data that layout associates with a node. pub struct PrivateLayoutData { diff --git a/components/layout/opaque_node.rs b/components/layout/opaque_node.rs index 666d3a9eb11..ea22df50668 100644 --- a/components/layout/opaque_node.rs +++ b/components/layout/opaque_node.rs @@ -10,7 +10,7 @@ use script::dom::bindings::js::LayoutJS; use script::dom::node::Node; use script::layout_interface::{TrustedNodeAddress}; use script_traits::UntrustedNodeAddress; -use wrapper::{LayoutNode, TLayoutNode, ThreadSafeLayoutNode}; +use wrapper::{LayoutNode, ThreadSafeLayoutNode}; pub trait OpaqueNodeMethods { /// Converts a DOM node (layout view) to an `OpaqueNode`. diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 895c80ba678..774873a2964 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -75,16 +75,6 @@ use style::node::{TElement, TElementAttributes, TNode}; use style::properties::{PropertyDeclaration, PropertyDeclarationBlock}; use url::Url; -/// Allows some convenience methods on generic layout nodes. -pub trait TLayoutNode { - /// Creates a new layout node with the same lifetime as this layout node. - unsafe fn new_with_this_lifetime(&self, node: &LayoutJS) -> Self; - - /// Returns the interior of this node as a `LayoutJS`. This is highly unsafe for layout to - /// call and as such is marked `unsafe`. - unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS; -} - /// A wrapper so that layout can access only the methods that it should have access to. Layout must /// only ever see these and must never see instances of `LayoutJS`. #[derive(Copy, Clone)] @@ -103,20 +93,15 @@ impl<'a> PartialEq for LayoutNode<'a> { } } -impl<'ln> TLayoutNode for LayoutNode<'ln> { - unsafe fn new_with_this_lifetime(&self, node: &LayoutJS) -> LayoutNode<'ln> { +impl<'ln> LayoutNode<'ln> { + /// Creates a new layout node with the same lifetime as this layout node. + pub unsafe fn new_with_this_lifetime(&self, node: &LayoutJS) -> LayoutNode<'ln> { LayoutNode { node: *node, chain: self.chain, } } - unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { - &self.node - } -} - -impl<'ln> LayoutNode<'ln> { /// Returns the type ID of this node. pub fn type_id(&self) -> NodeTypeId { unsafe { @@ -179,6 +164,8 @@ impl<'ln> LayoutNode<'ln> { } + /// Returns the interior of this node as a `LayoutJS`. This is highly unsafe for layout to + /// call and as such is marked `unsafe`. pub unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { &self.node } @@ -603,21 +590,15 @@ pub struct ThreadSafeLayoutNode<'ln> { pseudo: PseudoElementType, } -impl<'ln> TLayoutNode for ThreadSafeLayoutNode<'ln> { +impl<'ln> ThreadSafeLayoutNode<'ln> { /// Creates a new layout node with the same lifetime as this layout node. - unsafe fn new_with_this_lifetime(&self, node: &LayoutJS) -> ThreadSafeLayoutNode<'ln> { + pub unsafe fn new_with_this_lifetime(&self, node: &LayoutJS) -> ThreadSafeLayoutNode<'ln> { ThreadSafeLayoutNode { node: self.node.new_with_this_lifetime(node), pseudo: PseudoElementType::Normal, } } - unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { - self.node.get_jsmanaged() - } -} - -impl<'ln> ThreadSafeLayoutNode<'ln> { /// Creates a new `ThreadSafeLayoutNode` from the given `LayoutNode`. pub fn new<'a>(node: &LayoutNode<'a>) -> ThreadSafeLayoutNode<'a> { ThreadSafeLayoutNode { @@ -635,6 +616,12 @@ impl<'ln> ThreadSafeLayoutNode<'ln> { } } + /// Returns the interior of this node as a `LayoutJS`. This is highly unsafe for layout to + /// call and as such is marked `unsafe`. + pub unsafe fn get_jsmanaged<'a>(&'a self) -> &'a LayoutJS { + self.node.get_jsmanaged() + } + /// Returns the type ID of this node. /// Returns `None` if this is a pseudo-element; otherwise, returns `Some`. pub fn type_id(&self) -> Option { From a42e11a95f7cdc0c45147f7861c7185b690ebbc2 Mon Sep 17 00:00:00 2001 From: Ms2ger Date: Mon, 22 Jun 2015 11:34:31 +0200 Subject: [PATCH 13/13] Replace the LayoutDataAccess trait by inherent methods. --- components/layout/construct.rs | 2 +- components/layout/css/matching.rs | 2 +- components/layout/data.rs | 40 ++----------------------------- components/layout/layout_task.rs | 2 +- components/layout/parallel.rs | 2 +- components/layout/wrapper.rs | 24 ++++++++++++++++++- 6 files changed, 29 insertions(+), 43 deletions(-) diff --git a/components/layout/construct.rs b/components/layout/construct.rs index 27181f34e52..2742c18d271 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -16,7 +16,7 @@ use block::BlockFlow; use context::LayoutContext; use css::node_style::StyledNode; -use data::{HAS_NEWLY_CONSTRUCTED_FLOW, LayoutDataAccess, LayoutDataWrapper}; +use data::{HAS_NEWLY_CONSTRUCTED_FLOW, LayoutDataWrapper}; use floats::FloatKind; use flow::{Descendants, AbsDescendants}; use flow::{Flow, ImmutableFlowUtils, MutableFlowUtils, MutableOwnedFlowUtils}; diff --git a/components/layout/css/matching.rs b/components/layout/css/matching.rs index 24356af6be8..29edbd9e9aa 100644 --- a/components/layout/css/matching.rs +++ b/components/layout/css/matching.rs @@ -9,7 +9,7 @@ use animation; use context::SharedLayoutContext; use css::node_style::StyledNode; -use data::{LayoutDataAccess, LayoutDataWrapper}; +use data::LayoutDataWrapper; use incremental::{self, RestyleDamage}; use opaque_node::OpaqueNodeMethods; use smallvec::SmallVec16; diff --git a/components/layout/data.rs b/components/layout/data.rs index 6bee6916039..e26e0a28431 100644 --- a/components/layout/data.rs +++ b/components/layout/data.rs @@ -2,19 +2,14 @@ * 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/. */ -#![allow(unsafe_code)] - use construct::{ConstructionItem, ConstructionResult}; use incremental::RestyleDamage; use msg::constellation_msg::ConstellationChan; use parallel::DomParallelInfo; -use script::dom::node::{LayoutNodeHelpers, SharedLayoutData}; +use script::dom::node::SharedLayoutData; use script::layout_interface::LayoutChan; -use std::cell::{Ref, RefMut}; -use std::mem; use std::sync::Arc; use style::properties::ComputedValues; -use wrapper::LayoutNode; /// Data that layout associates with a node. pub struct PrivateLayoutData { @@ -95,41 +90,10 @@ impl LayoutDataWrapper { } } -#[allow(dead_code)] +#[allow(dead_code, unsafe_code)] fn static_assertion(x: Option) { unsafe { let _: Option<::script::dom::node::LayoutData> = ::std::intrinsics::transmute(x); } } - -/// A trait that allows access to the layout data of a DOM node. -pub trait LayoutDataAccess { - /// Borrows the layout data without checks. - unsafe fn borrow_layout_data_unchecked(&self) -> *const Option; - /// Borrows the layout data immutably. Fails on a conflicting borrow. - fn borrow_layout_data<'a>(&'a self) -> Ref<'a,Option>; - /// Borrows the layout data mutably. Fails on a conflicting borrow. - fn mutate_layout_data<'a>(&'a self) -> RefMut<'a,Option>; -} - -impl<'ln> LayoutDataAccess for LayoutNode<'ln> { - #[inline(always)] - unsafe fn borrow_layout_data_unchecked(&self) -> *const Option { - mem::transmute(self.get_jsmanaged().layout_data_unchecked()) - } - - #[inline(always)] - fn borrow_layout_data<'a>(&'a self) -> Ref<'a,Option> { - unsafe { - mem::transmute(self.get_jsmanaged().layout_data()) - } - } - - #[inline(always)] - fn mutate_layout_data<'a>(&'a self) -> RefMut<'a,Option> { - unsafe { - mem::transmute(self.get_jsmanaged().layout_data_mut()) - } - } -} diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index f4533dbaad5..178d08319a8 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -11,7 +11,7 @@ use animation; use construct::ConstructionResult; use context::{SharedLayoutContext, SharedLayoutContextWrapper, heap_size_of_local_context}; use css::node_style::StyledNode; -use data::{LayoutDataAccess, LayoutDataWrapper}; +use data::LayoutDataWrapper; use display_list_builder::ToGfxColor; use flow::{self, Flow, ImmutableFlowUtils, MutableFlowUtils, MutableOwnedFlowUtils}; use flow_ref::FlowRef; diff --git a/components/layout/parallel.rs b/components/layout/parallel.rs index 879c2a1eea2..9cc60f31aee 100644 --- a/components/layout/parallel.rs +++ b/components/layout/parallel.rs @@ -12,7 +12,7 @@ use context::{LayoutContext, SharedLayoutContextWrapper, SharedLayoutContext}; use flow::{Flow, MutableFlowUtils, PreorderFlowTraversal, PostorderFlowTraversal}; use flow; use flow_ref::FlowRef; -use data::{LayoutDataAccess, LayoutDataWrapper}; +use data::LayoutDataWrapper; use traversal::{BubbleISizes, AssignISizes, AssignBSizesAndStoreOverflow}; use traversal::{ComputeAbsolutePositions, BuildDisplayList}; use traversal::{RecalcStyleForNode, ConstructFlows}; diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 774873a2964..c4414a55dd6 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -34,7 +34,7 @@ use canvas_traits::CanvasMsg; use context::SharedLayoutContext; use css::node_style::StyledNode; use incremental::RestyleDamage; -use data::{LayoutDataAccess, LayoutDataFlags, LayoutDataWrapper, PrivateLayoutData}; +use data::{LayoutDataFlags, LayoutDataWrapper, PrivateLayoutData}; use opaque_node::OpaqueNodeMethods; use gfx::display_list::OpaqueNode; @@ -332,6 +332,28 @@ impl<'ln> LayoutNode<'ln> { pub unsafe fn set_dirty_descendants(&self, value: bool) { self.node.set_flag(HAS_DIRTY_DESCENDANTS, value) } + + /// Borrows the layout data without checks. + #[inline(always)] + pub unsafe fn borrow_layout_data_unchecked(&self) -> *const Option { + mem::transmute(self.get_jsmanaged().layout_data_unchecked()) + } + + /// Borrows the layout data immutably. Fails on a conflicting borrow. + #[inline(always)] + pub fn borrow_layout_data<'a>(&'a self) -> Ref<'a,Option> { + unsafe { + mem::transmute(self.get_jsmanaged().layout_data()) + } + } + + /// Borrows the layout data mutably. Fails on a conflicting borrow. + #[inline(always)] + pub fn mutate_layout_data<'a>(&'a self) -> RefMut<'a,Option> { + unsafe { + mem::transmute(self.get_jsmanaged().layout_data_mut()) + } + } } pub struct LayoutNodeChildrenIterator<'a> {