textPath not updating properly when path changes

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

12 years ago
textPath currently puts an observer on the nsSVGPathSegList, but a change I made last year to create the seglist lazily means that it ends up watching an orphaned list as soon as the path data is changed.
(Assignee)

Comment 1

12 years ago
Created attachment 255128 [details] [diff] [review]
use nsIMutationObserver on the path element itself

Won't properly update after deletion of the path element, but the current code is no better, and we're currently talking about how this could be best accomplished.
Attachment #255128 - Flags: review?(jonas)
(In reply to comment #1)
> Created an attachment (id=255128) [details]
> use nsIMutationObserver on the path element itself
> 
> Won't properly update after deletion of the path element, but the current code
> is no better, and we're currently talking about how this could be best
> accomplished.
> 

How about hoisting UpdateGraphic to nsSVGTextContainerFrame. 

It could then be used in nsSVGTSpanFrame::AttributeChanged although that's probably a follow up kind of thing.

(Assignee)

Comment 3

12 years ago
Yes, I can address that in a follow-up.

Comment 4

12 years ago
Is this related to 372232 which is also a textpath issue?
(In reply to comment #3)
> Yes, I can address that in a follow-up.
> 

UpdateGraphic changes checked in under bug 372689.
Comment on attachment 255128 [details] [diff] [review]
use nsIMutationObserver on the path element itself

>Index: layout/svg/base/src/nsSVGTextPathFrame.cpp
>+nsSVGPathListener::AttributeChanged(nsIDocument *aDocument,
>+                                    nsIContent *aContent,
>+                                    PRInt32 aNameSpaceID,
>+                                    nsIAtom *aAttribute,
>+                                    PRInt32 aModType)
>+{
>+  mTextPathFrame->UpdateGraphic();
>+}

How can you be sure that mTextPathFrame hasn't been deleted at this point? I guess the frame is the only thing keeping the listener alive, but it feels scary since at least nsNodeUtils will also at least temporarily hold a strong reference to the listener.

I'm fine with "fixing" this using just assertions if there really is no way this could be a problem.

I'd rather sr this since I don't fully understand the functional changes.
Attachment #255128 - Flags: review?(jonas) → superreview+
(Assignee)

Comment 7

12 years ago
Created attachment 260841 [details] [diff] [review]
update to tip, don't store frame pointer
Attachment #255128 - Attachment is obsolete: true
Attachment #260841 - Flags: review?(longsonr)
Comment on attachment 260841 [details] [diff] [review]
update to tip, don't store frame pointer

You could hsve stored a pointer to the textpath frame rather than the element in the listener.
You would not have to have a ref counted pointer to the frame, nor would you have to look it up in nsSVGPathListener::AttributeChanged which would be a tad faster.  
When the frame dies, its listener dies so we would not have any lifetime problems.
There's nothing functionally wrong with doing it this way though. r=longsonr either way.
Attachment #260841 - Flags: review?(longsonr) → review+
(Assignee)

Comment 9

12 years ago
Created attachment 261051 [details] [diff] [review]
back to frameptr
Attachment #261051 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #261051 - Flags: review? → review?(longsonr)
Attachment #261051 - Flags: review?(longsonr) → review+
(Assignee)

Updated

12 years ago
Attachment #261051 - Flags: superreview?(jonas)
Comment on attachment 261051 [details] [diff] [review]
back to frameptr

Use NS_DECL_NSIMUTATIONOBSERVER_ATTRIBUTECHANGED in the nsSVGPathListener.

sr=me with that
Attachment #261051 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 11

12 years ago
Created attachment 262896 [details] [diff] [review]
version for checkin - update to tip, use NS_DECL_NSIMUTATIONOBSERVER_ATTRIBUTECHANGED
(Assignee)

Comment 12

12 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.