From 28d48242d361c72242605404d4d6b05bcc33390f Mon Sep 17 00:00:00 2001 From: Jeremy Chen Date: Mon, 4 Sep 2017 13:18:41 +0800 Subject: [PATCH 1/2] stylo: Store location information for keyframe rules. 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. --- components/style/gecko/generated/bindings.rs | 7 +++--- .../style/stylesheets/keyframes_rule.rs | 25 ++++++++++++++++--- components/style/stylesheets/rule_parser.rs | 2 +- ports/geckolib/glue.rs | 15 +++++++---- 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/components/style/gecko/generated/bindings.rs b/components/style/gecko/generated/bindings.rs index f8e0913ba26..8694a24ceaf 100644 --- a/components/style/gecko/generated/bindings.rs +++ b/components/style/gecko/generated/bindings.rs @@ -2337,9 +2337,10 @@ extern "C" { -> 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" { diff --git a/components/style/stylesheets/keyframes_rule.rs b/components/style/stylesheets/keyframes_rule.rs index 6c860003b7e..ad41aded37a 100644 --- a/components/style/stylesheets/keyframes_rule.rs +++ b/components/style/stylesheets/keyframes_rule.rs @@ -20,7 +20,7 @@ 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. @@ -192,6 +192,9 @@ pub struct Keyframe { /// Note that `!important` rules in keyframes don't apply, but we keep this /// `Arc` just for convenience. pub block: Arc>, + + /// The line and column of the rule's source code. + pub source_location: SourceLocation, } impl ToCssWithGuard for Keyframe { @@ -249,6 +252,7 @@ impl DeepCloneWithLock for Keyframe { Keyframe { selector: self.selector.clone(), block: Arc::new(lock.wrap(self.block.read_with(guard).clone())), + source_location: self.source_location.clone(), } } } @@ -483,16 +487,28 @@ impl<'a, 'i, R> AtRuleParser<'i> for KeyframeListParser<'a, R> { 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>; type Error = SelectorParseError<'i, StyleParseError<'i>>; fn parse_prelude<'t>(&mut self, input: &mut Parser<'i, 't>) -> Result> { 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); @@ -530,8 +546,9 @@ impl<'a, 'i, R: ParseErrorReporter> QualifiedRuleParser<'i> for KeyframeListPars // `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, }))) } } diff --git a/components/style/stylesheets/rule_parser.rs b/components/style/stylesheets/rule_parser.rs index a9f9fd13f5d..470acbbc581 100644 --- a/components/style/stylesheets/rule_parser.rs +++ b/components/style/stylesheets/rule_parser.rs @@ -587,7 +587,7 @@ impl<'a, 'b, 'i, R: ParseErrorReporter> QualifiedRuleParser<'i> for NestedRulePa } /// 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. diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs index c446a916369..02919da5b9b 100644 --- a/ports/geckolib/glue.rs +++ b/ports/geckolib/glue.rs @@ -1509,11 +1509,16 @@ pub extern "C" fn Servo_KeyframesRule_GetCount(rule: RawServoKeyframesRuleBorrow } #[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::::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] From f9b7bebc442245a73a519375812d12d6f9787faa Mon Sep 17 00:00:00 2001 From: Jeremy Chen Date: Mon, 4 Sep 2017 22:20:45 +0800 Subject: [PATCH 2/2] stylo: Fix unit test failures caused by adding source_location member to Keyframe rule. In #18365, we add an extra source_location member to Keyframe rule, so that we can provide location information to devtool panel for Stylo. In order not to break the existing unit tests, we should add this new member to all the Keyframe rules used in our unit tests. As for the tests in stylesheets.rs, since we make the test by mocking a raw css string, there are actual source locations that we can add to the expected result. As to the tests in keyframes.rs, since we don't use raw css string, we can just add a dummy souce location for all the tests. We can further fix this if we decide to use a raw css string for the tests in keyframes.rs one day. --- tests/unit/style/keyframes.rs | 16 ++++++++++++++-- tests/unit/style/stylesheets.rs | 10 +++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/tests/unit/style/keyframes.rs b/tests/unit/style/keyframes.rs index 1cb706edf40..e5b7132b7ac 100644 --- a/tests/unit/style/keyframes.rs +++ b/tests/unit/style/keyframes.rs @@ -2,6 +2,7 @@ * 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; @@ -29,10 +30,12 @@ fn test_empty_keyframe() { #[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, @@ -73,15 +76,18 @@ fn test_missing_property_in_initial_keyframe() { 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, @@ -133,15 +139,18 @@ fn test_missing_property_in_final_keyframe() { 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, @@ -186,14 +195,17 @@ fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() { 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, diff --git a/tests/unit/style/stylesheets.rs b/tests/unit/style/stylesheets.rs index 69063931a5e..37eba2338d6 100644 --- a/tests/unit/style/stylesheets.rs +++ b/tests/unit/style/stylesheets.rs @@ -217,7 +217,11 @@ fn test_parse_stylesheet() { (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( @@ -231,6 +235,10 @@ fn test_parse_stylesheet() { vec![TimingFunction::ease()])), Importance::Normal), ]))), + source_location: SourceLocation { + line: 18, + column: 13, + }, })), ], vendor_prefix: None,