373089 - Provide nsIMutationObserver with notification of bind/unbind

Status: RESOLVED FIXED

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

Description

[tor]

Comment 1

[tor]

Attachment: add a CurrentDocumentChanged notification

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 257749 [details] [diff] [review]
add a CurrentDocumentChanged notification

>Index: content/base/public/nsIMutationObserver.h
>+  virtual void CurrentDocumentChanged(nsIDocument *aDocument,
>+                                      nsIContent *aContent,
>+                                      CurrentDocumentChangeInfo aInfo) = 0;

The aInfo parameter seems unnecessary to me. Just make aDocument be null if we're unbinding and non-null if we're binding. You can always get the owner document through aContent->GetOwnerDoc()

>Index: content/base/src/nsNodeUtils.cpp
>+nsNodeUtils::CurrentDocumentChanged(nsIDocument *aDocument,
>+                                    nsIContent *aContent,
>+                                    CurrentDocumentChangeInfo aInfo)
>+{
>+  // not using IMPL_MUTATION_NOTIFICATION since we don't need to
>+  // notify the parents

Please explain why we don't need to notify parents. Something like

// No need to notify observers on the parents since their current doc
// must have been changed too and so their observers were notified then.

>+  nsINode::nsSlots* slots = aContent->GetExistingSlots();
>+  if (slots && !slots->mMutationObservers.IsEmpty()) {
>+    /* No need to explicitly notify the first observer first
>+       since that'll happen anyway. */

This comment is moot these days so don't add it.

With that, r/sr=me

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, and add a comment stating that CurrentDocumentChanged will be called even when the observed node is moved out of an already orphaned subtree, as well as when a node is inserted into an orphaned subtree.

Hmm.. Come to think of it. That makes the choise in name bad. Either you should change the name to something like CurrentSubtreeChanged or make it only send out the notification when the CurrentDoc() is actually changed.

Comment 5

[tor]

Since the current/owner document can be examined, is passing a nsIDocument to the observers really the appropriate thing, or should there just be a boolean telling whether the action is a bind or unbind?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, lets pass neither. What i'm worried about is that if you pass a boolean indicating BIND then people might think that that means that the node is added to the main document tree, whereas it could have just been bound to an orphaned subtree.

If however we don't pass anything it'll be easy enough for someone to just check aContent->IsInDoc()

So how about the signature:  ParentChainChanged(nsIContent* aContent);

Comment 7

[tor]

(In reply to comment #6)
> So how about the signature:  ParentChainChanged(nsIContent* aContent);

/cue laughter from people familiar with the svg code.

Sounds good, I'll make that change.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, i seem to recall the SVG code having a function like that :)

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Speaking of which, that code seems dead now. No-one calls ParentChainChanged except CallParentChainChanged, no-one calls CallParentChainChanged except ParentChainChanged. AFAICT

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Nm, i'm on dope.

Comment 11

[tor]

SVG's ParentChainChanged is actually going away as part of bug 353172.

Comment 12

[tor]

Attachment: new notification signature

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 257843 [details] [diff] [review]
new notification signature

>Index: content/base/src/nsGenericDOMDataNode.cpp
>@@ -600,16 +600,18 @@ nsGenericDOMDataNode::BindToTree(nsIDocu
>     }
>   }
> 
>   NS_POSTCONDITION(aDocument == GetCurrentDoc(), "Bound to wrong document");
>   NS_POSTCONDITION(aParent == GetParent(), "Bound to wrong parent");
>   NS_POSTCONDITION(aBindingParent == GetBindingParent(),
>                    "Bound to wrong binding parent");
> 
>+  nsNodeUtils::ParentChainChanged(this);
>+

Do this before the POSTCONDITIONs.

>Index: content/base/src/nsGenericElement.cpp
>@@ -1817,17 +1817,19 @@ nsGenericElement::BindToTree(nsIDocument
> 
>   // XXXbz script execution during binding can trigger some of these
>   // postcondition asserts....  But we do want that, since things will
>   // generally be quite broken when that happens.
>   NS_POSTCONDITION(aDocument == GetCurrentDoc(), "Bound to wrong document");
>   NS_POSTCONDITION(aParent == GetParent(), "Bound to wrong parent");
>   NS_POSTCONDITION(aBindingParent == GetBindingParent(),
>                    "Bound to wrong binding parent");
>-  
>+
>+  nsNodeUtils::ParentChainChanged(this);

Same here

r/sr=me with that. But please keep an eye on the performance numbers when you check in.

Comment 14

[tor]

Attachment: version for checkin with notification moved

I'll try checking in tomorrow morning before the west coast gets in.

Comment 15

[Boris Zbarsky [:bzbarsky]]

So I have a question.  Right now we have various code that treats Bind and Unbind asymmetrically.  Do we not want to worry about supporting such use cases with this API?

I think we should also document whether ParentChainChanged is called on all nodes whose parent chain has changed or just the "root" such node... (it's the latter, right?).

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #15)
> So I have a question.  Right now we have various code that treats Bind and
> Unbind asymmetrically.  Do we not want to worry about supporting such use
> cases with this API?

Not sure I follow what you mean here. Can you give an example?

> I think we should also document whether ParentChainChanged is called on all
> nodes whose parent chain has changed or just the "root" such node... (it's the
> latter, right?).

Actually, it'll be notified on all nodes, otherwise it'd be much less useful. If you want to know if a specific node is remove from the document tree then you'll only attach an observer to that node. If then it or one of its parents are removed from the document UnbindFromTree will recurse down to the observer node and we'll notify there.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The alternative is to only notify on the node whose parent is changed, and require that observers register observers on all nodes in the parent chain. But I bet that'll be slower since we'd have many more observers to call AttributeChanged on every time someone sets an attr etc.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Not sure I follow what you mean here. Can you give an example?

Sure.  nsHTMLImageElement does an image load on BindToTree (because the base URI might have changed), but not on UnbindFromTree (even though it might still have changed).

> Actually, it'll be notified on all nodes, otherwise it'd be much less useful.

That was not at all obvious from the API documentation...

If the notification happens from inside UnbindFromTree, then we need some serious warnings about what one is allowed to do from inside these notifications (e.g. no touching the DOM, etc).  I do think this is the way to go, though, assuming all observers are very careful.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #18)
> > Not sure I follow what you mean here. Can you give an example?
> 
> Sure.  nsHTMLImageElement does an image load on BindToTree (because the base
> URI might have changed), but not on UnbindFromTree (even though it might
> still have changed).

You should be able to do that by only doing the load if aContent->IsInDoc() is true.

Note that just because a node is bound doesn't mean that it's part of the document since it could have gotten bound to an orphaned subtree. The current API forces people to look for the state they really care about, which I think is a good thing.

> > Actually, it'll be notified on all nodes, otherwise it'd be much less
> > useful.
> 
> That was not at all obvious from the API documentation...

How about:
Notification that the node's parent chain has changed. This happens when either the node or one of its ancestors is inserted or removed as a child of another node.
Note that when a node is inserted this notification is sent to all descendants of that node, since all such nodes have their parent chain changed.

> If the notification happens from inside UnbindFromTree, then we need some
> serious warnings about what one is allowed to do from inside these
> notifications (e.g. no touching the DOM, etc).  I do think this is the way to
> go, though, assuming all observers are very careful.

We're lacking such a warning no all functions in nsIMutationObserver :(

How about adding

Warning! You are not allowed to perform any mutations to the current or any other document during this notification. If you need to perform such mutations do that during the _last_ nsIDocumentObserver::EndUpdate notification.

somewhere in the nsIMutationObserver.h, either at the top of the file or before each function definition.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> You should be able to do that by only doing the load if aContent->IsInDoc()

I see no reason not to properly reload an image if an image create with new Image() is stuck into a document fragment which changes its base URI...

> The current API forces people to look for the state they really care about

But doesn't let you tell apart the "bound into a document fragment" and "unbound from somewhere" actions.

I'm happy with the comments if we also add no starting of network loads to the list.

I assume there's just no way to get these notifications for content added the way the sink adds things, right?

Comment 21

[tor]

Attachment: update comments in nsIMutationObserver

Modified comments in nsIMutationObserver based on sicking and bz's conversation.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #20)
> > You should be able to do that by only doing the load if aContent->IsInDoc()
> 
> I see no reason not to properly reload an image if an image create with new
> Image() is stuck into a document fragment which changes its base URI...

Is this really what we do though? And would you want to do the load because you got inserted or because you got inserted into a fragment? If the latter you can just check if the top of the parent chain is a fragment (though granted, it would be faster to do this only when called by bind)

In theory you can detect insertion by getting the length of the parent chain and comparing that to the last length of the parent chain, which would have to be stored. This is a pretty poor solution though.

I'm not really opposed to adding a boolean argument if we really do have uses for it. Or we can add it later if we don't yet but might in the future.

> I'm happy with the comments if we also add no starting of network loads to the
> list.
> 
> I assume there's just no way to get these notifications for content added the
> way the sink adds things, right?

Actually, this new notification is sent out even when aNotify is false. For better or worse...

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Is this really what we do though?

Right now, yes.

> And would you want to do the load because you got inserted

I'd want to do the load because the base URI may now be different.

Again, I'm not saying this API needs to support this use case, necessarily.  Just pointing out that this is a use case that does happen.  I don't think it's that common, to be honest.

> Actually, this new notification is sent out even when aNotify is false

In that case, the EndUpdate stuff makes no sense, since this notification can happen outside of an update...

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #23)
> > And would you want to do the load because you got inserted
> 
> I'd want to do the load because the base URI may now be different.

But that can happen both on bind and unbind ;)

> Again, I'm not saying this API needs to support this use case, necessarily. 
> Just pointing out that this is a use case that does happen.  I don't think 
> it's that common, to be honest.

Lets leave as is for now. We can always add a flag later if we think it's the right thing to do.

> > Actually, this new notification is sent out even when aNotify is false
> 
> In that case, the EndUpdate stuff makes no sense, since this notification can
> happen outside of an update...

True. Lets for now state state that for this function it should happen asynchronously. 

Comment 25

[Boris Zbarsky [:bzbarsky]]

> But that can happen both on bind and unbind ;)

Yes, but for unbind we don't do it for perf (and maybe slightly compat) reasons...

> Lets leave as is for now. We can always add a flag later 

Sounds good.

> for this function it should happen asynchronously. 

Also sounds good.

Comment 26

[tor]

Attachment: another nsIMutationObserver comment update

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Looks good to me

Comment 28

[tor]

> r/sr=me with that. But please keep an eye on the performance numbers when you
> check in.

Checked in - didn't seem to change the performance numbers.