From 85996826ab7493703b1953d6784c2835347f1896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 18 Aug 2017 17:41:47 +0200 Subject: [PATCH 1/3] style: Remove Stylist::is_device_dirty. More progress on unifying how Gecko and Servo track stylist dirtiness. --- components/layout_thread/lib.rs | 100 +++++++++++++++----------------- components/style/stylist.rs | 83 ++++++++++++-------------- 2 files changed, 84 insertions(+), 99 deletions(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 1a6bd945caf..097cf74a2ac 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -137,7 +137,7 @@ use style::selector_parser::SnapshotMap; use style::servo::restyle_damage::{REFLOW, REFLOW_OUT_OF_FLOW, REPAINT, REPOSITION, STORE_OVERFLOW}; use style::shared_lock::{SharedRwLock, SharedRwLockReadGuard, StylesheetGuards}; use style::stylesheet_set::StylesheetSet; -use style::stylesheets::{Origin, OriginSet, Stylesheet, StylesheetContents, StylesheetInDocument, UserAgentStylesheets}; +use style::stylesheets::{Origin, Stylesheet, StylesheetContents, StylesheetInDocument, UserAgentStylesheets}; use style::stylist::Stylist; use style::thread_state; use style::timer::Timer; @@ -434,20 +434,10 @@ impl<'a, 'b: 'a> RwData<'a, 'b> { /// use `block`. fn lock(&mut self) -> RWGuard<'b> { match self.possibly_locked_rw_data.take() { - None => RWGuard::Used(self.rw_data.lock().unwrap()), + None => RWGuard::Used(self.rw_data.lock().unwrap()), Some(x) => RWGuard::Held(x), } } - - /// If no reflow has ever been triggered, this will keep the lock, locked - /// (and saved in `possibly_locked_rw_data`). If it has been, the lock will - /// be unlocked. - fn block(&mut self, rw_data: RWGuard<'b>) { - match rw_data { - RWGuard::Used(x) => drop(x), - RWGuard::Held(x) => *self.possibly_locked_rw_data = Some(x), - } - } } fn add_font_face_rules(stylesheet: &Stylesheet, @@ -526,18 +516,6 @@ impl LayoutThread { let font_cache_receiver = ROUTER.route_ipc_receiver_to_new_mpsc_receiver(ipc_font_cache_receiver); - let stylist = Stylist::new(device, QuirksMode::NoQuirks); - let outstanding_web_fonts_counter = Arc::new(AtomicUsize::new(0)); - let ua_stylesheets = &*UA_STYLESHEETS; - let guard = ua_stylesheets.shared_lock.read(); - for stylesheet in &ua_stylesheets.user_or_user_agent_stylesheets { - add_font_face_rules(stylesheet, - &guard, - stylist.device(), - &font_cache_thread, - &ipc_font_cache_sender, - &outstanding_web_fonts_counter); - } LayoutThread { id: id, @@ -561,7 +539,7 @@ impl LayoutThread { generation: Cell::new(0), new_animations_sender: new_animations_sender, new_animations_receiver: new_animations_receiver, - outstanding_web_fonts: outstanding_web_fonts_counter, + outstanding_web_fonts: Arc::new(AtomicUsize::new(0)), root_flow: RefCell::new(None), document_shared_lock: None, running_animations: ServoArc::new(RwLock::new(FnvHashMap::default())), @@ -570,7 +548,7 @@ impl LayoutThread { viewport_size: Size2D::new(Au(0), Au(0)), webrender_api: webrender_api_sender.create_api(), webrender_document, - stylist, + stylist: Stylist::new(device, QuirksMode::NoQuirks), stylesheets: StylesheetSet::new(), rw_data: Arc::new(Mutex::new( LayoutThreadData { @@ -718,12 +696,7 @@ impl LayoutThread { match request { Msg::AddStylesheet(stylesheet, before_stylesheet) => { let guard = stylesheet.shared_lock.read(); - - self.handle_add_stylesheet( - &stylesheet, - &guard, - possibly_locked_rw_data, - ); + self.handle_add_stylesheet(&stylesheet, &guard); match before_stylesheet { Some(insertion_point) => { @@ -916,16 +889,13 @@ impl LayoutThread { let _ = self.parallel_traversal.take(); } - fn handle_add_stylesheet<'a, 'b>( + fn handle_add_stylesheet( &self, - stylesheet: &ServoArc, + stylesheet: &Stylesheet, guard: &SharedRwLockReadGuard, - possibly_locked_rw_data: &mut RwData<'a, 'b>, ) { // Find all font-face rules and notify the font cache of them. // GWTODO: Need to handle unloading web fonts. - - let rw_data = possibly_locked_rw_data.lock(); if stylesheet.is_effective_for_device(self.stylist.device(), &guard) { add_font_face_rules(&*stylesheet, &guard, @@ -934,8 +904,6 @@ impl LayoutThread { &self.font_cache_sender, &self.outstanding_web_fonts); } - - possibly_locked_rw_data.block(rw_data); } /// Advances the animation clock of the document. @@ -1215,7 +1183,8 @@ impl LayoutThread { self.document_shared_lock = Some(document_shared_lock.clone()); let author_guard = document_shared_lock.read(); let device = Device::new(MediaType::screen(), initial_viewport, device_pixel_ratio); - self.stylist.set_device(device, &author_guard, self.stylesheets.iter()); + let sheet_origins_affected_by_device_change = + self.stylist.set_device(device, &author_guard, self.stylesheets.iter()); self.viewport_size = self.stylist.viewport_constraints().map_or(current_screen_size, |constraints| { @@ -1269,20 +1238,45 @@ impl LayoutThread { }; let needs_dirtying = { - let mut extra_data = Default::default(); - let (iter, mut origins_dirty) = self.stylesheets.flush(Some(element)); - if data.stylesheets_changed { - origins_dirty = OriginSet::all(); + debug!("Flushing stylist"); + + let mut origins_dirty = sheet_origins_affected_by_device_change; + + debug!("Device changes: {:?}", origins_dirty); + + if self.first_reflow.get() { + debug!("First reflow, rebuilding user and UA rules"); + + origins_dirty |= Origin::User; + origins_dirty |= Origin::UserAgent; + for stylesheet in &ua_stylesheets.user_or_user_agent_stylesheets { + self.handle_add_stylesheet(stylesheet, &ua_or_user_guard); + } } - let origins_rebuilt = self.stylist.rebuild( - iter, - &guards, - Some(ua_stylesheets), - /* author_style_disabled = */ false, - &mut extra_data, - origins_dirty, - ); - !origins_rebuilt.is_empty() + + let (iter, invalidation_origins_dirty) = self.stylesheets.flush(Some(element)); + debug!("invalidation: {:?}", invalidation_origins_dirty); + + origins_dirty |= invalidation_origins_dirty; + + if data.stylesheets_changed { + debug!("Doc sheets changed, flushing author sheets too"); + origins_dirty |= Origin::Author; + } + + if !origins_dirty.is_empty() { + let mut extra_data = Default::default(); + self.stylist.rebuild( + iter, + &guards, + Some(ua_stylesheets), + /* author_style_disabled = */ false, + &mut extra_data, + origins_dirty, + ); + } + + !origins_dirty.is_empty() }; let needs_reflow = viewport_size_changed && !needs_dirtying; diff --git a/components/style/stylist.rs b/components/style/stylist.rs index 93d538d4c9c..e12dfe31a83 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -78,9 +78,6 @@ pub struct Stylist { #[cfg_attr(feature = "servo", ignore_heap_size_of = "defined in selectors")] quirks_mode: QuirksMode, - /// If true, the device has changed, and the stylist needs to be updated. - is_device_dirty: bool, - /// Selector maps for all of the style sheets in the stylist, after /// evalutaing media rules against the current device, split out per /// cascade level. @@ -130,7 +127,6 @@ impl Stylist { Stylist { viewport_constraints: None, device: device, - is_device_dirty: true, quirks_mode: quirks_mode, cascade_data: Default::default(), @@ -184,10 +180,6 @@ impl Stylist { /// Rebuild the stylist for the given document stylesheets, and optionally /// with a set of user agent stylesheets. - /// - /// This method resets all the style data each time the stylesheets change - /// (which is indicated by the `stylesheets_changed` parameter), or the - /// device is dirty, which means we need to re-evaluate media queries. pub fn rebuild<'a, I, S>( &mut self, doc_stylesheets: I, @@ -195,19 +187,13 @@ impl Stylist { ua_stylesheets: Option<&UserAgentStylesheets>, author_style_disabled: bool, extra_data: &mut PerOrigin, - mut origins_to_rebuild: OriginSet, - ) -> OriginSet + origins_to_rebuild: OriginSet, + ) where I: Iterator + Clone, S: StylesheetInDocument + ToMediaListKey + 'static, { - if self.is_device_dirty { - origins_to_rebuild = OriginSet::all(); - } - - if origins_to_rebuild.is_empty() { - return origins_to_rebuild; - } + debug_assert!(!origins_to_rebuild.is_empty()); self.num_rebuilds += 1; @@ -252,19 +238,40 @@ impl Stylist { if let Some(ua_stylesheets) = ua_stylesheets { for stylesheet in &ua_stylesheets.user_or_user_agent_stylesheets { + let sheet_origin = + stylesheet.contents(guards.ua_or_user).origin; + debug_assert!(matches!( - stylesheet.contents(guards.ua_or_user).origin, - Origin::UserAgent | Origin::User)); - self.add_stylesheet(stylesheet, guards.ua_or_user, extra_data); + sheet_origin, + Origin::UserAgent | Origin::User + )); + + if origins_to_rebuild.contains(sheet_origin.into()) { + self.add_stylesheet( + stylesheet, + guards.ua_or_user, + extra_data + ); + } } if self.quirks_mode != QuirksMode::NoQuirks { let stylesheet = &ua_stylesheets.quirks_mode_stylesheet; + let sheet_origin = + stylesheet.contents(guards.ua_or_user).origin; + debug_assert!(matches!( - stylesheet.contents(guards.ua_or_user).origin, - Origin::UserAgent | Origin::User)); - self.add_stylesheet(&ua_stylesheets.quirks_mode_stylesheet, - guards.ua_or_user, extra_data); + sheet_origin, + Origin::UserAgent | Origin::User + )); + + if origins_to_rebuild.contains(sheet_origin.into()) { + self.add_stylesheet( + &ua_stylesheets.quirks_mode_stylesheet, + guards.ua_or_user, + extra_data + ); + } } } @@ -280,9 +287,6 @@ impl Stylist { for stylesheet in sheets_to_add { self.add_stylesheet(stylesheet, guards.author, extra_data); } - - self.is_device_dirty = false; - origins_to_rebuild } fn add_stylesheet( @@ -831,18 +835,14 @@ impl Stylist { /// Set a given device, which may change the styles that apply to the /// document. /// + /// Returns the sheet origins that were actually affected. + /// /// This means that we may need to rebuild style data even if the /// stylesheets haven't changed. /// /// Also, the device that arrives here may need to take the viewport rules /// into account. /// - /// TODO(emilio): Probably should be unified with `update`, right now I - /// don't think we take into account dynamic updates to viewport rules. - /// - /// Probably worth to make the stylist own a single `Device`, and have a - /// `update_device` function? - /// /// feature = "servo" because gecko only has one device, and manually tracks /// when the device is dirty. /// @@ -854,7 +854,7 @@ impl Stylist { mut device: Device, guard: &SharedRwLockReadGuard, stylesheets: I, - ) + ) -> OriginSet where I: Iterator + Clone, S: StylesheetInDocument + ToMediaListKey + 'static, @@ -875,11 +875,10 @@ impl Stylist { } self.device = device; - let features_changed = self.media_features_change_changed_style( + self.media_features_change_changed_style( stylesheets, - guard - ); - self.is_device_dirty |= !features_changed.is_empty(); + guard, + ) } /// Returns whether, given a media feature change, any previously-applicable @@ -1067,7 +1066,6 @@ impl Stylist { V: Push + VecLike + Debug, F: FnMut(&E, ElementSelectorFlags), { - debug_assert!(!self.is_device_dirty); // Gecko definitely has pseudo-elements with style attributes, like // ::-moz-color-swatch. debug_assert!(cfg!(feature = "gecko") || @@ -1217,13 +1215,6 @@ impl Stylist { .any(|(d, _)| d.mapped_ids.might_contain_hash(id.get_hash())) } - /// Return whether the device is dirty, that is, whether the screen size or - /// media type have changed (for now). - #[inline] - pub fn is_device_dirty(&self) -> bool { - self.is_device_dirty - } - /// Returns the registered `@keyframes` animation for the specified name. #[inline] pub fn get_animation(&self, name: &Atom) -> Option<&KeyframesAnimation> { From 359ef7b7c3dad79de34a3465b88e03d771979ee7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sat, 19 Aug 2017 19:27:33 +0200 Subject: [PATCH 2/3] Don't rely on ahem being loaded sync in offset_properties_inline.html --- tests/wpt/mozilla/meta/MANIFEST.json | 2 +- tests/wpt/mozilla/tests/css/offset_properties_inline.html | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/wpt/mozilla/meta/MANIFEST.json b/tests/wpt/mozilla/meta/MANIFEST.json index f39a8b44904..7623a1a6a46 100644 --- a/tests/wpt/mozilla/meta/MANIFEST.json +++ b/tests/wpt/mozilla/meta/MANIFEST.json @@ -24766,7 +24766,7 @@ "support" ], "css/offset_properties_inline.html": [ - "e879ede3d52eed5ebdd90e988380ba4fa365817a", + "ed9cf0062ff6b08dc0cc578a0ae6acce146bec10", "testharness" ], "css/ol_japanese_iroha_a.html": [ diff --git a/tests/wpt/mozilla/tests/css/offset_properties_inline.html b/tests/wpt/mozilla/tests/css/offset_properties_inline.html index cb88fa3a246..9b93cde7fc0 100644 --- a/tests/wpt/mozilla/tests/css/offset_properties_inline.html +++ b/tests/wpt/mozilla/tests/css/offset_properties_inline.html @@ -30,6 +30,7 @@ --> From 293274fa5ef6e1104e97b7db788a2846b01e2e11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 20 Aug 2017 23:37:38 +0200 Subject: [PATCH 3/3] style: Make sure to set the initial value of the transition even if we don't start it. Otherwise we may get to the end of it directly, which is far from what we want. --- components/style/animation.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/components/style/animation.rs b/components/style/animation.rs index c1a49fca7a1..6736d39269f 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -437,21 +437,23 @@ pub fn start_transitions_if_applicable(new_animations_sender: &Sender old_style, Arc::make_mut(new_style)); for property_animation in property_animations { - // Per [1], don't trigger a new transition if the end state for that transition is - // the same as that of a transition that's already running on the same node. - // - // [1]: https://drafts.csswg.org/css-transitions/#starting - if possibly_expired_animations.iter().any(|animation| { - animation.has_the_same_end_value_as(&property_animation) - }) { - continue - } - // Set the property to the initial value. + // // NB: get_mut is guaranteed to succeed since we called make_mut() // above. property_animation.update(Arc::get_mut(new_style).unwrap(), 0.0); + // Per [1], don't trigger a new transition if the end state for that + // transition is the same as that of a transition that's already + // running on the same node. + // + // [1]: https://drafts.csswg.org/css-transitions/#starting + if possibly_expired_animations.iter().any(|animation| { + animation.has_the_same_end_value_as(&property_animation) + }) { + continue + } + // Kick off the animation. let box_style = new_style.get_box(); let now = timer.seconds();