344258 - Make <svg:use> live to id changes in document

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

See bug 343900 comment 5.

Comment 1

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

Specifically the last paragraph:

"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."

We would first have to move the id-hash from nsHTMLDocument up to nsDocument of course.

Comment 2

[tor]

*** Bug 311715 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'd love to push the ID hash up!

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have a fix for this in my tree. Part of the fix is to push up the ID table.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 1: move ID table to nsDocument from nsHTMLDocument

-- Move ID table up to nsDocument
-- Moves implement of GetElementById up to nsDocument
-- Make it use nsTHashtable
-- Avoids direct access to nsIdentifierMapEntry fields by encapsulating stuff in methods
-- removes nsXMLDocument::GetElementById since it's no longer needed

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 2: make XUL use the base ID-table

Gets rid of XUL's own ID table mess to use the new shared stuff.

This is a bit complicated since XUL stores both 'id' and 'ref'-named elements in its table. So I create a seperate ref-to-element map for XUL. This actually could change behaviour in situations where we have elements with both id="foo" and ref="foo" --- now we'll always prefer the id-element when you call getElementById. I think that should be OK.

I also fixed the tests to reflect the fact that this makes XUL more sane.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 3: Infrastructure for tracking ID-map changes

Creates change-observer hooks accessible through nsIDocument. These fire at risky times and are not all that convenient to use.

Creates nsReferencedElement which is the preferred way to watch for ID-mapping changes. Much more convenient to use and uses nsContentUtils::AddScriptRunner to ensure that its notifications only happen at safe times.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 4: Use nsReferencedElement to track ID-map changes for <use>

Not requesting reviews on this part yet since it depends parts 3 and 1 in a major way. But it shows how nsReferencedElement can be used, and it includes tests that exercise the functionality of parts 1 and 3.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 5: similar fix for filter/mask/clipPath

Again, not going for reviews yet. In fact it might make more sense to roll this up into some of my other filter work for review. Just putting it here because I have the patch around.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 325895 [details] [diff] [review]
part 1: move ID table to nsDocument from nsHTMLDocument

Looks good, but these two changes look unrelated (and incorrect):

 nsresult
-nsHTMLDocument::SetEditingState(EditingState aState)
-{
-  mEditingState = aState;
-  return NS_OK;
-}
-
-nsresult
 nsHTMLDocument::EditingStateChanged()
 {
-  if (mRemovedFromDocShell) {
-    return NS_OK;
-  }
-

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hrm. I'll do some archaeology to find out how those changes got into the patches. Thanks!

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in patch 1.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 325903 [details] [diff] [review]
part 4: Use nsReferencedElement to track ID-map changes for <use>

Ok, since jst approved the nsReferencedElement API let's go for review on the real patch now.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in part 2.

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 325903 [details] [diff] [review]
part 4: Use nsReferencedElement to track ID-map changes for <use>

Looks good to me. sr=mats

> content/svg/content/src/nsSVGUseElement.h

We need to bump the UUID for nsSVGUseElement, right?

+  SourceReference      mSource;  // observed element

One more space before the // and the comments would line up.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, I suppose I should bump that UUID.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in part 3.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in part 4. Thanks all.

Comment 19

[Jesse Ruderman]

Will patch 5 be reviewed in another bug?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe. There are other changes to that code in my tree and I have to figure out whether it's easier to review individual patches or combined patches.

Comment 21

[Aiko]

Comment on attachment 325900 [details] [diff] [review]
part 3: Infrastructure for tracking ID-map changes

>+      return NS_PTR_TO_INT32(aKey->mCallback) >> 2 +
>+             NS_PTR_TO_INT32(aKey->mData);

I'm getting a compiler warning about operator precedence here. Did you really mean mCallback >> (2 + mData)? If yes, then please fix the confusing formatting and put some parentheses around it.

Comment 22

[Boris Zbarsky [:bzbarsky]]

No, that warning is catching a bug.  I pushed changeset eaa47afa4c3a to fix that.