1290335 - stylo: Process change hints and trigger frame construction

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

This may require a bit more work, but I think it's worth visiting.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: stylo: Hoist OverflowChangedTracker to its own file.

Review commit: https://reviewboard.mozilla.org/r/67902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67902/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: Hoist frame-construction logic in RestyleManager to static members in RestyleManagerBase.

Review commit: https://reviewboard.mozilla.org/r/67904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67904/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: Improve error message of the anonymous box assertion.

Review commit: https://reviewboard.mozilla.org/r/67906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67906/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: Hoist GetNextContinuationWithSameStyle to RestyleManagerBase.

Review commit: https://reviewboard.mozilla.org/r/67908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67908/

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: stylo: Allow processing change hints generated from Servo.

Review commit: https://reviewboard.mozilla.org/r/67910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67910/

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: stylo: Assert the snapshot is taken in ServoRestyleManager::AttributeChanged.

Review commit: https://reviewboard.mozilla.org/r/67912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67912/

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8775849 [details]
Bug 1290335: stylo: Hoist OverflowChangedTracker to its own file.

https://reviewboard.mozilla.org/r/67902/#review64962

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8775851 [details]
Bug 1290335: Improve error message of the anonymous box assertion.

https://reviewboard.mozilla.org/r/67906/#review64964

::: layout/style/ServoStyleSet.cpp:15
(Diff revision 1)
>  #include "nsCSSAnonBoxes.h"
>  #include "nsCSSPseudoElements.h"
>  #include "nsIDocumentInlines.h"
>  #include "nsStyleContext.h"
>  #include "nsStyleSet.h"
> +#include "nsPrintfCString.h"

Nit: two lines up to keep the includes sorted.

::: layout/style/ServoStyleSet.cpp:198
(Diff revision 1)
> +    NS_ERROR(nsPrintfCString("stylo: could not get anon-box: %s", pseudo).get());
> +    MOZ_CRASH();

MOZ_ASSERT(false, nsPrintfCString(...).get()); ?

Comment 9

[Bobby Holley (:bholley)]

https://reviewboard.mozilla.org/r/67910/#review64972

The problem with this approach is that Gecko relies on the difference between Peek and Get to determine which of the style structs on the old context were actually computed, and avoids diffing them in that case. FakeStyleContext does the same thing for Peek() and Get(), and so that distinction is lost. This is why I made the old values a style context, rather than a ComputedValues.

I think we can expose both APIs if we need to, but we probably want to pass the style context where we can.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/67906/#review64964

> MOZ_ASSERT(false, nsPrintfCString(...).get()); ?

You can't do that, MOZ_ASSERT expects a literal for token-pasting.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775849 [details]
Bug 1290335: stylo: Hoist OverflowChangedTracker to its own file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67902/diff/1-2/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775850 [details]
Bug 1290335: Hoist frame-construction logic in RestyleManager to static members in RestyleManagerBase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67904/diff/1-2/

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775851 [details]
Bug 1290335: Improve error message of the anonymous box assertion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67906/diff/1-2/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775852 [details]
Bug 1290335: Hoist GetNextContinuationWithSameStyle to RestyleManagerBase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67908/diff/1-2/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775853 [details]
Bug 1290335: stylo: Allow processing change hints generated from Servo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67910/diff/1-2/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775854 [details]
Bug 1290335: stylo: Assert the snapshot is taken in ServoRestyleManager::AttributeChanged.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67912/diff/1-2/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: Reuse the OverflowChangedTracker between both restyle managers.

Review commit: https://reviewboard.mozilla.org/r/68442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68442/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: Implement dumb versions of RestyleForAppend and RestyleForInsertOrChange

This lets us take rid of the delay-layout hack on the stylo branch.

Review commit: https://reviewboard.mozilla.org/r/68444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68444/

Comment 19

[Cameron McCormack (:heycam)]

Comment on attachment 8775850 [details]
Bug 1290335: Hoist frame-construction logic in RestyleManager to static members in RestyleManagerBase.

https://reviewboard.mozilla.org/r/67904/#review65630

::: layout/base/RestyleManager.h:491
(Diff revision 2)
>    void StyleChangeReflow(nsIFrame* aFrame, nsChangeHint aHint);
>  
>    // Recursively add all the given frame and all children to the tracker.
>    void AddSubtreeToOverflowTracker(nsIFrame* aFrame);

Remove these two declarations now that we've removed the methods from RestyleManager.

::: layout/base/RestyleManagerBase.h:107
(Diff revision 2)
> +   * These are protected static methods that help with the frame construction
> +   * bits of the restyle managers.

Nit: GetNearestAncestorFrame and ProcessRestyledFrames relate to change hint processing, rather than frame reconstruction specifically (which is a particular kind of change hint processing).

::: layout/base/RestyleManagerBase.h:119
(Diff revision 2)
> +  static nsIFrame*
> +  GetNextBlockInInlineSibling(FramePropertyTable* aPropTable, nsIFrame* aFrame);
> +
> +  static nsresult
> +  ProcessRestyledFrames(nsStyleChangeList& aChangeList,
> +                        nsPresContext& aPresContext,

Nit: it's more idiomatic in Gecko code to pass nsPresContext by pointer.  But also, can we just use mPresContext and make this non-static?

::: layout/base/RestyleManagerBase.cpp:759
(Diff revision 2)
> +/* static */ void
> +SyncViewsAndInvalidateDescendants(nsIFrame* aFrame,
> +                                  nsChangeHint aChange)
> +{

We dropped the NS_PRECONDITION(gInApplyRenderingChangeToTree, ...) at the top of this function.

Comment 20

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/67904/#review65634

::: layout/base/RestyleManager.cpp:3423
(Diff revision 2)
>    selectorsForDescendants.AppendElements(
>        aRestyleHintData.mSelectorsForDescendants);
>    nsTArray<nsIContent*> visibleKidsOfHiddenElement;
>    nsIFrame* nextIBSibling;
>    for (nsIFrame* ibSibling = aFrame; ibSibling; ibSibling = nextIBSibling) {
> -    nextIBSibling = GetNextBlockInInlineSibling(propTable, ibSibling);
> +    nextIBSibling = RestyleManager::GetNextBlockInInlineSibling(propTable, ibSibling);

RestyleManagerBase

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 8775852 [details]
Bug 1290335: Hoist GetNextContinuationWithSameStyle to RestyleManagerBase.

https://reviewboard.mozilla.org/r/67908/#review65644

::: layout/base/RestyleManager.cpp:1736
(Diff revision 2)
>    if (aFrame->GetContent()->HasFlag(mRestyleTracker.RootBit())) {
>      aRestyleRoot = aFrame->GetContent()->AsElement();
>    }
>  
>    for (nsIFrame* f = aFrame; f;
> -       f = GetNextContinuationWithSameStyle(f, f->StyleContext())) {
> +       f = RestyleManager::GetNextContinuationWithSameStyle(f, f->StyleContext())) {

RestyleManagerBase (and elsewhere in the patch)

::: layout/base/RestyleManagerBase.cpp:918
(Diff revision 2)
> +RestyleManagerBase::GetNextContinuationWithSameStyle(nsIFrame* aFrame,
> +                                                     nsStyleContext* aOldStyleContext,
> +                                                     bool* aHaveMoreContinuations)

Nit: rewrap to fit 80 columns.  (I tend to use this style:

RestyleManagerBase::GetNextContinuationWithSameStyle(
    nsIFrame* aFrame,
    ...)
{
}

)

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1290335: Make ProcessRestyledFrames non static, and tidy up a bit.

Review commit: https://reviewboard.mozilla.org/r/68752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68752/

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775852 [details]
Bug 1290335: Hoist GetNextContinuationWithSameStyle to RestyleManagerBase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67908/diff/2-3/

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775853 [details]
Bug 1290335: stylo: Allow processing change hints generated from Servo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67910/diff/2-3/

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775854 [details]
Bug 1290335: stylo: Assert the snapshot is taken in ServoRestyleManager::AttributeChanged.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67912/diff/2-3/

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8776783 [details]
Bug 1290335: Reuse the OverflowChangedTracker between both restyle managers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68442/diff/1-2/

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8776784 [details]
Bug 1290335: Implement dumb versions of RestyleForAppend and RestyleForInsertOrChange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68444/diff/1-2/

Comment 28

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/67908/#review65644

> Nit: rewrap to fit 80 columns.  (I tend to use this style:
> 
> RestyleManagerBase::GetNextContinuationWithSameStyle(
>     nsIFrame* aFrame,
>     ...)
> {
> }
> 
> )

I clang-formatted it, and also other parts of the file (didn't include things that were a bit discussible).

Comment 29

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/67904/#review65630

> Nit: it's more idiomatic in Gecko code to pass nsPresContext by pointer.  But also, can we just use mPresContext and make this non-static?

I'd personally love to see more uses of references across the Gecko codebase, but yeah, I can do that.

> We dropped the NS_PRECONDITION(gInApplyRenderingChangeToTree, ...) at the top of this function.

Good catch!

Comment 30

[Cameron McCormack (:heycam)]

Comment on attachment 8775853 [details]
Bug 1290335: stylo: Allow processing change hints generated from Servo.

https://reviewboard.mozilla.org/r/67910/#review65646

::: layout/base/ServoRestyleManager.h:98
(Diff revision 3)
> -   * can access to the GetContext method without making it available to everyone
> -   * else.
>     */
> -  static void RecreateStyleContexts(nsIContent* aContent,
> +  void RecreateStyleContexts(nsIContent* aContent,
> -                                    nsStyleContext* aParentContext,
> +                             nsStyleContext* aParentContext,
> -                                    ServoStyleSet* aStyleSet);
> +                             ServoStyleSet& aStyleSet,

Nit: I think it's more idiomatic to pass the style set by pointer.

(I probably would struggle to explain a general rule for when pointers and when references are used in Gecko, but I don't see other uses of nsStyleSet&, but I do see various uses of nsStyleSet*.)

::: layout/base/ServoRestyleManager.cpp:78
(Diff revision 3)
> -                                           ServoStyleSet* aStyleSet)
> +                                           ServoStyleSet& aStyleSet,
> +                                           nsStyleChangeList& aChangeListToProcess)
>  {
>    nsIFrame* primaryFrame = aContent->GetPrimaryFrame();
> +  if (!primaryFrame && !aContent->IsDirtyForServo()) {
> +    NS_WARNING("Frame not found for non-dirty content");

Will this warn for display:none content that we have already styled?  Or are we changing how the dirty bits are used for display:none content?

::: layout/base/ServoRestyleManager.cpp:87
(Diff revision 3)
> +  if (aContent->IsDirtyForServo()) {
> +    nsChangeHint changeHint;
> +    if (primaryFrame) {
> +      changeHint = primaryFrame->StyleContext()->ConsumeStoredChangeHint();
> +    } else {
> +      changeHint = nsChangeHint_ReconstructFrame;

Why do we generate ReconstructFrame here?  Might we previously have had a display:none element, which we restyled and then remained display:none?  I don't think we want to try to reconstruct its frame in that case -- any change in display should produce a change hint to consume.

Oh, but now I see, the stored change hint is on the style context, so we don't have a way to get the previous style for the display:none element.

Per IRC conversion, I think we should be using the frame constructor's undisplayed node map to store style contexts for display:none children of displayed content.  Note that in stock Gecko's ElementRestyler::RestyleUndisplayedNodes, we don't do a full CalcStyleDifference -- instead we just compare 'display' values, and if there was a difference we generate ReconstructFrame.  And if not, we put the new style context back into the UndisplayedNode entry.

This is fine to do as a followup, but please add a (brief) comment in here saying we should be doing this and not unconditionally generating ReconstructFrame for frame-less elments.

::: layout/base/ServoRestyleManager.cpp:125
(Diff revision 3)
> +    for (nsIFrame* f = primaryFrame; f;
> +         f = GetNextContinuationWithSameStyle(f, oldStyleContext)) {
> +      f->SetStyleContext(newContext);
> +    }
> +
> +    // TODO: pseudos are the pending continuations, we can get its PseudoTag.

By "pending continuations" do you mean "continuations of primaryFrame that we still need to restyle"?

::: layout/base/ServoRestyleManager.cpp:137
(Diff revision 3)
> +    MOZ_ASSERT(primaryFrame,
> +               "Frame construction should be scheduled, and it takes the "
> +               "correct style for the children");
>      FlattenedChildIterator it(aContent);
>      for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {
> -      RecreateStyleContexts(n, primaryFrame->StyleContext(), aStyleSet);
> +      RecreateStyleContexts(n, primaryFrame->StyleContext(),

Just FYI, something to handle at some point: with display:contents, it is possible for an element's primary frame's style context on inherit from a grandparent element's primary frame's style context.  (There are probably a bunch of things that need to be changed for display:contents to work...)

::: layout/base/ServoRestyleManager.cpp:188
(Diff revision 3)
> +    // XXXEmilio SomeDescendants is a hack here, I guess.
> +  } else if ((aHint & eRestyle_Subtree) || (aHint & eRestyle_SomeDescendants)) {

Yeah, SomeDescendants really needs to work in conjunction with information stored on the RestyleHintData object that RestyleTracker would be storing in its table.  Since Servo doesn't generate SomeDescendants, and the only place Gecko generates it is in nsCSSRuleProcessor, I think we should leave it out of HANDLED_RESTYLE_HINTS.

::: layout/base/ServoRestyleManager.cpp:251
(Diff revision 3)
> +      // First do any queued-up frame creation. (see bugs 827239 and 997506).
> +      //
> +      // XXXEmilio I'm calling this to avoid random behavior changes, since we
> +      // delay frame construction after styling we should re-check once our
> +      // model is more stable whether we can skip this call.
> +      PresContext()->FrameConstructor()->CreateNeededFrames();

Should we do this before the styleSet->RestyleSubtree(root) call?  It might not matter, but probably best to match what RestyleManager::ProcessPendingRestyles is doing, i.e. ensuring lazy frame construction has been flushed before restyling anything.

::: layout/style/ServoBindings.h:190
(Diff revision 3)
> -nsChangeHint Gecko_CalcAndStoreStyleDifference(RawGeckoElement* element,
> +// TODO: We would avoid a few ffi calls if we decide to make an API like the
> +// former CalcAndStoreStyleDifference, but that would effectively mean breaking
> +// some safety guarantees in the servo side.

Can you explain why this is?

::: layout/style/ServoBindings.h:194
(Diff revision 3)
> +// Also, we might want a ComputedValues to ComputedValues API for animations?
> +// Not if we do them in Gecko...

Yeah, I don't think we will if transitions are initiated on the Gecko side.

::: layout/style/ServoBindings.cpp:188
(Diff revision 3)
>  {
>    aNode->UnsetFlags(aFlags);
>  }
>  
> +nsStyleContext*
> +Gecko_GetStyleContext(RawGeckoNode* aNode) {

Nit: "{" on new line.

::: layout/style/ServoBindings.cpp:226
(Diff revision 3)
> +  // XXXEmilio probably storing it in the nearest content parent is a sane thing
> +  // to do if this case can ever happen?
> +  MOZ_ASSERT(aNode->IsContent());

In stock Gecko, we never generate change hints for text nodes.  Maybe once we do the "hints handled for descendants" thing, we will end up not generating any here too.  In any case, we should be able to just drop any change hints for text nodes on the floor (or just not send them over from the Servo side) since the hints generated on the parent element will be sufficient.  And then we can assert in ServoRestyleManager::RecreateStyleContexts rather than check that the node that we got the change hint from is an element.

::: layout/style/ServoBindings.cpp:239
(Diff revision 3)
> +  if (!oldContext) {
> +    NS_WARNING("stylo: No context here, yet found the frame and received "
> +               "computed values? huh...");
> +    return;
> +  }

I don't think we need to check this -- frames always have a non-null style context.

::: layout/style/ServoBindings.cpp:245
(Diff revision 3)
> +  // XXX Can this becalled multiple times from Servo?
> +  oldContext->StoreChangeHint(aChangeHintToStore);

*be called

I think we shouldn't be called multiple times.  The assertions in StoreChangeHint should catch this if we do.

::: layout/style/nsStyleContext.h
(Diff revision 3)
>    mozilla::NonOwningStyleContextSource StyleSource() const { return mSource.AsRaw(); }
>  
>  #ifdef MOZ_STYLO
>    void StoreChangeHint(nsChangeHint aHint)
>    {
> -    MOZ_ASSERT(!mHasStoredChangeHint);

Why are we removing these assertions?

::: layout/style/nsStyleContext.cpp:904
(Diff revision 3)
> -template<class StyleContextLike>
> +template<class OneStyleContextLike, class OtherStyleContextLike>
>  nsChangeHint
> -nsStyleContext::CalcStyleDifferenceInternal(StyleContextLike* aNewContext,
> +GenericCalcStyleDifference(OneStyleContextLike* aOldContext,
> +                           OtherStyleContextLike* aNewContext,

Do we really need this refactoring to allow CalcStyleDifference to be called with a ServoComputedValues on the LHS?

::: layout/style/nsStyleContext.cpp:1056
(Diff revision 3)
>               "missing a call to DO_STRUCT_DIFFERENCE");
>  
>  #ifdef DEBUG
>    #define STYLE_STRUCT(name_, callback_)                                      \
>      MOZ_ASSERT(!!(structsFound & NS_STYLE_INHERIT_BIT(name_)) ==              \
> -               !!PeekStyle##name_(),                                          \
> +               !!aOldContext->PeekStyle##name_(),                              \

Nit: please align the backslash.

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8775854 [details]
Bug 1290335: stylo: Assert the snapshot is taken in ServoRestyleManager::AttributeChanged.

https://reviewboard.mozilla.org/r/67912/#review65820

::: layout/style/ServoElementSnapshot.cpp:28
(Diff revision 3)
>  void
>  ServoElementSnapshot::AddAttrs(Element* aElement)
>  {
>    MOZ_ASSERT(aElement);
>  
> -  if (!HasAny(Flags::Attributes)) {
> +  if (HasAny(Flags::Attributes)) {

:o

Comment 32

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/67910/#review65646

> Nit: I think it's more idiomatic to pass the style set by pointer.
> 
> (I probably would struggle to explain a general rule for when pointers and when references are used in Gecko, but I don't see other uses of nsStyleSet&, but I do see various uses of nsStyleSet*.)

Yes, I usually prefer passing non-nullable arguments by reference. Probably worth to be discussed, though I'm happy to concede.

> Will this warn for display:none content that we have already styled?  Or are we changing how the dirty bits are used for display:none content?

Yes, it will, I can remove it since it's expected.

> Why do we generate ReconstructFrame here?  Might we previously have had a display:none element, which we restyled and then remained display:none?  I don't think we want to try to reconstruct its frame in that case -- any change in display should produce a change hint to consume.
> 
> Oh, but now I see, the stored change hint is on the style context, so we don't have a way to get the previous style for the display:none element.
> 
> Per IRC conversion, I think we should be using the frame constructor's undisplayed node map to store style contexts for display:none children of displayed content.  Note that in stock Gecko's ElementRestyler::RestyleUndisplayedNodes, we don't do a full CalcStyleDifference -- instead we just compare 'display' values, and if there was a difference we generate ReconstructFrame.  And if not, we put the new style context back into the UndisplayedNode entry.
> 
> This is fine to do as a followup, but please add a (brief) comment in here saying we should be doing this and not unconditionally generating ReconstructFrame for frame-less elments.

Will do.

> By "pending continuations" do you mean "continuations of primaryFrame that we still need to restyle"?

Yes, I mean continuations that don't necessarily have the same style, like some pseudos (::-first-line?).

Will change the comment to be clear.

> Just FYI, something to handle at some point: with display:contents, it is possible for an element's primary frame's style context on inherit from a grandparent element's primary frame's style context.  (There are probably a bunch of things that need to be changed for display:contents to work...)

Yes, it also happens with anonymous table boxes, after discussion yesterday with bholley and bz we're probably special-casing those.

> Yeah, SomeDescendants really needs to work in conjunction with information stored on the RestyleHintData object that RestyleTracker would be storing in its table.  Since Servo doesn't generate SomeDescendants, and the only place Gecko generates it is in nsCSSRuleProcessor, I think we should leave it out of HANDLED_RESTYLE_HINTS.

Fair enough.

> Can you explain why this is?

Sure, because Servo borrows the data once to get the style and calculate the change hint, and would need to borrow the data again as mutable to store it. Then, either we borrow it bypassing the check, or we effectively crash due to double borrows.

Not such a big deal, since we know that mutating the data there is safe, but probably not desired.

> Yeah, I don't think we will if transitions are initiated on the Gecko side.

In that case I can remove the second template parameter, and revert most of the changes on nsStyleContext.

> In stock Gecko, we never generate change hints for text nodes.  Maybe once we do the "hints handled for descendants" thing, we will end up not generating any here too.  In any case, we should be able to just drop any change hints for text nodes on the floor (or just not send them over from the Servo side) since the hints generated on the parent element will be sufficient.  And then we can assert in ServoRestyleManager::RecreateStyleContexts rather than check that the node that we got the change hint from is an element.

In my testing I've never hit that assertion (I think Servo also has this property), so in practice I think it's fine to leave this assertion here.

> I don't think we need to check this -- frames always have a non-null style context.

Heh, I went the "better safe than sorry" approach. Will remove the check :P

> *be called
> 
> I think we shouldn't be called multiple times.  The assertions in StoreChangeHint should catch this if we do.

Yes, it is called multiple times in a row without consuming them though, since we don't consume the change hint after triggering frame construction for the parent (for example). I think it's fine this way, though, since we don't process the same hint twice. Probably can assert on that.

Other way could be hooking the frame constructor to consume the hint, something we probably need to do to clean dirty flags anyway (right now, leaving a dirty node around is correctness-wise ok, but it's flaky).

> Why are we removing these assertions?

See the comment above.

> Do we really need this refactoring to allow CalcStyleDifference to be called with a ServoComputedValues on the LHS?

Yes, but since we don't need it finally I can revert most of it. It's mostly leftovers from my initial ServoComputedValues to ServoComputedValues comparison. If we're only doing SC -> SC and SC -> CV it can be reverted (which is fine).

Comment 33

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/67910/#review65646

> In my testing I've never hit that assertion (I think Servo also has this property), so in practice I think it's fine to leave this assertion here.

And with "I think", I mean servo only stores damage on elements, so it's fine to have it.

Comment 34

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/67910/#review65646

> And with "I think", I mean servo only stores damage on elements, so it's fine to have it.

Sorry for the bugspam. Last comment is a lie (read servo's code too fast), though I don't hit that assertion because I'm stupid and didn't realise text nodes inherit from nsIContent, sigh. Yeah, definitely can do that check from Servo, I'm not sure servo relies on having the restyle damage on text nodes too, so we might be able to just skip calculating those.

That also makes me wonder... Should we change our nsINodes in the FFI functions for nsIContents?

Comment 35

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/67910/#review65646

> Sure, because Servo borrows the data once to get the style and calculate the change hint, and would need to borrow the data again as mutable to store it. Then, either we borrow it bypassing the check, or we effectively crash due to double borrows.
> 
> Not such a big deal, since we know that mutating the data there is safe, but probably not desired.

Can't we just borrow it once, mutably?

> Sorry for the bugspam. Last comment is a lie (read servo's code too fast), though I don't hit that assertion because I'm stupid and didn't realise text nodes inherit from nsIContent, sigh. Yeah, definitely can do that check from Servo, I'm not sure servo relies on having the restyle damage on text nodes too, so we might be able to just skip calculating those.
> 
> That also makes me wonder... Should we change our nsINodes in the FFI functions for nsIContents?

OK, so let's just assert IsElement() here and skip sending over changes for text nodes from Servo.

Changing various functions to take nsIContent is OK, though we might have some that need to operate on the document (which isn't content) too.

> Yes, it is called multiple times in a row without consuming them though, since we don't consume the change hint after triggering frame construction for the parent (for example). I think it's fine this way, though, since we don't process the same hint twice. Probably can assert on that.
> 
> Other way could be hooking the frame constructor to consume the hint, something we probably need to do to clean dirty flags anyway (right now, leaving a dirty node around is correctness-wise ok, but it's flaky).

OK, I guess that is an advantage of the change hints being stored in a table that can easily be cleared at the end of restyle processing.

So let's assert that we only consume a hint once, and add a comment somewhere saying that it would be nice if we could clear out the old change hint if we don't happen to consume it (due to reconstructing frames).

> Yes, but since we don't need it finally I can revert most of it. It's mostly leftovers from my initial ServoComputedValues to ServoComputedValues comparison. If we're only doing SC -> SC and SC -> CV it can be reverted (which is fine).

Thanks, yeah let's revert this.

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8775853 [details]
Bug 1290335: stylo: Allow processing change hints generated from Servo.

https://reviewboard.mozilla.org/r/67910/#review65840

r=me with these things addressed.

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8776783 [details]
Bug 1290335: Reuse the OverflowChangedTracker between both restyle managers.

https://reviewboard.mozilla.org/r/68442/#review65844

::: layout/base/RestyleManagerBase.h:11
(Diff revision 2)
>  
>  #ifndef mozilla_RestyleManagerBase_h
>  #define mozilla_RestyleManagerBase_h
>  
>  #include "nsChangeHint.h"
> +#include "mozilla/OverflowChangedTracker.h"

One line up, to keep this sorted.

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8776784 [details]
Bug 1290335: Implement dumb versions of RestyleForAppend and RestyleForInsertOrChange

https://reviewboard.mozilla.org/r/68444/#review65850

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8777136 [details]
Bug 1290335: Make ProcessRestyledFrames non static, and tidy up a bit.

https://reviewboard.mozilla.org/r/68752/#review65852

Comment 40

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #32)
> Yes, I usually prefer passing non-nullable arguments by reference. Probably
> worth to be discussed, though I'm happy to concede.

When in doubt, follow local style.

> > Can you explain why this is?
> 
> Sure, because Servo borrows the data once to get the style and calculate the
> change hint, and would need to borrow the data again as mutable to store it.
> Then, either we borrow it bypassing the check, or we effectively crash due
> to double borrows.
> 
> Not such a big deal, since we know that mutating the data there is safe, but
> probably not desired.

Can we just borrow it once, mutably?

Comment 41

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/67910/#review65646

> Fair enough.

Actually, I had to put it back again. eRestyle_SomeDescendants is RESTYLE_DESCENDANTS for Servo, which expects us to run selector matching on the children. Opened a servo issue about that (you're CC'd).

Also, as far as I understand it, eRestyle_Subtree implies for servo running selector matching on the element too, so I aligned that behavior.

> Can't we just borrow it once, mutably?

Not with a clean API. Both the style data and the restyle damage share a refcell in Servo.

This refcell must be borrowed mutably to do the cascade and set the styles on the node, and also to set the restyle damage. We can't expose that struct (because Geckolib doesn't have it), and also, now that I think of it, the global damage computation doesn't happen in one single step, but in multiple (we accumulate the damage of the "eagerly cascaded" pseudo-elements).

So not feasible at least without a big-ish refactor of how that works in Servo.

> OK, so let's just assert IsElement() here and skip sending over changes for text nodes from Servo.
> 
> Changing various functions to take nsIContent is OK, though we might have some that need to operate on the document (which isn't content) too.

May I do this in a followup? The corresponding Servo patch is already reviewed, and can be tricky if the change hints in textnodes are needed for Servo. If they're needed, then the most performant thing to avoid an extra FFI call is just ignoring them at this point, I'm fine with either option.

Comment 42

[Cameron McCormack (:heycam)]

https://reviewboard.mozilla.org/r/67910/#review65646

> Actually, I had to put it back again. eRestyle_SomeDescendants is RESTYLE_DESCENDANTS for Servo, which expects us to run selector matching on the children. Opened a servo issue about that (you're CC'd).
> 
> Also, as far as I understand it, eRestyle_Subtree implies for servo running selector matching on the element too, so I aligned that behavior.

OK.  Please comment in here that eRestyle_SomeDescendants is being generated by Servo with this slightly different meaning (i.e. all descendants, and excluding self), and that we need to align the restyle hint meanings.

> May I do this in a followup? The corresponding Servo patch is already reviewed, and can be tricky if the change hints in textnodes are needed for Servo. If they're needed, then the most performant thing to avoid an extra FFI call is just ignoring them at this point, I'm fine with either option.

Sure, a followup is fine.

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775849 [details]
Bug 1290335: stylo: Hoist OverflowChangedTracker to its own file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67902/diff/2-3/

Comment 44

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775850 [details]
Bug 1290335: Hoist frame-construction logic in RestyleManager to static members in RestyleManagerBase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67904/diff/2-3/

Comment 45

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775851 [details]
Bug 1290335: Improve error message of the anonymous box assertion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67906/diff/2-3/

Comment 46

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775852 [details]
Bug 1290335: Hoist GetNextContinuationWithSameStyle to RestyleManagerBase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67908/diff/3-4/

Comment 47

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775853 [details]
Bug 1290335: stylo: Allow processing change hints generated from Servo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67910/diff/3-4/

Comment 48

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8775854 [details]
Bug 1290335: stylo: Assert the snapshot is taken in ServoRestyleManager::AttributeChanged.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67912/diff/3-4/

Comment 49

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8776783 [details]
Bug 1290335: Reuse the OverflowChangedTracker between both restyle managers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68442/diff/2-3/

Comment 50

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8776784 [details]
Bug 1290335: Implement dumb versions of RestyleForAppend and RestyleForInsertOrChange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68444/diff/2-3/

Comment 51

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8777136 [details]
Bug 1290335: Make ProcessRestyledFrames non static, and tidy up a bit.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68752/diff/1-2/

Comment 52

[Emilio Cobos Álvarez (:emilio)]

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

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4714ae58c55e
stylo: Hoist OverflowChangedTracker to its own file. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d9d9b868d6
Hoist frame-construction logic in RestyleManager to static members in RestyleManagerBase. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/778a903685fe
Improve error message of the anonymous box assertion. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ec44549197
Hoist GetNextContinuationWithSameStyle to RestyleManagerBase. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d25be43bde
stylo: Allow processing change hints generated from Servo. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/f572c7858103
stylo: Assert the snapshot is taken in ServoRestyleManager::AttributeChanged. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/002fbc72afdd
Reuse the OverflowChangedTracker between both restyle managers. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/29bc71ca0e33
Implement dumb versions of RestyleForAppend and RestyleForInsertOrChange. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e710c0ff6d
Make ProcessRestyledFrames non static, and tidy up a bit. r=heycam

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d11e341efcb0
followup: Fix the ConsumeStoredChangeHint assertion. r=me

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4714ae58c55e
https://hg.mozilla.org/mozilla-central/rev/a8d9d9b868d6
https://hg.mozilla.org/mozilla-central/rev/778a903685fe
https://hg.mozilla.org/mozilla-central/rev/a3ec44549197
https://hg.mozilla.org/mozilla-central/rev/f1d25be43bde
https://hg.mozilla.org/mozilla-central/rev/f572c7858103
https://hg.mozilla.org/mozilla-central/rev/002fbc72afdd
https://hg.mozilla.org/mozilla-central/rev/29bc71ca0e33
https://hg.mozilla.org/mozilla-central/rev/89e710c0ff6d
https://hg.mozilla.org/mozilla-central/rev/d11e341efcb0