From f97d47188f5a2a19f34c3015c40a79d87dfb3e74 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 29 Nov 2016 17:41:46 -0800 Subject: [PATCH 1/5] layout_thread: Note in the debug message whether nodes require dirtying. --- components/layout_thread/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 72f1327620e..961abb7416b 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -1091,7 +1091,7 @@ impl LayoutThread { } let restyles = document.drain_pending_restyles(); - debug!("Draining restyles: {}", restyles.len()); + debug!("Draining restyles: {} (needs dirtying? {:?})", restyles.len(), needs_dirtying); if !needs_dirtying { for (el, restyle) in restyles { // Propagate the descendant bit up the ancestors. Do this before From 0e338d5b31376aee36f4edeb72a31714eb5cf817 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 29 Nov 2016 17:44:31 -0800 Subject: [PATCH 2/5] layout_thread: Log reflows that happen due to animation ticks if `-Z relayout-event` is on. --- components/layout_thread/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs index 961abb7416b..7ae942361ed 100644 --- a/components/layout_thread/lib.rs +++ b/components/layout_thread/lib.rs @@ -1282,6 +1282,10 @@ impl LayoutThread { } fn tick_animations(&mut self, rw_data: &mut LayoutThreadData) { + if opts::get().relayout_event { + println!("**** pipeline={}\tForDisplay\tSpecial\tAnimationTick", self.id); + } + let reflow_info = Reflow { goal: ReflowGoal::ForDisplay, page_clip_rect: max_rect(), From f378f2807e7854ca684a1b959f42426362cbbca0 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 29 Nov 2016 17:45:01 -0800 Subject: [PATCH 3/5] compositing: Only make the compositor responsible for initiating reflow for CSS transitions and animations, not `requestAnimationFrame()` callbacks. In the case of the latter, the script thread will kick off the reflow if it's necessary, so there's no need for the compositor to do it. Some pages, like nytimes.com, like to call `requestAnimationFrame()` without actually mutating the DOM in the callback. We should avoid reflowing in this case. --- components/compositing/compositor.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/components/compositing/compositor.rs b/components/compositing/compositor.rs index 29f51edc0d8..8f0c12e5bf7 100644 --- a/components/compositing/compositor.rs +++ b/components/compositing/compositor.rs @@ -1191,10 +1191,13 @@ impl IOCompositor { } } - // We still need to tick layout unfortunately, see things like #12749. - let msg = ConstellationMsg::TickAnimation(pipeline_id, AnimationTickType::Layout); - if let Err(e) = self.constellation_chan.send(msg) { - warn!("Sending tick to constellation failed ({}).", e); + // We may need to tick animations in layout. (See #12749.) + let animations_running = self.pipeline_details(pipeline_id).animations_running; + if animations_running { + let msg = ConstellationMsg::TickAnimation(pipeline_id, AnimationTickType::Layout); + if let Err(e) = self.constellation_chan.send(msg) { + warn!("Sending tick to constellation failed ({}).", e); + } } } From 89dff2d77f72f3f383de4a678aa1cf64ab02058a Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 29 Nov 2016 17:48:33 -0800 Subject: [PATCH 4/5] =?UTF-8?q?layout:=20Don't=20restart=20a=20transition?= =?UTF-8?q?=20if=20the=20end=20value=20for=20the=20new=20transition=20is?= =?UTF-8?q?=20the=20same=20as=20the=20end=20value=20for=20a=20running=20tr?= =?UTF-8?q?ansition=20per=20CSS-TRANSITIONS=20=C2=A7=203.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Besides being contrary to spec, the old behavior could cause a cascade of CSS transitions incorrectly triggering themselves when calls to `getComputedStyle()` were intermingled with them. Improves performance of nytimes.com. --- components/style/animation.rs | 18 +++++- components/style/matching.rs | 59 +++++++++++-------- .../helpers/animated_properties.mako.rs | 14 +++++ 3 files changed, 66 insertions(+), 25 deletions(-) diff --git a/components/style/animation.rs b/components/style/animation.rs index c9fd2ee5e5b..d9f357ec3cb 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -335,6 +335,11 @@ impl PropertyAnimation { fn does_animate(&self) -> bool { self.property.does_animate() && self.duration != Time(0.0) } + + #[inline] + pub fn has_the_same_end_value_as(&self, other: &PropertyAnimation) -> bool { + self.property.has_the_same_end_value_as(&other.property) + } } /// Inserts transitions into the queue of running animations as applicable for @@ -348,13 +353,24 @@ pub fn start_transitions_if_applicable(new_animations_sender: &Sender unsafe_node: UnsafeNode, old_style: &ComputedValues, new_style: &mut Arc, - timer: &Timer) + timer: &Timer, + possibly_expired_animations: &[PropertyAnimation]) -> bool { let mut had_animations = false; for i in 0..new_style.get_box().transition_property_count() { // Create any property animations, if applicable. let property_animations = PropertyAnimation::from_transition(i, old_style, Arc::make_mut(new_style)); for property_animation in property_animations { + // Per [1], don't trigger a new transition if the end state for that transition is + // the same as that of a transition that's already running on the same node. + // + // [1]: https://drafts.csswg.org/css-transitions/#starting + if possibly_expired_animations.iter().any(|animation| { + animation.has_the_same_end_value_as(&property_animation) + }) { + continue + } + // Set the property to the initial value. // NB: get_mut is guaranteed to succeed since we called make_mut() // above. diff --git a/components/style/matching.rs b/components/style/matching.rs index 09323631c7f..e883d11bc3e 100644 --- a/components/style/matching.rs +++ b/components/style/matching.rs @@ -7,7 +7,7 @@ #![allow(unsafe_code)] use {Atom, LocalName}; -use animation; +use animation::{self, Animation, PropertyAnimation}; use atomic_refcell::AtomicRefMut; use cache::LRUCache; use cascade_info::CascadeInfo; @@ -395,10 +395,10 @@ trait PrivateMatchMethods: TElement { parent_style: Option<&Arc>, old_style: Option<&Arc>, rule_node: &StrongRuleNode, + possibly_expired_animations: &[PropertyAnimation], booleans: CascadeBooleans) -> Arc - where Ctx: StyleContext<'a> - { + where Ctx: StyleContext<'a> { let shared_context = context.shared_context(); let mut cascade_info = CascadeInfo::new(); let mut cascade_flags = CascadeFlags::empty(); @@ -445,7 +445,8 @@ trait PrivateMatchMethods: TElement { self.as_node().to_unsafe(), &**style, &mut this_style, - &shared_context.timer); + &shared_context.timer, + &possibly_expired_animations); } } @@ -454,11 +455,11 @@ trait PrivateMatchMethods: TElement { fn update_animations_for_cascade(&self, context: &SharedStyleContext, - style: &mut Arc) -> bool { + style: &mut Arc, + possibly_expired_animations: &mut Vec) { // Finish any expired transitions. let this_opaque = self.as_node().opaque(); - let had_animations_to_expire = - animation::complete_expired_transitions(this_opaque, style, context); + animation::complete_expired_transitions(this_opaque, style, context); // Merge any running transitions into the current style, and cancel them. let had_running_animations = context.running_animations @@ -467,7 +468,7 @@ trait PrivateMatchMethods: TElement { .is_some(); if had_running_animations { let mut all_running_animations = context.running_animations.write(); - for mut running_animation in all_running_animations.get_mut(&this_opaque).unwrap() { + for running_animation in all_running_animations.get_mut(&this_opaque).unwrap() { // This shouldn't happen frequently, but under some // circumstances mainly huge load or debug builds, the // constellation might be delayed in sending the @@ -483,12 +484,12 @@ trait PrivateMatchMethods: TElement { animation::update_style_for_animation(context, running_animation, style); - running_animation.mark_as_expired(); + if let Animation::Transition(_, _, _, ref frame, _) = *running_animation { + possibly_expired_animations.push(frame.property_animation.clone()) + } } } } - - had_animations_to_expire || had_running_animations } fn share_style_with_candidate_if_possible(&self, @@ -729,6 +730,7 @@ pub trait MatchMethods : TElement { let parent_style = parent_data.as_ref().map(|x| &x.current_styles().primary.values); let mut new_styles; + let mut possibly_expired_animations = vec![]; let damage = { let (old_primary, old_pseudos) = match data.previous_styles_mut() { @@ -737,7 +739,8 @@ pub trait MatchMethods : TElement { // Update animations before the cascade. This may modify the // value of the old primary style. self.update_animations_for_cascade(context.shared_context(), - &mut previous.primary.values); + &mut previous.primary.values, + &mut possibly_expired_animations); (Some(&previous.primary.values), Some(&mut previous.pseudos)) } }; @@ -747,6 +750,7 @@ pub trait MatchMethods : TElement { parent_style, old_primary, &primary_rule_node, + &possibly_expired_animations, CascadeBooleans { shareable: primary_is_shareable, animate: true, @@ -761,7 +765,8 @@ pub trait MatchMethods : TElement { &new_styles.primary.values, &mut new_styles.pseudos, context, - pseudo_rule_nodes); + pseudo_rule_nodes, + &mut possibly_expired_animations); self.as_node().set_can_be_fragmented(parent.map_or(false, |p| { p.as_node().can_be_fragmented() || @@ -774,14 +779,16 @@ pub trait MatchMethods : TElement { data.finish_styling(new_styles, damage); } - fn compute_damage_and_cascade_pseudos<'a, Ctx>(&self, - old_primary: Option<&Arc>, - mut old_pseudos: Option<&mut PseudoStyles>, - new_primary: &Arc, - new_pseudos: &mut PseudoStyles, - context: &Ctx, - mut pseudo_rule_nodes: PseudoRuleNodes) - -> RestyleDamage + fn compute_damage_and_cascade_pseudos<'a, Ctx>( + &self, + old_primary: Option<&Arc>, + mut old_pseudos: Option<&mut PseudoStyles>, + new_primary: &Arc, + new_pseudos: &mut PseudoStyles, + context: &Ctx, + mut pseudo_rule_nodes: PseudoRuleNodes, + possibly_expired_animations: &mut Vec) + -> RestyleDamage where Ctx: StyleContext<'a> { // Here we optimise the case of the style changing but both the @@ -838,14 +845,18 @@ pub trait MatchMethods : TElement { // Update animations before the cascade. This may modify // the value of old_pseudo_style. self.update_animations_for_cascade(context.shared_context(), - &mut old_pseudo_style.values); + &mut old_pseudo_style.values, + possibly_expired_animations); } } let new_pseudo_values = - self.cascade_node_pseudo_element(context, Some(new_primary), - maybe_old_pseudo_style.as_ref().map(|s| &s.values), + self.cascade_node_pseudo_element(context, + Some(new_primary), + maybe_old_pseudo_style.as_ref() + .map(|s| &s.values), &new_rule_node, + &possibly_expired_animations, CascadeBooleans { shareable: false, animate: animate, diff --git a/components/style/properties/helpers/animated_properties.mako.rs b/components/style/properties/helpers/animated_properties.mako.rs index b37efac2aae..1053e8e6958 100644 --- a/components/style/properties/helpers/animated_properties.mako.rs +++ b/components/style/properties/helpers/animated_properties.mako.rs @@ -124,6 +124,20 @@ impl AnimatedProperty { } } + pub fn has_the_same_end_value_as(&self, other: &AnimatedProperty) -> bool { + match (self, other) { + % for prop in data.longhands: + % if prop.animatable: + (&AnimatedProperty::${prop.camel_case}(_, ref this_end_value), + &AnimatedProperty::${prop.camel_case}(_, ref other_end_value)) => { + this_end_value == other_end_value + } + % endif + % endfor + _ => false, + } + } + pub fn update(&self, style: &mut ComputedValues, progress: f64) { match *self { % for prop in data.longhands: From ca3d802f03110c8b572bdc26e102934c037af2e0 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Tue, 29 Nov 2016 17:51:31 -0800 Subject: [PATCH 5/5] layout: Minor style cleanup. --- components/layout/animation.rs | 8 +++++--- components/style/animation.rs | 4 +++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/components/layout/animation.rs b/components/layout/animation.rs index e26332a3499..b14771e5de9 100644 --- a/components/layout/animation.rs +++ b/components/layout/animation.rs @@ -88,7 +88,8 @@ pub fn update_animation_state(constellation_chan: &IpcSender, if let Animation::Transition(_, unsafe_node, _, ref frame, _) = running_animation { script_chan.send(ConstellationControlMsg::TransitionEnd(unsafe_node, - frame.property_animation.property_name(), + frame.property_animation + .property_name(), frame.duration)) .unwrap(); } @@ -113,7 +114,7 @@ pub fn update_animation_state(constellation_chan: &IpcSender, for new_running_animation in new_running_animations { running_animations.entry(*new_running_animation.node()) .or_insert_with(Vec::new) - .push(new_running_animation); + .push(new_running_animation) } let animation_state = if running_animations.is_empty() { @@ -122,7 +123,8 @@ pub fn update_animation_state(constellation_chan: &IpcSender, AnimationState::AnimationsPresent }; - constellation_chan.send(ConstellationMsg::ChangeRunningAnimationsState(pipeline_id, animation_state)) + constellation_chan.send(ConstellationMsg::ChangeRunningAnimationsState(pipeline_id, + animation_state)) .unwrap(); } diff --git a/components/style/animation.rs b/components/style/animation.rs index d9f357ec3cb..cf9f04e161f 100644 --- a/components/style/animation.rs +++ b/components/style/animation.rs @@ -359,7 +359,9 @@ pub fn start_transitions_if_applicable(new_animations_sender: &Sender let mut had_animations = false; for i in 0..new_style.get_box().transition_property_count() { // Create any property animations, if applicable. - let property_animations = PropertyAnimation::from_transition(i, old_style, Arc::make_mut(new_style)); + let property_animations = PropertyAnimation::from_transition(i, + old_style, + Arc::make_mut(new_style)); for property_animation in property_animations { // Per [1], don't trigger a new transition if the end state for that transition is // the same as that of a transition that's already running on the same node.