Skip to content

Commit

Permalink
Replace RwLock<Keyframe> with Locked<Keyframe>
Browse files Browse the repository at this point in the history
  • Loading branch information
SimonSapin committed Mar 19, 2017
1 parent adb97d4 commit 57724e5
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 44 deletions.
2 changes: 1 addition & 1 deletion components/script/dom/bindings/trace.rs
Expand Up @@ -514,7 +514,7 @@ unsafe impl JSTraceable for StyleLocked<CssRules> {
}
}

unsafe impl JSTraceable for RwLock<Keyframe> {
unsafe impl JSTraceable for StyleLocked<Keyframe> {
unsafe fn trace(&self, _trc: *mut JSTracer) {
// Do nothing.
}
Expand Down
26 changes: 16 additions & 10 deletions components/script/dom/csskeyframerule.rs
Expand Up @@ -12,21 +12,21 @@ use dom::cssstyledeclaration::{CSSModificationAccess, CSSStyleDeclaration, CSSSt
use dom::cssstylesheet::CSSStyleSheet;
use dom::window::Window;
use dom_struct::dom_struct;
use parking_lot::RwLock;
use std::sync::Arc;
use style::keyframes::Keyframe;
use style::shared_lock::Locked;
use style_traits::ToCss;

#[dom_struct]
pub struct CSSKeyframeRule {
cssrule: CSSRule,
#[ignore_heap_size_of = "Arc"]
keyframerule: Arc<RwLock<Keyframe>>,
keyframerule: Arc<Locked<Keyframe>>,
style_decl: MutNullableJS<CSSStyleDeclaration>,
}

impl CSSKeyframeRule {
fn new_inherited(parent_stylesheet: &CSSStyleSheet, keyframerule: Arc<RwLock<Keyframe>>)
fn new_inherited(parent_stylesheet: &CSSStyleSheet, keyframerule: Arc<Locked<Keyframe>>)
-> CSSKeyframeRule {
CSSKeyframeRule {
cssrule: CSSRule::new_inherited(parent_stylesheet),
Expand All @@ -37,7 +37,7 @@ impl CSSKeyframeRule {

#[allow(unrooted_must_root)]
pub fn new(window: &Window, parent_stylesheet: &CSSStyleSheet,
keyframerule: Arc<RwLock<Keyframe>>) -> Root<CSSKeyframeRule> {
keyframerule: Arc<Locked<Keyframe>>) -> Root<CSSKeyframeRule> {
reflect_dom_object(box CSSKeyframeRule::new_inherited(parent_stylesheet, keyframerule),
window,
CSSKeyframeRuleBinding::Wrap)
Expand All @@ -48,11 +48,16 @@ impl CSSKeyframeRuleMethods for CSSKeyframeRule {
// https://drafts.csswg.org/css-animations/#dom-csskeyframerule-style
fn Style(&self) -> Root<CSSStyleDeclaration> {
self.style_decl.or_init(|| {
CSSStyleDeclaration::new(self.global().as_window(),
CSSStyleOwner::CSSRule(JS::from_ref(self.upcast()),
self.keyframerule.read().block.clone()),
None,
CSSModificationAccess::ReadWrite)
let guard = self.cssrule.shared_lock().read();
CSSStyleDeclaration::new(
self.global().as_window(),
CSSStyleOwner::CSSRule(
JS::from_ref(self.upcast()),
self.keyframerule.read_with(&guard).block.clone(),
),
None,
CSSModificationAccess::ReadWrite,
)
})
}
}
Expand All @@ -64,6 +69,7 @@ impl SpecificCSSRule for CSSKeyframeRule {
}

fn get_css(&self) -> DOMString {
self.keyframerule.read().to_css_string().into()
let guard = self.cssrule.shared_lock().read();
self.keyframerule.read_with(&guard).to_css_string().into()
}
}
2 changes: 1 addition & 1 deletion components/script/dom/csskeyframesrule.rs
Expand Up @@ -67,7 +67,7 @@ impl CSSKeyframesRule {
// because that's the rule that applies. Thus, rposition
self.keyframesrule.read_with(&guard)
.keyframes.iter().rposition(|frame| {
frame.read().selector == sel
frame.read_with(&guard).selector == sel
})
} else {
None
Expand Down
33 changes: 20 additions & 13 deletions components/style/keyframes.rs
Expand Up @@ -15,6 +15,7 @@ use properties::{PropertyDeclarationId, LonghandId, ParsedDeclaration};
use properties::LonghandIdSet;
use properties::animated_properties::TransitionProperty;
use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
use shared_lock::{SharedRwLock, SharedRwLockReadGuard, Locked};
use std::fmt;
use std::sync::Arc;
use style_traits::ToCss;
Expand Down Expand Up @@ -125,7 +126,7 @@ impl Keyframe {
pub fn parse(css: &str,
parent_stylesheet: &Stylesheet,
extra_data: ParserContextExtraData)
-> Result<Arc<RwLock<Self>>, ()> {
-> Result<Arc<Locked<Self>>, ()> {
let error_reporter = MemoryHoleReporter;
let context = ParserContext::new_with_extra_data(parent_stylesheet.origin,
&parent_stylesheet.base_url,
Expand All @@ -135,6 +136,7 @@ impl Keyframe {

let mut rule_parser = KeyframeListParser {
context: &context,
shared_lock: &parent_stylesheet.shared_lock,
};
parse_one_rule(&mut input, &mut rule_parser)
}
Expand Down Expand Up @@ -239,13 +241,14 @@ pub struct KeyframesAnimation {
}

/// Get all the animated properties in a keyframes animation.
fn get_animated_properties(keyframes: &[Arc<RwLock<Keyframe>>]) -> Vec<TransitionProperty> {
fn get_animated_properties(keyframes: &[Arc<Locked<Keyframe>>], guard: &SharedRwLockReadGuard)
-> Vec<TransitionProperty> {
let mut ret = vec![];
let mut seen = LonghandIdSet::new();
// NB: declarations are already deduplicated, so we don't have to check for
// it here.
for keyframe in keyframes {
let keyframe = keyframe.read();
let keyframe = keyframe.read_with(&guard);
for &(ref declaration, importance) in keyframe.block.read().declarations().iter() {
assert!(!importance.important());

Expand All @@ -270,7 +273,8 @@ impl KeyframesAnimation {
///
/// Otherwise, this will compute and sort the steps used for the animation,
/// and return the animation object.
pub fn from_keyframes(keyframes: &[Arc<RwLock<Keyframe>>]) -> Self {
pub fn from_keyframes(keyframes: &[Arc<Locked<Keyframe>>], guard: &SharedRwLockReadGuard)
-> Self {
let mut result = KeyframesAnimation {
steps: vec![],
properties_changed: vec![],
Expand All @@ -280,13 +284,13 @@ impl KeyframesAnimation {
return result;
}

result.properties_changed = get_animated_properties(keyframes);
result.properties_changed = get_animated_properties(keyframes, guard);
if result.properties_changed.is_empty() {
return result;
}

for keyframe in keyframes {
let keyframe = keyframe.read();
let keyframe = keyframe.read_with(&guard);
for percentage in keyframe.selector.0.iter() {
result.steps.push(KeyframesStep::new(*percentage, KeyframesStepValue::Declarations {
block: keyframe.block.clone(),
Expand Down Expand Up @@ -322,24 +326,27 @@ impl KeyframesAnimation {
/// }
struct KeyframeListParser<'a> {
context: &'a ParserContext<'a>,
shared_lock: &'a SharedRwLock,
}

/// Parses a keyframe list from CSS input.
pub fn parse_keyframe_list(context: &ParserContext, input: &mut Parser) -> Vec<Arc<RwLock<Keyframe>>> {
RuleListParser::new_for_nested_rule(input, KeyframeListParser { context: context })
.filter_map(Result::ok)
.collect()
pub fn parse_keyframe_list(context: &ParserContext, input: &mut Parser, shared_lock: &SharedRwLock)
-> Vec<Arc<Locked<Keyframe>>> {
RuleListParser::new_for_nested_rule(input, KeyframeListParser {
context: context,
shared_lock: shared_lock,
}).filter_map(Result::ok).collect()
}

enum Void {}
impl<'a> AtRuleParser for KeyframeListParser<'a> {
type Prelude = Void;
type AtRule = Arc<RwLock<Keyframe>>;
type AtRule = Arc<Locked<Keyframe>>;
}

impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
type Prelude = KeyframeSelector;
type QualifiedRule = Arc<RwLock<Keyframe>>;
type QualifiedRule = Arc<Locked<Keyframe>>;

fn parse_prelude(&mut self, input: &mut Parser) -> Result<Self::Prelude, ()> {
let start = input.position();
Expand Down Expand Up @@ -372,7 +379,7 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
}
// `parse_important` is not called here, `!important` is not allowed in keyframe blocks.
}
Ok(Arc::new(RwLock::new(Keyframe {
Ok(Arc::new(self.shared_lock.wrap(Keyframe {
selector: prelude,
block: Arc::new(RwLock::new(block)),
})))
Expand Down
8 changes: 4 additions & 4 deletions components/style/stylesheets.rs
Expand Up @@ -451,12 +451,12 @@ pub struct KeyframesRule {
/// The name of the current animation.
pub name: Atom,
/// The keyframes specified for this CSS rule.
pub keyframes: Vec<Arc<RwLock<Keyframe>>>,
pub keyframes: Vec<Arc<Locked<Keyframe>>>,
}

impl ToCssWithGuard for KeyframesRule {
// Serialization of KeyframesRule is not specced.
fn to_css<W>(&self, _guard: &SharedRwLockReadGuard, dest: &mut W) -> fmt::Result
fn to_css<W>(&self, guard: &SharedRwLockReadGuard, dest: &mut W) -> fmt::Result
where W: fmt::Write {
try!(dest.write_str("@keyframes "));
try!(dest.write_str(&*self.name.to_string()));
Expand All @@ -468,7 +468,7 @@ impl ToCssWithGuard for KeyframesRule {
try!(dest.write_str(" "));
}
first = false;
let keyframe = lock.read();
let keyframe = lock.read_with(&guard);
try!(keyframe.to_css(dest));
}
dest.write_str(" }")
Expand Down Expand Up @@ -1009,7 +1009,7 @@ impl<'a, 'b> AtRuleParser for NestedRuleParser<'a, 'b> {
AtRulePrelude::Keyframes(name) => {
Ok(CssRule::Keyframes(Arc::new(self.shared_lock.wrap(KeyframesRule {
name: name,
keyframes: parse_keyframe_list(&self.context, input),
keyframes: parse_keyframe_list(&self.context, input, self.shared_lock),
}))))
}
}
Expand Down
3 changes: 2 additions & 1 deletion components/style/stylist.rs
Expand Up @@ -280,7 +280,8 @@ impl Stylist {
CssRule::Keyframes(ref keyframes_rule) => {
let keyframes_rule = keyframes_rule.read_with(guard);
debug!("Found valid keyframes rule: {:?}", *keyframes_rule);
let animation = KeyframesAnimation::from_keyframes(&keyframes_rule.keyframes);
let animation = KeyframesAnimation::from_keyframes(
&keyframes_rule.keyframes, guard);
debug!("Found valid keyframe animation: {:?}", animation);
self.animations.insert(keyframes_rule.name.clone(), animation);
}
Expand Down
30 changes: 18 additions & 12 deletions tests/unit/style/keyframes.rs
Expand Up @@ -8,12 +8,14 @@ use style::keyframes::{Keyframe, KeyframesAnimation, KeyframePercentage, Keyfra
use style::keyframes::{KeyframesStep, KeyframesStepValue};
use style::properties::{PropertyDeclaration, PropertyDeclarationBlock, Importance};
use style::properties::animated_properties::TransitionProperty;
use style::shared_lock::SharedRwLock;
use style::values::specified::{LengthOrPercentageOrAuto, NoCalcLength};

#[test]
fn test_empty_keyframe() {
let shared_lock = SharedRwLock::new();
let keyframes = vec![];
let animation = KeyframesAnimation::from_keyframes(&keyframes);
let animation = KeyframesAnimation::from_keyframes(&keyframes, &shared_lock.read());
let expected = KeyframesAnimation {
steps: vec![],
properties_changed: vec![],
Expand All @@ -24,13 +26,14 @@ fn test_empty_keyframe() {

#[test]
fn test_no_property_in_keyframe() {
let shared_lock = SharedRwLock::new();
let keyframes = vec![
Arc::new(RwLock::new(Keyframe {
Arc::new(shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
block: Arc::new(RwLock::new(PropertyDeclarationBlock::new()))
})),
];
let animation = KeyframesAnimation::from_keyframes(&keyframes);
let animation = KeyframesAnimation::from_keyframes(&keyframes, &shared_lock.read());
let expected = KeyframesAnimation {
steps: vec![],
properties_changed: vec![],
Expand All @@ -41,6 +44,7 @@ fn test_no_property_in_keyframe() {

#[test]
fn test_missing_property_in_initial_keyframe() {
let shared_lock = SharedRwLock::new();
let declarations_on_initial_keyframe =
Arc::new(RwLock::new(PropertyDeclarationBlock::with_one(
PropertyDeclaration::Width(
Expand All @@ -65,17 +69,17 @@ fn test_missing_property_in_initial_keyframe() {
}));

let keyframes = vec![
Arc::new(RwLock::new(Keyframe {
Arc::new(shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
block: declarations_on_initial_keyframe.clone(),
})),

Arc::new(RwLock::new(Keyframe {
Arc::new(shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
block: declarations_on_final_keyframe.clone(),
})),
];
let animation = KeyframesAnimation::from_keyframes(&keyframes);
let animation = KeyframesAnimation::from_keyframes(&keyframes, &shared_lock.read());
let expected = KeyframesAnimation {
steps: vec![
KeyframesStep {
Expand All @@ -97,6 +101,7 @@ fn test_missing_property_in_initial_keyframe() {

#[test]
fn test_missing_property_in_final_keyframe() {
let shared_lock = SharedRwLock::new();
let declarations_on_initial_keyframe =
Arc::new(RwLock::new({
let mut block = PropertyDeclarationBlock::new();
Expand All @@ -121,17 +126,17 @@ fn test_missing_property_in_final_keyframe() {
)));

let keyframes = vec![
Arc::new(RwLock::new(Keyframe {
Arc::new(shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
block: declarations_on_initial_keyframe.clone(),
})),

Arc::new(RwLock::new(Keyframe {
Arc::new(shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(1.)]),
block: declarations_on_final_keyframe.clone(),
})),
];
let animation = KeyframesAnimation::from_keyframes(&keyframes);
let animation = KeyframesAnimation::from_keyframes(&keyframes, &shared_lock.read());
let expected = KeyframesAnimation {
steps: vec![
KeyframesStep {
Expand All @@ -153,6 +158,7 @@ fn test_missing_property_in_final_keyframe() {

#[test]
fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() {
let shared_lock = SharedRwLock::new();
let declarations =
Arc::new(RwLock::new({
let mut block = PropertyDeclarationBlock::new();
Expand All @@ -170,16 +176,16 @@ fn test_missing_keyframe_in_both_of_initial_and_final_keyframe() {
}));

let keyframes = vec![
Arc::new(RwLock::new(Keyframe {
Arc::new(shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.)]),
block: Arc::new(RwLock::new(PropertyDeclarationBlock::new()))
})),
Arc::new(RwLock::new(Keyframe {
Arc::new(shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(vec![KeyframePercentage::new(0.5)]),
block: declarations.clone(),
})),
];
let animation = KeyframesAnimation::from_keyframes(&keyframes);
let animation = KeyframesAnimation::from_keyframes(&keyframes, &shared_lock.read());
let expected = KeyframesAnimation {
steps: vec![
KeyframesStep {
Expand Down
4 changes: 2 additions & 2 deletions tests/unit/style/stylesheets.rs
Expand Up @@ -238,7 +238,7 @@ fn test_parse_stylesheet() {
CssRule::Keyframes(Arc::new(stylesheet.shared_lock.wrap(KeyframesRule {
name: "foo".into(),
keyframes: vec![
Arc::new(RwLock::new(Keyframe {
Arc::new(stylesheet.shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(
vec![KeyframePercentage::new(0.)]),
block: Arc::new(RwLock::new(block_from(vec![
Expand All @@ -247,7 +247,7 @@ fn test_parse_stylesheet() {
Importance::Normal),
])))
})),
Arc::new(RwLock::new(Keyframe {
Arc::new(stylesheet.shared_lock.wrap(Keyframe {
selector: KeyframeSelector::new_for_unit_testing(
vec![KeyframePercentage::new(1.)]),
block: Arc::new(RwLock::new(block_from(vec![
Expand Down

0 comments on commit 57724e5

Please sign in to comment.