Use Arc<PropertyDeclarationBlock> everwhere it’s appropriate.

13 files changed, 85 insertions, 68 deletions

diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs
index ae62ebb483e..e36652c446a 100644
--- a/components/script/dom/cssstyledeclaration.rs
+++ b/components/script/dom/cssstyledeclaration.rs
@@ -15,6 +15,7 @@ use dom::node::{Node, NodeDamage, window_from_node};
 use dom::window::Window;
 use std::ascii::AsciiExt;
 use std::slice;
+use std::sync::Arc;
 use string_cache::Atom;
 use style::parser::ParserContextExtraData;
 use style::properties::{PropertyDeclaration, Shorthand, Importance};
@@ -367,7 +368,7 @@ impl CSSStyleDeclarationMethods for CSSStyleDeclaration {
         *element.style_attribute().borrow_mut() = if decl_block.declarations.is_empty() {
             None // Step 2
         } else {
-            Some(decl_block)
+            Some(Arc::new(decl_block))
         };
         element.sync_property_with_attrs_style();
         let node = element.upcast::<Node>();
diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs
index 01fb45e9eae..a38367f23cc 100644
--- a/components/script/dom/element.rs
+++ b/components/script/dom/element.rs
@@ -109,7 +109,7 @@ pub struct Element {
     prefix: Option<DOMString>,
     attrs: DOMRefCell<Vec<JS<Attr>>>,
     id_attribute: DOMRefCell<Option<Atom>>,
-    style_attribute: DOMRefCell<Option<PropertyDeclarationBlock>>,
+    style_attribute: DOMRefCell<Option<Arc<PropertyDeclarationBlock>>>,
     attr_list: MutNullableHeap<JS<NamedNodeMap>>,
     class_list: MutNullableHeap<JS<DOMTokenList>>,
     state: Cell<ElementState>,
@@ -297,7 +297,7 @@ pub trait LayoutElementHelpers {
     #[allow(unsafe_code)]
     unsafe fn html_element_in_html_document_for_layout(&self) -> bool;
     fn id_attribute(&self) -> *const Option<Atom>;
-    fn style_attribute(&self) -> *const Option<PropertyDeclarationBlock>;
+    fn style_attribute(&self) -> *const Option<Arc<PropertyDeclarationBlock>>;
     fn local_name(&self) -> &Atom;
     fn namespace(&self) -> &Namespace;
     fn get_checked_state_for_layout(&self) -> bool;
@@ -329,7 +329,10 @@ impl LayoutElementHelpers for LayoutJS<Element> {
         #[inline]
         fn from_declaration(rule: PropertyDeclaration) -> DeclarationBlock {
             DeclarationBlock::from_declarations(
-                Arc::new(vec![(rule, Importance::Normal)]),
+                Arc::new(PropertyDeclarationBlock {
+                    declarations: Arc::new(vec![(rule, Importance::Normal)]),
+                    important_count: 0,
+                }),
                 Importance::Normal)
         }
 
@@ -615,7 +618,7 @@ impl LayoutElementHelpers for LayoutJS<Element> {
     }
 
     #[allow(unsafe_code)]
-    fn style_attribute(&self) -> *const Option<PropertyDeclarationBlock> {
+    fn style_attribute(&self) -> *const Option<Arc<PropertyDeclarationBlock>> {
         unsafe {
             (*self.unsafe_get()).style_attribute.borrow_for_layout()
         }
@@ -704,7 +707,7 @@ impl Element {
         self.attrs.borrow()
     }
 
-    pub fn style_attribute(&self) -> &DOMRefCell<Option<PropertyDeclarationBlock>> {
+    pub fn style_attribute(&self) -> &DOMRefCell<Option<Arc<PropertyDeclarationBlock>>> {
         &self.style_attribute
     }
 
@@ -774,6 +777,7 @@ impl Element {
                     matching
                 });
                 if let Some(index) = index {
+                    let declarations = Arc::make_mut(declarations);
                     Arc::make_mut(&mut declarations.declarations).remove(index);
                     if importance.unwrap().important() {
                         declarations.important_count -= 1;
@@ -796,6 +800,7 @@ impl Element {
                 {
                     // Usually, the reference count will be 1 here. But transitions could make it greater
                     // than that.
+                    let declaration_block = Arc::make_mut(declaration_block);
                     let existing_declarations = Arc::make_mut(&mut declaration_block.declarations);
 
                     'outer: for incoming_declaration in declarations {
@@ -829,10 +834,10 @@ impl Element {
                 0
             };
 
-            *inline_declarations = Some(PropertyDeclarationBlock {
+            *inline_declarations = Some(Arc::new(PropertyDeclarationBlock {
                 declarations: Arc::new(declarations.into_iter().map(|d| (d, importance)).collect()),
                 important_count: important_count,
-            });
+            }));
         }
 
         update(self, declarations, importance);
@@ -847,6 +852,7 @@ impl Element {
             if let &mut Some(ref mut block) = &mut *inline_declarations {
                 // Usually, the reference counts of `from` and `to` will be 1 here. But transitions
                 // could make them greater than that.
+                let block = Arc::make_mut(block);
                 let declarations = Arc::make_mut(&mut block.declarations);
                 for &mut (ref declaration, ref mut importance) in declarations {
                     if properties.iter().any(|p| declaration.name() == **p) {
@@ -2102,8 +2108,11 @@ impl VirtualMethods for Element {
                 *self.style_attribute.borrow_mut() =
                     mutation.new_value(attr).map(|value| {
                         let win = window_from_node(self);
-                        parse_style_attribute(&value, &doc.base_url(), win.css_error_reporter(),
-                                              ParserContextExtraData::default())
+                        Arc::new(parse_style_attribute(
+                            &value,
+                            &doc.base_url(),
+                            win.css_error_reporter(),
+                            ParserContextExtraData::default()))
                     });
                 if node.is_in_doc() {
                     node.dirty(NodeDamage::NodeStyleDamaged);
diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs
index 906bde60550..2b6c8080f06 100644
--- a/components/script/layout_wrapper.rs
+++ b/components/script/layout_wrapper.rs
@@ -451,9 +451,9 @@ impl<'le> TElement for ServoLayoutElement<'le> {
         ServoLayoutNode::from_layout_js(self.element.upcast())
     }
 
-    fn style_attribute(&self) -> &Option<PropertyDeclarationBlock> {
+    fn style_attribute(&self) -> Option<&Arc<PropertyDeclarationBlock>> {
         unsafe {
-            &*self.element.style_attribute()
+            (*self.element.style_attribute()).as_ref()
         }
     }
 
diff --git a/components/style/dom.rs b/components/style/dom.rs
index ce7ba70a1bf..509fa0dd5bc 100644
--- a/components/style/dom.rs
+++ b/components/style/dom.rs
@@ -192,7 +192,7 @@ pub trait TElement : PartialEq + Debug + Sized + Copy + Clone + ElementExt + Pre
 
     fn as_node(&self) -> Self::ConcreteNode;
 
-    fn style_attribute(&self) -> &Option<PropertyDeclarationBlock>;
+    fn style_attribute(&self) -> Option<&Arc<PropertyDeclarationBlock>>;
 
     fn get_state(&self) -> ElementState;
 
diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs
index 16620eccc3b..266dd04a591 100644
--- a/components/style/keyframes.rs
+++ b/components/style/keyframes.rs
@@ -7,7 +7,7 @@ use cssparser::{DeclarationListParser, DeclarationParser};
 use parser::{ParserContext, log_css_error};
 use properties::PropertyDeclarationParseResult;
 use properties::animated_properties::TransitionProperty;
-use properties::{PropertyDeclaration, Importance};
+use properties::{PropertyDeclaration, PropertyDeclarationBlock, Importance};
 use std::sync::Arc;
 
 /// A number from 1 to 100, indicating the percentage of the animation where
@@ -77,7 +77,7 @@ pub struct Keyframe {
     /// so the second value of these tuples is always `Importance::Normal`.
     /// But including them enables `compute_style_for_animation_step` to create a `DeclarationBlock`
     /// by cloning an `Arc<_>` (incrementing a reference count) rather than re-creating a `Vec<_>`.
-    pub declarations: Arc<Vec<(PropertyDeclaration, Importance)>>,
+    pub block: Arc<PropertyDeclarationBlock>,
 }
 
 /// A keyframes step value. This can be a synthetised keyframes animation, that
@@ -88,7 +88,7 @@ pub struct Keyframe {
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub enum KeyframesStepValue {
     /// See `Keyframe::declarations`’s docs about the presence of `Importance`.
-    Declarations(Arc<Vec<(PropertyDeclaration, Importance)>>),
+    Declarations(Arc<PropertyDeclarationBlock>),
     ComputedValues,
 }
 
@@ -113,8 +113,8 @@ impl KeyframesStep {
     fn new(percentage: KeyframePercentage,
            value: KeyframesStepValue) -> Self {
         let declared_timing_function = match value {
-            KeyframesStepValue::Declarations(ref declarations) => {
-                declarations.iter().any(|&(ref prop_decl, _)| {
+            KeyframesStepValue::Declarations(ref block) => {
+                block.declarations.iter().any(|&(ref prop_decl, _)| {
                     match *prop_decl {
                         PropertyDeclaration::AnimationTimingFunction(..) => true,
                         _ => false,
@@ -154,7 +154,7 @@ fn get_animated_properties(keyframe: &Keyframe) -> Vec<TransitionProperty> {
     let mut ret = vec![];
     // NB: declarations are already deduplicated, so we don't have to check for
     // it here.
-    for &(ref declaration, _) in keyframe.declarations.iter() {
+    for &(ref declaration, _) in keyframe.block.declarations.iter() {
         if let Some(property) = TransitionProperty::from_declaration(declaration) {
             ret.push(property);
         }
@@ -179,7 +179,7 @@ impl KeyframesAnimation {
         for keyframe in keyframes {
             for percentage in keyframe.selector.0.iter() {
                 steps.push(KeyframesStep::new(*percentage,
-                                              KeyframesStepValue::Declarations(keyframe.declarations.clone())));
+                                              KeyframesStepValue::Declarations(keyframe.block.clone())));
             }
         }
 
@@ -265,7 +265,10 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
         }
         Ok(Arc::new(Keyframe {
             selector: prelude,
-            declarations: Arc::new(declarations),
+            block: Arc::new(PropertyDeclarationBlock {
+                declarations: Arc::new(declarations),
+                important_count: 0,
+            }),
         }))
     }
 }
diff --git a/components/style/matching.rs b/components/style/matching.rs
index ca040c9670b..749187fa58d 100644
--- a/components/style/matching.rs
+++ b/components/style/matching.rs
@@ -14,7 +14,7 @@ use context::{StyleContext, SharedStyleContext};
 use data::PrivateStyleData;
 use dom::{TElement, TNode, TRestyleDamage, UnsafeNode};
 use properties::longhands::display::computed_value as display;
-use properties::{ComputedValues, cascade};
+use properties::{ComputedValues, cascade, PropertyDeclarationBlock};
 use selector_impl::{TheSelectorImpl, PseudoElement};
 use selector_matching::{DeclarationBlock, Stylist};
 use selectors::bloom::BloomFilter;
@@ -139,7 +139,7 @@ impl<'a> Hash for ApplicableDeclarationsCacheQuery<'a> {
         for declaration in self.declarations {
             // Each declaration contians an Arc, which is a stable
             // pointer; we use that for hashing and equality.
-            let ptr: *const Vec<_> = &*declaration.mixed_declarations;
+            let ptr: *const PropertyDeclarationBlock = &*declaration.mixed_declarations;
             ptr.hash(state);
             declaration.importance.hash(state);
         }
@@ -651,7 +651,7 @@ pub trait ElementMatchMethods : TElement {
                      applicable_declarations: &mut ApplicableDeclarations)
                      -> StyleRelations {
         use traversal::relations_are_shareable;
-        let style_attribute = self.style_attribute().as_ref();
+        let style_attribute = self.style_attribute();
 
         let mut relations =
             stylist.push_applicable_declarations(self,
diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs
index 1c3c87501e3..2b70561b23b 100644
--- a/components/style/properties/properties.mako.rs
+++ b/components/style/properties/properties.mako.rs
@@ -282,7 +282,7 @@ impl Importance {
 
 /// Overridden declarations are skipped.
 // FIXME (https://github.com/servo/servo/issues/3426)
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Clone)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct PropertyDeclarationBlock {
     #[cfg_attr(feature = "servo", ignore_heap_size_of = "#7038")]
diff --git a/components/style/selector_matching.rs b/components/style/selector_matching.rs
index 66f533d174b..de588fbccfc 100644
--- a/components/style/selector_matching.rs
+++ b/components/style/selector_matching.rs
@@ -181,7 +181,7 @@ impl Stylist {
                             selector: selector.complex_selector.clone(),
                             declarations: DeclarationBlock {
                                 specificity: selector.specificity,
-                                mixed_declarations: $style_rule.declarations.declarations.clone(),
+                                mixed_declarations: $style_rule.declarations.clone(),
                                 importance: $importance,
                                 source_order: rules_source_order,
                             },
@@ -346,7 +346,7 @@ impl Stylist {
                                         &self,
                                         element: &E,
                                         parent_bf: Option<&BloomFilter>,
-                                        style_attribute: Option<&PropertyDeclarationBlock>,
+                                        style_attribute: Option<&Arc<PropertyDeclarationBlock>>,
                                         pseudo_element: Option<&PseudoElement>,
                                         applicable_declarations: &mut V) -> StyleRelations
         where E: Element<Impl=TheSelectorImpl> +
@@ -398,14 +398,12 @@ impl Stylist {
         debug!("author normal: {:?}", relations);
 
         // Step 4: Normal style attributes.
-        if let Some(ref sa)  = style_attribute {
+        if let Some(sa)  = style_attribute {
             if sa.declarations.len() as u32 - sa.important_count > 0 {
                 relations |= AFFECTED_BY_STYLE_ATTRIBUTE;
                 Push::push(
                     applicable_declarations,
-                    DeclarationBlock::from_declarations(
-                        sa.declarations.clone(),
-                        Importance::Normal));
+                    DeclarationBlock::from_declarations(sa.clone(), Importance::Normal));
             }
         }
 
@@ -420,14 +418,12 @@ impl Stylist {
         debug!("author important: {:?}", relations);
 
         // Step 6: `!important` style attributes.
-        if let Some(ref sa) = style_attribute {
+        if let Some(sa) = style_attribute {
             if sa.important_count > 0 {
                 relations |= AFFECTED_BY_STYLE_ATTRIBUTE;
                 Push::push(
                     applicable_declarations,
-                    DeclarationBlock::from_declarations(
-                        sa.declarations.clone(),
-                        Importance::Important));
+                    DeclarationBlock::from_declarations(sa.clone(), Importance::Important));
             }
         }
 
@@ -845,7 +841,7 @@ pub struct Rule {
 pub struct DeclarationBlock {
     /// Contains declarations of either importance, but only those of self.importance are relevant.
     /// Use DeclarationBlock::iter
-    pub mixed_declarations: Arc<Vec<(PropertyDeclaration, Importance)>>,
+    pub mixed_declarations: Arc<PropertyDeclarationBlock>,
     pub importance: Importance,
     pub source_order: usize,
     pub specificity: u32,
@@ -853,7 +849,7 @@ pub struct DeclarationBlock {
 
 impl DeclarationBlock {
     #[inline]
-    pub fn from_declarations(declarations: Arc<Vec<(PropertyDeclaration, Importance)>>,
+    pub fn from_declarations(declarations: Arc<PropertyDeclarationBlock>,
                              importance: Importance)
                              -> Self {
         DeclarationBlock {
@@ -866,7 +862,7 @@ impl DeclarationBlock {
 
     pub fn iter(&self) -> DeclarationBlockIter {
         DeclarationBlockIter {
-            iter: self.mixed_declarations.iter(),
+            iter: self.mixed_declarations.declarations.iter(),
             importance: self.importance,
         }
     }
diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs
index ae682ac72e0..0874b0a44b9 100644
--- a/ports/geckolib/glue.rs
+++ b/ports/geckolib/glue.rs
@@ -356,7 +356,7 @@ pub extern "C" fn Servo_StyleSet_Drop(data: *mut RawServoStyleSet) -> () {
 }
 
 pub struct GeckoDeclarationBlock {
-    pub declarations: Option<PropertyDeclarationBlock>,
+    pub declarations: Option<Arc<PropertyDeclarationBlock>>,
     // XXX The following two fields are made atomic to work around the
     // ownership system so that they can be changed inside a shared
     // instance. It wouldn't provide safety as Rust usually promises,
@@ -377,7 +377,7 @@ pub extern "C" fn Servo_ParseStyleAttribute(bytes: *const u8, length: u32,
                                             -> ServoDeclarationBlockStrong {
     let value = unsafe { from_utf8_unchecked(slice::from_raw_parts(bytes, length as usize)) };
     GeckoDeclarationBlock::from_arc(Arc::new(GeckoDeclarationBlock {
-        declarations: GeckoElement::parse_style_attribute(value),
+        declarations: GeckoElement::parse_style_attribute(value).map(Arc::new),
         cache: AtomicPtr::new(cache),
         immutable: AtomicBool::new(false),
     }))
diff --git a/ports/geckolib/wrapper.rs b/ports/geckolib/wrapper.rs
index c168b1dcaaa..0c209c8b926 100644
--- a/ports/geckolib/wrapper.rs
+++ b/ports/geckolib/wrapper.rs
@@ -35,7 +35,6 @@ use snapshot::GeckoElementSnapshot;
 use snapshot_helpers;
 use std::fmt;
 use std::marker::PhantomData;
-use std::mem::transmute;
 use std::ops::BitOr;
 use std::ptr;
 use std::sync::Arc;
@@ -460,8 +459,6 @@ lazy_static! {
     };
 }
 
-static NO_STYLE_ATTRIBUTE: Option<PropertyDeclarationBlock> = None;
-
 impl<'le> TElement for GeckoElement<'le> {
     type ConcreteNode = GeckoNode<'le>;
     type ConcreteDocument = GeckoDocument<'le>;
@@ -470,14 +467,16 @@ impl<'le> TElement for GeckoElement<'le> {
         unsafe { GeckoNode::from_raw(self.element as *mut RawGeckoNode) }
     }
 
-    fn style_attribute(&self) -> &Option<PropertyDeclarationBlock> {
+    fn style_attribute(&self) -> Option<&Arc<PropertyDeclarationBlock>> {
         let declarations = unsafe { Gecko_GetServoDeclarationBlock(self.element) };
         if declarations.is_null() {
-            &NO_STYLE_ATTRIBUTE
+            None
         } else {
-            GeckoDeclarationBlock::with(declarations, |declarations| {
-                unsafe { transmute(&declarations.declarations) }
-            })
+            let opt_ptr = GeckoDeclarationBlock::with(declarations, |declarations| {
+                // Use a raw poointer to extend the lifetime
+                declarations.declarations.as_ref().map(|r| r as *const Arc<_>)
+            });
+            opt_ptr.map(|ptr| unsafe { &*ptr })
         }
     }
 
diff --git a/tests/unit/script/size_of.rs b/tests/unit/script/size_of.rs
index 96981ca001f..3930180a9cf 100644
--- a/tests/unit/script/size_of.rs
+++ b/tests/unit/script/size_of.rs
@@ -40,10 +40,10 @@ macro_rules! sizeof_checker (
 // Update the sizes here
 sizeof_checker!(size_event_target, EventTarget, 40);
 sizeof_checker!(size_node, Node, 152);
-sizeof_checker!(size_element, Element, 328);
-sizeof_checker!(size_htmlelement, HTMLElement, 344);
-sizeof_checker!(size_div, HTMLDivElement, 344);
-sizeof_checker!(size_span, HTMLSpanElement, 344);
+sizeof_checker!(size_element, Element, 320);
+sizeof_checker!(size_htmlelement, HTMLElement, 336);
+sizeof_checker!(size_div, HTMLDivElement, 336);
+sizeof_checker!(size_span, HTMLSpanElement, 336);
 sizeof_checker!(size_text, Text, 184);
 sizeof_checker!(size_characterdata, CharacterData, 184);
 sizeof_checker!(size_servothreadsafelayoutnode, ServoThreadSafeLayoutNode, 16);
diff --git a/tests/unit/style/selector_matching.rs b/tests/unit/style/selector_matching.rs
index 22e11288c48..d11ebb99907 100644
--- a/tests/unit/style/selector_matching.rs
+++ b/tests/unit/style/selector_matching.rs
@@ -6,7 +6,7 @@ use cssparser::Parser;
 use selectors::parser::{LocalName, ParserContext, parse_selector_list};
 use std::sync::Arc;
 use string_cache::Atom;
-use style::properties::Importance;
+use style::properties::{Importance, PropertyDeclarationBlock};
 use style::selector_matching::{DeclarationBlock, Rule, SelectorMap};
 
 /// Helper method to get some Rules from selector strings.
@@ -19,7 +19,10 @@ fn get_mock_rules(css_selectors: &[&str]) -> Vec<Vec<Rule>> {
             Rule {
                 selector: s.complex_selector.clone(),
                 declarations: DeclarationBlock {
-                    mixed_declarations: Arc::new(Vec::new()),
+                    mixed_declarations: Arc::new(PropertyDeclarationBlock {
+                        declarations: Arc::new(Vec::new()),
+                        important_count: 0,
+                    }),
                     importance: Importance::Normal,
                     specificity: s.specificity,
                     source_order: i,
diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs
index 49eb451d7f7..bf57f19a066 100644
--- a/tests/unit/style/stylesheets.rs
+++ b/tests/unit/style/stylesheets.rs
@@ -236,24 +236,30 @@ fn test_parse_stylesheet() {
                     Arc::new(Keyframe {
                         selector: KeyframeSelector::new_for_unit_testing(
                                       vec![KeyframePercentage::new(0.)]),
-                        declarations: Arc::new(vec![
-                            (PropertyDeclaration::Width(DeclaredValue::Value(
-                                LengthOrPercentageOrAuto::Percentage(Percentage(0.)))),
-                             Importance::Normal),
-                        ]),
+                        block: Arc::new(PropertyDeclarationBlock {
+                            declarations: Arc::new(vec![
+                                (PropertyDeclaration::Width(DeclaredValue::Value(
+                                    LengthOrPercentageOrAuto::Percentage(Percentage(0.)))),
+                                 Importance::Normal),
+                            ]),
+                            important_count: 0,
+                        })
                     }),
                     Arc::new(Keyframe {
                         selector: KeyframeSelector::new_for_unit_testing(
                                       vec![KeyframePercentage::new(1.)]),
-                        declarations: Arc::new(vec![
-                            (PropertyDeclaration::Width(DeclaredValue::Value(
-                                LengthOrPercentageOrAuto::Percentage(Percentage(1.)))),
-                             Importance::Normal),
-                            (PropertyDeclaration::AnimationPlayState(DeclaredValue::Value(
-                                animation_play_state::SpecifiedValue(
-                                    vec![animation_play_state::SingleSpecifiedValue::running]))),
-                             Importance::Normal),
-                        ]),
+                        block: Arc::new(PropertyDeclarationBlock {
+                            declarations: Arc::new(vec![
+                                (PropertyDeclaration::Width(DeclaredValue::Value(
+                                    LengthOrPercentageOrAuto::Percentage(Percentage(1.)))),
+                                 Importance::Normal),
+                                (PropertyDeclaration::AnimationPlayState(DeclaredValue::Value(
+                                    animation_play_state::SpecifiedValue(
+                                        vec![animation_play_state::SingleSpecifiedValue::running]))),
+                                 Importance::Normal),
+                            ]),
+                            important_count: 0,
+                        }),
                     }),
                 ]
             }))