Closed Bug 1395351 Opened 7 years ago Closed 7 years ago

stylo: Stop clobbering dirty bits from the frame constructor.

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(4 files)

Bug 1395351: Don't clobber restyle root flags from frame construction.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1395351: Use the style flattened tree in EffectCompositor::PreTraverseInSubtree.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1395351: Use the parallel traversal flag more often.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1395351: Assert the root element is styled in StyleDocument.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
See bug 1394935, which landed a commented-out assertion that should hold, but doesn't.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #0)
> See bug 1394935, which landed a commented-out assertion that should hold,
> but doesn't.

Well, will land, technically...
Priority: -- → P2
Comment on attachment 8903572 [details]
Bug 1395351: Don't clobber restyle root flags from frame construction.

https://reviewboard.mozilla.org/r/175378/#review180524

This will disable parallelism on the initial style, because StyleNewSubtree isn't set up to pass the parallel flag.

Now that we have the adaptive traversal, I think all the callers can probably just pass in the parallel flag if the presshell is active. So we probably need another patch underneath this one to do that. We can then probably get rid of the isInitialForMainDoc stuff in StyleDocument and assert against it.

Then there's the fact that PreTraverseInSubtree behaves a bit different than PreTraverse. The nsContentUtils::ContentIsFlattenedTreeDescendantOf call should probably become ForStyle...

So anyway, we need to be a bit more careful if we're going to switch to StyleNewSubtree. An alternative lower-risk approach would just to be more careful about clobbering the bits, though it's probably worthwhile to clean things up.
Attachment #8903572 - Flags: review?(bobbyholley) → review-
Comment on attachment 8903691 [details]
Bug 1395351: Use the style flattened tree in EffectCompositor::PreTraverseInSubtree.

https://reviewboard.mozilla.org/r/175468/#review180574

::: dom/animation/EffectCompositor.cpp:1017
(Diff revision 1)
>      }
>  
>      // Ignore restyles that aren't in the flattened tree subtree rooted at
>      // aRoot.
>      if (aRoot &&
> -        !nsContentUtils::ContentIsFlattenedTreeDescendantOf(target.mElement,
> +        aRoot != target.mElement &&

Why do you need this new check? The function should handle it AFAICT, so please remove it.
Attachment #8903691 - Flags: review?(bobbyholley) → review+
Comment on attachment 8903692 [details]
Bug 1395351: Use the parallel traversal flag more often.

https://reviewboard.mozilla.org/r/175470/#review180578

::: layout/style/ServoStyleSet.cpp:1428
(Diff revision 1)
> +bool
> +ServoStyleSet::ShouldTraverseInParallelFrom(const Element& aRoot) const
> +{
> +  if (aRoot.IsInNativeAnonymousSubtree()) {
> +    return false;
> +  }
> +
> +  return mPresContext->PresShell()->IsActive();
> +}

So, I think we probably don't need the IsInNativeAnonymousSubtree check now that we have the adaptive traversal. It was just there to prevent us from engaging the parallel traversal on tiny NAC subtrees, but I think that shouldn't be a problem anymore.

So I think it would be better to just make this ShouldTraverseInParallel(). I won't insist if you want to save that for later though, since the current state of affairs isn't really harmful, just overly-complex.
Attachment #8903692 - Flags: review?(bobbyholley) → review+
Comment on attachment 8903572 [details]
Bug 1395351: Don't clobber restyle root flags from frame construction.

https://reviewboard.mozilla.org/r/175378/#review180580
Attachment #8903572 - Flags: review?(bobbyholley) → review+
Comment on attachment 8903693 [details]
Bug 1395351: Assert the root element is styled in StyleDocument.

https://reviewboard.mozilla.org/r/175472/#review180582

::: layout/style/ServoStyleSet.cpp:934
(Diff revision 1)
> +    auto flags = aBaseFlags | ServoTraversalFlags::ParallelTraversal;
> +

This should use the new helper to avoid enabling parallelism on background tabs.
Attachment #8903693 - Flags: review?(bobbyholley) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1feb294747ee
Use the style flattened tree in EffectCompositor::PreTraverseInSubtree. r=bholley
https://hg.mozilla.org/integration/autoland/rev/ec07132e0715
Don't clobber restyle root flags from frame construction. r=bholley
https://hg.mozilla.org/integration/autoland/rev/2c92da356508
Assert the root element is styled in StyleDocument. r=bholley
https://hg.mozilla.org/integration/autoland/rev/c2bc5c2bbb2e
Use the parallel traversal flag more often. r=bholley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: