Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Infra to define mutation observers #609

Merged
merged 6 commits into from Mar 29, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented Mar 16, 2018

Fixes #132.

The main thing I still need to look into is figuring out how transient registered observers are actually removed, as the current text doesn't state that in much detail.


Preview | Diff

@annevk
Copy link
Member Author

annevk commented Mar 16, 2018

@smaug---- when the specification does

Remove all transient registered observers whose observer is mo.

Remove all transient registered observers whose source is registered.

is this some kind of global search? And the goal is to remove them from each node's registered observer list? Seems quite expensive.

@annevk
Copy link
Member Author

annevk commented Mar 17, 2018

https://www.w3.org/Bugs/Public/show_bug.cgi?id=22759#c9 by @travisleithead has the better model here (the first suggestion) that didn't make it into the specification. That would make how this works much more explicit.

@annevk
Copy link
Member Author

annevk commented Mar 21, 2018

So, we could give each "registered observer" a "transient registered observer node list". When removing a node, in step 16 of https://whatpr.org/dom/609.html#concept-node-remove, aside from appending a new transient registered observer to node's registered observer list, we also append node to registered's transient registered observer node list.

Then we could rewrite step 7 of https://whatpr.org/dom/609.html#dom-mutationobserver-observe to use this new node list on registered to remove the transient registered observers from those nodes.

However, we also remove transient registered observers in https://whatpr.org/dom/609.html#notify-mutation-observers based on the observer field. Should MutationObserver (the value of that observer field) also keep a list of nodes that have transient registered observers?

@ajklein if you could help with this further formalizing of mutation observers that'd be appreciated. If it's too much to page in I can relate.

@smaug----
Copy link
Collaborator

is this some kind of global search?

no. MutationObserver has the list of transient observers, so it can just go through the list.
In particular in Gecko there is MutationReceiver per observed target internally doing actually observing, and it has list of transient receivers.

@ajklein
Copy link

ajklein commented Mar 21, 2018

I suspect that @smaug---- has this swapped in better than I do, have been out of DOM land for several years now...

@annevk
Copy link
Member Author

annevk commented Mar 21, 2018

<smaug> annevk: so, in Gecko DOMMutationObserver is the thing representing MutationObserver object. It has list of MutationReceivers which actually observers the mutations. Each such receiver may the have transient receivers. Right before a mutationobserver calls its callback, it goes through its mutationreceivers, and ask them to clear transient receivers
I guess that should answer to the first one... then the second question 
<smaug> and same happens with observe() https://searchfox.org/mozilla-central/source/dom/base/nsDOMMutationObserver.cpp#743
<annevk> hmm, that doesn't really map to the spec cleanly
<smaug> how so
<annevk> smaug: just that the spec has different concepts
<smaug> trying to see where
<annevk> smaug: oh so mutationreceivers are the nodes?
MutationObserver does have a node list
in the spec
<smaug> well, attached to nodes
<annevk> okay
so we'd iterate over the nodes and remove the transient registered observers from their registered observer list
<smaug> each receiver has target, and that target is a node
right, something like that
<annevk> smaug: and in observe we'd look at the registered observer's observer's node list and do the same is what you're saying?
<smaug> yeah, https://searchfox.org/mozilla-central/source/dom/base/nsDOMMutationObserver.cpp#743
in that code we update the receiver and remove the transient stuff

@annevk
Copy link
Member Author

annevk commented Mar 22, 2018

@ArkadiuszMichalski interested in reviewing this?

@@ -3636,6 +3638,8 @@ that is a <a>document</a>.
<a>node</a>'s <a>assigned slot</a>, if <a>node</a> is <a>assigned</a>, and <a>node</a>'s
<a for=tree>parent</a> otherwise.

<p class="note no-backref">Each <a for=/>node</a> also has a <a>registered observer list</a>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each node has also ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The precedent in the document is "also has". And I think that's correct.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But which one reads better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"also has" reads more natural to me.

dom.bs Outdated
<li><p><a for=list>For each</a> <var>node</var> of the <a>context object</a>'s
<a for=MutationObserver>node list</a>, <a for=list>remove</a> all
<a>transient registered observers</a> whose <a for="transient registered observers">source</a> is
<var>registered</var> from <var>node</var>'s <a>registered observer list</a>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something weird with this sentence.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you be more specific? I can't really find anything odd.

@annevk annevk force-pushed the annevk/mutation-observers-rewrite branch from b21a1c3 to 46a5301 Compare March 29, 2018 10:06
@annevk annevk merged commit 50b3051 into master Mar 29, 2018
@annevk annevk deleted the annevk/mutation-observers-rewrite branch March 29, 2018 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants