-- Add an mInUse flag to nsSVGRenderingObserver -- Add AcquireReferencedFrame() and ReleaseReferencedFrame() setting it to true and false -- Add GetReferencedElement() because we want to use nsSVGRenderingObserver even for elements without frames.

Rendering observers are put into the frame's property table - one observer per property. That's enough for normal SVG properties. However, background-image can reference multiple elements. So instead of putting a rendering observer directly into the property, I'm creating a hashtable of observers, keyed by observation URI, into the property. This adds some minor code duplication. In theory, the other properties wouldn't notice if they were put in such a hashtable, too. Should I convert all of them to use the hashtable?

This makes use of an additional parameter to nsReferencedElement::Reset that I added in bug 572688 - it means that mozSetImageElement overrides should be honored. It's only set to true for aProperty == nsSVGEffects::BackgroundImageProperty().

This is to make images and videos without frames work. Instead of only doing AddMutationObserver, call StartMonitoring() which also listens for "load" and "timeupdate" events. The lifecycle considerations here are a little tricky. nsSVGRenderingObservers should only have a right to live while the frame they were created for is alive. That was the case up to now: After being created, they're put in the frame's property table, and when the frame dies, the property table is emptied, which calls our DestroySupports method, which releases the one and only reference to the observer, and the observer dies. However, when the observer listens for events, the monitored element takes another reference to the observer. So we need to make sure these references are unlinked before our frame goes away - otherwise we'd leak the observer, or we'd have a stale mFrame pointer in it which might crash. (There's a crashtest somewhere that found this.) I'm calling StartMonitoring() after the call to aCreate. We can't call it directly from the constructor because we pass "this" to things that want to AddRef() us, and calling virtual methods is hairy while you're in the superclass constructor.

Comment on attachment 451953 [details] [diff] [review] part 1: element accessor and recursion guards >diff --git a/layout/svg/base/src/nsSVGEffects.cpp b/layout/svg/base/src/nsSVGEffects.cpp >--- a/layout/svg/base/src/nsSVGEffects.cpp >+++ b/layout/svg/base/src/nsSVGEffects.cpp >@@ -72,17 +72,18 @@ NS_IMPL_ISUPPORTS1(nsSVGRenderingObserve > #pragma warning(push) > #pragma warning(disable:4355) > #endif > nsSVGRenderingObserver::nsSVGRenderingObserver(nsIURI *aURI, > nsIFrame *aFrame) > : mElement(this), mFrame(aFrame), > mFramePresShell(aFrame->PresContext()->PresShell()), > mReferencedFrame(nsnull), >- mReferencedFramePresShell(nsnull) >+ mReferencedFramePresShell(nsnull), >+ mInUse(PR_FALSE) class member initialisation and declaration should be in the same order. The order above should be fixed. > nsIPresShell *mFramePresShell; > nsIFrame *mReferencedFrame; > nsIPresShell *mReferencedFramePresShell; >+ // True if this property reference is currently "in use" for painting. >+ // This is used to prevent reentry and infinite recursion. >+ PRPackedBool mInUse; > };

Huh? Forgive me if I'm blind, but aren't they already in the same order?

Yes they are. I don't know what I was thinking. Sorry aboutbthat.

Comment on attachment 451953 [details] [diff] [review] part 1: element accessor and recursion guards Currently SVG patterns have a flag in the pattern frame that prevents infinite recursion. I think we need to be consistent and either fix patterns to rely on nsSVGRenderingObserver::mInUse instead, or use the patternframe approach instead of mInUse here.

(In reply to comment #2) > This adds some minor code duplication. In theory, the other properties wouldn't > notice if they were put in such a hashtable, too. Should I convert all of them > to use the hashtable? Yeah, let's just have a single hash and get rid of this plethora of frame properties! Good idea!

Hmm, why do we need part 4? Shouldn't loads and video frame updates invalidate the rendering observers by calling nsSVGRenderingObserverList::InvalidateAll? Elements that don't have frames but still get used (images, canvases, videos) should call that directly when they update, while for elements with frames I presume somewhere in your patch queue is the patch that makes nsFrame::InvalidateInternal notifying rendering observers automatically?

> Shouldn't loads and video frame updates invalidate > the rendering observers by calling nsSVGRenderingObserverList::InvalidateAll? I mean, instead of part 4, I think we should have something like that.

(In reply to comment #9) > (In reply to comment #2) > > This adds some minor code duplication. In theory, the other properties wouldn't > > notice if they were put in such a hashtable, too. Should I convert all of them > > to use the hashtable? > > Yeah, let's just have a single hash and get rid of this plethora of frame > properties! Good idea! That's actually not what I meant. I meant creating a hashtable for each frame property, and for all properties that are not BackgroundImageProperty this hashtable would always only contain one entry. That's not very efficient but it would simplify the code. Putting all types of rendering observers in a single hash sounds good but I'm not sure how it would work. The hashtable is keyed by URI, so you couldn't have two different svg properties (say mask and filter) listen to the same element, could you? These properties need different concrete classes (nsSVGPaintingProperty vs nsSVGFilterProperty).

I guess we'd need one hashtable per type. It still sounds like a net win, if it's simple enough.

(In reply to comment #8) > (From update of attachment 451953 [details] [diff] [review]) > Currently SVG patterns have a flag in the pattern frame that prevents infinite > recursion. I think we need to be consistent and either fix patterns to rely on > nsSVGRenderingObserver::mInUse instead, I think I'll do that. > or use the patternframe approach instead of mInUse here. Where would that flag live, then? On nsIFrame? Does the SVG spec define what should happen in the recursive case? Right now patterns that are already in use just draw nothing, but if something references a pattern frame that doesn't exist, it will use a fallback color (see SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns to use AcquireFrame(), an in-use observer would return null, so we'd go the fallback color route.

(In reply to comment #10) > Hmm, why do we need part 4? Shouldn't loads and video frame updates invalidate > the rendering observers by calling nsSVGRenderingObserverList::InvalidateAll? > Elements that don't have frames but still get used (images, canvases, videos) > should call that directly when they update Unfortunately, rendering observers operate on nsIFrames at the moment, and the observer list is stored in the frame's property table. Should I convert that to operate on elements instead? I see that nsINode has a property table, too, so I could use that. > while for elements with frames I > presume somewhere in your patch queue is the patch that makes > nsFrame::InvalidateInternal notifying rendering observers automatically? Exactly, that's in bug 506826 part 4.

(In reply to comment #14) > > Does the SVG spec define what should happen in the recursive case? Right now > patterns that are already in use just draw nothing, but if something references > a pattern frame that doesn't exist, it will use a fallback color (see > SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns > to use AcquireFrame(), an in-use observer would return null, so we'd go the > fallback color route. That counts as an error and the specification says don't draw objects that are in error.

(In reply to comment #14) > (In reply to comment #8) > > (From update of attachment 451953 [details] [diff] [review] [details]) > > Currently SVG patterns have a flag in the pattern frame that prevents infinite > > recursion. I think we need to be consistent and either fix patterns to rely on > > nsSVGRenderingObserver::mInUse instead, > > I think I'll do that. Sounds good. > Does the SVG spec define what should happen in the recursive case? Right now > patterns that are already in use just draw nothing, but if something references > a pattern frame that doesn't exist, it will use a fallback color (see > SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns > to use AcquireFrame(), an in-use observer would return null, so we'd go the > fallback color route. That sounds OK to me. I don't know that the spec says what to do for this kind of recursion. (In reply to comment #15) > Unfortunately, rendering observers operate on nsIFrames at the moment, and the > observer list is stored in the frame's property table. Should I convert that to > operate on elements instead? I see that nsINode has a property table, too, so I > could use that. Good point. I think we could put an nsSVGRenderingObserverList in that table, actually.

(In reply to comment #16) > (In reply to comment #14) > > > > Does the SVG spec define what should happen in the recursive case? Right now > > patterns that are already in use just draw nothing, but if something references > > a pattern frame that doesn't exist, it will use a fallback color (see > > SetupFallbackOrPaintColor in nsSVGGeometryFrame.cpp). If I converted patterns > > to use AcquireFrame(), an in-use observer would return null, so we'd go the > > fallback color route. > > That counts as an error and the specification says don't draw objects that are > in error. But why is the fallback used in the case where the referenced frame doesn't exist, then? Isn't that also an error?

Comment on attachment 451954 [details] [diff] [review] part 2: Add BackgroundImageProperty and multiple observers per property (In reply to comment #13) > I guess we'd need one hashtable per type. It still sounds like a net win, if > it's simple enough. Looks like I was wrong all along... keying them by URI instead of by property name doesn't seem to be such a good idea after all. You'd have to supply the URI every time you want to get the property, and things like GetClipPathFrame are called from places where the URI isn't readily available.

(In reply to comment #17) > (In reply to comment #15) > > Unfortunately, rendering observers operate on nsIFrames at the moment, and the > > observer list is stored in the frame's property table. Should I convert that to > > operate on elements instead? I see that nsINode has a property table, too, so I > > could use that. > > Good point. I think we could put an nsSVGRenderingObserverList in that table, > actually. I've started doing this and hit a small problem: Moving the observers to the node means that I have to add a node flag for may_have_rendering_observers. Unfortunately, it looks like all node bits are taken. What should I do?

I think we should have 64-bit flags. On 64-bit machines we already allocate 64 bits for PtrBits, so this would only add 32 bits to each node on 32bit machines.

Comment on attachment 451954 [details] [diff] [review] part 2: Add BackgroundImageProperty and multiple observers per property + if (prop && !hashtable->Put(aURI, prop)) { + delete prop; + return nsnull; + } I don't think we need to handle Put failures in the new infallible malloc world.

I'd be ok with 64-bit flags, I think. I can also free up a flag by merging some of the lazy frame stuff in with style system stuff; it'd just take a few days. Either way works for me.... The other thing we could do is just move the slots pointer and flags apart; that costs us space on both architectures but speeds up flag access and frees up the has_slots flag immediately.

Filed bug 581177 on making node flags 64-bit.

(In reply to comment #20) > > Good point. I think we could put an nsSVGRenderingObserverList in that table, > > actually. > > I've started doing this and hit a small problem: Moving the observers to the > node means that I have to add a node flag for may_have_rendering_observers. There's another option: we could just stop using a flag and always check for the observer list property. So instead of checking HasFlag(...) we check GetProperty(...) != nsnull. This might be slightly slower, and as far as I can tell the only place where it would hurt us would be frame destruction (nsFrame::DestroyFrom calls InvalidateDirectRenderingObservers). What performance tests exercise frame destruction?

I don't know if any of our tests exercise frame destruction much (other than when we tear down the whole frame tree). I think 64-bit node flags is the way to go.

> What performance tests exercise frame destruction? Our performance tests? Or in-the-wild ones? For our tests, I think comment 26 is correct. In general, tests that involve DOM or style manipulation would tend to test it.

What patch adds that extra parameter added to Reset? What does it mean?

The patch in bug 572688 - it means that mozSetImageElement should be allowed to override the ID target. We only want this to be the case for the BackgroundImageProperty, not for other properties.

Comment on attachment 464010 [details] [diff] [review] part 3: pass aReferenceImage = PR_TRUE to nsReferenceElement::Reset for the BackgroundImageProperty Ah, of course. Call your parameters here aReferenceImage instead of aWatchImage, I think.

Renamed the paramater.

http://hg.mozilla.org/mozilla-central/rev/77d39c005c6f http://hg.mozilla.org/mozilla-central/rev/1bf9a4c8c8b4 http://hg.mozilla.org/mozilla-central/rev/8b129680a3bb