Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Hoist StyleNew{Children,Subtree} into their own paths.
The buggy animation handling isn't a regression, since currently we pass
UnstyledChildrenOnly in those cases, which blocks the animation traversal
in Servo_TraverseSubtree.

In general I really wanted to handle these two paths together. But there's
enough broken with the NewChildren path that I wanted to scope the buginess
as tightly as possible. And I really need to separate the handling here from
StyleDocument() to make the restyle root stuff work.

MozReview-Commit-ID: 9F0mcQl7AAX
  • Loading branch information
bholley committed Aug 11, 2017
1 parent 32790be commit f4ccbf3
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 103 deletions.
7 changes: 6 additions & 1 deletion components/style/dom.rs
Expand Up @@ -34,7 +34,7 @@ use std::hash::Hash;
use std::ops::Deref;
use stylist::Stylist;
use thread_state;
use traversal_flags::TraversalFlags;
use traversal_flags::{TraversalFlags, self};

pub use style_traits::UnsafeNode;

Expand Down Expand Up @@ -489,6 +489,11 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
!data.restyle.hint.has_animation_hint_or_recascade();
}

if traversal_flags.contains(traversal_flags::UnstyledOnly) {
// We don't process invalidations in UnstyledOnly mode.
return data.has_styles();
}

if self.has_snapshot() && !self.handled_snapshot() {
return false;
}
Expand Down
39 changes: 9 additions & 30 deletions components/style/parallel.rs
Expand Up @@ -59,47 +59,26 @@ pub fn traverse_dom<E, D>(traversal: &D,
where E: TElement,
D: DomTraversal<E>,
{
let dump_stats = traversal.shared_context().options.dump_style_statistics;
let start_time = if dump_stats { Some(time::precise_time_s()) } else { None };

// Set up the SmallVec. We need to move this, and in most cases this is just
// one node, so keep it small.
let mut nodes = SmallVec::<[SendNode<E::ConcreteNode>; 8]>::new();

debug_assert!(traversal.is_parallel());
// Handle Gecko's eager initial styling. We don't currently support it
// in conjunction with bottom-up traversal. If we did, we'd need to put
// it on the context to make it available to the bottom-up phase.
let depth = if token.traverse_unstyled_children_only() {
debug_assert!(!D::needs_postorder_traversal());
for kid in root.as_node().traversal_children() {
if kid.as_element().map_or(false, |el| el.get_data().is_none()) {
nodes.push(unsafe { SendNode::new(kid) });
}
}
root.depth() + 1
} else {
nodes.push(unsafe { SendNode::new(root.as_node()) });
root.depth()
};
debug_assert!(token.should_traverse());

if nodes.is_empty() {
return;
}
let dump_stats = traversal.shared_context().options.dump_style_statistics;
let start_time = if dump_stats { Some(time::precise_time_s()) } else { None };

let traversal_data = PerLevelTraversalData {
current_dom_depth: depth,
current_dom_depth: root.depth(),
};
let tls = ScopedTLS::<ThreadLocalStyleContext<E>>::new(pool);
let root = root.as_node().opaque();
let send_root = unsafe { SendNode::new(root.as_node()) };

pool.install(|| {
rayon::scope(|scope| {
let nodes = nodes;
traverse_nodes(&*nodes,
let root = send_root;
let root_opaque = root.opaque();
traverse_nodes(&[root],
DispatchMode::TailCall,
0,
root,
root_opaque,
traversal_data,
scope,
pool,
Expand Down
11 changes: 1 addition & 10 deletions components/style/sequential.rs
Expand Up @@ -35,16 +35,7 @@ pub fn traverse_dom<E, D>(traversal: &D,
};

let root_depth = root.depth();

if token.traverse_unstyled_children_only() {
for kid in root.as_node().traversal_children() {
if kid.as_element().map_or(false, |el| el.get_data().is_none()) {
discovered.push_back(WorkItem(kid, root_depth + 1));
}
}
} else {
discovered.push_back(WorkItem(root.as_node(), root_depth));
}
discovered.push_back(WorkItem(root.as_node(), root_depth));

// Process the nodes breadth-first, just like the parallel traversal does.
// This helps keep similar traversal characteristics for the style sharing
Expand Down
97 changes: 41 additions & 56 deletions components/style/traversal.rs
Expand Up @@ -31,23 +31,12 @@ pub struct PerLevelTraversalData {
pub current_dom_depth: usize,
}

/// This structure exists to enforce that callers invoke pre_traverse, and also
/// to pass information from the pre-traversal into the primary traversal.
pub struct PreTraverseToken {
traverse: bool,
unstyled_children_only: bool,
}

/// We use this structure, rather than just returning a boolean from pre_traverse,
/// to enfore that callers process root invalidations before starting the traversal.
pub struct PreTraverseToken(bool);
impl PreTraverseToken {
/// Whether we should traverse children.
pub fn should_traverse(&self) -> bool {
self.traverse
}

/// Whether we should traverse only unstyled children.
pub fn traverse_unstyled_children_only(&self) -> bool {
self.unstyled_children_only
}
pub fn should_traverse(&self) -> bool { self.0 }
}

/// The kind of traversals we could perform.
Expand Down Expand Up @@ -157,33 +146,20 @@ pub trait DomTraversal<E: TElement> : Sync {
}
}

/// Must be invoked before traversing the root element to determine whether
/// a traversal is needed. Returns a token that allows the caller to prove
/// that the call happened.
///
/// The traversal_flags is used in Gecko.
///
/// If traversal_flag::UNSTYLED_CHILDREN_ONLY is specified, style newly-
/// appended children without restyling the parent.
///
/// If traversal_flag::ANIMATION_ONLY is specified, style only elements for
/// animations.
/// Style invalidations happen when traversing from a parent to its children.
/// However, this mechanism can't handle style invalidations on the root. As
/// such, we have a pre-traversal step to handle that part and determine whether
/// a full traversal is needed.
fn pre_traverse(
root: E,
shared_context: &SharedStyleContext,
traversal_flags: TraversalFlags
) -> PreTraverseToken {
if traversal_flags.contains(traversal_flags::UnstyledChildrenOnly) {
if root.borrow_data().map_or(true, |d| d.has_styles() && d.styles.is_display_none()) {
return PreTraverseToken {
traverse: false,
unstyled_children_only: false,
};
}
return PreTraverseToken {
traverse: true,
unstyled_children_only: true,
};
// If this is an unstyled-only traversal, the caller has already verified
// that there's something to traverse, and we don't need to do any
// invalidation since we're not doing any restyling.
if traversal_flags.contains(traversal_flags::UnstyledOnly) {
return PreTraverseToken(true)
}

let flags = shared_context.traversal_flags;
Expand All @@ -205,10 +181,7 @@ pub trait DomTraversal<E: TElement> : Sync {
parent_data.as_ref().map(|d| &**d)
);

PreTraverseToken {
traverse: should_traverse,
unstyled_children_only: false,
}
PreTraverseToken(should_traverse)
}

/// Returns true if traversal should visit a text node. The style system
Expand All @@ -231,16 +204,32 @@ pub trait DomTraversal<E: TElement> : Sync {
) -> bool {
debug!("element_needs_traversal({:?}, {:?}, {:?}, {:?})",
el, traversal_flags, data, parent_data);
let data = match data {
Some(d) if d.has_styles() => d,
_ => return !traversal_flags.for_animation_only(),
};

if traversal_flags.contains(traversal_flags::UnstyledOnly) {
return data.map_or(true, |d| !d.has_styles()) || el.has_dirty_descendants();
}


// In case of animation-only traversal we need to traverse the element
// if the element has animation only dirty descendants bit,
// animation-only restyle hint or recascade.
if traversal_flags.for_animation_only() {
return data.map_or(false, |d| d.has_styles()) &&
(el.has_animation_only_dirty_descendants() ||
data.as_ref().unwrap().restyle.hint.has_animation_hint_or_recascade());
}

// Non-incremental layout visits every node.
if is_servo_nonincremental_layout() {
return true;
}

// Unwrap the data.
let data = match data {
Some(d) if d.has_styles() => d,
_ => return true,
};

// If the element is native-anonymous and an ancestor frame will be
// reconstructed, the child and all its descendants will be destroyed.
// In that case, we wouldn't need to traverse the subtree...
Expand Down Expand Up @@ -283,14 +272,6 @@ pub trait DomTraversal<E: TElement> : Sync {
}
}

// In case of animation-only traversal we need to traverse the element
// if the element has animation only dirty descendants bit,
// animation-only restyle hint or recascade.
if traversal_flags.for_animation_only() {
return el.has_animation_only_dirty_descendants() ||
data.restyle.hint.has_animation_hint_or_recascade();
}

// If the dirty descendants bit is set, we need to traverse no matter
// what. Skip examining the ElementData.
if el.has_dirty_descendants() {
Expand Down Expand Up @@ -488,7 +469,7 @@ where
let flags = context.shared.traversal_flags;
context.thread_local.begin_element(element, data);
context.thread_local.statistics.elements_traversed += 1;
debug_assert!(flags.for_animation_only() ||
debug_assert!(flags.intersects(AnimationOnly | UnstyledOnly) ||
!element.has_snapshot() || element.handled_snapshot(),
"Should've handled snapshots here already");

Expand Down Expand Up @@ -535,8 +516,12 @@ where
}

// Now that matching and cascading is done, clear the bits corresponding to
// those operations and compute the propagated restyle hint.
let mut propagated_hint = {
// those operations and compute the propagated restyle hint (unless we're
// not processing invalidations, in which case don't need to propagate it
// and must avoid clearing it).
let mut propagated_hint = if flags.contains(UnstyledOnly) {
RestyleHint::empty()
} else {
debug_assert!(flags.for_animation_only() ||
!data.restyle.hint.has_animation_hint(),
"animation restyle hint should be handled during \
Expand Down
7 changes: 4 additions & 3 deletions components/style/traversal_flags.rs
Expand Up @@ -16,8 +16,9 @@ bitflags! {
/// Traverse and update all elements with CSS animations since
/// @keyframes rules may have changed. Triggered by CSS rule changes.
const ForCSSRuleChanges = 1 << 1,
/// Traverse only unstyled children of the root and their descendants.
const UnstyledChildrenOnly = 1 << 2,
/// Styles unstyled elements, but does not handle invalidations on
/// already-styled elements.
const UnstyledOnly = 1 << 2,
/// A forgetful traversal ignores the previous state of the frame tree, and
/// thus does not compute damage or maintain other state describing the styles
/// pre-traversal. A forgetful traversal is usually the right thing if you
Expand Down Expand Up @@ -55,7 +56,7 @@ pub fn assert_traversal_flags_match() {
check_traversal_flags! {
ServoTraversalFlags_AnimationOnly => AnimationOnly,
ServoTraversalFlags_ForCSSRuleChanges => ForCSSRuleChanges,
ServoTraversalFlags_UnstyledChildrenOnly => UnstyledChildrenOnly,
ServoTraversalFlags_UnstyledOnly => UnstyledOnly,
ServoTraversalFlags_Forgetful => Forgetful,
ServoTraversalFlags_AggressivelyForgetful => AggressivelyForgetful,
ServoTraversalFlags_ClearDirtyDescendants => ClearDirtyDescendants,
Expand Down
5 changes: 2 additions & 3 deletions ports/geckolib/glue.rs
Expand Up @@ -260,10 +260,9 @@ pub extern "C" fn Servo_TraverseSubtree(root: RawGeckoElementBorrowed,
let element = GeckoElement(root);

debug!("Servo_TraverseSubtree (flags={:?})", traversal_flags);

// It makes no sense to do an animation restyle when we're restyling
// It makes no sense to do an animation restyle when we're styling
// newly-inserted content.
if !traversal_flags.contains(traversal_flags::UnstyledChildrenOnly) {
if !traversal_flags.contains(traversal_flags::UnstyledOnly) {
let needs_animation_only_restyle =
element.has_animation_only_dirty_descendants() ||
element.has_animation_restyle_hints();
Expand Down

0 comments on commit f4ccbf3

Please sign in to comment.