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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: tor, Assigned: roc)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

12 years ago
See bug 343900 comment 5.
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.
(Reporter)

Comment 2

12 years ago
*** Bug 311715 has been marked as a duplicate of this bug. ***
I'd love to push the ID hash up!

Updated

10 years ago
Blocks: 372579
I have a fix for this in my tree. Part of the fix is to push up the ID table.
Assignee: general → roc
Created attachment 325895 [details] [diff] [review]
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
Attachment #325895 - Flags: superreview?(jst)
Attachment #325895 - Flags: review?(jst)
Created attachment 325897 [details] [diff] [review]
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.
Attachment #325897 - Flags: superreview?(jst)
Attachment #325897 - Flags: review?(jst)
Created attachment 325900 [details] [diff] [review]
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.
Attachment #325900 - Flags: superreview?(jst)
Attachment #325900 - Flags: review?(jst)
Created attachment 325903 [details] [diff] [review]
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.
Created attachment 325905 [details] [diff] [review]
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.
Attachment #325900 - Attachment is patch: true
Attachment #325900 - Attachment mime type: application/text → text/plain
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;
-  }
-
Attachment #325895 - Flags: superreview?(jst)
Attachment #325895 - Flags: superreview+
Attachment #325895 - Flags: review?(jst)
Attachment #325895 - Flags: review+

Updated

10 years ago
Attachment #325897 - Flags: superreview?(jst)
Attachment #325897 - Flags: superreview+
Attachment #325897 - Flags: review?(jst)
Attachment #325897 - Flags: review+

Updated

10 years ago
Attachment #325900 - Flags: superreview?(jst)
Attachment #325900 - Flags: superreview+
Attachment #325900 - Flags: review?(jst)
Attachment #325900 - Flags: review+
Hrm. I'll do some archaeology to find out how those changes got into the patches. Thanks!
Checked in patch 1.
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.
Attachment #325903 - Flags: superreview?(mats.palmgren)
Attachment #325903 - Flags: review?(longsonr)
Checked in part 2.
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.
Attachment #325903 - Flags: superreview?(mats.palmgren) → superreview+
Yeah, I suppose I should bump that UUID.
Attachment #325903 - Flags: review?(longsonr) → review+
Checked in part 3.
checked in part 4. Thanks all.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 441994

Updated

10 years ago
Depends on: 441607

Comment 19

10 years ago
Will patch 5 be reviewed in another bug?
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

9 years ago
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.
No, that warning is catching a bug.  I pushed changeset eaa47afa4c3a to fix that.

Updated

9 years ago
Depends on: 461027

Updated

9 years ago
Depends on: 468511

Updated

9 years ago
Depends on: 497448

Updated

9 years ago
Depends on: 514487

Updated

9 years ago
Depends on: 515080

Updated

9 years ago
No longer depends on: 515080
You need to log in before you can comment on or make changes to this bug.