Auto merge of #8341 - bholley:state_hint_selector_ordering, r=pcwalton

Fix restyle hints to handle non-last psuedo-selectors, and track pristine state values rather than changesets

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8341)
<!-- Reviewable:end -->

components/layout/layout_task.rs

@@ -323,12 +323,20 @@ fn add_font_face_rules(stylesheet: &Stylesheet,
                        outstanding_web_fonts_counter: &Arc<AtomicUsize>) {
     for font_face in stylesheet.effective_rules(&device).font_face() {
         for source in &font_face.sources {
+            if opts::get().load_webfonts_synchronously {
+                let (sender, receiver) = channel();
+                font_cache_task.add_web_font(font_face.family.clone(),
+                                             (*source).clone(),
+                                             sender);
+                receiver.recv().unwrap();
+            } else {
                 outstanding_web_fonts_counter.fetch_add(1, Ordering::SeqCst);
                 font_cache_task.add_web_font(font_face.family.clone(),
                                              (*source).clone(),
                                              (*font_cache_sender).clone());
             }
         }
+    }
 }
 
 impl LayoutTask {
@@ -1198,13 +1206,12 @@ impl LayoutTask {
             }
         }
 
-        let state_changes = document.drain_element_state_changes();
+        let modified_elements = document.drain_modified_elements();
         if !needs_dirtying {
-            for &(el, state_change) in state_changes.iter() {
+            for &(el, old_state) in modified_elements.iter() {
-                debug_assert!(!state_change.is_empty());
                 let hint = rw_data.stylist.restyle_hint_for_state_change(&el,
                                                                          el.get_state(),
-                                                                         state_change);
+                                                                         old_state);
                 el.note_restyle_hint(hint);
             }
         }

components/layout/wrapper.rs

@@ -375,10 +375,10 @@ impl<'le> LayoutDocument<'le> {
         self.as_node().children().find(LayoutNode::is_element)
     }
 
-    pub fn drain_element_state_changes(&self) -> Vec<(LayoutElement, ElementState)> {
+    pub fn drain_modified_elements(&self) -> Vec<(LayoutElement, ElementState)> {
         unsafe {
-            let changes = self.document.drain_element_state_changes();
+            let elements = self.document.drain_modified_elements();
-            Vec::from_iter(changes.iter().map(|&(el, state)|
+            Vec::from_iter(elements.iter().map(|&(el, state)|
                 (LayoutElement {
                     element: el,
                     chain: PhantomData,

components/script/dom/document.rs

@@ -175,8 +175,8 @@ pub struct Document {
     /// This field is set to the document itself for inert documents.
     /// https://html.spec.whatwg.org/multipage/#appropriate-template-contents-owner-document
     appropriate_template_contents_owner_document: MutNullableHeap<JS<Document>>,
-    /// The collection of ElementStates that have been changed since the last restyle.
+    /// For each element that has had a state change since the last restyle, track the original state.
-    element_state_changes: DOMRefCell<HashMap<JS<Element>, ElementState>>,
+    modified_elements: DOMRefCell<HashMap<JS<Element>, ElementState>>,
     /// http://w3c.github.io/touch-events/#dfn-active-touch-point
     active_touch_points: DOMRefCell<Vec<JS<Touch>>>,
 }
@@ -308,7 +308,7 @@ impl Document {
 
     pub fn needs_reflow(&self) -> bool {
         self.GetDocumentElement().is_some() &&
-        (self.upcast::<Node>().get_has_dirty_descendants() || !self.element_state_changes.borrow().is_empty())
+        (self.upcast::<Node>().get_has_dirty_descendants() || !self.modified_elements.borrow().is_empty())
     }
 
     /// Returns the first `base` element in the DOM that has an `href` attribute.
@@ -1239,7 +1239,7 @@ pub enum DocumentSource {
 #[allow(unsafe_code)]
 pub trait LayoutDocumentHelpers {
     unsafe fn is_html_document_for_layout(&self) -> bool;
-    unsafe fn drain_element_state_changes(&self) -> Vec<(LayoutJS<Element>, ElementState)>;
+    unsafe fn drain_modified_elements(&self) -> Vec<(LayoutJS<Element>, ElementState)>;
 }
 
 #[allow(unsafe_code)]
@@ -1251,9 +1251,9 @@ impl LayoutDocumentHelpers for LayoutJS<Document> {
 
     #[inline]
     #[allow(unrooted_must_root)]
-    unsafe fn drain_element_state_changes(&self) -> Vec<(LayoutJS<Element>, ElementState)> {
+    unsafe fn drain_modified_elements(&self) -> Vec<(LayoutJS<Element>, ElementState)> {
-        let mut changes = (*self.unsafe_get()).element_state_changes.borrow_mut_for_layout();
+        let mut elements = (*self.unsafe_get()).modified_elements.borrow_mut_for_layout();
-        let drain = changes.drain();
+        let drain = elements.drain();
         let layout_drain = drain.map(|(k, v)| (k.to_layout(), v));
         Vec::from_iter(layout_drain)
     }
@@ -1322,7 +1322,7 @@ impl Document {
             reflow_timeout: Cell::new(None),
             base_element: Default::default(),
             appropriate_template_contents_owner_document: Default::default(),
-            element_state_changes: DOMRefCell::new(HashMap::new()),
+            modified_elements: DOMRefCell::new(HashMap::new()),
             active_touch_points: DOMRefCell::new(Vec::new()),
         }
     }
@@ -1389,18 +1389,9 @@ impl Document {
         self.idmap.borrow().get(&id).map(|ref elements| Root::from_ref(&*(*elements)[0]))
     }
 
-    pub fn record_element_state_change(&self, el: &Element, which: ElementState) {
+    pub fn element_state_will_change(&self, el: &Element) {
-        let mut map = self.element_state_changes.borrow_mut();
+        let mut map = self.modified_elements.borrow_mut();
-        let empty;
+        map.entry(JS::from_ref(el)).or_insert(el.get_state());
-        {
-            let states = map.entry(JS::from_ref(el))
-                            .or_insert(ElementState::empty());
-            states.toggle(which);
-            empty = states.is_empty();
-        }
-        if empty {
-            map.remove(&JS::from_ref(el));
-        }
     }
 }
 

components/script/dom/element.rs

@@ -1814,14 +1814,13 @@ impl Element {
         if state.contains(which) == value {
             return
         }
+        let node = self.upcast::<Node>();
+        node.owner_doc().element_state_will_change(self);
         match value {
             true => state.insert(which),
             false => state.remove(which),
         };
         self.state.set(state);
-
-        let node = self.upcast::<Node>();
-        node.owner_doc().record_element_state_change(self, which);
     }
 
     pub fn get_active_state(&self) -> bool {

components/style/restyle_hints.rs

@@ -195,11 +195,10 @@ impl StateDependencySet {
         StateDependencySet { deps: Vec::new() }
     }
 
-    pub fn compute_hint<E>(&self, el: &E, current_state: ElementState, state_changes: ElementState)
+    pub fn compute_hint<E>(&self, el: &E, current_state: ElementState, old_state: ElementState)
                           -> RestyleHint where E: Element, E: Clone {
         let mut hint = RestyleHint::empty();
-        let mut old_state = current_state;
+        let state_changes = current_state ^ old_state;
-        old_state.toggle(state_changes);
         for dep in &self.deps {
             if state_changes.intersects(dep.state) {
                 let old_el: ElementWrapper<E> = ElementWrapper::new_with_override(el.clone(), old_state);
@@ -220,16 +219,17 @@ impl StateDependencySet {
         let mut cur = selector;
         let mut combinator: Option<Combinator> = None;
         loop {
-            if let Some(rightmost) = cur.simple_selectors.last() {
+            let mut deps = ElementState::empty();
-                let state_dep = selector_to_state(rightmost);
+            for s in &cur.simple_selectors {
-                if !state_dep.is_empty() {
+                deps.insert(selector_to_state(s));
+            }
+            if !deps.is_empty() {
                 self.deps.push(StateDependency {
                     selector: cur.clone(),
                     combinator: combinator,
-                        state: state_dep,
+                    state: deps,
                 });
             }
-            }
 
             cur = match cur.next {
                 Some((ref sel, comb)) => {

components/style/selector_matching.rs

@@ -174,10 +174,10 @@ impl Stylist {
 
     pub fn restyle_hint_for_state_change<E>(&self, element: &E,
                                             current_state: ElementState,
-                                            state_change: ElementState)
+                                            old_state: ElementState)
                                             -> RestyleHint
                                             where E: Element + Clone {
-        self.state_deps.compute_hint(element, current_state, state_change)
+        self.state_deps.compute_hint(element, current_state, old_state)
     }
 
     pub fn set_device(&mut self, device: Device) {

components/util/opts.rs

@@ -74,6 +74,9 @@ pub struct Opts {
     /// Log GC passes and their durations.
     pub gc_profile: bool,
 
+    /// Load web fonts synchronously to avoid non-deterministic network-driven reflows.
+    pub load_webfonts_synchronously: bool,
+
     pub headless: bool,
     pub hard_fail: bool,
 
@@ -267,6 +270,9 @@ pub struct DebugOptions {
 
     /// Log GC passes and their durations.
     pub gc_profile: bool,
+
+    /// Load web fonts synchronously to avoid non-deterministic network-driven reflows.
+    pub load_webfonts_synchronously: bool,
 }
 
 
@@ -301,6 +307,7 @@ impl DebugOptions {
                 "convert-mouse-to-touch" => debug_options.convert_mouse_to_touch = true,
                 "replace-surrogates" => debug_options.replace_surrogates = true,
                 "gc-profile" => debug_options.gc_profile = true,
+                "load-webfonts-synchronously" => debug_options.load_webfonts_synchronously = true,
                 "" => {},
                 _ => return Err(option)
             };
@@ -345,6 +352,8 @@ pub fn print_debug_usage(app: &str) -> ! {
     print_option("replace-surrogates", "Replace unpaires surrogates in DOM strings with U+FFFD. \
                                         See https://github.com/servo/servo/issues/6564");
     print_option("gc-profile", "Log GC passes and their durations.");
+    print_option("load-webfonts-synchronously",
+                 "Load web fonts synchronously to avoid non-deterministic network-driven reflows");
 
     println!("");
 
@@ -434,6 +443,7 @@ pub fn default_opts() -> Opts {
         output_file: None,
         replace_surrogates: false,
         gc_profile: false,
+        load_webfonts_synchronously: false,
         headless: true,
         hard_fail: true,
         bubble_inline_sizes_separately: false,
@@ -652,6 +662,7 @@ pub fn from_cmdline_args(args: &[String]) {
         output_file: opt_match.opt_str("o"),
         replace_surrogates: debug_options.replace_surrogates,
         gc_profile: debug_options.gc_profile,
+        load_webfonts_synchronously: debug_options.load_webfonts_synchronously,
         headless: opt_match.opt_present("z"),
         hard_fail: opt_match.opt_present("f"),
         bubble_inline_sizes_separately: bubble_inline_sizes_separately,

tests/wpt/harness/wptrunner/executors/executorservo.py

@@ -203,7 +203,7 @@ class ServoRefTestExecutor(ProcessTestExecutor):
             debug_args, command = browser_command(
                 self.binary,
                 [render_arg(self.browser.render_backend), "--hard-fail", "--exit",
-                 "-u", "Servo/wptrunner", "-Z", "disable-text-aa",
+                 "-u", "Servo/wptrunner", "-Z", "disable-text-aa,load-webfonts-synchronously",
                  "--output=%s" % output_path, full_url],
                 self.debug_info)
 

tests/wpt/mozilla/tests/css/restyle_hints_state.css

@@ -19,11 +19,15 @@ fieldset:enabled div {
 fieldset:enabled > div {
   background-color: yellow;
 }
-fieldset:enabled ~ div {
+
+/* Add an unnecessary :first-child to make sure that restyle hints see
+ * non-rightmost pseudo-selectors.
+ * */
+fieldset:enabled:first-child ~ div {
   color: pink;
   background-color: purple;
 }
-fieldset:enabled + div {
+fieldset:enabled:first-child + div {
   color: brown;
   background-color: orange;
 }