343900 - Switch <use> from nsIDOMMutationListener to nsIMutationObserver

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: testcase - click on object

Comment 2

[tor]

Attachment: switch to new api

Comment 3

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

Comment on attachment 228454 [details] [diff] [review]
switch to new api

Won't this patch run a risk of registring as observer on one element, and then forgetting to ever unregister if the href attribute is changed? Or if the id-attribute is changed on the referenced (or another) element.

I think what you might want to do is to keep a weak pointer to the element you registered at, and then make sure to unregister if you ever register somewhere else. You need to null out that pointer of course in NodeWillBeDestroyed

>+nsSVGUseElement::NodeWillBeDestroyed(const nsINode *aNode)
> {
>   TriggerReclone();
>-  return NS_OK;
> }

Why trigger a reclone here? Your clone should be up-to-date at this point anyway.

>@@ -432,18 +410,22 @@ nsSVGUseElement::CreateAnonymousContent(
>                                    getter_AddRefs(useImpl));
> 
>         if (useImpl && useImpl->mOriginal == mOriginal)
>           return NS_ERROR_FAILURE;
>       }
>     }
>   }
> 
>+  nsCOMPtr<nsIDOMNode> original = do_QueryInterface(targetContent);
>+  if (!original)
>+    return NS_ERROR_FAILURE;
>+
>   nsCOMPtr<nsIDOMNode> newchild;
>-  element->CloneNode(PR_TRUE, getter_AddRefs(newchild));
>+  original->CloneNode(PR_TRUE, getter_AddRefs(newchild));

You could probably call nsIContent::CloneContent here.

> nsSVGUseElement::RemoveListeners()
...
>+  nsCOMPtr<nsIContent> element = LookupHref();
>+  if (element)
>+    element->RemoveMutationObserver(this);
> }

When is this called? See above comment regarding making sure that LookupHref returning a different element here.

r/sr=me with that, though if you do the CloneContent change please attach a new patch

Comment 4

[tor]

(In reply to comment #3)
> >+nsSVGUseElement::NodeWillBeDestroyed(const nsINode *aNode)
> > {
> >   TriggerReclone();
> >-  return NS_OK;
> > }
> 
> Why trigger a reclone here? Your clone should be up-to-date at this point
> anyway.

Reading the comments in nsIMutationObserver, it seemed like ContentRemoved won't be called for the top element being observed.

> >   nsCOMPtr<nsIDOMNode> newchild;
> >-  element->CloneNode(PR_TRUE, getter_AddRefs(newchild));
> >+  original->CloneNode(PR_TRUE, getter_AddRefs(newchild));
> 
> You could probably call nsIContent::CloneContent here.

Ok, I'll take a look at that.

> > nsSVGUseElement::RemoveListeners()
> ...
> >+  nsCOMPtr<nsIContent> element = LookupHref();
> >+  if (element)
> >+    element->RemoveMutationObserver(this);
> > }
> 
> When is this called? See above comment regarding making sure that LookupHref
> returning a different element here.

This is called in the destructor and WillModifySVGObservable.

However this doesn't handle id changes.  Need to think about how to handle that in general - removing/changing the id of an element we're referencing is easy to cope with, but handling some other element being set to that id is more tricky.

Comment 5

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

> > Why trigger a reclone here? Your clone should be up-to-date at this point
> > anyway.
> 
> Reading the comments in nsIMutationObserver, it seemed like ContentRemoved
> won't be called for the top element being observed.

That's right, but NodeWillBeDestroyed might not be called either since the object may still be alive. I guess what you need here is an ChangedCurrentDoc() notification. I've thought about adding this and I think we should.

Does the current code deal with this? If not, you might just want to wait with dealing until we've added ChangedCurrentDoc()

> However this doesn't handle id changes.  Need to think about how to handle 
> that in general - removing/changing the id of an element we're referencing is
> easy to cope with, but handling some other element being set to that id is
> more tricky.

Yeah, potentially any attribute change could cause a new element to be the one to be returned from GetElementById. For HTML document we could actually add the ability to be notified if the id->element mapping changes for a particular ID since we keep hashes of that stuff.

But since you need this on XML documents we would need to move some code up to nsDocument for it to be useful to you.

I would suggest you just don't deal for now. It should be rare that this occurs anyway (just don't leave stray observers as mentioned at the top of comment 3).


Note to self and other content people:
The way we could implement an observer mechanism is to add an array to the 
IdAndNameMapEntry class that contains all observers that are interested in knowing whenever the mapping for this id changes. Should be very performant.

Comment 6

[tor]

Attachment: adjust per comments

Comment 7

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

Comment on attachment 228690 [details] [diff] [review]
adjust per comments

> nsSVGUseElement::CreateAnonymousContent(nsPresContext*    aPresContext,
...
>+  if (!targetContent)
>+    return NS_ERROR_FAILURE;
>+
>+  targetContent->AddMutationObserver(this);
>+  mSourceContent = targetContent;

Could mSourceContent already have a value here? I.e. could you already be registered as a listener somewhere? Like if the <use> is removed from the DOM and then reinserted, with the id chainging inbetween. If so you probably want to call RemoveListeners first.

> nsSVGUseElement::RemoveListeners()
> {
...
>+  if (mSourceContent)
>+    mSourceContent->RemoveMutationObserver(this);
>+  mSourceContent = nsnull;

Nit: You could move the nsnull-setting inside the |if|. Also, should this function now be named RemoveListener?

r/sr=me either way

Comment 8

[tor]

Attachment: more tweaks per comments

Comment 9

[tor]

Attachment: remove WillModifySVGObservable

Now that we're removing the listener when cloning, we don't need another call to remove it in WillModifySVGObservable.

Comment 10

[tor]

Filed bug 344258 about making <use> live to id changes.

Comment 11

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

Comment on attachment 228825 [details] [diff] [review]
remove WillModifySVGObservable

Looks great! r/sr=me

Comment 12

[tor]

Checked in.