370416 - textPath not updating properly when path changes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

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.

Comment 1

[tor]

Attachment: 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.

Comment 2

[Robert Longson [:longsonr]]

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

Comment 3

[tor]

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

Comment 4

[Craig]

Is this related to 372232 which is also a textpath issue?

Comment 5

[Robert Longson [:longsonr]]

(In reply to comment #3)
> Yes, I can address that in a follow-up.
> 

UpdateGraphic changes checked in under bug 372689.

Comment 6

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

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.

Comment 7

[tor]

Attachment: update to tip, don't store frame pointer

Comment 8

[Robert Longson [: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.

Comment 9

[tor]

Attachment: back to frameptr

Comment 10

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

Comment on attachment 261051 [details] [diff] [review]
back to frameptr

Use NS_DECL_NSIMUTATIONOBSERVER_ATTRIBUTECHANGED in the nsSVGPathListener.

sr=me with that

Comment 11

[tor]

Attachment: version for checkin - update to tip, use NS_DECL_NSIMUTATIONOBSERVER_ATTRIBUTECHANGED

Comment 12

[tor]

Checked in.