servo: Merge #18365 - stylo: Store location information for keyframe rules (from chenpighead:stylo-keyframe-location); r=upsuper,emilio
authorJeremy Chen <jeremychen@mozilla.com>
Tue, 05 Sep 2017 11:22:20 -0500
changeset 428431 4d8ef05f79083351ad1b28c428f0eb15f4eef50a
parent 428430 afca47b3d59b841b248a1db605275e67c1f3b8b9
child 428432 279b7e7e46b077e978707f9c29011825f48db758
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersupsuper, emilio
bugs1394994
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #18365 - stylo: Store location information for keyframe rules (from chenpighead:stylo-keyframe-location); r=upsuper,emilio So far, we only store location info for the whole Keyframes block, not for each of the keyframe rule. Without this info, the devtool can't present the rules on the devtool panel properly. In this patch, we collect the source location info while parsing keyframe selector. The binding function, Servo_KeyframesRule_GetKeyframe, is also fixed (and renamed to Servo_KeyframesRule_GetKeyframeAt to match the fix) to accept line and column parameters from Gecko, so we can pass/set them with the source location from Servo. This is the servo part of [Bug 1394994](https://bugzilla.mozilla.org/show_bug.cgi?id=1394994). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix [Bug 1394994](https://bugzilla.mozilla.org/show_bug.cgi?id=1394994) Source-Repo: https://github.com/servo/servo Source-Revision: f648e12935cb94405a815216391f2527912ba4c2
servo/components/style/gecko/generated/bindings.rs
servo/components/style/stylesheets/keyframes_rule.rs
servo/components/style/stylesheets/rule_parser.rs
servo/ports/geckolib/glue.rs
servo/tests/unit/style/keyframes.rs
servo/tests/unit/style/stylesheets.rs
--- a/servo/components/style/gecko/generated/bindings.rs
+++ b/servo/components/style/gecko/generated/bindings.rs
@@ -2337,19 +2337,20 @@ extern "C" {
     pub fn Servo_KeyframesRule_SetName(rule: RawServoKeyframesRuleBorrowed,
                                        name: *mut nsIAtom);
 }
 extern "C" {
     pub fn Servo_KeyframesRule_GetCount(rule: RawServoKeyframesRuleBorrowed)
      -> u32;
 }
 extern "C" {
-    pub fn Servo_KeyframesRule_GetKeyframe(rule:
-                                               RawServoKeyframesRuleBorrowed,
-                                           index: u32)
+    pub fn Servo_KeyframesRule_GetKeyframeAt(rule:
+                                                 RawServoKeyframesRuleBorrowed,
+                                             index: u32, line: *mut u32,
+                                             column: *mut u32)
      -> RawServoKeyframeStrong;
 }
 extern "C" {
     pub fn Servo_KeyframesRule_FindRule(rule: RawServoKeyframesRuleBorrowed,
                                         key: *const nsACString) -> u32;
 }
 extern "C" {
     pub fn Servo_KeyframesRule_AppendRule(rule: RawServoKeyframesRuleBorrowed,
--- a/servo/components/style/stylesheets/keyframes_rule.rs
+++ b/servo/components/style/stylesheets/keyframes_rule.rs
@@ -15,17 +15,17 @@ use properties::animated_properties::Ani
 use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
 use selectors::parser::SelectorParseError;
 use servo_arc::Arc;
 use shared_lock::{DeepCloneParams, DeepCloneWithLock, SharedRwLock, SharedRwLockReadGuard, Locked, ToCssWithGuard};
 use std::fmt;
 use style_traits::{PARSING_MODE_DEFAULT, ToCss, ParseError, StyleParseError};
 use style_traits::PropertyDeclarationParseError;
 use stylesheets::{CssRuleType, StylesheetContents};
-use stylesheets::rule_parser::VendorPrefix;
+use stylesheets::rule_parser::{VendorPrefix, get_location_with_offset};
 use values::{KeyframesName, serialize_percentage};
 
 /// A [`@keyframes`][keyframes] rule.
 ///
 /// [keyframes]: https://drafts.csswg.org/css-animations/#keyframes
 #[derive(Debug)]
 pub struct KeyframesRule {
     /// The name of the current animation.
@@ -187,16 +187,19 @@ pub struct Keyframe {
     /// The selector this keyframe was specified from.
     pub selector: KeyframeSelector,
 
     /// The declaration block that was declared inside this keyframe.
     ///
     /// Note that `!important` rules in keyframes don't apply, but we keep this
     /// `Arc` just for convenience.
     pub block: Arc<Locked<PropertyDeclarationBlock>>,
+
+    /// The line and column of the rule's source code.
+    pub source_location: SourceLocation,
 }
 
 impl ToCssWithGuard for Keyframe {
     fn to_css<W>(&self, guard: &SharedRwLockReadGuard, dest: &mut W) -> fmt::Result
     where W: fmt::Write {
         self.selector.to_css(dest)?;
         dest.write_str(" { ")?;
         self.block.read_with(guard).to_css(dest)?;
@@ -244,16 +247,17 @@ impl DeepCloneWithLock for Keyframe {
         &self,
         lock: &SharedRwLock,
         guard: &SharedRwLockReadGuard,
         _params: &DeepCloneParams,
     ) -> Keyframe {
         Keyframe {
             selector: self.selector.clone(),
             block: Arc::new(lock.wrap(self.block.read_with(guard).clone())),
+            source_location: self.source_location.clone(),
         }
     }
 }
 
 /// A keyframes step value. This can be a synthetised keyframes animation, that
 /// is, one autogenerated from the current computed values, or a list of
 /// declarations to apply.
 ///
@@ -478,26 +482,38 @@ pub fn parse_keyframe_list<R>(
 
 impl<'a, 'i, R> AtRuleParser<'i> for KeyframeListParser<'a, R> {
     type PreludeNoBlock = ();
     type PreludeBlock = ();
     type AtRule = Arc<Locked<Keyframe>>;
     type Error = SelectorParseError<'i, StyleParseError<'i>>;
 }
 
+/// A wrapper to wraps the KeyframeSelector with its source location
+struct KeyframeSelectorParserPrelude {
+    selector: KeyframeSelector,
+    source_location: SourceLocation,
+}
+
 impl<'a, 'i, R: ParseErrorReporter> QualifiedRuleParser<'i> for KeyframeListParser<'a, R> {
-    type Prelude = KeyframeSelector;
+    type Prelude = KeyframeSelectorParserPrelude;
     type QualifiedRule = Arc<Locked<Keyframe>>;
     type Error = SelectorParseError<'i, StyleParseError<'i>>;
 
     fn parse_prelude<'t>(&mut self, input: &mut Parser<'i, 't>) -> Result<Self::Prelude, ParseError<'i>> {
         let start_position = input.position();
         let start_location = input.current_source_location();
+        let location = get_location_with_offset(start_location);
         match KeyframeSelector::parse(input) {
-            Ok(sel) => Ok(sel),
+            Ok(sel) => {
+                Ok(KeyframeSelectorParserPrelude {
+                    selector: sel,
+                    source_location: location,
+                })
+            },
             Err(e) => {
                 let error = ContextualParseError::InvalidKeyframeRule(input.slice_from(start_position), e.clone());
                 self.context.log_css_error(self.error_context, start_location, error);
                 Err(e)
             }
         }
     }
 
@@ -525,18 +541,19 @@ impl<'a, 'i, R: ParseErrorReporter> Qual
                     iter.parser.declarations.clear();
                     let error = ContextualParseError::UnsupportedKeyframePropertyDeclaration(err.slice, err.error);
                     context.log_css_error(self.error_context, err.location, error);
                 }
             }
             // `parse_important` is not called here, `!important` is not allowed in keyframe blocks.
         }
         Ok(Arc::new(self.shared_lock.wrap(Keyframe {
-            selector: prelude,
+            selector: prelude.selector,
             block: Arc::new(self.shared_lock.wrap(block)),
+            source_location: prelude.source_location,
         })))
     }
 }
 
 struct KeyframeDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
     declarations: &'a mut SourcePropertyDeclaration,
 }
--- a/servo/components/style/stylesheets/rule_parser.rs
+++ b/servo/components/style/stylesheets/rule_parser.rs
@@ -582,15 +582,15 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> 
             selectors: prelude.selectors,
             block: Arc::new(self.shared_lock.wrap(declarations)),
             source_location: prelude.source_location,
         }))))
     }
 }
 
 /// Adjust a location's column to accommodate DevTools.
-fn get_location_with_offset(location: SourceLocation) -> SourceLocation {
+pub fn get_location_with_offset(location: SourceLocation) -> SourceLocation {
     SourceLocation {
         line: location.line,
         // Column offsets are not yet supported, but Gecko devtools expect 1-based columns.
         column: location.column + 1,
     }
 }
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -1523,21 +1523,26 @@ pub extern "C" fn Servo_KeyframesRule_Se
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_KeyframesRule_GetCount(rule: RawServoKeyframesRuleBorrowed) -> u32 {
     read_locked_arc(rule, |rule: &KeyframesRule| rule.keyframes.len() as u32)
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_KeyframesRule_GetKeyframe(rule: RawServoKeyframesRuleBorrowed, index: u32)
-                                                  -> RawServoKeyframeStrong {
-    read_locked_arc(rule, |rule: &KeyframesRule| {
-        rule.keyframes[index as usize].clone().into_strong()
-    })
+pub extern "C" fn Servo_KeyframesRule_GetKeyframeAt(rule: RawServoKeyframesRuleBorrowed, index: u32,
+                                                    line: *mut u32, column: *mut u32) -> RawServoKeyframeStrong {
+    let global_style_data = &*GLOBAL_STYLE_DATA;
+    let guard = global_style_data.shared_lock.read();
+    let key = Locked::<KeyframesRule>::as_arc(&rule).read_with(&guard)
+                  .keyframes[index as usize].clone();
+    let location = key.read_with(&guard).source_location;
+    *unsafe { line.as_mut().unwrap() } = location.line as u32;
+    *unsafe { column.as_mut().unwrap() } = location.column as u32;
+    key.into_strong()
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_KeyframesRule_FindRule(rule: RawServoKeyframesRuleBorrowed,
                                                key: *const nsACString) -> u32 {
     let key = unsafe { key.as_ref().unwrap().as_str_unchecked() };
     let global_style_data = &*GLOBAL_STYLE_DATA;
     let guard = global_style_data.shared_lock.read();
--- a/servo/tests/unit/style/keyframes.rs
+++ b/servo/tests/unit/style/keyframes.rs
@@ -1,12 +1,13 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+use cssparser::SourceLocation;
 use servo_arc::Arc;
 use style::properties::{PropertyDeclaration, PropertyDeclarationBlock, Importance};
 use style::properties::animated_properties::AnimatableLonghand;
 use style::shared_lock::SharedRwLock;
 use style::stylesheets::keyframes_rule::{Keyframe, KeyframesAnimation, KeyframePercentage,  KeyframeSelector};
 use style::stylesheets::keyframes_rule::{KeyframesStep, KeyframesStepValue};
 use style::values::specified::{LengthOrPercentageOrAuto, NoCalcLength};
 
@@ -24,20 +25,22 @@ fn test_empty_keyframe() {
     };
 
     assert_eq!(format!("{:#?}", animation), format!("{:#?}", expected));
 }
 
 #[test]
 fn test_no_property_in_keyframe() {
     let shared_lock = SharedRwLock::new();
+    let dummy_location = SourceLocation { line: 0, column: 0 };
     let keyframes = vec![
         Arc::new(shared_lock.wrap(Keyframe {
             selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
-            block: Arc::new(shared_lock.wrap(PropertyDeclarationBlock::new()))
+            block: Arc::new(shared_lock.wrap(PropertyDeclarationBlock::new())),
+            source_location: dummy_location,
         })),
     ];
     let animation = KeyframesAnimation::from_keyframes(&keyframes,
                                                        /* vendor_prefix = */ None,
                                                        &shared_lock.read());
     let expected = KeyframesAnimation {
         steps: vec![],
         properties_changed: vec![],
@@ -68,25 +71,28 @@ fn test_missing_property_in_initial_keyf
             block.push(
                 PropertyDeclaration::Height(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
                 Importance::Normal
             );
             block
         }));
 
+    let dummy_location = SourceLocation { line: 0, column: 0 };
     let keyframes = vec![
         Arc::new(shared_lock.wrap(Keyframe {
             selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
             block: declarations_on_initial_keyframe.clone(),
+            source_location: dummy_location,
         })),
 
         Arc::new(shared_lock.wrap(Keyframe {
             selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
             block: declarations_on_final_keyframe.clone(),
+            source_location: dummy_location,
         })),
     ];
     let animation = KeyframesAnimation::from_keyframes(&keyframes,
                                                        /* vendor_prefix = */ None,
                                                        &shared_lock.read());
     let expected = KeyframesAnimation {
         steps: vec![
             KeyframesStep {
@@ -128,25 +134,28 @@ fn test_missing_property_in_final_keyfra
 
     let declarations_on_final_keyframe =
         Arc::new(shared_lock.wrap(PropertyDeclarationBlock::with_one(
             PropertyDeclaration::Height(
                 LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
             Importance::Normal,
         )));
 
+    let dummy_location = SourceLocation { line: 0, column: 0 };
     let keyframes = vec![
         Arc::new(shared_lock.wrap(Keyframe {
             selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
             block: declarations_on_initial_keyframe.clone(),
+            source_location: dummy_location,
         })),
 
         Arc::new(shared_lock.wrap(Keyframe {
             selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
             block: declarations_on_final_keyframe.clone(),
+            source_location: dummy_location,
         })),
     ];
     let animation = KeyframesAnimation::from_keyframes(&keyframes,
                                                        /* vendor_prefix = */ None,
                                                        &shared_lock.read());
     let expected = KeyframesAnimation {
         steps: vec![
             KeyframesStep {
@@ -181,24 +190,27 @@ fn test_missing_keyframe_in_both_of_init
             block.push(
                 PropertyDeclaration::Height(
                     LengthOrPercentageOrAuto::Length(NoCalcLength::from_px(20f32))),
                 Importance::Normal
             );
             block
         }));
 
+    let dummy_location = SourceLocation { line: 0, column: 0 };
     let keyframes = vec![
         Arc::new(shared_lock.wrap(Keyframe {
             selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
-            block: Arc::new(shared_lock.wrap(PropertyDeclarationBlock::new()))
+            block: Arc::new(shared_lock.wrap(PropertyDeclarationBlock::new())),
+            source_location: dummy_location,
         })),
         Arc::new(shared_lock.wrap(Keyframe {
             selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.5)]),
             block: declarations.clone(),
+            source_location: dummy_location,
         })),
     ];
     let animation = KeyframesAnimation::from_keyframes(&keyframes,
                                                        /* vendor_prefix = */ None,
                                                        &shared_lock.read());
     let expected = KeyframesAnimation {
         steps: vec![
             KeyframesStep {
--- a/servo/tests/unit/style/stylesheets.rs
+++ b/servo/tests/unit/style/stylesheets.rs
@@ -212,30 +212,38 @@ fn test_parse_stylesheet() {
                     keyframes: vec![
                         Arc::new(stylesheet.shared_lock.wrap(Keyframe {
                             selector: KeyframeSelector::new_for_unit_testing(
                                           vec![KeyframePercentage::new(0.)]),
                             block: Arc::new(stylesheet.shared_lock.wrap(block_from(vec![
                                 (PropertyDeclaration::Width(
                                     LengthOrPercentageOrAuto::Percentage(Percentage(0.))),
                                  Importance::Normal),
-                            ])))
+                            ]))),
+                            source_location: SourceLocation {
+                                line: 17,
+                                column: 13,
+                            },
                         })),
                         Arc::new(stylesheet.shared_lock.wrap(Keyframe {
                             selector: KeyframeSelector::new_for_unit_testing(
                                           vec![KeyframePercentage::new(1.)]),
                             block: Arc::new(stylesheet.shared_lock.wrap(block_from(vec![
                                 (PropertyDeclaration::Width(
                                     LengthOrPercentageOrAuto::Percentage(Percentage(1.))),
                                  Importance::Normal),
                                 (PropertyDeclaration::AnimationTimingFunction(
                                     animation_timing_function::SpecifiedValue(
                                         vec![TimingFunction::ease()])),
                                  Importance::Normal),
                             ]))),
+                            source_location: SourceLocation {
+                                line: 18,
+                                column: 13,
+                            },
                         })),
                     ],
                     vendor_prefix: None,
                     source_location: SourceLocation {
                         line: 16,
                         column: 19,
                     },
                 })))