The default bug view has changed. See this FAQ.

Add ability to observe mutations in just a subtree

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Attaching a patch that breaks out the DOM-mutation related notifications in nsIDocumentObserver into a separate interface, nsIMutationObserver, which nsIDocumentObserver inherits.

Users can then register as mutationobservers on any node and will then get notifications for any change that happens to a descendant of that node. These notifications will be sent out even if the node is in an orphaned subtree. The result is that if you register as a mutation-observer on the document node you'll get the same notifications as the current nsIDocumentObservers get (but only the DOM-mutation related ones).

It's possible that we should move ContentStatesChanged from nsIDocumentObserver to nsIMutationObserver, however i'd like to leave that for a later patch. One problem is that the notification contains two separate nodes that changed.

Since the notifications doesn't go through the document any more (which they can't since there might not even be a document) the documents will have to regiester as observers of themselvs to be able to do what they currently do (update getElementById hashes etc). However, in order to avoid changing behaviour they should execute before any other observers. This required some trickery in the notification code. I'm not actually sure if this is needed or not, but i'd rather leave as is for this initial patch.

I'm a little worried about performance with this patch, so I'll make sure to check in when there's little activity on tinderbox and watch all numbers.
Created attachment 226215 [details] [diff] [review]
Patch to fix

This will need some merging in nsNodeUtils.cpp once bug 340733 is checked in, so i'm not requesting reviews yet. Input always welcome of course.
Assignee: general → bugmail
Status: NEW → ASSIGNED
> One problem is that the notification contains two separate nodes that changed.

See bug 313351.  ;)
Depends on: 342252
Created attachment 227484 [details] [diff] [review]
Synced to tip

So here goes a patch that is synced to tip and ready for review. I made nsIImageMap into a real mutation-observer observing just the subtree it needs to.

bz: if you're not comfortable giving both r/sr and want a second set of eyes on this, let me know. The patch is definitely towards the larger side.
Attachment #226215 - Attachment is obsolete: true
Attachment #227484 - Flags: superreview?(bzbarsky)
Attachment #227484 - Flags: review?(bzbarsky)
Comment on attachment 227484 [details] [diff] [review]
Synced to tip

>Index: content/base/public/nsIDocument.h

Rev the IID.

>-  // Observation hooks for style data to propagate notifications
>-  // to document observers

Please leave this comment?

>Index: content/base/public/nsIDocumentObserver.h

Rev the IID.

>Index: content/base/public/nsIMutationObserver.h

>+   * @param aDocument The owner-document of aContent. Can be null.

Hmmm...   At the moment we pass the current document.  Do we really want to change this?

In fact, why bother passing in a document for these methods at all?  Maybe a followup patch to not do that?

>+   * Notification that one or more contents node has been appended as last
>+   * children to another node in the tree.

"content nodes have been appended to the child list of another node in the tree"

>+   * Notification that a content node has been removed as child from another
>+   * node in the tree.

Maybe "removed from the child list of another node in the tree"?

>+   * @param aChild     The removed inserted child.

The child that was removed.

>+   * The node is in the process of being destroyed. The children and
>+   * attributes (if any) can still be accessed through nsINode type
>+   * interfaces, however the node might not QI to nsIDOMNode any more.

We should probably be a little more explicit -- calling nsINode methods is still ok, but QI to any other interface, or casting to any subclass of nsINode, is not OK...  Or is casting to nsIContent ok?  In that case we should probably document what casts are OK....

Would it make sense to pass a |const nsINode *| here?  That should prevent QI by simple dint of that not being a const method, right?

We should also document that this will only be called when the node that we actually called AddMutationObserver is being destroyed, not for other nodes in the subtree.

>Index: content/base/public/nsINode.h

Rev the IID (and on nsIContent, nsIAttribute, of course).

>+  /**
>+   * Adds a mutation observer to be notified when this node, or any of its
>+   * descendants, are modified.
>+   */
>+  virtual void RemoveMutationObserver(nsIMutationObserver* aMutationObserver);

Fix the comment?

Document that AddMutationObserver holds a weak ref (and that hence it's the callee's responsibility to remove itself when it dies)?


>+    // If needed we could remove the vtable pointer this dtor cases by putting

s/cases/causes/

>+     * Storage for flags for this node. This is the same flags as the

"These are the same flags"

>Index: content/base/src/nsContentList.cpp

> nsContentList::Item(PRUint32 aIndex, PRBool aDoFlush)

>+    // Ideally we should call GetCurrentDoc here, i think, but that doesn't
>+    // live on nsINode

Please add "XXX sXBL/XBL2 issue" to the beginning of that comment?  And I agree hat we should use GetCurrentDoc; maybe file a followup to put GetDocument on nsINode, if we can?

>+nsContentList::NodeWillBeDestroyed(nsINode* aNode)

>+  mRootNode->RemoveMutationObserver(this);

No need for that.

>+  Reset();

Document that we need that because we'll never Reset() in PopulateSelf() after this?

> nsContentList::ContentAppended(nsIDocument *aDocument, nsIContent* aContainer,
>+      !MayContainRelevantNodes(NODE_FROM(aContainer, aDocument)))

Just aContainer; it's not gonna be null in ContentAppended.

>+nsContentList::PopulateWith(nsIContent *aContent, PRUint32& aElementsToAppend)
>+  NS_PRECONDITION(mDeep || aContent == mRootNode ||
>+                  aContent->GetNodeParent() == mRootNode,
>                   "PopulateWith called on nodes we can't possibly match");

Actually, now that we no longer have that aIncludeRoot boolean, we can lose the "aContent == mRootNode thing here -- we in fact assert that that's not true right in the next assert.

>+nsContentList::PopulateWithStartingAfter(nsINode *aStartRoot,
>+  NS_PRECONDITION(mDeep || aStartRoot == mRootNode ||
>+                  (aStartRoot->GetParent() == mRootNode &&

GetNodeParent() here?

> nsContentList::PopulateSelf(PRUint32 aNeededLength)
>-  if (count != 0) {

OK.  I guess this works because generally we don't match the kids of a document that are not the root element, right?  We should make sure to write some content list tests; I'm sorta working on some unit tests for that right now and I guess I'll add this to the list...

> nsContentList::BringSelfUpToDate(PRBool aDoFlush)
>+    nsIDocument* doc = mRootNode->GetOwnerDoc();

This needs an XXX comment like the other place where we flush on the owner doc.

>Index: content/base/src/nsContentList.h

>     return
>       NS_PTR_TO_INT32(mMatchAtom.get()) ^
>-      (NS_PTR_TO_INT32(mRootContent) << 8) ^
>-      (NS_PTR_TO_INT32(mDocument) << 16) ^
>+      (NS_PTR_TO_INT32(mRootNode) << 8) ^
>       (mMatchNameSpaceId << 24);

Instead of doing 8, and 24, how about 12 and 24?  Then we'll use 12 bits of
both the atom and mRootNode pointers...

>+  void PopulateWith(nsIContent *aContent, PRUint32 & aElementsToAppend);
>   /**

Toss in a newline before the comment?

>Index: content/base/src/nsNodeUtils.cpp

>+#define IMPL_MUTATION_NOTIFICATION_BW(func_, content_, params_)   \
>+       * Notify the first observer first even if we're doing a    \
>+       * reverse walk. This is since we want to notify the        \
>+       * document first when |node| is a document.                \
>+       * This may not actually be needed, but it's safer for now. \
>+       * This can be removed once we always walk forward          \

So... non-XUL and non-HTML documents don't actually observe themselves, do they?  Doesn't that break them?  As in, doesn't it notify the binding manager before the presshells, thus negating the whole point of going backwards in nsNodeUtils::ContentRemoved?  This needs to get fixed.

>Index: content/base/src/nsNodeUtils.h
>+   * @param aContent  Node whos data changed

s/whos/whose/.  Similar elsewhere in this file.

>Index: content/html/document/src/nsHTMLDocument.cpp
>+nsHTMLDocument::AttributeChanged(nsIDocument* aDocument,

Assert aDocument == this in here?

>Index: content/xslt/src/xpath/nsXPathResult.cpp

>+nsXPathResult::NodeWillBeDestroyed(nsINode* aNode)
>+{
>+    Invalidate();

Set mDocument to null first so Invalidate() doesn't call RemoveObserver on it?

>Index: content/xul/document/src/nsXULDocument.cpp
>+nsXULDocument::AttributeChanged(nsIDocument* aDocument,

Assert aDocument == this.

> nsXULDocument::Init()
>+    slots->mMutationObservers.PrependObserver(this);

We should fail out if this fails.  Same for HTMLDocument, I guess.

>Index: docshell/shistory/src/nsSHEntry.cpp
>+  NS_ERROR("Document destroyed while we're holding a strong ref to it");

NS_NOTREACHED

>Index: layout/inspector/src/inDOMView.cpp
>+  NS_ERROR("Document destroyed while we're holding a strong ref to it");

NS_NOTREACHED

>Index: widget/src/mac/nsMenuBarX.cpp
> nsMenuBarX :: RegisterAsDocumentObserver ( nsIDocShell* inDocShell )
>+  doc->AddObserver(this);

AddMutationObserver

r+sr=bzbarsky with these issues fixed
Attachment #227484 - Flags: superreview?(bzbarsky)
Attachment #227484 - Flags: superreview+
Attachment #227484 - Flags: review?(bzbarsky)
Attachment #227484 - Flags: review+
> >Index: content/base/public/nsIMutationObserver.h
> 
> >+   * @param aDocument The owner-document of aContent. Can be null.
> 
> Hmmm...   At the moment we pass the current document.  Do we really want to
> change this?

At the moment we only notify for nodes in the document so
ownerdoc === currentdoc. I figured it was more useful to pass in a document rather then null for the cases the node isn't in the document.

> In fact, why bother passing in a document for these methods at all?  Maybe a
> followup patch to not do that?

Yeah, that's what I was thinking, this patch is big and regression-prone. We might want to change from passing nsIContent+nsIDocument to passing just nsINode. Anyhow, that's up to a later patch.

> Would it make sense to pass a |const nsINode *| here?  That should prevent QI
> by simple dint of that not being a const method, right?

Yeah, I'll try that.

> We should also document that this will only be called when the node that we
> actually called AddMutationObserver is being destroyed, not for other nodes in
> the subtree.

Well.. a node that is being destroyed can't have a parent, so it is actually following the same rules as all other notifications

> >Index: content/base/src/nsContentList.cpp
> 
> > nsContentList::Item(PRUint32 aIndex, PRBool aDoFlush)
> 
> >+    // Ideally we should call GetCurrentDoc here, i think, but that doesn't
> >+    // live on nsINode
> 
> Please add "XXX sXBL/XBL2 issue" to the beginning of that comment?  And I agree
> hat we should use GetCurrentDoc; maybe file a followup to put GetDocument on
> nsINode, if we can?

Filed bug 343288

> > nsContentList::PopulateSelf(PRUint32 aNeededLength)
> >-  if (count != 0) {
> 
> OK.  I guess this works because generally we don't match the kids of a document
> that are not the root element, right?  We should make sure to write some
> content list tests; I'm sorta working on some unit tests for that right now and
> I guess I'll add this to the list...

I'm not following you here. The two branches (if and else) basically turned into the same thing once i converted the code to use nsINode rather than nsIContent/nsIDocument.

I do want to make nsContentList walk the children of the document if it is easy. Not really because we need it right now, but it seems like the right thing to do and we might need it in the future.

Hmm.. or is the question if walking the children will mess things up since some match-functions may not expect it?

> ownerdoc === currentdoc.

OK, fair enough.  Come XBL2 we might have to think about what document to pass, I guess...  In any case, just dropping this arg in a followup will make me happy.  ;)

> Well.. a node that is being destroyed can't have a parent, so it is actually
> following the same rules as all other notifications

That's pretty non-obvious to an nsIMutationObserver implementor, really.  I think we should just spell it out.

> I'm not following you here.

Sorry.  That was a semi-stream-of-consciousness comment about why we didn't need the boolean arg to PopulateWith anymore...  Don't mind it too much.  ;)

> I do want to make nsContentList walk the children of the document if it is
> easy.

Right.  Makes sense.

> or is the question if walking the children will mess things up since some
> match-functions may not expect it?

It could, in theory, but in practice I think all the match functions we have are OK.  We could verify, of course...
I'll add:

Note that this notification is only called on observers registered directly on the node. This is because when the node is destroyed it can not have any ancestors. If you want to know when a descendant node is being removed from the observed node, use the ContentRemoved notification.
Created attachment 227870 [details] [diff] [review]
Fix review comments

This is what i'm landing.
Attachment #227484 - Attachment is obsolete: true
Checked in, with no noticable regressions in any perf numbers!
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
I can't think of a way to write testcases for this. Mostly just make sure that the rest of our features work with mutating DOMs, like dhtml, prettyprint and nodelists. But we should have testcases for those things separately.
You need to log in before you can comment on or make changes to this bug.