Closed Bug 1554498 Opened 5 months ago Closed 2 months ago

Make ShadowRoot not be a mutation observer of its host.

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(3 files)

Instead, do the state tracking from UnbindFromTree / BindFromTree / AfterSetAttr.

Explanation for that incoming.

This was in preparation to implement CSS Shadow Parts, where I'm going to need
to keep track of all the parts in the shadow tree.

Unlike slotted children (which can only be direct children of the host), parts
can be anywhere in the shadow tree.

That means that the regular nsIMutationObserver notifications don't quite cut
it, since we'd need notified only of subtree roots and we'd need to tree-walk
around in order to figure out if we have any new part.

As a preparation to make the ShadowRoot keep track of which parts does it have,
and given that the checks are quite similar, I wanted to move the shadow-DOM
related checks to work in BindToTree / UnbindFromTree instead, so they remain
all together.

The AttributeChanged notifications could just work as-is for part too, though
I've moved them to Element::AfterSetAttr just for consistency. It adds a
null-check in a somewhat hot part of the code, but I think it should be ok, and
for the case shadow DOM is used it'll be much faster.

However, as part of writing this patch, I realized that I wasn't going to be
able to share as much code as I wanted, really.

This penalizes a bit non-shadow-DOM content, in exchange of making Shadow DOM
slightly faster as well. So not sure where the bar is or whether we should take
this patch.

Depends on D32638

Assignee: nobody → emilio
Priority: -- → P2
Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/849ba4e85044
Some miscellaneous cleanups while I was going through this code. r=edgar
Attachment #9067654 - Attachment description: Bug 1554498 - Don't use nsIMutationObserver for ShadowRoot. r=bzbarsky → Bug 1554498 - Don't use nsIMutationObserver for ShadowRoot. r=smaug
Attachment #9067654 - Attachment description: Bug 1554498 - Don't use nsIMutationObserver for ShadowRoot. r=smaug → Bug 1554498 - Don't use nsIMutationObserver for ShadowRoot.
Attachment #9067654 - Attachment description: Bug 1554498 - Don't use nsIMutationObserver for ShadowRoot. → Bug 1554498 - Don't use nsIMutationObserver for ShadowRoot. r=smaug

After addressing review comments in the other patch of this bug, debug builds
assert all the time here when called from UnbindFromTree() on an already-unbound
subtree.

We clear the binding parent on unbind, so this is only guaranteed to match for
connected nodes, as far as I can tell.

Attachment #9088481 - Attachment description: Bug 1554498 - Fix an assertion that starts firing with the previous patch after addressing review feedback. → Bug 1554498 - Fix an assertion that starts firing with the previous patch after addressing review feedback. r=smaug
Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2a619224d379
Don't use nsIMutationObserver for ShadowRoot. r=smaug
https://hg.mozilla.org/integration/autoland/rev/43e96d742919
Fix an assertion that starts firing with the previous patch after addressing review feedback. r=smaug
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/73353f375d1d
followup: Add a null-check since XBL can call into UnbindFromTree at odd times. a=Aryx,btara
See Also: → 1577191
Regressions: 1579391
You need to log in before you can comment on or make changes to this bug.