style: Fix pseudo-element restyling.

components/script/layout_wrapper.rs

@@ -61,7 +61,7 @@ use style::dom::{PresentationalHintsSynthetizer, OpaqueNode, TDocument, TElement
 use style::element_state::*;
 use style::properties::{ComputedValues, PropertyDeclaration, PropertyDeclarationBlock};
 use style::refcell::{Ref, RefCell, RefMut};
-use style::selector_impl::{ElementSnapshot, NonTSPseudoClass, ServoSelectorImpl};
+use style::selector_impl::{ElementSnapshot, NonTSPseudoClass, PseudoElement, ServoSelectorImpl};
 use style::sink::Push;
 use style::str::is_whitespace;
 use url::Url;
@@ -266,7 +266,8 @@ impl<'ln> TNode for ServoLayoutNode<'ln> {
 
     #[inline]
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             current_cv: Option<&'a Arc<ComputedValues>>)
+                                             current_cv: Option<&'a Arc<ComputedValues>>,
+                                             _pseudo_element: Option<&PseudoElement>)
                                              -> Option<&'a Arc<ComputedValues>> {
         current_cv
     }

components/style/dom.rs

@@ -12,7 +12,7 @@ use element_state::ElementState;
 use properties::{ComputedValues, PropertyDeclaration, PropertyDeclarationBlock};
 use refcell::{Ref, RefMut};
 use restyle_hints::{RESTYLE_DESCENDANTS, RESTYLE_LATER_SIBLINGS, RESTYLE_SELF, RestyleHint};
-use selector_impl::ElementExt;
+use selector_impl::{ElementExt, PseudoElement};
 use selectors::matching::DeclarationBlock;
 use sink::Push;
 use std::fmt::Debug;
@@ -177,7 +177,8 @@ pub trait TNode : Sized + Copy + Clone {
     /// as an argument here, but otherwise Servo would crash due to double
     /// borrows to return it.
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             current_computed_values: Option<&'a Arc<ComputedValues>>)
+                                             current_computed_values: Option<&'a Arc<ComputedValues>>,
+                                             pseudo: Option<&PseudoElement>)
         -> Option<&'a <Self::ConcreteRestyleDamage as TRestyleDamage>::PreExistingComputedValues>;
 }
 

components/style/matching.rs

@@ -639,7 +639,7 @@ pub trait ElementMatchMethods : TElement {
                 let style = &mut node.mutate_data().unwrap().style;
 
                 let damage =
-                    match node.existing_style_for_restyle_damage((*style).as_ref()) {
+                    match node.existing_style_for_restyle_damage((*style).as_ref(), None) {
                         Some(ref source) => {
                             <<Self as TElement>::ConcreteNode as TNode>
                             ::ConcreteRestyleDamage::compute(source, &shared_style)
@@ -719,8 +719,11 @@ pub trait MatchMethods : TNode {
 
     fn compute_restyle_damage(&self,
                               old_style: Option<&Arc<ComputedValues>>,
-                              new_style: &Arc<ComputedValues>) -> Self::ConcreteRestyleDamage {
+                              new_style: &Arc<ComputedValues>,
-        match self.existing_style_for_restyle_damage(old_style) {
+                              pseudo: Option<&PseudoElement>)
+                              -> Self::ConcreteRestyleDamage
+    {
+        match self.existing_style_for_restyle_damage(old_style, pseudo) {
             Some(ref source) => {
                 Self::ConcreteRestyleDamage::compute(source,
                                                      new_style)
@@ -748,7 +751,8 @@ pub trait MatchMethods : TNode {
                 // like children of replaced content. Arguably, we shouldn't be
                 // styling those here, but until we implement that we'll have to
                 // stick without the assertions.
-                debug_assert!(new_style.get_box().clone_display() != display::T::none);
+                debug_assert!(pseudo.is_none() ||
+                              new_style.get_box().clone_display() != display::T::none);
                 Self::ConcreteRestyleDamage::rebuild_and_reflow()
             }
         }
@@ -783,7 +787,7 @@ pub trait MatchMethods : TNode {
             let cloned_parent_style = ComputedValues::style_for_child_text_node(parent_style.unwrap());
 
             let damage =
-                self.compute_restyle_damage(data.style.as_ref(), &cloned_parent_style);
+                self.compute_restyle_damage(data.style.as_ref(), &cloned_parent_style, None);
 
             data.style = Some(cloned_parent_style);
 
@@ -860,21 +864,10 @@ pub trait MatchMethods : TNode {
         // Otherwise, we just compute the damage normally, and sum up the damage
         // related to pseudo-elements.
         let mut damage =
-            self.compute_restyle_damage(data.style.as_ref(), &final_style);
+            self.compute_restyle_damage(data.style.as_ref(), &final_style, None);
 
         data.style = Some(final_style);
 
-        // FIXME(emilio): This is not pretty, and in the Gecko case means
-        // effectively comparing with the old computed values (given our style
-        // source is the old nsStyleContext).
-        //
-        // We should be diffing the old pseudo-element styles with the new ones,
-        // see https://bugzilla.mozilla.org/show_bug.cgi?id=1293399.
-        //
-        // For now we do the following, which preserves the old Servo behavior.
-        let existing_style =
-            self.existing_style_for_restyle_damage(data.style.as_ref());
-
         let data_per_pseudo = &mut data.per_pseudo;
         let new_style = data.style.as_ref();
 
@@ -883,36 +876,72 @@ pub trait MatchMethods : TNode {
         let rebuild_and_reflow =
             Self::ConcreteRestyleDamage::rebuild_and_reflow();
 
-        debug_assert!(existing_style.is_some() || damage == rebuild_and_reflow);
-
         <Self::ConcreteElement as MatchAttr>::Impl::each_eagerly_cascaded_pseudo_element(|pseudo| {
+            use std::collections::hash_map::Entry;
+
             let applicable_declarations_for_this_pseudo =
                 applicable_declarations.per_pseudo.get(&pseudo).unwrap();
 
-            if !applicable_declarations_for_this_pseudo.is_empty() {
+            let has_declarations =
+                !applicable_declarations_for_this_pseudo.is_empty();
+
+            // If there are declarations matching, we're going to need to
+            // recompute the style anyway, so do it now to simplify the logic
+            // below.
+            let pseudo_style_if_declarations = if has_declarations {
                 // NB: Transitions and animations should only work for
                 // pseudo-elements ::before and ::after
                 let should_animate_properties =
                     <Self::ConcreteElement as MatchAttr>::Impl::pseudo_is_before_or_after(&pseudo);
 
-                let new_pseudo_style =
+                Some(self.cascade_node_pseudo_element(context,
-                    self.cascade_node_pseudo_element(context,
+                                                      new_style,
-                                                     new_style,
+                                                      &*applicable_declarations_for_this_pseudo,
-                                                     &*applicable_declarations_for_this_pseudo,
+                                                      data_per_pseudo.get_mut(&pseudo),
-                                                     data_per_pseudo.get_mut(&pseudo),
+                                                      &mut applicable_declarations_cache,
-                                                     &mut applicable_declarations_cache,
+                                                      /* shareable = */ false,
-                                                     /* shareable = */ false,
+                                                      should_animate_properties))
-                                                     should_animate_properties);
+            } else {
+                None
+            };
 
-                // No point in computing more damage if we're already doing all
+            // Let's see what we had before.
-                // the work.
+            match data_per_pseudo.entry(pseudo.clone()) {
-                if damage != rebuild_and_reflow {
+                Entry::Vacant(vacant_entry) => {
-                    let new_damage =
+                    // If we had a vacant entry, and no rules that match, we're
-                        Self::ConcreteRestyleDamage::compute(existing_style.unwrap(),
+                    // fine so far.
-                                                             &new_pseudo_style);
+                    if !has_declarations {
-                    damage = damage | new_damage;
+                        return;
+                    }
+
+                    // Otherwise, we need to insert the new computed styles, and
+                    // generate a rebuild_and_reflow damage.
+                    damage = damage | Self::ConcreteRestyleDamage::rebuild_and_reflow();
+                    vacant_entry.insert(pseudo_style_if_declarations.unwrap());
+                }
+                Entry::Occupied(mut occupied_entry) => {
+                    // If there was an existing style, and no declarations, we
+                    // need to remove us from the map, and ensure we're
+                    // reconstructing.
+                    if !has_declarations {
+                        damage = damage | Self::ConcreteRestyleDamage::rebuild_and_reflow();
+                        occupied_entry.remove();
+                        return;
+                    }
+
+                    // If there's a new style, we need to diff it and add the
+                    // damage, except if the damage was already
+                    // rebuild_and_reflow, in which case we can avoid it.
+                    if damage != rebuild_and_reflow {
+                        damage = damage |
+                                 self.compute_restyle_damage(Some(occupied_entry.get()),
+                                                             pseudo_style_if_declarations.as_ref().unwrap(),
+                                                             Some(&pseudo));
+                    }
+
+                    // And now, of course, use the new style.
+                    occupied_entry.insert(pseudo_style_if_declarations.unwrap());
                 }
-                data_per_pseudo.insert(pseudo, new_pseudo_style);
             }
         });
 

components/style/selector_impl.rs

@@ -79,7 +79,8 @@ pub trait ElementExt: Element<Impl=TheSelectorImpl> {
 impl TheSelectorImpl {
     #[inline]
     pub fn each_eagerly_cascaded_pseudo_element<F>(mut fun: F)
-        where F: FnMut(<Self as SelectorImpl>::PseudoElement) {
+        where F: FnMut(PseudoElement)
+    {
         Self::each_pseudo_element(|pseudo| {
             if Self::pseudo_element_cascade_type(&pseudo).is_eager() {
                 fun(pseudo)
@@ -89,7 +90,8 @@ impl TheSelectorImpl {
 
     #[inline]
     pub fn each_precomputed_pseudo_element<F>(mut fun: F)
-        where F: FnMut(<Self as SelectorImpl>::PseudoElement) {
+        where F: FnMut(PseudoElement)
+    {
         Self::each_pseudo_element(|pseudo| {
             if Self::pseudo_element_cascade_type(&pseudo).is_precomputed() {
                 fun(pseudo)

ports/geckolib/wrapper.rs

@@ -44,7 +44,7 @@ use style::dom::{TDocument, TElement, TNode, TRestyleDamage, UnsafeNode};
 use style::element_state::ElementState;
 use style::error_reporting::StdoutErrorReporter;
 use style::gecko_glue::ArcHelpers;
-use style::gecko_selector_impl::{GeckoSelectorImpl, NonTSPseudoClass};
+use style::gecko_selector_impl::{GeckoSelectorImpl, NonTSPseudoClass, PseudoElement};
 use style::parser::ParserContextExtraData;
 use style::properties::{ComputedValues, parse_style_attribute};
 use style::properties::{PropertyDeclaration, PropertyDeclarationBlock};
@@ -312,13 +312,21 @@ impl<'ln> TNode for GeckoNode<'ln> {
     }
 
     fn existing_style_for_restyle_damage<'a>(&'a self,
-                                             current_cv: Option<&'a Arc<ComputedValues>>)
+                                             current_cv: Option<&'a Arc<ComputedValues>>,
+                                             pseudo: Option<&PseudoElement>)
                                              -> Option<&'a nsStyleContext> {
         if current_cv.is_none() {
             // Don't bother in doing an ffi call to get null back.
             return None;
         }
 
+        if pseudo.is_some() {
+            // FIXME(emilio): This makes us reconstruct frame for pseudos every
+            // restyle, add a FFI call to get the style context associated with
+            // a PE.
+            return None;
+        }
+
         unsafe {
             let context_ptr = Gecko_GetStyleContext(self.node);
             context_ptr.as_ref()