Closed Bug 309220 Opened 16 years ago Closed 13 years ago

SVG markers should be live to id changes in document

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: tor, Assigned: longsonr)

References

Details

(Keywords: qawanted, verified1.9.1)

Attachments

(2 files, 2 obsolete files)

 
Is this the same issue as bug 329758 or something else?
Depends on: 371563
Keywords: qawanted
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. 
Summary: SVG markers should be live → SVG markers should be live to id changes in document
I have infrastructure in my tree that will make this pretty easy to fix.
Attached patch patch (obsolete) — Splinter Review
(In reply to comment #3)
> I have infrastructure in my tree that will make this pretty easy to fix.
> 

Indeed.
Assignee: general → longsonr
Status: NEW → ASSIGNED
Attachment #334699 - Flags: superreview?(roc)
Attachment #334699 - Flags: review?(roc)
Attached image reftest
Oops, imagine I've also deleted these lines from nsSVGUtils.h too...

- #define NS_STATE_SVG_HAS_MARKERS      0x00200000
-
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
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?
Just one that we currently implement: text-path.
Comment on attachment 334699 [details] [diff] [review]
patch


I guess I can wait
Attachment #334699 - Flags: superreview?(roc)
Attachment #334699 - Flags: review?(roc)
OK, now that bug 455984 has landed, we can fix this bug using nsSVGRenderingObservers.
Depends on: 455984
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.
No, there is no bug for text-path.
We should do this for 1.9.1 since it's really required for incremental XML loading.
Flags: wanted1.9.1?
Attached patch updated patch (obsolete) — Splinter Review
nsSVGPaintingProperty::DoUpdate has to change because marker changes (and eventually path changes in a text-path) can change the covered region of the object.
Attachment #334699 - Attachment is obsolete: true
Attachment #341417 - Flags: superreview?(roc)
Attachment #341417 - Flags: review?(roc)
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.
(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?
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.
(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.
In that case text-path should be able to use an nsSVGPaintingProperty too so markers are the odd one out.
Attachment #341417 - Attachment is obsolete: true
Attachment #341426 - Flags: superreview?(roc)
Attachment #341426 - Flags: review?(roc)
Attachment #341417 - Flags: superreview?(roc)
Attachment #341417 - Flags: review?(roc)
Attachment #341426 - Flags: superreview?(roc)
Attachment #341426 - Flags: superreview+
Attachment #341426 - Flags: review?(roc)
Attachment #341426 - Flags: review+
checked in 0569b4a4b379
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
checked in reftest http://hg.mozilla.org/mozilla-central/rev/4f32fd012f1a
Flags: in-testsuite? → in-testsuite+
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1
Depends on: 472135
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.