309220 - SVG markers should be live to id changes in document

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[Robert Longson [:longsonr]]

Is this the same issue as bug 329758 or something else?

Comment 2

[Robert Longson [:longsonr]]

I understand now, it's a different issue. The problem is that if you mess with the id of marker elements we don't track that. 

Comment 3

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

I have infrastructure in my tree that will make this pretty easy to fix.

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

(In reply to comment #3)
> I have infrastructure in my tree that will make this pretty easy to fix.
> 

Indeed.

Comment 5

[Robert Longson [:longsonr]]

Attachment: reftest

Comment 6

[Robert Longson [:longsonr]]

Oops, imagine I've also deleted these lines from nsSVGUtils.h too...

- #define NS_STATE_SVG_HAS_MARKERS      0x00200000
-

Comment 7

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

Sorry Robert, but it gets even easier to fix a few patches later in my svg-integration branch. I think you should make your patch against http://hg.mozilla.org/users/rocallahan_mozilla.com/svg-integration/, or just wait until trunk catches up enough. Revision d56beffbeb10 should contain everything you'd need:
http://hg.mozilla.org/users/rocallahan_mozilla.com/svg-integration/index.cgi/rev/d56beffbeb10

Comment 8

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

So on trunk we're live for ID changes for filter, mask, clip-path and use. My branch adds gradients and patterns. Here you do markers. Are there any other cases?

Comment 9

[Robert Longson [:longsonr]]

Just one that we currently implement: text-path.

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 334699 [details] [diff] [review]
patch


I guess I can wait

Comment 11

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

OK, now that bug 455984 has landed, we can fix this bug using nsSVGRenderingObservers.

Comment 12

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

BTW is there a bug filed on making text-path live for ID changes? It's the last user of nsSVGUtils::GetReferencedFrame ... it would be nice to get rid of that function, since anything that uses it is basically wrong.

Comment 13

[Robert Longson [:longsonr]]

No, there is no bug for text-path.

Comment 14

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

We should do this for 1.9.1 since it's really required for incremental XML loading.

Comment 15

[Robert Longson [:longsonr]]

Attachment: updated patch

nsSVGPaintingProperty::DoUpdate has to change because marker changes (and eventually path changes in a text-path) can change the covered region of the object.

Comment 16

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

I think it would be best to subclass nsSVGPaintingProperty and override its DoUpdate to do the extra work for markers.

At some point we might want to optimize UpdateEffects. The marker, stroke and fill properties only need to be deleted for IsFrameOfType(eSVG) frames, and we could use SVG frame state bits to indicate whether those properties exist. But it probably doesn't matter yet.

+nsSVGMarkerFrame::AttributeChanged(PRInt32         aNameSpaceID,
+                                   nsIAtom*        aAttribute,
+                                   PRInt32         aModType)

Fix indent of names

+    if (!(properties.MarkersExist()))

Just "if (!properties.MarkersExist())"

+    nsSVGPaintingProperty*   mMarkerStart;
+    nsSVGPaintingProperty*   mMarkerMid;
+    nsSVGPaintingProperty*   mMarkerEnd;

Fix indent.

Otherwise, looks great. I'm really pleased with the way this approach to references is turning out.

Comment 17

[Robert Longson [:longsonr]]

(In reply to comment #16)
> I think it would be best to subclass nsSVGPaintingProperty and override its
> DoUpdate to do the extra work for markers.
> 

fill and stroke would need to use the same override. If you change to fill or stroke="none" from something else or vice versa you can change the object bounds. Filter uses its own class anyway so the original code is correct only for mask and clipPath. 

Perhaps therefore it is mask and clipPath which should have the optimised version. Any thoughts on a name? nsSVGFixedRegionPaintingProperty?

Comment 18

[Robert Longson [:longsonr]]

You probably remember this anyway but you did put in a check in UpdateAndInvalidateCoveredRegion that if the regions are the same then it only does the one invalidate so we are only doing a redundant UpdateCoveredRegion in the mask and clipPath case.

Comment 19

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

(In reply to comment #17)
> (In reply to comment #16)
> > I think it would be best to subclass nsSVGPaintingProperty and override its
> > DoUpdate to do the extra work for markers.
> 
> fill and stroke would need to use the same override. If you change to fill or
> stroke="none" from something else or vice versa you can change the object
> bounds. Filter uses its own class anyway so the original code is correct only
> for mask and clipPath.

If the fill or stroke style changes, then we reach nsSVGPathGeometryFrame::DidSetStyleContext which calls nsSVGUtils::UpdateGraphic which does an UpdateAndInvalidateCoveredRegion. All nsSVGPaintingProperty needs to take care of is changes caused by the *referenced* content. I don't think paint servers can change the covered region of the path frame that references them.

Comment 20

[Robert Longson [:longsonr]]

Attachment: address review comments

In that case text-path should be able to use an nsSVGPaintingProperty too so markers are the odd one out.

Comment 21

[Robert Longson [:longsonr]]

checked in 0569b4a4b379

Comment 22

[Robert Longson [:longsonr]]

checked in reftest http://hg.mozilla.org/mozilla-central/rev/4f32fd012f1a

Comment 23

[Tony Chung [:tchung]]

Reftest covers the fix.  

Verified on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre