Auto merge of #18267 - emilio:dirty-on-viewport-size-change, r=SimonSapin

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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18267)
<!-- Reviewable:end -->

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();

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())),

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),

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<Au> {
-        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<Au> {
+        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)

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,
+    pub fn new(
-               viewport_size: TypedSize2D<f32, CSSPixel>,
+        media_type: MediaType,
-               device_pixel_ratio: ScaleFactor<f32, CSSPixel, DevicePixel>)
+        viewport_size: TypedSize2D<f32, CSSPixel>,
-               -> Device {
+        device_pixel_ratio: ScaleFactor<f32, CSSPixel, DevicePixel>
+    ) -> Device {
         Device {
-            media_type: media_type,
+            media_type,
-            viewport_size: viewport_size,
+            viewport_size,
-            device_pixel_ratio: device_pixel_ratio,
+            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.
+    /// Like the above, but records that we've used viewport units.
-    #[inline]
+    pub fn au_viewport_size_for_viewport_unit_resolution(&self) -> Size2D<Au> {
-    pub fn px_viewport_size(&self) -> TypedSize2D<f32, CSSPixel> {
+        self.used_viewport_units.store(true, Ordering::Relaxed);
-        self.viewport_size
+        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.

components/style/stylesheets/stylesheet.rs

@@ -58,8 +58,6 @@ pub struct StylesheetContents {
     pub namespaces: RwLock<Namespaces>,
     /// 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<Option<String>>,
 }
@@ -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<CssRule>, bool, Option<String>) {
+    ) -> (Vec<CssRule>, Option<String>) {
         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 {

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();

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());
             }
         }
 

components/style/values/computed/mod.rs

@@ -127,9 +127,9 @@ impl<'a> Context<'a> {
         self.builder.device
     }
 
-    /// The current viewport size.
+    /// The current viewport size, used to resolve viewport units.
-    pub fn viewport_size(&self) -> Size2D<Au> {
+    pub fn viewport_size_for_viewport_unit_resolution(&self) -> Size2D<Au> {
-        self.builder.device.au_viewport_size()
+        self.builder.device.au_viewport_size_for_viewport_unit_resolution()
     }
 
     /// The default computed style we're getting our reset style from.

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 {