style: Make Element::id not clone the attribute.

components/layout_thread/dom_wrapper.rs

@@ -341,7 +341,7 @@ pub struct ServoLayoutElement<'le> {
 impl<'le> fmt::Debug for ServoLayoutElement<'le> {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         write!(f, "<{}", self.element.local_name())?;
-        if let &Some(ref id) = unsafe { &*self.element.id_attribute() } {
+        if let Some(id) = self.id() {
             write!(f, " id={}", id)?;
         }
         write!(f, "> ({:#x})", self.as_node().opaque().0)
@@ -382,9 +382,9 @@ impl<'le> TElement for ServoLayoutElement<'le> {
     }
 
     #[inline]
-    fn id(&self) -> Option<Atom> {
+    fn id(&self) -> Option<&Atom> {
         unsafe {
-            (*self.element.id_attribute()).clone()
+            (*self.element.id_attribute()).as_ref()
         }
     }
 

components/style/dom.rs

@@ -7,7 +7,7 @@
 #![allow(unsafe_code)]
 #![deny(missing_docs)]
 
-use {Atom, Namespace, LocalName};
+use {Atom, Namespace, LocalName, WeakAtom};
 use applicable_declarations::ApplicableDeclarationBlock;
 use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
 #[cfg(feature = "gecko")] use context::PostAnimationTasks;
@@ -467,7 +467,7 @@ pub trait TElement
     fn has_attr(&self, namespace: &Namespace, attr: &LocalName) -> bool;
 
     /// The ID for this element.
-    fn id(&self) -> Option<Atom>;
+    fn id(&self) -> Option<&WeakAtom>;
 
     /// Internal iterator for the classes of this element.
     fn each_class<F>(&self, callback: F) where F: FnMut(&Atom);

components/style/gecko/snapshot.rs

@@ -5,6 +5,7 @@
 //! A gecko snapshot, that stores the element attributes and state before they
 //! change in order to properly calculate restyle hints.
 
+use WeakAtom;
 use dom::TElement;
 use element_state::ElementState;
 use gecko::snapshot_helpers;
@@ -80,17 +81,20 @@ impl GeckoElementSnapshot {
     }
 
     /// selectors::Element::attr_matches
-    pub fn attr_matches(&self,
+    pub fn attr_matches(
+        &self,
         ns: &NamespaceConstraint<&Namespace>,
         local_name: &Atom,
-                        operation: &AttrSelectorOperation<&Atom>)
+        operation: &AttrSelectorOperation<&Atom>,
-                        -> bool {
+    ) -> bool {
         unsafe {
             match *operation {
                 AttrSelectorOperation::Exists => {
-                    bindings:: Gecko_SnapshotHasAttr(self,
+                    bindings:: Gecko_SnapshotHasAttr(
+                        self,
                         ns.atom_or_null(),
-                                                     local_name.as_ptr())
+                        local_name.as_ptr(),
+                    )
                 }
                 AttrSelectorOperation::WithValue { operator, case_sensitivity, expected_value } => {
                     let ignore_case = match case_sensitivity {
@@ -163,7 +167,7 @@ impl ElementSnapshot for GeckoElementSnapshot {
     }
 
     #[inline]
-    fn id_attr(&self) -> Option<Atom> {
+    fn id_attr(&self) -> Option<&WeakAtom> {
         if !self.has_any(Flags::Id) {
             return None
         }
@@ -172,10 +176,11 @@ impl ElementSnapshot for GeckoElementSnapshot {
             bindings::Gecko_SnapshotAtomAttrValue(self, atom!("id").as_ptr())
         };
 
+        // FIXME(emilio): This should assert, since this flag is exact.
         if ptr.is_null() {
             None
         } else {
-            Some(Atom::from(ptr))
+            Some(unsafe { WeakAtom::new(ptr) })
         }
     }
 

components/style/gecko/wrapper.rs

@@ -1176,7 +1176,7 @@ impl<'le> TElement for GeckoElement<'le> {
 
     // FIXME(emilio): we should probably just return a reference to the Atom.
     #[inline]
-    fn id(&self) -> Option<Atom> {
+    fn id(&self) -> Option<&WeakAtom> {
         if !self.has_id() {
             return None;
         }
@@ -1185,10 +1185,12 @@ impl<'le> TElement for GeckoElement<'le> {
             bindings::Gecko_AtomAttrValue(self.0, atom!("id").as_ptr())
         };
 
+        // FIXME(emilio): Pretty sure the has_id flag is exact and we could
+        // assert here.
         if ptr.is_null() {
             None
         } else {
-            Some(Atom::from(ptr))
+            Some(unsafe { WeakAtom::new(ptr) })
         }
     }
 

components/style/invalidation/element/element_wrapper.rs

@@ -5,7 +5,7 @@
 //! A wrapper over an element and a snapshot, that allows us to selector-match
 //! against a past state of the element.
 
-use {Atom, CaseSensitivityExt, LocalName, Namespace};
+use {Atom, CaseSensitivityExt, LocalName, Namespace, WeakAtom};
 use dom::TElement;
 use element_state::ElementState;
 use selector_parser::{NonTSPseudoClass, PseudoElement, SelectorImpl, Snapshot, SnapshotMap, AttrValue};
@@ -44,7 +44,7 @@ pub trait ElementSnapshot : Sized {
 
     /// The ID attribute per this snapshot. Should only be called if
     /// `has_attrs()` returns true.
-    fn id_attr(&self) -> Option<Atom>;
+    fn id_attr(&self) -> Option<&WeakAtom>;
 
     /// Whether this snapshot contains the class `name`. Should only be called
     /// if `has_attrs()` returns true.
@@ -53,7 +53,8 @@ pub trait ElementSnapshot : Sized {
     /// A callback that should be called for each class of the snapshot. Should
     /// only be called if `has_attrs()` returns true.
     fn each_class<F>(&self, F)
-        where F: FnMut(&Atom);
+    where
+        F: FnMut(&Atom);
 
     /// The `xml:lang=""` or `lang=""` attribute value per this snapshot.
     fn lang_attr(&self) -> Option<AttrValue>;
@@ -63,7 +64,8 @@ pub trait ElementSnapshot : Sized {
 /// selector-match against a past state of the element.
 #[derive(Clone)]
 pub struct ElementWrapper<'a, E>
-    where E: TElement,
+where
+    E: TElement,
 {
     element: E,
     cached_snapshot: Cell<Option<&'a Snapshot>>,

components/style/invalidation/element/state_and_attributes.rs

@@ -5,7 +5,7 @@
 //! An invalidation processor for style changes due to state and attribute
 //! changes.
 
-use Atom;
+use {Atom, WeakAtom};
 use context::{QuirksMode, SharedStyleContext};
 use data::ElementData;
 use dom::TElement;
@@ -42,8 +42,8 @@ where
     snapshot: &'a Snapshot,
     quirks_mode: QuirksMode,
     lookup_element: E,
-    removed_id: Option<&'a Atom>,
+    removed_id: Option<&'a WeakAtom>,
-    added_id: Option<&'a Atom>,
+    added_id: Option<&'a WeakAtom>,
     classes_removed: &'a SmallVec<[Atom; 8]>,
     classes_added: &'a SmallVec<[Atom; 8]>,
     state_changes: ElementState,
@@ -239,8 +239,8 @@ where
                 snapshot: &snapshot,
                 quirks_mode: self.shared_context.quirks_mode(),
                 nth_index_cache: self.matching_context.nth_index_cache.as_mut().map(|c| &mut **c),
-                removed_id: id_removed.as_ref(),
+                removed_id: id_removed,
-                added_id: id_added.as_ref(),
+                added_id: id_added,
                 classes_removed: &classes_removed,
                 classes_added: &classes_added,
                 descendant_invalidations,

components/style/selector_map.rs

@@ -5,7 +5,7 @@
 //! A data structure to efficiently index structs containing selectors by local
 //! name, ids and hash.
 
-use {Atom, LocalName};
+use {Atom, LocalName, WeakAtom};
 use applicable_declarations::ApplicableDeclarationList;
 use context::QuirksMode;
 use dom::TElement;
@@ -176,7 +176,7 @@ impl SelectorMap<Rule> {
         // where we began.
         let init_len = matching_rules_list.len();
         if let Some(id) = rule_hash_target.id() {
-            if let Some(rules) = self.id_hash.get(&id, quirks_mode) {
+            if let Some(rules) = self.id_hash.get(id, quirks_mode) {
                 SelectorMap::get_matching_rules(
                     element,
                     rules,
@@ -316,7 +316,7 @@ impl<T: SelectorMapEntry> SelectorMap<T> {
     {
         // Id.
         if let Some(id) = element.id() {
-            if let Some(v) = self.id_hash.get(&id, quirks_mode) {
+            if let Some(v) = self.id_hash.get(id, quirks_mode) {
                 for entry in v.iter() {
                     if !f(&entry) {
                         return false;
@@ -374,7 +374,7 @@ impl<T: SelectorMapEntry> SelectorMap<T> {
         &'a self,
         element: E,
         quirks_mode: QuirksMode,
-        additional_id: Option<&Atom>,
+        additional_id: Option<&WeakAtom>,
         additional_classes: &[Atom],
         mut f: F,
     ) -> bool
@@ -535,7 +535,7 @@ impl<V: 'static> MaybeCaseInsensitiveHashMap<Atom, V> {
     }
 
     /// HashMap::get
-    pub fn get(&self, key: &Atom, quirks_mode: QuirksMode) -> Option<&V> {
+    pub fn get(&self, key: &WeakAtom, quirks_mode: QuirksMode) -> Option<&V> {
         if quirks_mode == QuirksMode::Quirks {
             self.0.get(&key.to_ascii_lowercase())
         } else {

components/style/servo/selector_parser.rs

@@ -696,8 +696,8 @@ impl ElementSnapshot for ServoElementSnapshot {
         self.attrs.is_some()
     }
 
-    fn id_attr(&self) -> Option<Atom> {
+    fn id_attr(&self) -> Option<&Atom> {
-        self.get_attr(&ns!(), &local_name!("id")).map(|v| v.as_atom().clone())
+        self.get_attr(&ns!(), &local_name!("id")).map(|v| v.as_atom())
     }
 
     fn has_class(&self, name: &Atom, case_sensitivity: CaseSensitivity) -> bool {

components/style/sharing/checks.rs

@@ -155,7 +155,7 @@ where
     let stylist = &shared_context.stylist;
 
     let may_have_rules_for_element = match element_id {
-        Some(ref id) => stylist.may_have_rules_for_id(id, element),
+        Some(id) => stylist.may_have_rules_for_id(id, element),
         None => false
     };
 
@@ -164,7 +164,7 @@ where
     }
 
     match candidate_id {
-        Some(ref id) => stylist.may_have_rules_for_id(id, candidate),
+        Some(id) => stylist.may_have_rules_for_id(id, candidate),
         None => false
     }
 }

components/style/stylist.rs

@@ -4,7 +4,7 @@
 
 //! Selector matching.
 
-use {Atom, LocalName, Namespace};
+use {Atom, LocalName, Namespace, WeakAtom};
 use applicable_declarations::{ApplicableDeclarationBlock, ApplicableDeclarationList};
 use context::{CascadeInputs, QuirksMode};
 use dom::TElement;
@@ -1385,7 +1385,7 @@ impl Stylist {
     #[inline]
     pub fn may_have_rules_for_id<E>(
         &self,
-        id: &Atom,
+        id: &WeakAtom,
         element: E,
     ) -> bool
     where