So far looks promising, so much code going away! servo/components/style/context.rs | 100 +------------ servo/components/style/data.rs | 29 +--- servo/components/style/dom.rs | 14 +- servo/components/style/lib.rs | 1 + servo/components/style/matching.rs | 986 +++++++++++++++---------------------------------------------------------------------------------------------------------------- servo/components/style/properties/declaration_block.rs | 6 +- servo/components/style/rule_tree/mod.rs | 54 ++----- servo/components/style/style_resolver.rs | 383 +++++++++++++++++++++++++++++++++++++++++++++++++ servo/components/style/stylist.rs | 59 +++----- servo/components/style/traversal.rs | 267 ++++++++++++++++------------------- servo/ports/geckolib/glue.rs | 20 +-- 11 files changed, 679 insertions(+), 1240 deletions(-)

Let's see... I think this should work as-is now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b07dbf6d4bc06fc46b9aa356723c190e45f13064

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10) > Let's see... I think this should work as-is now: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=b07dbf6d4bc06fc46b9aa356723c190e45f13064 Animation failures are because i forgot the !is_animation_only_restyle() check before process_animations. Assertions are because now we restyle the page frame (which I don't know why we didn't before, but I traced it and the process looks legit). Also there are a few new passes, so it looks that we may just have been missing some restyles before.

Comment on attachment 8884688 [details] Remove unused AFFECTED_BY_PSEUDO_ELEMENTS StyleRelation. https://reviewboard.mozilla.org/r/155554/#review160628

And, for the curious: layout/generic/nsFrame.cpp | 2 +- servo/components/style/context.rs | 365 ++------------------------------------- servo/components/style/data.rs | 29 +--- servo/components/style/dom.rs | 14 +- servo/components/style/lib.rs | 1 + servo/components/style/matching.rs | 1197 ++++++++++++++++++++---------------------------------------------------------------------------------------------------------- servo/components/style/properties/declaration_block.rs | 6 +- servo/components/style/properties/properties.mako.rs | 4 +- servo/components/style/rule_tree/mod.rs | 54 ++---- servo/components/style/style_resolver.rs | 511 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ servo/components/style/stylist.rs | 83 ++++----- servo/components/style/traversal.rs | 238 ++++++++++--------------- servo/ports/geckolib/glue.rs | 63 +++---- 13 files changed, 895 insertions(+), 1672 deletions(-) That's almost half of the code deleted! \o/

Comment on attachment 8884691 [details] Make RuleTree::root return a reference instead of a strong pointer. https://reviewboard.mozilla.org/r/155560/#review160632

Comment on attachment 8884757 [details] Bug 1379505: Account for the page frame in UpdateStyleOfOwnedAnonBoxes. https://reviewboard.mozilla.org/r/155644/#review160634 ::: layout/generic/nsFrame.cpp:10214 (Diff revision 1) > - MOZ_ASSERT((!GetContent() && IsViewportFrame()) || > + MOZ_ASSERT(!GetContent() || !aChildFrame->GetContent() || > aChildFrame->GetContent() == GetContent(), > "What content node is it a frame for?"); If we don't want to explicitly enumerate the types of frames that are allowed to have null content, then I think the assertion should just be: MOZ_ASSERT(!GetContent() || aChildFrame->GetCotent() == GetContent(), ...) so that we don't allow aChildFrame to have null content but for this to have non-null content (which, AFAICT, shouldn't happen).

Regarding the get_animation_rules() de-optimization, does that mean that whenever we restyle an element with a current animation or transition, that we'll do the work of converting the animation values into a property declaration block twice, once in the animation-only restyle, and again in the regular restyle? And so every animation tick we'll end up creating two rule nodes for the animations (because the one generated in the animation-only restyle an the one generated in the regular restyle won't be ptr_equals)?

(In reply to Cameron McCormack (:heycam) from comment #18) > Regarding the get_animation_rules() de-optimization, does that mean that > whenever we restyle an element with a current animation or transition, that > we'll do the work of converting the animation values into a property > declaration block twice, once in the animation-only restyle, and again in > the regular restyle? And so every animation tick we'll end up creating two > rule nodes for the animations (because the one generated in the > animation-only restyle an the one generated in the regular restyle won't be > ptr_equals)? Yes, but only when there's actually an animation restyle and another kind of restyle on the same refresh driver tick, so I think it shouldn't be extremely common, if I understand how it all works, given we only set the animation_only_dirty_descendants bit during the animation traversal, and not the normal dirty bit.

Comment on attachment 8884688 [details] Remove unused AFFECTED_BY_PSEUDO_ELEMENTS StyleRelation. https://reviewboard.mozilla.org/r/155554/#review160970 ::: servo/components/style/matching.rs (Diff revision 1) > self.cascade_pseudos(context, data, CascadeVisitedMode::Unvisited); > } > > - // If we have any pseudo elements, indicate so in the primary StyleRelations. > - if !data.styles.pseudos.is_empty() { > - primary_results.relations |= AFFECTED_BY_PSEUDO_ELEMENTS; Seems like we should also remove the value `AFFECTED_BY_PSEUDO_ELEMENTS` in selectors/context.rs, right? It's now unused.

Comment on attachment 8884798 [details] Bug 1379505: Allow calling GetBaseComputedStylesForElement for an unstyled element. https://reviewboard.mozilla.org/r/155690/#review160996 LGTM

Comment on attachment 8884688 [details] Remove unused AFFECTED_BY_PSEUDO_ELEMENTS StyleRelation. https://reviewboard.mozilla.org/r/155554/#review160970 > Seems like we should also remove the value `AFFECTED_BY_PSEUDO_ELEMENTS` in selectors/context.rs, right? It's now unused. Yup, good catch, done :)

Comment on attachment 8884757 [details] Bug 1379505: Account for the page frame in UpdateStyleOfOwnedAnonBoxes. https://reviewboard.mozilla.org/r/155644/#review160634 > If we don't want to explicitly enumerate the types of frames that are allowed to have null content, then I think the assertion should just be: > > MOZ_ASSERT(!GetContent() || > aChildFrame->GetCotent() == GetContent(), > ...) > > so that we don't allow aChildFrame to have null content but for this to have non-null content (which, AFAICT, shouldn't happen). This definitely happens with page frames, as weird as it may be...

This is now totally green on try, yay! :)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #49) > This is now totally green on try, yay! :) Err, I was wrong and the "XBL on the root" fix actually made things worse... working on a fix.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #50) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #49) > > This is now totally green on try, yay! :) > > Err, I was wrong and the "XBL on the root" fix actually made things worse... > working on a fix. But note that only the three "XBL on the root when it's unstyled" test-cases are broken right now, and I know the cause (I want to come up with the proper fix though). The rest should be good to go. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3704668bda646f6dc5921c9ffac0339e269bf877

Ok, I think I'm happy with that last one approach, let's see if try thinks the same: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4b677ed7b08269978ad46cfbf1e6d213327d00c Going to sleep for today :)

Comment on attachment 8884689 [details] style: Undo the optimization for grabbing animation rules from the style data. https://reviewboard.mozilla.org/r/155556/#review161062 ::: servo/components/style/matching.rs:240 (Diff revision 3) > } > } > > /// Returns whether there might be visited values that should be inserted > /// within the regular computed values based on the cascade mode. > - fn visited_values_for_insertion(&self) -> bool { > + pub fn visited_values_for_insertion(&self) -> bool { I think this belongs in the "style: Introduce StyleResolverForElement." patch. ::: servo/components/style/matching.rs:261 (Diff revision 3) > *self == CascadeVisitedMode::Unvisited > } > > /// Returns whether the cascade should filter to only visited dependent > /// properties based on the cascade mode. > - fn visited_dependent_only(&self) -> bool { > + pub fn visited_dependent_only(&self) -> bool { This too. ::: servo/components/style/matching.rs:450 (Diff revision 3) > let only_default_rules = context.shared.traversal_flags.for_default_styles(); > if pseudo.is_eager() && !only_default_rules { > debug_assert!(pseudo.is_before_or_after()); > let parent = self.parent_element().unwrap(); > if !parent.may_have_animations() || > - primary_inputs.rules().get_animation_rules().is_empty() { > + self.get_animation_rules().is_empty() { I wonder if it's worth having a function on TElement that returns whether we have any animation rules, without having to construct them? ::: servo/components/style/matching.rs:998 (Diff revision 3) > - fn match_and_cascade(&self, > + fn match_and_cascade( > + &self, > - context: &mut StyleContext<Self>, > + context: &mut StyleContext<Self>, > - data: &mut ElementData, > + data: &mut ElementData, > - sharing: StyleSharingBehavior) > - -> ChildCascadeRequirement > + sharing: StyleSharingBehavior > + ) -> ChildCascadeRequirement { > - { Since this reformatting (and the reformatting in the following two patch hunks) seems unrelated to the point of this patch, it really belonged in a separate formatting only patch. (It does take me more time -- not much, but non-zero -- to review when I'm hunting for the effect of the change, when I think it's related to the intent of the patch.)

Comment on attachment 8884692 [details] style: Introduce StyleResolverForElement. https://reviewboard.mozilla.org/r/155562/#review161074 r=me with an answer to the can_be_fragmented question. ::: servo/components/style/style_resolver.rs:30 (Diff revision 3) > +where > + 'b: 'a, > + E: TElement + MatchMethods + 'static, > +{ > + element: E, > + context: &'a mut StyleContext<'b, E>, Incidentally, what advantage is there of doing this, rather than just: content: &'a mut StyleContext<'a, E>, ? ::: servo/components/style/style_resolver.rs:39 (Diff revision 3) > +struct MatchingResults { > + rule_node: StrongRuleNode, > + relevant_link_found: bool, > +} > + > +/// The primary style of an element or pseudo-element. Should this say something like "or an element-backed pseudo-element" (or whatever the right term is)? ::: servo/components/style/style_resolver.rs:70 (Diff revision 3) > + pub fn resolve_primary_style( > + &mut self, > + parent_style: Option<&ComputedValues>, > + layout_parent_style: Option<&ComputedValues>, > + ) -> PrimaryStyle { > + let primary_results = It looks like we've dropped a lot of comments that are helpful for understanding the code in this file. (I'm comparing with the functions in matching.rs that these functions are equivalent to.) Can we port them over? ::: servo/components/style/style_resolver.rs:86 (Diff revision 3) > + }; > + > + let mut visited_style = None; > + let should_compute_visited_style = > + relevant_link_found || > + parent_style.map_or(false, |s| s.get_visited_style().is_some()); This could be parent_style.and_then(|s| s.get_visited_style()).is_some() ::: servo/components/style/style_resolver.rs:151 (Diff revision 3) > + pseudo_styles.set(&pseudo, style); > + } > + }) > + } > + > + return ElementStyles { Nit: Drop the "return"? ::: servo/components/style/style_resolver.rs:213 (Diff revision 3) > + debug!("Match primary for {:?}, visited: {:?}", > + self.element, visited_handling); > + let mut applicable_declarations = ApplicableDeclarationList::new(); > + > + let stylist = &self.context.shared.stylist; > + let style_attribute = self.element.style_attribute(); Nit: Can we move this into the block where we call push_applicable_declarations? ::: servo/components/style/style_resolver.rs:225 (Diff revision 3) > + Some(bloom_filter), > + visited_handling, > + self.context.shared.quirks_mode > + ); > + > + let implemented_pseudo = self.element.implemented_pseudo_element(); Is it that we don't need to do the "if this is an eager pseudo, just grab rules off the parent" thing here, because implemented_pseudo will only be Some if this is a lazy pseudo? ::: servo/components/style/style_resolver.rs:239 (Diff revision 3) > + style_attribute, > + smil_override, > + animation_rules, Nit: I think it would as clear to just call self.element.{style_attribute,get_smil_override,get_animation_rules} directly in the push_applicable_declarations call (and less code). ::: servo/components/style/style_resolver.rs:371 (Diff revision 3) > + > + cascade_info.finish(&self.element.as_node()); > + values Where does the Servo-only can_be_fragmented stuff go?

Comment on attachment 8884693 [details] stylo: Rewrite getComputedStyle/getDefaultComputedStyle using StyleResolverForElement. https://reviewboard.mozilla.org/r/155564/#review161116 I like it! :) ::: servo/components/style/style_resolver.rs:24 (Diff revision 3) > -pub struct StyleResolverForElement<'a, 'b, E> > +pub struct StyleResolverForElement<'a, 'ctx, 'le, E> > where > - 'b: 'a, > - E: TElement + MatchMethods + 'static, > + 'ctx: 'a, > + 'le: 'ctx, > + E: TElement + MatchMethods + 'le, > { > element: E, > - context: &'a mut StyleContext<'b, E>, > + context: &'a mut StyleContext<'ctx, E>, > rule_inclusion: RuleInclusion, > + _marker: ::std::marker::PhantomData<&'le E>, These changes should probably be in the previous patch. ::: servo/components/style/traversal.rs:539 (Diff revision 3) > + debug_assert!(element.borrow_data().map_or(true, |d| !d.has_styles()), > + "Why being here?"); Nit: s/being/are we/ (or perhaps s/being/be/) ::: servo/components/style/traversal.rs:575 (Diff revision 3) > + if context.thread_local.bloom_filter.is_empty() { > + context.thread_local.bloom_filter.rebuild(*ancestor); > - } > + } Can we pull this out of the loop, rebuild() it with ancestor's parent, then call bloom_filter.push() at the top of the loop? Shouldn't need the is_empty() check then either.

Comment on attachment 8884693 [details] stylo: Rewrite getComputedStyle/getDefaultComputedStyle using StyleResolverForElement. https://reviewboard.mozilla.org/r/155564/#review161120

Comment on attachment 8884694 [details] Bug 1379505: Rewrite restyling to split between resolving styles and handling changes. https://reviewboard.mozilla.org/r/155566/#review161122 ::: servo/components/style/context.rs:233 (Diff revision 5) > } > inputs > })) > } > > - /// Returns whether there are any pseudo inputs. > + /// Grabs a reference to the list of rules, if they exist. Nit: Maybe "Takes the list of rules, if the exist", since the function doesn't return a reference. ::: servo/components/style/matching.rs (Diff revision 5) > - context.thread_local > - .style_sharing_candidate_cache > - .insert_if_possible(self, > - data.styles.primary(), > - primary_results.relations, > - validation_data, > - dom_depth); With this removed, where do we insert into the style sharing cache now? ::: servo/components/style/matching.rs:504 (Diff revision 5) > - if source.is_some() { > - trace!(" > {:?}", source); > - } > - } > + } > + > + // If it matches different pseudos, reconstruct. Nit: This comment sounds a bit like this is the only test done for different pseudos. How about instead: // If it matched a different number of pseudos, reconstruct. ::: servo/components/style/matching.rs:540 (Diff revision 5) > /// TODO(emilio): This is somewhat inefficient, because of a variety of > /// reasons: I guess this is just one reason now instead of a variety of reasons? :) ::: servo/components/style/matching.rs:849 (Diff revision 5) > // This currently ignores visited styles, which seems acceptable, > // as existing browsers don't appear to animate visited styles. I just noticed this comment seems in opposition to the comment in get_after_change_style saying that we should animate visited styles... ::: servo/components/style/style_resolver.rs:41 (Diff revision 5) > /// The primary style of an element or pseudo-element. > pub struct PrimaryStyle { > /// The style per se. > pub style: Arc<ComputedValues>, > +} At this point is it worth keeping this struct? I guess it does give us a small amount of type safety as resolve_pseudo_style's originating_element_style argument's type. ::: servo/components/style/traversal.rs:794 (Diff revision 5) > } > > context.thread_local.statistics.elements_matched += 1; > > // Perform the matching and cascading. > - element.match_and_cascade( > + important_rules_changed = true; Do we want to more accurately determine this? Or is it not a big deal that we'll post a sequential task if we restyle while animations are running to re-check whether compositor animations should be updated/stopped? ::: servo/ports/geckolib/glue.rs:679 (Diff revision 5) > - let pseudo = PseudoElement::from_pseudo_type(pseudo_type); > - let pseudos = &styles.pseudos; > - let pseudo_style = match pseudo { > - Some(ref p) => { > - let style = pseudos.get(p); > - debug_assert!(style.is_some()); > + if let Some(pseudo) = PseudoElement::from_pseudo_type(pseudo_type) { > + // This style already doesn't have animations. > + return styles > + .pseudos > + .get(&pseudo) > + .expect("GetBaseComputedValuesForElement for an unexisting pseudo?") Nit: s/an unexisting/a non-existent/

Comment on attachment 8884690 [details] style: Remove unnecessary TraversalFlags::FOR_DEFAULT_STYLES. https://reviewboard.mozilla.org/r/155558/#review161132

Comment on attachment 8885047 [details] Bug 1379505: Less fishyness when resolving the style of the document element. https://reviewboard.mozilla.org/r/155914/#review161136 ::: layout/base/nsCSSFrameConstructor.cpp:2581 (Diff revision 3) > + // See the comment in AddFrameConstructionItemsInternal for why this is > + // needed. > + mPresShell->StyleSet()->AsServo()->StyleNewChildren(aDocElement); Is this specifically to handle the case where -moz-binding handle a url() but it was invalid? If so, then I think the comment in AddFrameConstructionItemsInternal doesn't really mention that. So it might be worth saying it here.

Comment on attachment 8885071 [details] style: Derive Default for EagerPseudoStyles. https://reviewboard.mozilla.org/r/155922/#review161142

Comment on attachment 8885072 [details] Bug 1379505: Update reftest expectations. https://reviewboard.mozilla.org/r/155924/#review161144 I'm still curious why these tests start passing...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a04afd93a0a779a76a64ca757d7fdf1f4d5d0a6 Actually all those failures in my previous link are because the CurrentElementInfo::is_initial_style thingie (because we clear the restyle flag in some cases), so I'm reverting that for now.

Comment on attachment 8884689 [details] style: Undo the optimization for grabbing animation rules from the style data. https://reviewboard.mozilla.org/r/155556/#review161062 > I think this belongs in the "style: Introduce StyleResolverForElement." patch. Will move. > This too. Will move too. > I wonder if it's worth having a function on TElement that returns whether we have any animation rules, without having to construct them? Makes sense, though this goes away on the next patch anyway.

Comment on attachment 8884692 [details] style: Introduce StyleResolverForElement. https://reviewboard.mozilla.org/r/155562/#review161074 > It looks like we've dropped a lot of comments that are helpful for understanding the code in this file. (I'm comparing with the functions in matching.rs that these functions are equivalent to.) Can we port them over? Sure, on it. > This could be > > parent_style.and_then(|s| s.get_visited_style()).is_some() nice :) > Nit: Can we move this into the block where we call push_applicable_declarations? Agreed, will do. > Is it that we don't need to do the "if this is an eager pseudo, just grab rules off the parent" thing here, because implemented_pseudo will only be Some if this is a lazy pseudo? Right, we don't need to. It's pretty much an optimization (of dubious worthiness), but it can't be here because I don't want to rely on the parent data. I plan to add it back (in the traversal code) in a followup bug. > Nit: I think it would as clear to just call self.element.{style_attribute,get_smil_override,get_animation_rules} directly in the push_applicable_declarations call (and less code). Agreed. > Where does the Servo-only can_be_fragmented stuff go? Whoopsies... Fixed.

Comment on attachment 8884693 [details] stylo: Rewrite getComputedStyle/getDefaultComputedStyle using StyleResolverForElement. https://reviewboard.mozilla.org/r/155564/#review161116 > Can we pull this out of the loop, rebuild() it with ancestor's parent, then call bloom_filter.push() at the top of the loop? Shouldn't need the is_empty() check then either. Yes, will do

Comment on attachment 8884694 [details] Bug 1379505: Rewrite restyling to split between resolving styles and handling changes. https://reviewboard.mozilla.org/r/155566/#review161122 > With this removed, where do we insert into the style sharing cache now? I moved it into the MatchAndCascade branch (to preserve behaviour, but I think we could move it before the finish_restyle branch, will do in a followup). I also removed the optimization I added to fill the empty declaration blocks, because I added it without measuring, and now we don't have a StyleRelations there, and that allows doing the above. > I guess this is just one reason now instead of a variety of reasons? :) Yes, we fixed that long time ago. > I just noticed this comment seems in opposition to the comment in get_after_change_style saying that we should animate visited styles... Yeah, we probably need to animate it as well, will change the comment. > At this point is it worth keeping this struct? I guess it does give us a small amount of type safety as resolve_pseudo_style's originating_element_style argument's type. Yeah, I think it's ok to keep it. We can remove it if it becomes annoying. > Do we want to more accurately determine this? Or is it not a big deal that we'll post a sequential task if we restyle while animations are running to re-check whether compositor animations should be updated/stopped? I don't think it's a big deal, since we need to update the cascade anyway, and the task will be posted regardless... I can add a comment though.

Comment on attachment 8885047 [details] Bug 1379505: Less fishyness when resolving the style of the document element. https://reviewboard.mozilla.org/r/155914/#review161136 > Is this specifically to handle the case where -moz-binding handle a url() but it was invalid? If so, then I think the comment in AddFrameConstructionItemsInternal doesn't really mention that. So it might be worth saying it here. Well, the comment there says: // For -moz-binding URLs that can // be resolved, we will load the binding above, which will style the // children after they have been rearranged in the flattened tree. // If the URL couldn't be resolved, we still need to style the children, // so we do that here. But I can also add it here I guess.

(In reply to Cameron McCormack (:heycam) from comment #64) > Comment on attachment 8885072 [details] > Bug 1379505: Update reftest expectations. > > https://reviewboard.mozilla.org/r/155924/#review161144 > > I'm still curious why these tests start passing... FWIW, I'm pretty sure this is because of the root element properly skipping styling of children when it has XBL bindings, which didn't happen before. A lot of the failures in comment 55 are very similar.

Comment on attachment 8884694 [details] Bug 1379505: Rewrite restyling to split between resolving styles and handling changes. https://reviewboard.mozilla.org/r/155566/#review161506

Comment on attachment 8885285 [details] style: Remove StyleRelations. https://reviewboard.mozilla.org/r/156150/#review161508 ::: servo/components/style/stylist.rs (Diff revision 1) > applicable_declarations, > context, > self.quirks_mode, > flags_setter, > CascadeLevel::UANormal); > - debug!("UA normal: {:?}", context.relations); I never did like those debug!() statements. ;)

Thanks for doing this refactoring!

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/b61c652f8425 Account for the page frame in UpdateStyleOfOwnedAnonBoxes. r=heycam https://hg.mozilla.org/integration/autoland/rev/b6728735c6f6 Allow calling GetBaseComputedStylesForElement for an unstyled element. r=boris https://hg.mozilla.org/integration/autoland/rev/183625671488 Less fishyness when resolving the style of the document element. r=heycam https://hg.mozilla.org/integration/autoland/rev/039a8c4306ac Update reftest expectations. r=heycam

https://hg.mozilla.org/mozilla-central/rev/b61c652f8425 https://hg.mozilla.org/mozilla-central/rev/b6728735c6f6 https://hg.mozilla.org/mozilla-central/rev/183625671488 https://hg.mozilla.org/mozilla-central/rev/039a8c4306ac