1317016 - stylo: Drive style traversal with RestyleHints

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

I've been landing a lot of pieces on servo/servo directly, but this change is big enough that it's going to need a megapatch, at least with our current clunky workflow.

I have something that seems to work well for Gecko, though I haven't started writing the servo pieces yet.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Basic infrastructure for RestyleHint-driven traversal. Servo pieces missing. WIP

MozReview-Commit-ID: 7wH5XcILVmX

Comment 2

[Bobby Holley (:bholley)]

Updating the summary to be a little more informative.

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f487cb8d1d125aab3308eb9d51fa87dbfb1bac21

Comment 4

[Bobby Holley (:bholley)]

Attachment: Basic infrastructure for RestyleHint-driven traversal. WIP 2

Getting closer...

MozReview-Commit-ID: 7wH5XcILVmX

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8812699 [details] [diff] [review]
Basic infrastructure for RestyleHint-driven traversal. WIP 2

Review of attachment 8812699 [details] [diff] [review]:
-----------------------------------------------------------------

I need to run now, I haven't reviewed a lot of the rust stuff (only a bit of the glue). But looks good overall.

::: dom/base/ElementInlines.h
@@ +29,5 @@
> +inline Element*
> +Element::GetFlattenedTreeParentElement() const
> +{
> +  nsINode* parentNode = GetFlattenedTreeParentNode();
> +  return parentNode->IsElement() ? parentNode->AsElement() : nullptr;

probably want MOZ_LIKELY here.

@@ +55,5 @@
> +      return false;
> +    }
> +    nsINode* parentNode = curr->GetParentNode();
> +    curr = curr->GetFlattenedTreeParentElement();
> +    MOZ_ASSERT_IF(!curr, parentNode == parentNode->OwnerDoc());

nit: parentNode == OwnerDoc() may be clearer.

::: layout/base/RestyleManagerHandle.h
@@ +117,5 @@
>      inline void ContentInserted(nsINode* aContainer,
>                                  nsIContent* aChild);
>      inline void ContentAppended(nsIContent* aContainer,
>                                  nsIContent* aFirstNewContent);
> +    inline void EnsureStylesForInsert(nsINode* aContainer,

I'd personally prefer to use `AsServo` for this instead of empty RestyleManager methods, but I guess it's ok.

::: layout/base/ServoRestyleManager.cpp
@@ +88,3 @@
>  {
> +  aElement->ClearServoData();
> +  aElement->UnsetHasDirtyDescendantsForServo();

as an uber-nit, I'd prefer this to be cleared _after_ clearing the children, since this technically leaves the bits in an inconsistent state during this function.

Not a big deal of course.

@@ +113,4 @@
>    }
>  
> +  // If we have a frame and a non-zero + non-reconstruct change hint, we need to
> +  // attach a new style context.

Please add a note here about how frame construction both resolves the style and expects the old style there.

@@ +296,5 @@
> +    mReentrantChanges = &newChanges;
> +    while (!currentChanges.IsEmpty()) {
> +      ProcessRestyledFrames(currentChanges);
> +      MOZ_ASSERT(currentChanges.IsEmpty());
> +      for (ReentrantChange& change: newChanges)  {

I don't know the size of this array, but if you make ReentrantChangeList a normal change list, then you can just SwapElements + Clear here.

@@ +449,5 @@
>                                &restyleHint);
>  
>    EventStates previousState = aElement->StyleState() ^ aChangedBits;
> +  ServoElementSnapshot* snapshot = Servo_Element_GetSnapshot(aElement);
> +  if (snapshot) {

When can this be null? Shouldn't we ensure we have a snapshot?

(Maybe I discover later)

::: layout/base/nsDocumentViewer.cpp
@@ +4472,5 @@
>  
> +  // We're creating a new presentation context for an existing document.
> +  // Drop any associated Servo data.
> +#ifdef MOZ_STYLO
> +  Element* root = mDocument->GetRootElement();

nit: You can save a line with |if (Element* root = mDocument->GetRootElement())|

@@ +4473,5 @@
> +  // We're creating a new presentation context for an existing document.
> +  // Drop any associated Servo data.
> +#ifdef MOZ_STYLO
> +  Element* root = mDocument->GetRootElement();
> +  if (root) {

Also, you probably don't want to do it in the case we're using the old style back-end, but there's nothing here to prevent that, right?

::: layout/base/nsStyleChangeList.h
@@ +39,5 @@
>  
>    nsStyleChangeList() { MOZ_COUNT_CTOR(nsStyleChangeList); }
>    ~nsStyleChangeList() { MOZ_COUNT_DTOR(nsStyleChangeList); }
>    void AppendChange(nsIFrame* aFrame, nsIContent* aContent, nsChangeHint aHint);
> +  void SwapElements(nsStyleChangeList& aOther) { AutoTArray::SwapElements(aOther); }

|using base_type::SwapElements| instead.

::: layout/style/ServoStyleSet.cpp
@@ +450,4 @@
>  {
>    // Grab the root.
>    nsIDocument* doc = mPresContext->Document();
>    nsIContent* root = doc->GetRootElement();

nit: Element* root, and remove AsElement() below.

@@ +476,5 @@
> +void
> +ServoStyleSet::AssertTreeIsClean()
> +{
> +  Element* root = mPresContext->Document()->GetRootElement();
> +  if (root) {

nit: |if (Element* ...)|

::: layout/style/ServoTypes.h
@@ +34,5 @@
>    void Set(T arg) { value.value = arg; }
>    ServoCell() : value() {};
>  };
>  
> +enum ConsumeStyleBehavior {

Making these enum classes could make these more easily greppable across both Servo and Gecko, but this is fine too.

It should prevent stupid mistakes though, like switching the order of arguments, so I'd personally prefer it.

@@ +39,5 @@
> +  ConsumeStyle,
> +  DontConsumeStyle,
> +};
> +
> +enum MayComputeBehavior {

ComputeStyleBehavior { Maybe, Always } could be clearer, maybe?

::: layout/style/nsStyleSet.h
@@ +115,5 @@
>  
>    already_AddRefed<nsStyleContext>
>    ResolveStyleFor(mozilla::dom::Element* aElement,
>                    nsStyleContext* aParentContext,
> +                  mozilla::ConsumeStyleBehavior aConsume,

nit: Probably you can skip warnings removing the names of |aConsume| and |aMayCompute|, here and below.

::: servo/ports/geckolib/glue.rs
@@ +628,5 @@
> +    r
> +}
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_Element_GetSnapshot(element: RawGeckoElementBorrowed) -> *mut structs::ServoElementSnapshot

Ok, makes sense, so if the element is not styled yet we return null here. Is there any other case we're returning a null snapshot?

@@ +695,5 @@
> +
> +    if compute == bindings::MayComputeBehavior::MayComputeStyle {
> +        let should_compute = match element.styling_mode() {
> +            StylingMode::Initial => true,
> +            StylingMode::Restyle => true,

nit: Use | here.

@@ +706,5 @@
> +            let mut per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
> +            let shared_style_context = create_shared_context(&mut per_doc_data);
> +            let context = StandaloneStyleContext::new(&shared_style_context);
> +            recalc_style_at::<_, _, RecalcStyleOnly>(&context, element.as_node().opaque(), element);
> +            unsafe { element.set_dirty_descendants() };

This seems slightly out of place, doesn't it? I'm not reviewing this file thoroughly yet, but probably a comment here would be nice.

@@ +728,5 @@
> +}
> +
> +#[cfg(debug_assertions)]
> +#[no_mangle]
> +pub extern "C" fn Servo_AssertTreeIsClean(root: RawGeckoElementBorrowed) {

Probably if cfg!(not(debug_assertions)) { panic!("Servo_AssertTreeIsClean called on release build") }?

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8812699 [details] [diff] [review]
Basic infrastructure for RestyleHint-driven traversal. WIP 2

Review of attachment 8812699 [details] [diff] [review]:
-----------------------------------------------------------------

I took a quick look through the C++ changes and nothing really jumped out to me, so f+ from me.

::: layout/base/ServoRestyleManager.cpp
@@ +269,2 @@
>  
> +  // XXXbholley: Should this be while() per bug 1316247?

I guess the while you have below should be enough, since that's where we're accumulating reentrantly generated restyle hints now.  Maybe assert after the while loop that !HasPendingRestyles()?  Although I guess that's obviously going to hold, since we're using mInStyleRefresh to ensure we don't go through the normal restyle event posting path.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> Comment on attachment 8812699 [details] [diff] [review]
> Basic infrastructure for RestyleHint-driven traversal. WIP 2
> 
> Review of attachment 8812699 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I took a quick look through the C++ changes and nothing really jumped out to
> me, so f+ from me.
> 
> ::: layout/base/ServoRestyleManager.cpp
> @@ +269,2 @@
> >  
> > +  // XXXbholley: Should this be while() per bug 1316247?
> 
> I guess the while you have below should be enough, since that's where we're
> accumulating reentrantly generated restyle hints now.

Whoops yeah, that was a holdover from before I fixed that.

>  Maybe assert after
> the while loop that !HasPendingRestyles()?  Although I guess that's
> obviously going to hold, since we're using mInStyleRefresh to ensure we
> don't go through the normal restyle event posting path.

I'll assert it. Thanks!

Comment 9

[Bobby Holley (:bholley)]

Addressed this feedback, will upload a new patch shortly.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> probably want MOZ_LIKELY here.

Added.

> nit: parentNode == OwnerDoc() may be clearer.

Fixed.


> I'd personally prefer to use `AsServo` for this instead of empty
> RestyleManager methods, but I guess it's ok.

I agree that's nicer. I've gone ahead and done it.
 
> ::: layout/base/ServoRestyleManager.cpp
> @@ +88,3 @@
> >  {
> > +  aElement->ClearServoData();
> > +  aElement->UnsetHasDirtyDescendantsForServo();
> 
> as an uber-nit, I'd prefer this to be cleared _after_ clearing the children,
> since this technically leaves the bits in an inconsistent state during this
> function.

Fair point, I've fixed it.

> Please add a note here about how frame construction both resolves the style
> and expects the old style there.

Done.

> I don't know the size of this array, but if you make ReentrantChangeList a
> normal change list, then you can just SwapElements + Clear here.

I can't, because it's not a change list. See the comment about it being too early to resolve an nsIFrame. I've moved that comment to the definition of ReentrantChange.

> 
> @@ +449,5 @@
> >                                &restyleHint);
> >  
> >    EventStates previousState = aElement->StyleState() ^ aChangedBits;
> > +  ServoElementSnapshot* snapshot = Servo_Element_GetSnapshot(aElement);
> > +  if (snapshot) {
> 
> When can this be null? Shouldn't we ensure we have a snapshot?

It is null when the element has never been styled. I'll add a comment.

> nit: You can save a line with |if (Element* root =
> mDocument->GetRootElement())|

Not doing this because I now need two conditions due to the next point.

> Also, you probably don't want to do it in the case we're using the old style
> back-end, but there's nothing here to prevent that, right?

Good catch.

> |using base_type::SwapElements| instead.

Good point, but I actually don't need this method anymore at all, since I had to make ReentrantChange a separate type. I've removed it.

> nit: Element* root, and remove AsElement() below.

Fixed, thanks.

> nit: |if (Element* ...)|

Fixed.

> 
> ::: layout/style/ServoTypes.h
> @@ +34,5 @@
> >    void Set(T arg) { value.value = arg; }
> >    ServoCell() : value() {};
> >  };
> >  
> > +enum ConsumeStyleBehavior {
> 
> Making these enum classes could make these more easily greppable across both
> Servo and Gecko, but this is fine too.
> 
> It should prevent stupid mistakes though, like switching the order of
> arguments, so I'd personally prefer it.

Yeah, I opted against it for verbosity reasons, but I think you're right that type safety wins. I'll change it.

> 
> @@ +39,5 @@
> > +  ConsumeStyle,
> > +  DontConsumeStyle,
> > +};
> > +
> > +enum MayComputeBehavior {
> 
> ComputeStyleBehavior { Maybe, Always } could be clearer, maybe?

That's not really accurate, since it's really a question of allowing lazy computation, or asserting that  the computation has been done eagerly. I've changed it to LazyComputeBehavior { Allow, Assert}.

> nit: Probably you can skip warnings removing the names of |aConsume| and
> |aMayCompute|, here and below.

Fixed.

> > +pub extern "C" fn Servo_Element_GetSnapshot(element: RawGeckoElementBorrowed) -> *mut structs::ServoElementSnapshot
> 
> Ok, makes sense, so if the element is not styled yet we return null here. Is
> there any other case we're returning a null snapshot?

Nope, I've added a comment.

> 
> @@ +695,5 @@
> > +
> > +    if compute == bindings::MayComputeBehavior::MayComputeStyle {
> > +        let should_compute = match element.styling_mode() {
> > +            StylingMode::Initial => true,
> > +            StylingMode::Restyle => true,
> 
> nit: Use | here.

I switched the logic around.

> 
> @@ +706,5 @@
> > +            let mut per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
> > +            let shared_style_context = create_shared_context(&mut per_doc_data);
> > +            let context = StandaloneStyleContext::new(&shared_style_context);
> > +            recalc_style_at::<_, _, RecalcStyleOnly>(&context, element.as_node().opaque(), element);
> > +            unsafe { element.set_dirty_descendants() };
> 
> This seems slightly out of place, doesn't it? I'm not reviewing this file
> thoroughly yet, but probably a comment here would be nice.

The set_dirty_descendants, or something else? I'll add a comment for it.

> Probably if cfg!(not(debug_assertions)) { panic!("Servo_AssertTreeIsClean
> called on release build") }?

Hm, how come? Isn't it better to catch that error at link time than at runtime?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> Hm, how come? Isn't it better to catch that error at link time than at
> runtime?

Ofc, missed the `cfg` on top of the function, you're right :)

Comment 11

[Bobby Holley (:bholley)]

Attachment: Basic infrastructure for RestyleHint-driven traversal. WIP 3

MozReview-Commit-ID: 7wH5XcILVmX

Comment 12

[Bobby Holley (:bholley)]

Attachment: Basic infrastructure for RestyleHint-driven traversal.

MozReview-Commit-ID: 7wH5XcILVmX

Comment 13

[Bobby Holley (:bholley)]

Normal try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff2dbb6102727e841628f072aae9794b12eda18e

stylo try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eac2a3f8096abc7c10ee02d2772b51bcbc55e2f

Servo PR: https://github.com/servo/servo/pull/14300

Hitting try crashes on nsCSSFrameConstructor::GetInsertionPrevSibling, but works on Wikipedia, and giving the pending US holidays I decided with emilio and heycam to push it and handle things as followups. Cameron said he could look at the try crashes tomorrow.

I'll wait for the Servo PR to hit and then push this to inbound.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c852ac4acf8
Basic infrastructure for RestyleHint-driven traversal. r=emilio

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7c852ac4acf8