Closed Bug 1294572 Opened 3 years ago Closed 3 years ago

stylo: Consider skipping eager traversal on subtrees with an XBL binding

Categories

(Core :: CSS Parsing and Computation, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

When we install XBL, we generally shuffling around all of descendants of the bound element, adding anonymous content and moving explicit children to insertion points. This means that we need to re-cascade everything, and so the eager cascading that we do with Servo is wasted work.

I don't know if this is significant in practice, especially with non-chrome documents. But we could probably detect this case and stop traversing children if an element ends up with a non-trivial -moz-binding in its computed values.
Priority: -- → P3
Duplicate of this bug: 1297233
I have a patch for this, which fixes the !el.has_dirty_descendants() assertion we're getting on layout/reftests/bugs/404553-1.html .

However, I think we'll also want a patch to explicitly clear the ElementData any time the XBL insertion parent changes, because it's difficult to handle the restyle case (removing/applying a binding) during the servo traversal.

This in turn means that we'll need to be sure that we re-invoke TraverseNewChildren whenever we add _or_ remove bindings from an Element.

I'll work on this second part now.
Assignee: nobody → bobbyholley
Per bug 1323356, it looks like there's no uninstallation path for bindings aside from LoadBindings, so that should be a sufficient place to perform our re-traversal on the new flattened tree. I'll write up a patch.
MozReview-Commit-ID: JHABvLnMYco
Attachment #8818436 - Flags: review?(cam)
Attachment #8818436 - Flags: review?(cam) → review+
Comment on attachment 8818437 [details] [diff] [review]
Part 2 - Drop Servo data in SetXBLInsertionParent, and call StyleNewSubtree after all bindings have been removed and applied. v1

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

::: dom/xbl/nsXBLBinding.cpp
@@ +420,5 @@
>        mContent->UnsetAttr(namespaceID, name, false);
>    }
>  
>    // Now that we've finished shuffling the tree around, go ahead and restyle it
>    // since frame construction is about to happen.

I think this comment can be removed too.

::: dom/xbl/nsXBLService.cpp
@@ +414,5 @@
> +public:
> +  AutoStyleNewChildren(Element* aElement) : mElement(aElement) { MOZ_ASSERT(mElement); }
> +  ~AutoStyleNewChildren()
> +  {
> +    nsIPresShell* presShell = mElement->OwnerDoc()->GetShell();

Is it true that loading a binding can cause the pres shell to go away (due to the script that it runs)?  If so, we should null check presShell here.
Attachment #8818437 - Flags: review?(cam) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ad968c5c5d
Drop Servo data in SetXBLInsertionParent, and call StyleNewSubtree after all bindings have been removed and applied. r=heycam
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dac601a343ca for crashing pretty much everything on every platform, though oddly only Marionette and Android seem capable of pointing to where on the dolly it hurts, https://treeherder.mozilla.org/logviewer.html#?job_id=40563406&repo=mozilla-inbound / https://treeherder.mozilla.org/logviewer.html#?job_id=40563200&repo=mozilla-inbound
Doh, I keep forgetting that HasServoData asserts instead of returning false on non-stylo builds (which explains why this didn't bust on my try push above).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51081e419ad537cd4142ee2ce0d3b6afaa7dbfac
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> Doh, I keep forgetting that HasServoData asserts instead of returning false
> on non-stylo builds (which explains why this didn't bust on my try push
> above).
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=51081e419ad537cd4142ee2ce0d3b6afaa7dbfac

Looks green modulo a static analysis failure for a missing 'explicit'. Added that and pushed.
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/824c4af81844
Drop Servo data in SetXBLInsertionParent, and call StyleNewSubtree after all bindings have been removed and applied. r=heycam
https://hg.mozilla.org/mozilla-central/rev/824c4af81844
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.