From 516e8e0aa600cdef34e506e0ed7180d12dd9ac7d Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Fri, 3 Apr 2020 14:34:37 +0200 Subject: [PATCH] Don't expose any AtomicRefCell directly from style traits This lets us experiment with how we store this data on the DOM side. --- components/layout/query.rs | 2 +- components/layout_thread/dom_wrapper.rs | 27 ++++++++++++------- components/layout_thread/lib.rs | 4 +-- components/layout_thread_2020/dom_wrapper.rs | 27 ++++++++++++------- components/layout_thread_2020/lib.rs | 4 +-- components/style/dom.rs | 14 ++++------ components/style/gecko/wrapper.rs | 27 ++++++++++++++----- .../element/state_and_attributes.rs | 2 +- components/style/traversal.rs | 2 +- 9 files changed, 67 insertions(+), 42 deletions(-) diff --git a/components/layout/query.rs b/components/layout/query.rs index d7c4d7e8987..7472a8a706d 100644 --- a/components/layout/query.rs +++ b/components/layout/query.rs @@ -761,7 +761,7 @@ pub fn process_resolved_style_request<'dom>( // We call process_resolved_style_request after performing a whole-document // traversal, so in the common case, the element is styled. - if element.get_data().is_some() { + if element.has_data() { return process_resolved_style_request_internal(node, pseudo, property, layout_root); } diff --git a/components/layout_thread/dom_wrapper.rs b/components/layout_thread/dom_wrapper.rs index 4614f30cb93..381a0cfa77d 100644 --- a/components/layout_thread/dom_wrapper.rs +++ b/components/layout_thread/dom_wrapper.rs @@ -30,7 +30,7 @@ #![allow(unsafe_code)] -use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut}; +use atomic_refcell::{AtomicRef, AtomicRefMut}; use gfx_traits::ByteIndex; use html5ever::{LocalName, Namespace}; use layout::data::StyleAndLayoutData; @@ -551,8 +551,20 @@ impl<'le> TElement for ServoLayoutElement<'le> { self.mutate_data().unwrap() } - fn get_data(&self) -> Option<&AtomicRefCell> { - self.get_style_data().map(|data| &data.element_data) + /// Whether there is an ElementData container. + fn has_data(&self) -> bool { + self.get_style_data().is_some() + } + + /// Immutably borrows the ElementData. + fn borrow_data(&self) -> Option> { + self.get_style_data().map(|data| data.element_data.borrow()) + } + + /// Mutably borrows the ElementData. + fn mutate_data(&self) -> Option> { + self.get_style_data() + .map(|data| data.element_data.borrow_mut()) } fn skip_item_display_fixup(&self) -> bool { @@ -688,7 +700,7 @@ impl<'le> ServoLayoutElement<'le> { } fn get_style_data(&self) -> Option<&StyleData> { - self.get_style_and_layout_data().map(|opaque| { + self.get_style_and_layout_data().map(|opaque| unsafe { &opaque .downcast_ref::() .unwrap() @@ -1027,7 +1039,7 @@ impl<'ln> ThreadSafeLayoutNode<'ln> for ServoThreadSafeLayoutNode<'ln> { fn parent_style(&self) -> Arc { let parent = self.node.parent_node().unwrap().as_element().unwrap(); - let parent_data = parent.get_data().unwrap().borrow(); + let parent_data = parent.borrow_data().unwrap(); parent_data.styles.primary().clone() } @@ -1312,10 +1324,7 @@ impl<'le> ThreadSafeLayoutElement<'le> for ServoThreadSafeLayoutElement<'le> { } fn style_data(&self) -> AtomicRef { - self.element - .get_data() - .expect("Unstyled layout node?") - .borrow() + self.element.borrow_data().expect("Unstyled layout node?") } fn is_shadow_host(&self) -> bool { diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 54f1ec8d656..2e50a2f7764 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -1472,7 +1472,7 @@ impl LayoutThread { // If we haven't styled this node yet, we don't need to track a // restyle. - let style_data = match el.get_data() { + let mut style_data = match el.mutate_data() { Some(d) => d, None => { unsafe { el.unset_snapshot_flags() }; @@ -1485,8 +1485,6 @@ impl LayoutThread { map.insert(el.as_node().opaque(), s); } - let mut style_data = style_data.borrow_mut(); - // Stash the data on the element for processing by the style system. style_data.hint.insert(restyle.hint.into()); style_data.damage = restyle.damage; diff --git a/components/layout_thread_2020/dom_wrapper.rs b/components/layout_thread_2020/dom_wrapper.rs index 6fd9451e8b1..bbca53acc62 100644 --- a/components/layout_thread_2020/dom_wrapper.rs +++ b/components/layout_thread_2020/dom_wrapper.rs @@ -30,7 +30,7 @@ #![allow(unsafe_code)] -use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut}; +use atomic_refcell::{AtomicRef, AtomicRefMut}; use gfx_traits::ByteIndex; use html5ever::{LocalName, Namespace}; use layout::data::StyleAndLayoutData; @@ -558,8 +558,20 @@ impl<'le> TElement for ServoLayoutElement<'le> { self.mutate_data().unwrap() } - fn get_data(&self) -> Option<&AtomicRefCell> { - self.get_style_data().map(|data| &data.element_data) + /// Whether there is an ElementData container. + fn has_data(&self) -> bool { + self.get_style_data().is_some() + } + + /// Immutably borrows the ElementData. + fn borrow_data(&self) -> Option> { + self.get_style_data().map(|data| data.element_data.borrow()) + } + + /// Mutably borrows the ElementData. + fn mutate_data(&self) -> Option> { + self.get_style_data() + .map(|data| data.element_data.borrow_mut()) } fn skip_item_display_fixup(&self) -> bool { @@ -695,7 +707,7 @@ impl<'le> ServoLayoutElement<'le> { } fn get_style_data(&self) -> Option<&StyleData> { - self.get_style_and_layout_data().map(|opaque| { + self.get_style_and_layout_data().map(|opaque| unsafe { &opaque .downcast_ref::() .unwrap() @@ -1034,7 +1046,7 @@ impl<'ln> ThreadSafeLayoutNode<'ln> for ServoThreadSafeLayoutNode<'ln> { fn parent_style(&self) -> Arc { let parent = self.node.parent_node().unwrap().as_element().unwrap(); - let parent_data = parent.get_data().unwrap().borrow(); + let parent_data = parent.borrow_data().unwrap(); parent_data.styles.primary().clone() } @@ -1319,10 +1331,7 @@ impl<'le> ThreadSafeLayoutElement<'le> for ServoThreadSafeLayoutElement<'le> { } fn style_data(&self) -> AtomicRef { - self.element - .get_data() - .expect("Unstyled layout node?") - .borrow() + self.element.borrow_data().expect("Unstyled layout node?") } fn is_shadow_host(&self) -> bool { diff --git a/components/layout_thread_2020/lib.rs b/components/layout_thread_2020/lib.rs index 05663f7d68b..3e2b73875fe 100644 --- a/components/layout_thread_2020/lib.rs +++ b/components/layout_thread_2020/lib.rs @@ -1118,7 +1118,7 @@ impl LayoutThread { // If we haven't styled this node yet, we don't need to track a // restyle. - let style_data = match el.get_data() { + let mut style_data = match el.mutate_data() { Some(d) => d, None => { unsafe { el.unset_snapshot_flags() }; @@ -1131,8 +1131,6 @@ impl LayoutThread { map.insert(el.as_node().opaque(), s); } - let mut style_data = style_data.borrow_mut(); - // Stash the data on the element for processing by the style system. style_data.hint.insert(restyle.hint.into()); style_data.damage = restyle.damage; diff --git a/components/style/dom.rs b/components/style/dom.rs index 7629bb11e18..d07cac998ae 100644 --- a/components/style/dom.rs +++ b/components/style/dom.rs @@ -22,7 +22,7 @@ use crate::shared_lock::Locked; use crate::stylist::CascadeData; use crate::traversal_flags::TraversalFlags; use crate::{Atom, LocalName, Namespace, WeakAtom}; -use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut}; +use atomic_refcell::{AtomicRef, AtomicRefMut}; use selectors::matching::{ElementSelectorFlags, QuirksMode, VisitedHandlingMode}; use selectors::sink::Push; use selectors::Element as SelectorsElement; @@ -689,18 +689,14 @@ pub trait TElement: /// Unsafe following the same reasoning as ensure_data. unsafe fn clear_data(&self); - /// Gets a reference to the ElementData container. - fn get_data(&self) -> Option<&AtomicRefCell>; + /// Whether there is an ElementData container. + fn has_data(&self) -> bool; /// Immutably borrows the ElementData. - fn borrow_data(&self) -> Option> { - self.get_data().map(|x| x.borrow()) - } + fn borrow_data(&self) -> Option>; /// Mutably borrows the ElementData. - fn mutate_data(&self) -> Option> { - self.get_data().map(|x| x.borrow_mut()) - } + fn mutate_data(&self) -> Option>; /// Whether we should skip any root- or item-based display property /// blockification on this element. (This function exists so that Gecko diff --git a/components/style/gecko/wrapper.rs b/components/style/gecko/wrapper.rs index ba1c724d58f..2b842e72d9b 100644 --- a/components/style/gecko/wrapper.rs +++ b/components/style/gecko/wrapper.rs @@ -557,6 +557,11 @@ impl<'le> fmt::Debug for GeckoElement<'le> { } impl<'le> GeckoElement<'le> { + #[inline(always)] + fn get_data(&self) -> Option<&AtomicRefCell> { + unsafe { self.0.mServoData.get().as_ref() } + } + #[inline(always)] fn attrs(&self) -> &[structs::AttrArray_InternalAttr] { unsafe { @@ -1299,7 +1304,7 @@ impl<'le> TElement for GeckoElement<'le> { } unsafe fn set_handled_snapshot(&self) { - debug_assert!(self.get_data().is_some()); + debug_assert!(self.has_data()); self.set_flags(ELEMENT_HANDLED_SNAPSHOT as u32) } @@ -1309,7 +1314,7 @@ impl<'le> TElement for GeckoElement<'le> { } unsafe fn set_dirty_descendants(&self) { - debug_assert!(self.get_data().is_some()); + debug_assert!(self.has_data()); debug!("Setting dirty descendants: {:?}", self); self.set_flags(ELEMENT_HAS_DIRTY_DESCENDANTS_FOR_SERVO as u32) } @@ -1378,13 +1383,23 @@ impl<'le> TElement for GeckoElement<'le> { panic!("Atomic child count not implemented in Gecko"); } - #[inline(always)] - fn get_data(&self) -> Option<&AtomicRefCell> { - unsafe { self.0.mServoData.get().as_ref() } + /// Whether there is an ElementData container. + fn has_data(&self) -> bool { + self.get_data().is_some() + } + + /// Immutably borrows the ElementData. + fn borrow_data(&self) -> Option> { + self.get_data().map(|x| x.borrow()) + } + + /// Mutably borrows the ElementData. + fn mutate_data(&self) -> Option> { + self.get_data().map(|x| x.borrow_mut()) } unsafe fn ensure_data(&self) -> AtomicRefMut { - if self.get_data().is_none() { + if !self.has_data() { debug!("Creating ElementData for {:?}", self); let ptr = Box::into_raw(Box::new(AtomicRefCell::new(ElementData::default()))); self.0.mServoData.set(ptr); diff --git a/components/style/invalidation/element/state_and_attributes.rs b/components/style/invalidation/element/state_and_attributes.rs index 628f174a3d7..e906f8d8405 100644 --- a/components/style/invalidation/element/state_and_attributes.rs +++ b/components/style/invalidation/element/state_and_attributes.rs @@ -90,7 +90,7 @@ pub fn invalidated_descendants(element: E, child: E) where E: TElement, { - if child.get_data().is_none() { + if !child.has_data() { return; } diff --git a/components/style/traversal.rs b/components/style/traversal.rs index 254d40f0393..a6aa220ccfa 100644 --- a/components/style/traversal.rs +++ b/components/style/traversal.rs @@ -845,7 +845,7 @@ where // // By consequence, any element without data has no descendants with // data. - if kid.get_data().is_some() { + if kid.has_data() { kid.clear_data(); parents.push(kid); }