From 15e71c5670244b77d3141db2c7e9aac3e5bffcb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 28 Aug 2017 11:21:14 +0200 Subject: [PATCH] style: Don't look for viewport units in stylesheets. Use whether we've computed any viewport unit instead. This is more accurate (we avoid restyling unnecessarily if we've found anything ever on the stylesheet, but that hasn't matched). This has the benefit of also matching Gecko, and simplify some code and fishyness around, and also hopefully speeding up stylesheet parsing. --- components/layout_thread/lib.rs | 3 +- components/script/dom/htmlmetaelement.rs | 4 -- components/script/stylesheet_loader.rs | 1 - components/style/gecko/media_queries.rs | 8 +++- components/style/servo/media_queries.rs | 32 +++++++++----- components/style/stylesheets/stylesheet.rs | 42 ++----------------- components/style/stylesheets/viewport_rule.rs | 4 -- components/style/values/computed/length.rs | 4 +- components/style/values/computed/mod.rs | 6 +-- tests/unit/style/stylesheets.rs | 1 - 10 files changed, 39 insertions(+), 66 deletions(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index d592b28391c..0517c504b26 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -1143,6 +1143,7 @@ impl LayoutThread { let document_shared_lock = document.style_shared_lock(); self.document_shared_lock = Some(document_shared_lock.clone()); let author_guard = document_shared_lock.read(); + let had_used_viewport_units = self.stylist.device().used_viewport_units(); let device = Device::new(MediaType::screen(), initial_viewport, device_pixel_ratio); let sheet_origins_affected_by_device_change = self.stylist.set_device(device, &author_guard); @@ -1165,7 +1166,7 @@ impl LayoutThread { .send(ConstellationMsg::ViewportConstrained(self.id, constraints.clone())) .unwrap(); } - if self.stylist.iter_stylesheets().any(|sheet| sheet.0.dirty_on_viewport_size_change()) { + if had_used_viewport_units { let mut iter = element.as_node().traverse_preorder(); let mut next = iter.next(); diff --git a/components/script/dom/htmlmetaelement.rs b/components/script/dom/htmlmetaelement.rs index d2cb332e7e4..5e2d9c1c0b4 100644 --- a/components/script/dom/htmlmetaelement.rs +++ b/components/script/dom/htmlmetaelement.rs @@ -109,10 +109,6 @@ impl HTMLMetaElement { namespaces: Default::default(), quirks_mode: document.quirks_mode(), url_data: RwLock::new(window_from_node(self).get_url()), - // Viewport constraints are always recomputed on - // resize; they don't need to force all styles to be - // recomputed. - dirty_on_viewport_size_change: AtomicBool::new(false), source_map_url: RwLock::new(None), }, media: Arc::new(shared_lock.wrap(MediaList::empty())), diff --git a/components/script/stylesheet_loader.rs b/components/script/stylesheet_loader.rs index 2271a8bba0a..1e6b2470e5c 100644 --- a/components/script/stylesheet_loader.rs +++ b/components/script/stylesheet_loader.rs @@ -289,7 +289,6 @@ impl<'a> StyleStylesheetLoader for StylesheetLoader<'a> { rules: CssRules::new(Vec::new(), lock), origin: context.stylesheet_origin, url_data: RwLock::new(context.url_data.clone()), - dirty_on_viewport_size_change: AtomicBool::new(false), quirks_mode: context.quirks_mode, namespaces: RwLock::new(Namespaces::default()), source_map_url: RwLock::new(None), diff --git a/components/style/gecko/media_queries.rs b/components/style/gecko/media_queries.rs index 9736c595ac4..83935752697 100644 --- a/components/style/gecko/media_queries.rs +++ b/components/style/gecko/media_queries.rs @@ -156,7 +156,6 @@ impl Device { /// Returns the current viewport size in app units. pub fn au_viewport_size(&self) -> Size2D { - self.used_viewport_size.store(true, Ordering::Relaxed); unsafe { // TODO(emilio): Need to take into account scrollbars. let area = &self.pres_context().mVisibleArea; @@ -164,6 +163,13 @@ impl Device { } } + /// Returns the current viewport size in app units, recording that it's been + /// used for viewport unit resolution. + pub fn au_viewport_size_for_viewport_unit_resolution(&self) -> Size2D { + self.used_viewport_size.store(true, Ordering::Relaxed); + self.au_viewport_size() + } + /// Returns whether we ever looked up the viewport size of the Device. pub fn used_viewport_size(&self) -> bool { self.used_viewport_size.load(Ordering::Relaxed) diff --git a/components/style/servo/media_queries.rs b/components/style/servo/media_queries.rs index 4a75a112906..437a7332240 100644 --- a/components/style/servo/media_queries.rs +++ b/components/style/servo/media_queries.rs @@ -48,21 +48,26 @@ pub struct Device { /// by using rem units. #[ignore_heap_size_of = "Pure stack type"] used_root_font_size: AtomicBool, + /// Whether any styles computed in the document relied on the viewport size. + #[ignore_heap_size_of = "Pure stack type"] + used_viewport_units: AtomicBool, } impl Device { /// Trivially construct a new `Device`. - pub fn new(media_type: MediaType, - viewport_size: TypedSize2D, - device_pixel_ratio: ScaleFactor) - -> Device { + pub fn new( + media_type: MediaType, + viewport_size: TypedSize2D, + device_pixel_ratio: ScaleFactor + ) -> Device { Device { - media_type: media_type, - viewport_size: viewport_size, - device_pixel_ratio: device_pixel_ratio, + media_type, + viewport_size, + device_pixel_ratio, // FIXME(bz): Seems dubious? root_font_size: AtomicIsize::new(font_size::get_initial_value().value() as isize), used_root_font_size: AtomicBool::new(false), + used_viewport_units: AtomicBool::new(false), } } @@ -98,10 +103,15 @@ impl Device { Au::from_f32_px(self.viewport_size.height)) } - /// Returns the viewport size in pixels. - #[inline] - pub fn px_viewport_size(&self) -> TypedSize2D { - self.viewport_size + /// Like the above, but records that we've used viewport units. + pub fn au_viewport_size_for_viewport_unit_resolution(&self) -> Size2D { + self.used_viewport_units.store(true, Ordering::Relaxed); + self.au_viewport_size() + } + + /// Whether viewport units were used since the last device change. + pub fn used_viewport_units(&self) -> bool { + self.used_viewport_units.load(Ordering::Relaxed) } /// Returns the device pixel ratio. diff --git a/components/style/stylesheets/stylesheet.rs b/components/style/stylesheets/stylesheet.rs index bf219f691bd..7080ccfa09a 100644 --- a/components/style/stylesheets/stylesheet.rs +++ b/components/style/stylesheets/stylesheet.rs @@ -58,8 +58,6 @@ pub struct StylesheetContents { pub namespaces: RwLock, /// The quirks mode of this stylesheet. pub quirks_mode: QuirksMode, - /// Whether this stylesheet would be dirty when the viewport size changes. - pub dirty_on_viewport_size_change: AtomicBool, /// This stylesheet's source map URL. pub source_map_url: RwLock>, } @@ -78,7 +76,7 @@ impl StylesheetContents { line_number_offset: u64 ) -> Self { let namespaces = RwLock::new(Namespaces::default()); - let (rules, dirty_on_viewport_size_change, source_map_url) = Stylesheet::parse_rules( + let (rules, source_map_url) = Stylesheet::parse_rules( css, &url_data, origin, @@ -95,7 +93,6 @@ impl StylesheetContents { origin: origin, url_data: RwLock::new(url_data), namespaces: namespaces, - dirty_on_viewport_size_change: AtomicBool::new(dirty_on_viewport_size_change), quirks_mode: quirks_mode, source_map_url: RwLock::new(source_map_url), } @@ -132,12 +129,8 @@ impl DeepCloneWithLock for StylesheetContents { self.rules.read_with(guard) .deep_clone_with_lock(lock, guard, params); - let dirty_on_viewport_size_change = - AtomicBool::new(self.dirty_on_viewport_size_change.load(Ordering::Relaxed)); - Self { rules: Arc::new(lock.wrap(rules)), - dirty_on_viewport_size_change, quirks_mode: self.quirks_mode, origin: self.origin, url_data: RwLock::new((*self.url_data.read()).clone()), @@ -322,7 +315,7 @@ impl Stylesheet { where R: ParseErrorReporter { let namespaces = RwLock::new(Namespaces::default()); - let (rules, dirty_on_viewport_size_change, source_map_url) = + let (rules, source_map_url) = Stylesheet::parse_rules( css, &url_data, @@ -340,8 +333,6 @@ impl Stylesheet { &mut *existing.contents.namespaces.write(), &mut *namespaces.write() ); - existing.contents.dirty_on_viewport_size_change - .store(dirty_on_viewport_size_change, Ordering::Release); // Acquire the lock *after* parsing, to minimize the exclusive section. let mut guard = existing.shared_lock.write(); @@ -359,7 +350,7 @@ impl Stylesheet { error_reporter: &R, quirks_mode: QuirksMode, line_number_offset: u64 - ) -> (Vec, bool, Option) { + ) -> (Vec, Option) { let mut rules = Vec::new(); let mut input = ParserInput::new(css); let mut input = Parser::new(&mut input); @@ -385,8 +376,6 @@ impl Stylesheet { namespaces: namespaces, }; - input.look_for_viewport_percentages(); - { let mut iter = RuleListParser::new_for_stylesheet(&mut input, rule_parser); @@ -404,7 +393,7 @@ impl Stylesheet { } let source_map_url = input.current_source_map_url().map(String::from); - (rules, input.seen_viewport_percentages(), source_map_url) + (rules, source_map_url) } /// Creates an empty stylesheet and parses it with a given base url, origin @@ -443,29 +432,6 @@ impl Stylesheet { } } - /// Whether this stylesheet can be dirty on viewport size change. - pub fn dirty_on_viewport_size_change(&self) -> bool { - self.contents.dirty_on_viewport_size_change.load(Ordering::SeqCst) - } - - /// When CSSOM inserts a rule or declaration into this stylesheet, it needs to call this method - /// with the return value of `cssparser::Parser::seen_viewport_percentages`. - /// - /// FIXME: actually make these calls - /// - /// Note: when *removing* a rule or declaration that contains a viewport percentage, - /// to keep the flag accurate we’d need to iterator through the rest of the stylesheet to - /// check for *other* such values. - /// - /// Instead, we conservatively assume there might be some. - /// Restyling will some some more work than necessary, but give correct results. - pub fn inserted_has_viewport_percentages(&self, has_viewport_percentages: bool) { - self.contents.dirty_on_viewport_size_change.fetch_or( - has_viewport_percentages, - Ordering::SeqCst - ); - } - /// Returns whether the stylesheet has been explicitly disabled through the /// CSSOM. pub fn disabled(&self) -> bool { diff --git a/components/style/stylesheets/viewport_rule.rs b/components/style/stylesheets/viewport_rule.rs index 1e58553e676..f7a37c232dc 100644 --- a/components/style/stylesheets/viewport_rule.rs +++ b/components/style/stylesheets/viewport_rule.rs @@ -700,10 +700,6 @@ impl MaybeNew for ViewportConstraints { // // Note: DEVICE-ADAPT § 5. states that relative length values are // resolved against initial values - // - // Note, we set used_viewport_size flag for Gecko in au_viewport_size. - // If we ever start supporting ViewportRule in Gecko, we probably want - // to avoid doing so at this place. let initial_viewport = device.au_viewport_size(); let provider = get_metrics_provider_for_product(); diff --git a/components/style/values/computed/length.rs b/components/style/values/computed/length.rs index a2145d37745..b4f4cfc5101 100644 --- a/components/style/values/computed/length.rs +++ b/components/style/values/computed/length.rs @@ -32,7 +32,7 @@ impl ToComputedValue for specified::NoCalcLength { specified::NoCalcLength::FontRelative(length) => length.to_computed_value(context, FontBaseSize::CurrentStyle), specified::NoCalcLength::ViewportPercentage(length) => - length.to_computed_value(context.viewport_size()), + length.to_computed_value(context.viewport_size_for_viewport_unit_resolution()), specified::NoCalcLength::ServoCharacterWidth(length) => length.to_computed_value(context.style().get_font().clone_font_size().0), #[cfg(feature = "gecko")] @@ -240,7 +240,7 @@ impl specified::CalcLengthOrPercentage { self.vmin.map(ViewportPercentageLength::Vmin), self.vmax.map(ViewportPercentageLength::Vmax)] { if let Some(val) = *val { - length += val.to_computed_value(context.viewport_size()); + length += val.to_computed_value(context.viewport_size_for_viewport_unit_resolution()); } } diff --git a/components/style/values/computed/mod.rs b/components/style/values/computed/mod.rs index 22d958f90a6..8a15ef1162c 100644 --- a/components/style/values/computed/mod.rs +++ b/components/style/values/computed/mod.rs @@ -127,9 +127,9 @@ impl<'a> Context<'a> { self.builder.device } - /// The current viewport size. - pub fn viewport_size(&self) -> Size2D { - self.builder.device.au_viewport_size() + /// The current viewport size, used to resolve viewport units. + pub fn viewport_size_for_viewport_unit_resolution(&self) -> Size2D { + self.builder.device.au_viewport_size_for_viewport_unit_resolution() } /// The default computed style we're getting our reset style from. diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index 6683b26377a..f64b7bce622 100644 --- a/tests/unit/style/stylesheets.rs +++ b/tests/unit/style/stylesheets.rs @@ -78,7 +78,6 @@ fn test_parse_stylesheet() { origin: Origin::UserAgent, namespaces: RwLock::new(namespaces), url_data: RwLock::new(url), - dirty_on_viewport_size_change: AtomicBool::new(false), quirks_mode: QuirksMode::NoQuirks, rules: CssRules::new(vec![ CssRule::Namespace(Arc::new(stylesheet.shared_lock.wrap(NamespaceRule {