Backed out changeset fc813bf68348 for failing reftest layout/reftests/bugs/272646-1.xul on OS X. r=backout
authorSebastian Hengst <archaeopteryx@coole-files.de>
Sat, 28 Oct 2017 10:07:46 +0200
changeset 388845 39400e032624c7cd7b3c63c62957c5c32aa8b008
parent 388844 6087df884cb256fce27d97574eedb12e91db73eb
child 388866 8fe5c82c3dcd998eaa5f9b038d083abc764931af
push id54277
push userarchaeopteryx@coole-files.de
push dateSat, 28 Oct 2017 08:08:04 +0000
treeherderautoland@39400e032624 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbackout
bugs272646
milestone58.0a1
backs outfc813bf68348121aeac663dd48a23221c4116dbb
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset fc813bf68348 for failing reftest layout/reftests/bugs/272646-1.xul on OS X. r=backout
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/sharing/checks.rs
servo/components/style/sharing/mod.rs
servo/components/style/stylist.rs
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -383,25 +383,16 @@ pub trait TElement
         while let Some(parent) = curr.traversal_parent() {
             depth += 1;
             curr = parent;
         }
 
         depth
     }
 
-    /// The style scope of this element is a node that represents which rules
-    /// apply to the element.
-    ///
-    /// In Servo, where we don't know about Shadow DOM or XBL, the style scope
-    /// is always the document.
-    fn style_scope(&self) -> Self::ConcreteNode {
-        self.as_node().owner_doc().as_node()
-    }
-
     /// Get this node's parent element from the perspective of a restyle
     /// traversal.
     fn traversal_parent(&self) -> Option<Self> {
         self.as_node().traversal_parent()
     }
 
     /// Get this node's children from the perspective of a restyle traversal.
     fn traversal_children(&self) -> LayoutIterator<Self::TraversalChildrenIterator>;
@@ -742,26 +733,24 @@ pub trait TElement
     /// Returns the anonymous content for the current element's XBL binding,
     /// given if any.
     ///
     /// This is used in Gecko for XBL and shadow DOM.
     fn xbl_binding_anonymous_content(&self) -> Option<Self::ConcreteNode> {
         None
     }
 
-    /// Return the element which we can use to look up rules in the selector
-    /// maps.
-    ///
-    /// This is always the element itself, except in the case where we are an
-    /// element-backed pseudo-element, in which case we return the originating
-    /// element.
+    /// Returns the rule hash target given an element.
     fn rule_hash_target(&self) -> Self {
         let is_implemented_pseudo =
             self.implemented_pseudo_element().is_some();
 
+        // NB: This causes use to rule has pseudo selectors based on the
+        // properties of the originating element (which is fine, given the
+        // find_first_from_right usage).
         if is_implemented_pseudo {
             self.closest_non_native_anonymous_ancestor().unwrap()
         } else {
             *self
         }
     }
 
     /// Implements Gecko's `nsBindingManager::WalkRules`.
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -514,23 +514,18 @@ impl<'le> GeckoElement<'le> {
     fn get_extended_slots(
         &self,
     ) -> Option<&structs::FragmentOrElement_nsExtendedDOMSlots> {
         self.get_dom_slots()
             .and_then(|s| unsafe { s.mExtendedSlots.mPtr.as_ref() })
     }
 
     #[inline]
-    fn may_be_in_binding_manager(&self) -> bool {
-        self.flags() & (structs::NODE_MAY_BE_IN_BINDING_MNGR as u32) != 0
-    }
-
-    #[inline]
     fn get_xbl_binding(&self) -> Option<GeckoXBLBinding<'le>> {
-        if !self.may_be_in_binding_manager() {
+        if self.flags() & (structs::NODE_MAY_BE_IN_BINDING_MNGR as u32) == 0 {
             return None;
         }
 
         unsafe { bindings::Gecko_GetXBLBinding(self.0).map(GeckoXBLBinding) }
     }
 
     #[inline]
     fn get_xbl_binding_with_content(&self) -> Option<GeckoXBLBinding<'le>> {
@@ -553,21 +548,19 @@ impl<'le> GeckoElement<'le> {
             // where the binding parent is stored in a member variable
             // rather than in slots.  So just get it through FFI for now.
             unsafe {
                 bindings::Gecko_GetBindingParent(self.0).map(GeckoElement)
             }
         } else {
             let binding_parent = unsafe {
                 self.get_non_xul_xbl_binding_parent_raw_content().as_ref()
-            }.map(GeckoNode::from_content).and_then(|n| n.as_element());
-
-            debug_assert!(binding_parent == unsafe {
-                bindings::Gecko_GetBindingParent(self.0).map(GeckoElement)
-            });
+            }.map(GeckoNode::from_content)
+                .and_then(|n| n.as_element());
+            debug_assert!(binding_parent == unsafe { bindings::Gecko_GetBindingParent(self.0).map(GeckoElement) });
             binding_parent
         }
     }
 
     fn get_non_xul_xbl_binding_parent_raw_content(&self) -> *mut nsIContent {
         debug_assert!(!self.is_xul_element());
         self.get_extended_slots()
             .map_or(ptr::null_mut(), |slots| slots.mBindingParent)
@@ -927,38 +920,16 @@ impl<'le> TElement for GeckoElement<'le>
     fn before_pseudo_element(&self) -> Option<Self> {
         self.get_before_or_after_pseudo(/* is_before = */ true)
     }
 
     fn after_pseudo_element(&self) -> Option<Self> {
         self.get_before_or_after_pseudo(/* is_before = */ false)
     }
 
-    /// Ensure this accurately represents the rules that an element may ever
-    /// match, even in the native anonymous content case.
-    fn style_scope(&self) -> Self::ConcreteNode {
-        if self.implemented_pseudo_element().is_some() {
-            return self.closest_non_native_anonymous_ancestor().unwrap().style_scope();
-        }
-
-        if self.is_in_native_anonymous_subtree() {
-            return self.as_node().owner_doc().as_node();
-        }
-
-        if self.get_xbl_binding().is_some() {
-            return self.as_node();
-        }
-
-        if let Some(parent) = self.get_xbl_binding_parent() {
-            return parent.as_node();
-        }
-
-        self.as_node().owner_doc().as_node()
-    }
-
     /// Execute `f` for each anonymous content child element (apart from
     /// ::before and ::after) whose originating element is `self`.
     fn each_anonymous_content_child<F>(&self, mut f: F)
     where
         F: FnMut(Self),
     {
         let array: *mut structs::nsTArray<*mut nsIContent> =
             unsafe { bindings::Gecko_GetAnonymousContentForElement(self.0) };
--- a/servo/components/style/sharing/checks.rs
+++ b/servo/components/style/sharing/checks.rs
@@ -1,16 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! Different checks done during the style sharing process in order to determine
 //! quickly whether it's worth to share style, and whether two different
 //! elements can indeed share the same style.
 
+use Atom;
 use bloom::StyleBloom;
 use context::{SelectorFlagsMap, SharedStyleContext};
 use dom::TElement;
 use selectors::NthIndexCache;
 use sharing::{StyleSharingCandidate, StyleSharingTarget};
 
 /// Determines whether a target and a candidate have compatible parents for sharing.
 pub fn parents_allow_sharing<E>(
@@ -90,26 +91,24 @@ pub fn have_same_class<E>(target: &mut S
 
 /// Whether a given element and a candidate match the same set of "revalidation"
 /// selectors.
 ///
 /// Revalidation selectors are those that depend on the DOM structure, like
 /// :first-child, etc, or on attributes that we don't check off-hand (pretty
 /// much every attribute selector except `id` and `class`.
 #[inline]
-pub fn revalidate<E>(
-    target: &mut StyleSharingTarget<E>,
-    candidate: &mut StyleSharingCandidate<E>,
-    shared_context: &SharedStyleContext,
-    bloom: &StyleBloom<E>,
-    nth_index_cache: &mut NthIndexCache,
-    selector_flags_map: &mut SelectorFlagsMap<E>,
-) -> bool
-where
-    E: TElement,
+pub fn revalidate<E>(target: &mut StyleSharingTarget<E>,
+                     candidate: &mut StyleSharingCandidate<E>,
+                     shared_context: &SharedStyleContext,
+                     bloom: &StyleBloom<E>,
+                     nth_index_cache: &mut NthIndexCache,
+                     selector_flags_map: &mut SelectorFlagsMap<E>)
+                     -> bool
+    where E: TElement,
 {
     let stylist = &shared_context.stylist;
 
     let for_element = target.revalidation_match_results(
         stylist,
         bloom,
         nth_index_cache,
         selector_flags_map,
@@ -126,39 +125,30 @@ where
     // comparing represent the same set of selectors.
     debug_assert_eq!(for_element.len(), for_candidate.len());
 
     for_element == for_candidate
 }
 
 /// Checks whether we might have rules for either of the two ids.
 #[inline]
-pub fn may_match_different_id_rules<E>(
-    shared_context: &SharedStyleContext,
-    element: E,
-    candidate: E,
-) -> bool
-where
-    E: TElement,
+pub fn may_have_rules_for_ids(shared_context: &SharedStyleContext,
+                              element_id: Option<&Atom>,
+                              candidate_id: Option<&Atom>) -> bool
 {
-    let element_id = element.get_id();
-    let candidate_id = candidate.get_id();
-
-    if element_id == candidate_id {
-        return false;
-    }
-
+    // We shouldn't be called unless the ids are different.
+    debug_assert!(element_id.is_some() || candidate_id.is_some());
     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),
         None => false
     };
 
     if may_have_rules_for_element {
         return true;
     }
 
     match candidate_id {
-        Some(ref id) => stylist.may_have_rules_for_id(id, candidate),
+        Some(id) => stylist.may_have_rules_for_id(id),
         None => false
     }
 }
--- a/servo/components/style/sharing/mod.rs
+++ b/servo/components/style/sharing/mod.rs
@@ -201,21 +201,20 @@ impl ValidationData {
     #[inline]
     fn revalidation_match_results<E, F>(
         &mut self,
         element: E,
         stylist: &Stylist,
         bloom: &StyleBloom<E>,
         nth_index_cache: &mut NthIndexCache,
         bloom_known_valid: bool,
-        flags_setter: &mut F,
+        flags_setter: &mut F
     ) -> &SmallBitVec
-    where
-        E: TElement,
-        F: FnMut(&E, ElementSelectorFlags),
+        where E: TElement,
+              F: FnMut(&E, ElementSelectorFlags),
     {
         if self.revalidation_match_results.is_none() {
             // The bloom filter may already be set up for our element.
             // If it is, use it.  If not, we must be in a candidate
             // (i.e. something in the cache), and the element is one
             // of our cousins, not a sibling.  In that case, we'll
             // just do revalidation selector matching without a bloom
             // filter, to avoid thrashing the filter.
@@ -226,22 +225,20 @@ impl ValidationData {
             } else {
                 if bloom.current_parent() == element.traversal_parent() {
                     Some(bloom.filter())
                 } else {
                     None
                 }
             };
             self.revalidation_match_results =
-                Some(stylist.match_revalidation_selectors(
-                    element,
-                    bloom_to_use,
-                    nth_index_cache,
-                    flags_setter,
-                ));
+                Some(stylist.match_revalidation_selectors(&element,
+                                                          bloom_to_use,
+                                                          nth_index_cache,
+                                                          flags_setter));
         }
 
         self.revalidation_match_results.as_ref().unwrap()
     }
 }
 
 /// Information regarding a style sharing candidate, that is, an entry in the
 /// style sharing cache.
@@ -659,25 +656,16 @@ impl<E: TElement> StyleSharingCache<E> {
 
         if target.is_native_anonymous() {
             debug_assert!(!candidate.element.is_native_anonymous(),
                           "Why inserting NAC into the cache?");
             trace!("Miss: Native Anonymous Content");
             return None;
         }
 
-        // Note that in the XBL case, we should be able to assert that the
-        // scopes are different, since two elements with different XBL bindings
-        // need to necessarily have different style (and thus children of them
-        // would never pass the parent check).
-        if target.element.style_scope() != candidate.element.style_scope() {
-            trace!("Miss: Different style scopes");
-            return None;
-        }
-
         if *target.get_local_name() != *candidate.element.get_local_name() {
             trace!("Miss: Local Name");
             return None;
         }
 
         if *target.get_namespace() != *candidate.element.get_namespace() {
             trace!("Miss: Namespace");
             return None;
@@ -697,27 +685,25 @@ impl<E: TElement> StyleSharingCache<E> {
         // We do not ignore visited state here, because Gecko
         // needs to store extra bits on visited style contexts,
         // so these contexts cannot be shared
         if target.element.get_state() != candidate.get_state() {
             trace!("Miss: User and Author State");
             return None;
         }
 
-        // It's possible that there are no styles for either id.
-        let may_match_different_id_rules =
-            checks::may_match_different_id_rules(
-                shared,
-                target.element,
-                candidate.element,
-            );
-
-        if may_match_different_id_rules {
-            trace!("Miss: ID Attr");
-            return None;
+        let element_id = target.element.get_id();
+        let candidate_id = candidate.element.get_id();
+        if element_id != candidate_id {
+            // It's possible that there are no styles for either id.
+            if checks::may_have_rules_for_ids(shared, element_id.as_ref(),
+                                              candidate_id.as_ref()) {
+                trace!("Miss: ID Attr");
+                return None;
+            }
         }
 
         if !checks::have_same_style_attribute(target, candidate) {
             trace!("Miss: Style Attr");
             return None;
         }
 
         if !checks::have_same_class(target, candidate) {
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -1404,55 +1404,36 @@ impl Stylist {
         } else {
             debug!("skipping transition rules");
         }
     }
 
     /// Given an id, returns whether there might be any rules for that id in any
     /// of our rule maps.
     #[inline]
-    pub fn may_have_rules_for_id<E>(
-        &self,
-        id: &Atom,
-        element: E,
-    ) -> bool
-    where
-        E: TElement,
-    {
-        let hash = id.get_hash();
-        for (data, _) in self.cascade_data.iter_origins() {
-            if data.mapped_ids.might_contain_hash(hash) {
-                return true;
-            }
-        }
-
-        let mut xbl_rules_may_contain = false;
-
-        element.each_xbl_stylist(|stylist| {
-            xbl_rules_may_contain = xbl_rules_may_contain ||
-                stylist.cascade_data.author.mapped_ids.might_contain_hash(hash)
-        });
-
-        xbl_rules_may_contain
+    pub fn may_have_rules_for_id(&self, id: &Atom) -> bool {
+        self.cascade_data
+            .iter_origins()
+            .any(|(d, _)| d.mapped_ids.might_contain_hash(id.get_hash()))
     }
 
     /// Returns the registered `@keyframes` animation for the specified name.
     #[inline]
     pub fn get_animation(&self, name: &Atom) -> Option<&KeyframesAnimation> {
         self.cascade_data
             .iter_origins()
             .filter_map(|(d, _)| d.animations.get(name))
             .next()
     }
 
     /// Computes the match results of a given element against the set of
     /// revalidation selectors.
     pub fn match_revalidation_selectors<E, F>(
         &self,
-        element: E,
+        element: &E,
         bloom: Option<&BloomFilter>,
         nth_index_cache: &mut NthIndexCache,
         flags_setter: &mut F
     ) -> SmallBitVec
     where
         E: TElement,
         F: FnMut(&E, ElementSelectorFlags),
     {
@@ -1468,50 +1449,32 @@ impl Stylist {
         // Note that, by the time we're revalidating, we're guaranteed that the
         // candidate and the entry have the same id, classes, and local name.
         // This means we're guaranteed to get the same rulehash buckets for all
         // the lookups, which means that the bitvecs are comparable. We verify
         // this in the caller by asserting that the bitvecs are same-length.
         let mut results = SmallBitVec::new();
         for (data, _) in self.cascade_data.iter_origins() {
             data.selectors_for_cache_revalidation.lookup(
-                element,
+                *element,
                 self.quirks_mode,
                 &mut |selector_and_hashes| {
                     results.push(matches_selector(
                         &selector_and_hashes.selector,
                         selector_and_hashes.selector_offset,
                         Some(&selector_and_hashes.hashes),
-                        &element,
+                        element,
                         &mut matching_context,
                         flags_setter
                     ));
                     true
                 }
             );
         }
 
-        element.each_xbl_stylist(|stylist| {
-            stylist.cascade_data.author.selectors_for_cache_revalidation.lookup(
-                element,
-                stylist.quirks_mode,
-                &mut |selector_and_hashes| {
-                    results.push(matches_selector(
-                        &selector_and_hashes.selector,
-                        selector_and_hashes.selector_offset,
-                        Some(&selector_and_hashes.hashes),
-                        &element,
-                        &mut matching_context,
-                        flags_setter
-                    ));
-                    true
-                }
-            );
-        });
-
         results
     }
 
     /// Computes styles for a given declaration with parent_style.
     pub fn compute_for_declarations(
         &self,
         guards: &StylesheetGuards,
         parent_style: &ComputedValues,