From 77c253fd43a8ef1723c48709b5fececa742ba4fa Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 4 Nov 2015 17:14:04 -0800 Subject: [PATCH 1/3] Load web fonts synchronously during wpt. --- components/layout/layout_task.rs | 16 ++++++++++++---- components/util/opts.rs | 11 +++++++++++ .../harness/wptrunner/executors/executorservo.py | 2 +- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index c86ecdedaff..7a615b02f6b 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -326,10 +326,18 @@ fn add_font_face_rules(stylesheet: &Stylesheet, outstanding_web_fonts_counter: &Arc) { for font_face in stylesheet.effective_rules(&device).font_face() { for source in &font_face.sources { - 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()); + 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()); + } } } } diff --git a/components/util/opts.rs b/components/util/opts.rs index fb68c24964d..6d91e0d6c42 100644 --- a/components/util/opts.rs +++ b/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!(""); @@ -414,6 +423,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, @@ -632,6 +642,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, diff --git a/tests/wpt/harness/wptrunner/executors/executorservo.py b/tests/wpt/harness/wptrunner/executors/executorservo.py index a1e0ea97b31..b1beea9934a 100644 --- a/tests/wpt/harness/wptrunner/executors/executorservo.py +++ b/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) From d89816bb5fa90421a4dea49ff6387f635ce7b389 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 4 Nov 2015 18:15:30 -0800 Subject: [PATCH 2/3] Consider all the pseudo-classes in a given compound selector when computing restyle hints, not just the rightmost one. For some reason when I wrote this code I mixed up the rules for pseudo-elements (rightmost, maximum of one) with those of pseudo-classes (which have no such restriction). This caused a correctness issue which can be demonstrated with the associated reftest modification. --- components/style/restyle_hints.rs | 19 ++++++++++--------- .../mozilla/tests/css/restyle_hints_state.css | 8 ++++++-- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index acb521b0f10..fe143c3e315 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -220,15 +220,16 @@ impl StateDependencySet { let mut cur = selector; let mut combinator: Option = None; loop { - if let Some(rightmost) = cur.simple_selectors.last() { - let state_dep = selector_to_state(rightmost); - if !state_dep.is_empty() { - self.deps.push(StateDependency { - selector: cur.clone(), - combinator: combinator, - state: state_dep, - }); - } + let mut deps = ElementState::empty(); + for s in &cur.simple_selectors { + deps.insert(selector_to_state(s)); + } + if !deps.is_empty() { + self.deps.push(StateDependency { + selector: cur.clone(), + combinator: combinator, + state: deps, + }); } cur = match cur.next { diff --git a/tests/wpt/mozilla/tests/css/restyle_hints_state.css b/tests/wpt/mozilla/tests/css/restyle_hints_state.css index 673ac25d572..6a152778e42 100644 --- a/tests/wpt/mozilla/tests/css/restyle_hints_state.css +++ b/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; } From 7dba4447f196bbd75676402dac7858b0f7741cd2 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Wed, 4 Nov 2015 18:31:17 -0800 Subject: [PATCH 3/3] Store pristine element state rather than a set of changes. This is the strategy we'll need to take for attributes, and so this change puts us in a position to handle attributes and state the same way. This does mean that we stop taking care to track the situations where our state has reverted to the original state, with no net change. I think that's probably of negligible value though. --- components/layout/layout_task.rs | 7 +++--- components/layout/wrapper.rs | 6 +++--- components/script/dom/document.rs | 31 ++++++++++----------------- components/script/dom/element.rs | 5 ++--- components/style/restyle_hints.rs | 5 ++--- components/style/selector_matching.rs | 4 ++-- 6 files changed, 23 insertions(+), 35 deletions(-) diff --git a/components/layout/layout_task.rs b/components/layout/layout_task.rs index 7a615b02f6b..bdf1b613562 100644 --- a/components/layout/layout_task.rs +++ b/components/layout/layout_task.rs @@ -1213,13 +1213,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() { - debug_assert!(!state_change.is_empty()); + for &(el, old_state) in modified_elements.iter() { let hint = rw_data.stylist.restyle_hint_for_state_change(&el, el.get_state(), - state_change); + old_state); el.note_restyle_hint(hint); } } diff --git a/components/layout/wrapper.rs b/components/layout/wrapper.rs index 04d3470fb64..ae8462426e8 100644 --- a/components/layout/wrapper.rs +++ b/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(); - Vec::from_iter(changes.iter().map(|&(el, state)| + let elements = self.document.drain_modified_elements(); + Vec::from_iter(elements.iter().map(|&(el, state)| (LayoutElement { element: el, chain: PhantomData, diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 90e5697dd33..161a5639ceb 100644 --- a/components/script/dom/document.rs +++ b/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>, - /// The collection of ElementStates that have been changed since the last restyle. - element_state_changes: DOMRefCell, ElementState>>, + /// For each element that has had a state change since the last restyle, track the original state. + modified_elements: DOMRefCell, ElementState>>, /// http://w3c.github.io/touch-events/#dfn-active-touch-point active_touch_points: DOMRefCell>>, } @@ -308,7 +308,7 @@ impl Document { pub fn needs_reflow(&self) -> bool { self.GetDocumentElement().is_some() && - (self.upcast::().get_has_dirty_descendants() || !self.element_state_changes.borrow().is_empty()) + (self.upcast::().get_has_dirty_descendants() || !self.modified_elements.borrow().is_empty()) } /// Returns the first `base` element in the DOM that has an `href` attribute. @@ -1230,7 +1230,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, ElementState)>; + unsafe fn drain_modified_elements(&self) -> Vec<(LayoutJS, ElementState)>; } #[allow(unsafe_code)] @@ -1242,9 +1242,9 @@ impl LayoutDocumentHelpers for LayoutJS { #[inline] #[allow(unrooted_must_root)] - unsafe fn drain_element_state_changes(&self) -> Vec<(LayoutJS, ElementState)> { - let mut changes = (*self.unsafe_get()).element_state_changes.borrow_mut_for_layout(); - let drain = changes.drain(); + unsafe fn drain_modified_elements(&self) -> Vec<(LayoutJS, ElementState)> { + let mut elements = (*self.unsafe_get()).modified_elements.borrow_mut_for_layout(); + let drain = elements.drain(); let layout_drain = drain.map(|(k, v)| (k.to_layout(), v)); Vec::from_iter(layout_drain) } @@ -1313,7 +1313,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()), } } @@ -1380,18 +1380,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) { - let mut map = self.element_state_changes.borrow_mut(); - let empty; - { - 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)); - } + pub fn element_state_will_change(&self, el: &Element) { + let mut map = self.modified_elements.borrow_mut(); + map.entry(JS::from_ref(el)).or_insert(el.get_state()); } } diff --git a/components/script/dom/element.rs b/components/script/dom/element.rs index 7d605ecd133..d420e9b5405 100644 --- a/components/script/dom/element.rs +++ b/components/script/dom/element.rs @@ -1814,14 +1814,13 @@ impl Element { if state.contains(which) == value { return } + let node = self.upcast::(); + 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.owner_doc().record_element_state_change(self, which); } pub fn get_active_state(&self) -> bool { diff --git a/components/style/restyle_hints.rs b/components/style/restyle_hints.rs index fe143c3e315..c180c13dcde 100644 --- a/components/style/restyle_hints.rs +++ b/components/style/restyle_hints.rs @@ -195,11 +195,10 @@ impl StateDependencySet { StateDependencySet { deps: Vec::new() } } - pub fn compute_hint(&self, el: &E, current_state: ElementState, state_changes: ElementState) + pub fn compute_hint(&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; - old_state.toggle(state_changes); + let state_changes = current_state ^ old_state; for dep in &self.deps { if state_changes.intersects(dep.state) { let old_el: ElementWrapper = ElementWrapper::new_with_override(el.clone(), old_state); diff --git a/components/style/selector_matching.rs b/components/style/selector_matching.rs index 53f1a0d3589..8cc56e693eb 100644 --- a/components/style/selector_matching.rs +++ b/components/style/selector_matching.rs @@ -174,10 +174,10 @@ impl Stylist { pub fn restyle_hint_for_state_change(&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) {