From 2cfe4de09b8efeeb5c60d718db5fb494769c6fdd Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 24 Nov 2015 11:25:49 -0800 Subject: [PATCH 1/4] Constrain the ConcreteLayoutFoo associated types to provide full tri-ality among the types. Otherwise we end up with this problem: http://is.gd/ACBLAU --- components/layout/wrapper.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 828d9bf778d..b26bd79d95d 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -75,8 +75,10 @@ use util::str::{is_whitespace, search_index}; /// only ever see these and must never see instances of `LayoutJS`. pub trait LayoutNode<'ln> : Sized + Copy + Clone { - type ConcreteLayoutElement: LayoutElement<'ln>; - type ConcreteLayoutDocument: LayoutDocument<'ln>; + type ConcreteLayoutElement: LayoutElement<'ln, ConcreteLayoutNode = Self, + ConcreteLayoutDocument = Self::ConcreteLayoutDocument>; + type ConcreteLayoutDocument: LayoutDocument<'ln, ConcreteLayoutNode = Self, + ConcreteLayoutElement = Self::ConcreteLayoutElement>; /// Returns the type ID of this node. fn type_id(&self) -> NodeTypeId; @@ -162,8 +164,10 @@ pub trait LayoutNode<'ln> : Sized + Copy + Clone { } pub trait LayoutDocument<'ld> : Sized + Copy + Clone { - type ConcreteLayoutNode: LayoutNode<'ld>; - type ConcreteLayoutElement: LayoutElement<'ld>; + type ConcreteLayoutNode: LayoutNode<'ld, ConcreteLayoutElement = Self::ConcreteLayoutElement, + ConcreteLayoutDocument = Self>; + type ConcreteLayoutElement: LayoutElement<'ld, ConcreteLayoutNode = Self::ConcreteLayoutNode, + ConcreteLayoutDocument = Self>; fn as_node(&self) -> Self::ConcreteLayoutNode; @@ -173,8 +177,10 @@ pub trait LayoutDocument<'ld> : Sized + Copy + Clone { } pub trait LayoutElement<'le> : Sized + Copy + Clone + ::selectors::Element + TElementAttributes { - type ConcreteLayoutNode: LayoutNode<'le>; - type ConcreteLayoutDocument: LayoutDocument<'le>; + type ConcreteLayoutNode: LayoutNode<'le, ConcreteLayoutElement = Self, + ConcreteLayoutDocument = Self::ConcreteLayoutDocument>; + type ConcreteLayoutDocument: LayoutDocument<'le, ConcreteLayoutNode = Self::ConcreteLayoutNode, + ConcreteLayoutElement = Self>; fn as_node(&self) -> Self::ConcreteLayoutNode; @@ -799,7 +805,7 @@ impl PseudoElementType { /// node does not allow any parents or siblings of nodes to be accessed, to avoid races. pub trait ThreadSafeLayoutNode<'ln> : Clone + Copy + Sized { - type ConcreteThreadSafeLayoutElement: ThreadSafeLayoutElement<'ln>; + type ConcreteThreadSafeLayoutElement: ThreadSafeLayoutElement<'ln, ConcreteThreadSafeLayoutNode = Self>; /// Converts self into an `OpaqueNode`. fn opaque(&self) -> OpaqueNode; @@ -947,8 +953,8 @@ trait DangerousThreadSafeLayoutNode<'ln> : ThreadSafeLayoutNode<'ln> { unsafe fn dangerous_next_sibling(&self) -> Option; } -pub trait ThreadSafeLayoutElement<'le> { - type ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode<'le>; +pub trait ThreadSafeLayoutElement<'le>: Clone + Copy + Sized { + type ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode<'le, ConcreteThreadSafeLayoutElement = Self>; #[inline] fn get_attr(&self, namespace: &Namespace, name: &Atom) -> Option<&'le str>; @@ -1258,6 +1264,7 @@ impl<'ln, ConcreteNode> Iterator for ThreadSafeLayoutNodeChildrenIterator<'ln, C /// A wrapper around elements that ensures layout can only ever access safe properties and cannot /// race on elements. +#[derive(Copy, Clone)] pub struct ServoThreadSafeLayoutElement<'le> { element: &'le Element, } From 3aeaff35deb012860fa7afa5861cc76b6c4098bb Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Tue, 24 Nov 2015 15:02:15 -0800 Subject: [PATCH 2/4] Switch ChildrenIterator to be an associated type. If we use ThreadsafeLayoutNodeChildrenIterator directly as the return type of children(), we need to export the DangerousThreadSafeLayoutNode which the iterator implementation relies upon. --- components/layout/wrapper.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index b26bd79d95d..0a31ccfc429 100644 --- a/components/layout/wrapper.rs +++ b/components/layout/wrapper.rs @@ -806,6 +806,7 @@ impl PseudoElementType { pub trait ThreadSafeLayoutNode<'ln> : Clone + Copy + Sized { type ConcreteThreadSafeLayoutElement: ThreadSafeLayoutElement<'ln, ConcreteThreadSafeLayoutNode = Self>; + type ChildrenIterator: Iterator + Sized; /// Converts self into an `OpaqueNode`. fn opaque(&self) -> OpaqueNode; @@ -819,7 +820,7 @@ pub trait ThreadSafeLayoutNode<'ln> : Clone + Copy + Sized { fn flow_debug_id(self) -> usize; /// Returns an iterator over this node's children. - fn children(&self) -> ThreadSafeLayoutNodeChildrenIterator<'ln, Self>; + fn children(&self) -> Self::ChildrenIterator; /// If this is an element, accesses the element data. Fails if this is not an element node. #[inline] @@ -1022,6 +1023,7 @@ impl<'ln> ServoThreadSafeLayoutNode<'ln> { impl<'ln> ThreadSafeLayoutNode<'ln> for ServoThreadSafeLayoutNode<'ln> { type ConcreteThreadSafeLayoutElement = ServoThreadSafeLayoutElement<'ln>; + type ChildrenIterator = ThreadSafeLayoutNodeChildrenIterator<'ln, Self>; fn opaque(&self) -> OpaqueNode { OpaqueNodeMethods::from_jsmanaged(unsafe { self.get_jsmanaged() }) @@ -1043,7 +1045,7 @@ impl<'ln> ThreadSafeLayoutNode<'ln> for ServoThreadSafeLayoutNode<'ln> { self.node.flow_debug_id() } - fn children(&self) -> ThreadSafeLayoutNodeChildrenIterator<'ln, Self> { + fn children(&self) -> Self::ChildrenIterator { ThreadSafeLayoutNodeChildrenIterator::new(*self) } From 77a80919967446639141321ba83b3b0b6d1d1665 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 19 Nov 2015 11:12:44 -0800 Subject: [PATCH 3/4] Generalize css/matching.rs to operate on generic Layout*. --- components/layout/css/matching.rs | 45 ++++++++++++++++++------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/components/layout/css/matching.rs b/components/layout/css/matching.rs index 83d17463712..cebb9463495 100644 --- a/components/layout/css/matching.rs +++ b/components/layout/css/matching.rs @@ -32,7 +32,7 @@ use util::arc_ptr_eq; use util::cache::{LRUCache, SimpleHashCache}; use util::opts; use util::vec::ForgetfulSink; -use wrapper::{LayoutElement, LayoutNode, ServoLayoutElement, ServoLayoutNode}; +use wrapper::{LayoutElement, LayoutNode}; pub struct ApplicableDeclarations { pub normal: SmallVec<[DeclarationBlock; 16]>, @@ -161,7 +161,7 @@ pub struct StyleSharingCandidateCache { cache: LRUCache, } -fn create_common_style_affecting_attributes_from_element(element: &ServoLayoutElement) +fn create_common_style_affecting_attributes_from_element<'le, E: LayoutElement<'le>>(element: &E) -> CommonStyleAffectingAttributes { let mut flags = CommonStyleAffectingAttributes::empty(); for attribute_info in &common_style_affecting_attributes() { @@ -212,7 +212,7 @@ impl StyleSharingCandidate { /// Attempts to create a style sharing candidate from this node. Returns /// the style sharing candidate or `None` if this node is ineligible for /// style sharing. - fn new(element: &ServoLayoutElement) -> Option { + fn new<'le, E: LayoutElement<'le>>(element: &E) -> Option { let parent_element = match element.parent_element() { None => return None, Some(parent_element) => parent_element, @@ -254,11 +254,11 @@ impl StyleSharingCandidate { link: element.is_link(), namespace: (*element.get_namespace()).clone(), common_style_affecting_attributes: - create_common_style_affecting_attributes_from_element(&element) + create_common_style_affecting_attributes_from_element::<'le, E>(&element) }) } - fn can_share_style_with(&self, element: &ServoLayoutElement) -> bool { + fn can_share_style_with<'a, E: LayoutElement<'a>>(&self, element: &E) -> bool { if *element.get_local_name() != self.local_name { return false } @@ -343,7 +343,7 @@ impl StyleSharingCandidateCache { self.cache.iter() } - pub fn insert_if_possible(&mut self, element: &ServoLayoutElement) { + pub fn insert_if_possible<'le, E: LayoutElement<'le>>(&mut self, element: &E) { match StyleSharingCandidate::new(element) { None => {} Some(candidate) => self.cache.insert(candidate, ()) @@ -364,7 +364,7 @@ pub enum StyleSharingResult { StyleWasShared(usize, RestyleDamage), } -pub trait ElementMatchMethods { +pub trait ElementMatchMethods<'le, ConcreteLayoutElement: LayoutElement<'le>> { fn match_element(&self, stylist: &Stylist, parent_bf: Option<&BloomFilter>, @@ -377,11 +377,11 @@ pub trait ElementMatchMethods { unsafe fn share_style_if_possible(&self, style_sharing_candidate_cache: &mut StyleSharingCandidateCache, - parent: Option) + parent: Option) -> StyleSharingResult; } -pub trait MatchMethods { +pub trait MatchMethods<'ln, ConcreteLayoutNode: LayoutNode<'ln>> { /// Inserts and removes the matching `Descendant` selectors from a bloom /// filter. This is used to speed up CSS selector matching to remove /// unnecessary tree climbs for `Descendant` queries. @@ -397,7 +397,7 @@ pub trait MatchMethods { unsafe fn cascade_node(&self, layout_context: &SharedLayoutContext, - parent: Option, + parent: Option, applicable_declarations: &ApplicableDeclarations, applicable_declarations_cache: &mut ApplicableDeclarationsCache, new_animations_sender: &Mutex>); @@ -421,14 +421,15 @@ trait PrivateMatchMethods { -> bool; } -trait PrivateElementMatchMethods { +trait PrivateElementMatchMethods<'le, ConcreteLayoutElement: LayoutElement<'le>> { fn share_style_with_candidate_if_possible(&self, - parent_node: Option, + parent_node: Option, candidate: &StyleSharingCandidate) -> Option>; } -impl<'ln> PrivateMatchMethods for ServoLayoutNode<'ln> { +impl<'ln, ConcreteLayoutNode> PrivateMatchMethods for ConcreteLayoutNode + where ConcreteLayoutNode: LayoutNode<'ln> { fn cascade_node_pseudo_element(&self, layout_context: &SharedLayoutContext, parent_style: Option<&Arc>, @@ -547,9 +548,11 @@ impl<'ln> PrivateMatchMethods for ServoLayoutNode<'ln> { } } -impl<'ln> PrivateElementMatchMethods for ServoLayoutElement<'ln> { +impl<'le, ConcreteLayoutElement> PrivateElementMatchMethods<'le, ConcreteLayoutElement> + for ConcreteLayoutElement + where ConcreteLayoutElement: LayoutElement<'le> { fn share_style_with_candidate_if_possible(&self, - parent_node: Option, + parent_node: Option, candidate: &StyleSharingCandidate) -> Option> { let parent_node = match parent_node { @@ -582,7 +585,9 @@ impl<'ln> PrivateElementMatchMethods for ServoLayoutElement<'ln> { } } -impl<'ln> ElementMatchMethods for ServoLayoutElement<'ln> { +impl<'le, ConcreteLayoutElement> ElementMatchMethods<'le, ConcreteLayoutElement> + for ConcreteLayoutElement + where ConcreteLayoutElement: LayoutElement<'le> { fn match_element(&self, stylist: &Stylist, parent_bf: Option<&BloomFilter>, @@ -615,7 +620,7 @@ impl<'ln> ElementMatchMethods for ServoLayoutElement<'ln> { unsafe fn share_style_if_possible(&self, style_sharing_candidate_cache: &mut StyleSharingCandidateCache, - parent: Option) + parent: Option) -> StyleSharingResult { if opts::get().disable_share_style_cache { return StyleSharingResult::CannotShare @@ -648,7 +653,9 @@ impl<'ln> ElementMatchMethods for ServoLayoutElement<'ln> { } } -impl<'ln> MatchMethods for ServoLayoutNode<'ln> { +impl<'ln, ConcreteLayoutNode> MatchMethods<'ln, ConcreteLayoutNode> + for ConcreteLayoutNode + where ConcreteLayoutNode: LayoutNode<'ln> { // The below two functions are copy+paste because I can't figure out how to // write a function which takes a generic function. I don't think it can // be done. @@ -690,7 +697,7 @@ impl<'ln> MatchMethods for ServoLayoutNode<'ln> { unsafe fn cascade_node(&self, layout_context: &SharedLayoutContext, - parent: Option, + parent: Option, applicable_declarations: &ApplicableDeclarations, applicable_declarations_cache: &mut ApplicableDeclarationsCache, new_animations_sender: &Mutex>) { From cf33f00018c7dc44a09086d6bb68b253153635ae Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 20 Nov 2015 09:51:05 -0800 Subject: [PATCH 4/4] Generalize the rest of layout to operate on generic Layout*. There wasn't a good way to split this up, unfortunately. With this change, the only remaining usage of the Servo-specific structures is in layout_task, where the root node is received from the script task. \o/ --- components/layout/construct.rs | 87 ++++++++++++++++++-------------- components/layout/flow.rs | 6 +-- components/layout/fragment.rs | 26 +++++----- components/layout/layout_task.rs | 4 +- components/layout/parallel.rs | 65 +++++++++++++++--------- components/layout/query.rs | 41 +++++++-------- components/layout/sequential.rs | 14 +++-- components/layout/table_cell.rs | 8 ++- components/layout/traversal.rs | 60 ++++++++++++---------- components/layout/wrapper.rs | 43 ++++++++++------ 10 files changed, 198 insertions(+), 156 deletions(-) diff --git a/components/layout/construct.rs b/components/layout/construct.rs index 53b2416ee8d..49cc3567d57 100644 --- a/components/layout/construct.rs +++ b/components/layout/construct.rs @@ -38,6 +38,7 @@ use script::dom::bindings::inheritance::{HTMLElementTypeId, NodeTypeId}; use script::dom::htmlobjectelement::is_image_data; use std::borrow::ToOwned; use std::collections::LinkedList; +use std::marker::PhantomData; use std::mem; use std::sync::Arc; use std::sync::atomic::Ordering; @@ -57,7 +58,7 @@ use traversal::PostorderNodeMutTraversal; use url::Url; use util::linked_list; use util::opts; -use wrapper::{PseudoElementType, ServoThreadSafeLayoutNode, TextContent, ThreadSafeLayoutElement, ThreadSafeLayoutNode}; +use wrapper::{PseudoElementType, TextContent, ThreadSafeLayoutElement, ThreadSafeLayoutNode}; /// The results of flow construction for a DOM node. #[derive(Clone)] @@ -208,7 +209,8 @@ impl InlineFragmentsAccumulator { } } - fn from_inline_node(node: &ServoThreadSafeLayoutNode) -> InlineFragmentsAccumulator { + fn from_inline_node<'ln, N>(node: &N) -> InlineFragmentsAccumulator + where N: ThreadSafeLayoutNode<'ln> { InlineFragmentsAccumulator { fragments: IntermediateInlineFragments::new(), enclosing_node: Some(InlineFragmentNodeInfo { @@ -265,29 +267,35 @@ impl InlineFragmentsAccumulator { } /// An object that knows how to create flows. -pub struct FlowConstructor<'a> { +pub struct FlowConstructor<'a, 'ln, N: ThreadSafeLayoutNode<'ln>> { /// The layout context. pub layout_context: &'a LayoutContext<'a>, + /// Satisfy the compiler about the unused parameters, which we use to improve the ergonomics of + /// the ensuing impl {} by removing the need to parameterize all the methods individually. + phantom1: PhantomData<&'ln ()>, + phantom2: PhantomData, } -impl<'a> FlowConstructor<'a> { +impl<'a, 'ln, ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode<'ln>> + FlowConstructor<'a, 'ln, ConcreteThreadSafeLayoutNode> { /// Creates a new flow constructor. - pub fn new<'b>(layout_context: &'b LayoutContext<'b>) - -> FlowConstructor<'b> { + pub fn new(layout_context: &'a LayoutContext<'a>) -> Self { FlowConstructor { layout_context: layout_context, + phantom1: PhantomData, + phantom2: PhantomData, } } #[inline] fn set_flow_construction_result(&self, - node: &ServoThreadSafeLayoutNode, + node: &ConcreteThreadSafeLayoutNode, result: ConstructionResult) { node.set_flow_construction_result(result); } /// Builds the fragment for the given block or subclass thereof. - fn build_fragment_for_block(&mut self, node: &ServoThreadSafeLayoutNode) -> Fragment { + fn build_fragment_for_block(&mut self, node: &ConcreteThreadSafeLayoutNode) -> Fragment { let specific_fragment_info = match node.type_id() { Some(NodeTypeId::Element(ElementTypeId::HTMLElement( HTMLElementTypeId::HTMLIFrameElement))) => { @@ -343,7 +351,7 @@ impl<'a> FlowConstructor<'a> { fn generate_anonymous_table_flows_if_necessary(&mut self, flow: &mut FlowRef, child: &mut FlowRef, - child_node: &ServoThreadSafeLayoutNode) { + child_node: &ConcreteThreadSafeLayoutNode) { if !flow.is_block_flow() { return } @@ -401,7 +409,7 @@ impl<'a> FlowConstructor<'a> { flow: &mut FlowRef, flow_list: &mut Vec, absolute_descendants: &mut AbsoluteDescendants, - node: &ServoThreadSafeLayoutNode) { + node: &ConcreteThreadSafeLayoutNode) { let mut fragments = fragment_accumulator.to_intermediate_inline_fragments(); if fragments.is_empty() { return @@ -486,8 +494,8 @@ impl<'a> FlowConstructor<'a> { &mut self, flow: &mut FlowRef, consecutive_siblings: &mut Vec, - node: &ServoThreadSafeLayoutNode, - kid: ServoThreadSafeLayoutNode, + node: &ConcreteThreadSafeLayoutNode, + kid: ConcreteThreadSafeLayoutNode, inline_fragment_accumulator: &mut InlineFragmentsAccumulator, abs_descendants: &mut AbsoluteDescendants) { match kid.swap_out_construction_result() { @@ -595,7 +603,7 @@ impl<'a> FlowConstructor<'a> { fn build_flow_for_block_starting_with_fragments( &mut self, mut flow: FlowRef, - node: &ServoThreadSafeLayoutNode, + node: &ConcreteThreadSafeLayoutNode, initial_fragments: IntermediateInlineFragments) -> ConstructionResult { // Gather up fragments for the inline flows we might need to create. @@ -662,7 +670,7 @@ impl<'a> FlowConstructor<'a> { /// /// FIXME(pcwalton): It is not clear to me that there isn't a cleaner way to handle /// `