Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Description • 16 years ago

Summary says it all really. This will make paint servers respect ID-element-map changes and fix some bugs. I just need to dig the patch out of my branch. The patch fixes some missing invalidation issues too.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 1 • 16 years ago

This patch is merged to trunk and compiles, but doesn't actually work. I need to hack on it some more.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 2 • 16 years ago

-- Rename NS_FRAME_MAY_BE_TRANSFORMED to NS_FRAME_MAY_BE_TRANSFORMED_OR_HAVE_RENDERING_OBSERVERS, which is slightly clumsy but honest about how we're overloading this flag. Overloading the flag is ugly but there are no spare flag bits and in the future, when any element can be used as a background-image (or some similar feature exists) and hence have rendering observers, it actually makes sense because both CSS transforms and rendering observers have to hook paths in nsIFrame::InvalidateInternal.

-- nsSVGPropertyBase is a vague name so rename it to nsSVGRenderingObserver. (This object represents a dependency of one frame on the rendering of another frame identified by a URI. One or more of them are attached as frame properties to the observing frame.)

-- Maintain an nsSVGRenderingObserverList for each observed frame which records which nsSVGRenderingObservers are observing it. This is stored as a dynamic frame property.

-- When something relevant in the observed frame (or descendant) changes, it calls nsSVGEffects::InvalidateRenderingObservers. This calls InvalidateViaReferencedFrame on all nsSVGRenderingObservers and breaks the observer relationships. The idea is that a relationship will be reestablished the next time the observing frame needs to reference the observed frame. This "one-shot" behaviour makes the code a lot simpler; for example we can treat frame destruction just the same way as a style change.

-- When an observing frame is destroyed, the associated nsSVGRenderingObserver properties are destroyed so that lets us remove the nsSVGRenderingObservers from the observed frame's nsSVGRenderingObserverList ... unless the presshell is being torn down, in which case we destroy all frames before destroying all properties so the observed frame pointer is bogus. So we have to detect when the observed frame's presshell is being torn down and avoid doing work in that case.

-- When a notification propagates from an observed frame to an observing frame, we automatically propagate the notification if the observing frame is itself observed...

-- Add code to invalidate non-SVG frames when their nsSVGRenderingObservers change...

-- We don't need to watch ParentChainChanged DOM changes for the referenced/observed content because that's just telling us when the content is removed from the document, which causes an ID-element-map change anyway.

-- Clip-path and mask nsSVGRenderingObservers are basically the same: when things change, we just need to repaint. Since the same goes for fill and stroke as well, make them all use a single nsSVGPaintingProperty subclass of nsSVGRenderingObserver.

-- Treat fill and stroke URI references much like clip-path/mask/filter URI references, using an nsSVGPaintingProperty to track relationships. In particular, style changes are handled with an UpdateEffects style-change-hint which calls nsSVGUtils::UpdateEffects which clears cached properties

-- Note that the href 'inheritance' feature of patterns and gradients also uses an nsSVGPaintingProperty so we get all the liveness goodness for that too.

-- This gets rid of a ton of code in the paint servers as you can see.

-- I got rid of HasStroke and HasFill in favour of just returning booleans from Setup methods to indicate whether there was anything to set up.

-- I refactored gradients to simplify things a bit but mainly to eliminate the caching of mNextGrad, which isn't really compatible with the use of nsSVGPaintingProperty to track the relationship. Because I seem to recall that gradient painting needs to be really fast, I do cache a value mNoHRefURI (by far the common case) so we can very quickly avoid doing any work when there's no href set.

-- Patterns get similar treatment. The refactoring provides even more simplifications there.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 3 • 16 years ago

Oh, one of the driving forces behind the one-shot design for nsSVGRenderingObserver(List) is to avoid infinite invalidation loops when you have a cyclic reference structure. One-shot just completely solves that issue. (The existing loop detection flags avoid infinite recursion for painting.)

After this we should give markers the same treatment as paint servers get here. We may need a new nsSVGRenderingObserver subclass because markers can affect bounding boxes, much like filters, but I'm not sure.

The reason nsSVGRenderingObserverList is stored as a property on the frame instead of directly in the SVG frames that currently need it is that eventually I want to allow any frame to support it (e.g. when I allow any element to be a background-image for another element). It's also fairly simple compared to adding a virtual function to get the storage, implemented in each SVG frame type that can have observers.

	tor
	Comment 4 • 16 years ago

Looks like a nice simplification of the current logic.  A few things came up when looking through the patch:

nsStyleSVG::CalcDifference - linecap and linejoin can change the bounding box of the geometry, necessitating more than just a repaint.

nsStyleSVGReset::CalcDifference - dominantBaseline.  ditto (I think).

Using nsReferencedElement doesn't allow us to get rid of the mutation observers?

You have a comment saying that gradients hardly ever refer to one another.  Back when the gradient code was implemented, we saw that inkscape used gradient cross-references frequently.

nsSVGPathGeometryFrame::UpdateCoveredRegion - you missed deleting the now extraneous call to SetupCairoStrokeGeometry.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 16 years ago

(In reply to comment #4)
> Looks like a nice simplification of the current logic.  A few things came up
> when looking through the patch:
> 
> nsStyleSVG::CalcDifference - linecap and linejoin can change the bounding box
> of the geometry, necessitating more than just a repaint.
> 
> nsStyleSVGReset::CalcDifference - dominantBaseline.  ditto (I think).

OK, I'll look into what's required there.

> Using nsReferencedElement doesn't allow us to get rid of the mutation
> observers?

They're still needed in some situations to catch DOM changes in the referenced subtree that should repaint the referring element. Ideally they would go away and we'd use UpdateGraphic and InvalidateRenderingObservers via nsIFrame::AttributeChanged etc whenever rendering could change due to DOM changes.

> You have a comment saying that gradients hardly ever refer to one another. 
> Back when the gradient code was implemented, we saw that inkscape used gradient
> cross-references frequently.

OK, I'll remove the comment. Performance should still be good for that case since nsReferencedElement caches the referenced frame, so the only extra cost I'm adding here is the property lookup.

> nsSVGPathGeometryFrame::UpdateCoveredRegion - you missed deleting the now
> extraneous call to SetupCairoStrokeGeometry.

OK.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 16 years ago

(In reply to comment #5)
> (In reply to comment #4)
> > Looks like a nice simplification of the current logic.  A few things came up
> > when looking through the patch:
> > 
> > nsStyleSVG::CalcDifference - linecap and linejoin can change the bounding box
> > of the geometry, necessitating more than just a repaint.
> > 
> > nsStyleSVGReset::CalcDifference - dominantBaseline.  ditto (I think).
> 
> OK, I'll look into what's required there.

Currently linecap and linejoin are handled because nsSVGPathGeometryFrame::DidSetStyleContext calls UpdateGraphic. Not sure about dominantBaseline, but I'm not changing any of this stuff anyway, so if it's broken that's a separate bug.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 7 • 16 years ago

Tor's comments addressed.

	Mats Palmgren (inactive)
	Comment 8 • 16 years ago

Comment on attachment 339872 [details] [diff] [review]
fix v2

Nice work!

There's a problem with one of the crash tests though:
  layout/svg/crashtests/308917-1.svg
leads to a circular call sequence that blows up the stack:

nsSVGUtils::PaintChildWithEffects
nsSVGDisplayContainerFrame::PaintSVG
nsSVGUtils::PaintChildWithEffects
nsSVGPatternFrame::PaintPattern
nsSVGPatternFrame::SetupPaintServer
nsSVGGeometryFrame::SetupCairoStroke
nsSVGGlyphFrame::PaintSVG


Other than that, a few nits you may want to address:

> nsSVGClipPathFrame.h
> nsSVGGradientFrame.h
> nsSVGFilterFrame.h
> nsSVGMaskFrame.h
> nsSVGPaintServerFrame.h

+#include "nsSVGEffects.h"

No need to add this #include in those files.


> nsSVGPatternFrame.cpp

This file should have a #include "nsSVGEffects.h"


> nsSVGContainerFrame.h
+class nsSVGRenderingObserverList;
+

This declaration isn't needed.


> nsSVGEffects.cpp

+nsIFrame*
+nsSVGRenderingObserver::GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK)
+{
+  nsIFrame* frame = GetReferencedFrame();
+  if (frame && frame->GetType() != aFrameType) {
+    frame = nsnull;
+  }
+  if (aOK && !frame) {
+    *aOK = PR_FALSE;
+  }
+  return frame;
+}

I like the old version better, like so:

nsIFrame*
nsSVGRenderingObserver::GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK)
{
  nsIFrame* frame = GetReferencedFrame();
  if (frame && frame->GetType() == aFrameType) {
    return frame;
  }
  if (aOK) {
    *aOK = PR_FALSE;
  }
  return nsnull;
}


> nsSVGEffects.h

Is there a reason to make nsSVGRenderingObserver::DoUpdate() public?

 
> nsSVGGeometryFrame.cpp

+nsSVGGeometryFrame::GetPaintServer(const nsStyleSVGPaint *aPaint,
...
   nsIURI *uri;
   uri = aPaint->mPaint.mPaintServer;
   if (!uri)
     return nsnull;
 
+  nsSVGPaintingProperty *property =
+    nsSVGEffects::GetPaintingProperty(uri, this, aType);
+  nsIFrame *result = property->GetReferencedFrame();

You could skip the explicit 'uri' null-check and write:
   nsSVGPaintingProperty *property =
     nsSVGEffects::GetPaintingProperty(aPaint->mPaint.mPaintServer,
                                       this, aType);

(as in nsSVGEffects::GetEffectProperties())
But even if you keep the existing 'uri' null-check you still need to
null-check 'property' before using it (OOM).


> nsSVGGradientFrame.cpp
 
In nsSVGGradientFrame::GetGradientTransform()
+  nsSVGGradientElement *element =
+    GetGradientWithAttr(nsGkAtoms::gradientTransform, mContent);

Looks like GetGradientWithAttr() returns null in some loop case,
so we need to null-check 'element' here.
(or did you intend GetGradientWithAttr to return 'aDefault' in that case?)

In nsSVGGradientFrame::GetSpreadMethod()
In nsSVGLinearGradientFrame::GradientLookupAttribute()
In nsSVGGradientFrame::GetGradientUnits()

Ditto.

Please also check callers of GetLinearGradientWithAttr() and
GetRadialGradientWithAttr().


+nsSVGGradientFrame::GetReferencedGradient()
+    property = nsSVGEffects::GetPaintingProperty(targetURI, this, nsGkAtoms::href);

Need an early return if GetPaintingProperty() fails and return null.
(not just OOM in this case, but also if the URI is invalid)


> nsSVGPatternFrame.cpp
+nsSVGPatternFrame::GetReferencedPattern()

Ditto.

 
> nsSVGStopFrame.cpp
In nsSVGStopFrame::DidSetStyleContext()

Should probably call "nsSVGStopFrameBase::DidSetStyleContext()"
just in case someone makes it do something in the future.


> nsSVGUtils.h
 
NS_STATE_SVG_PROPAGATE_TRANSFORM can now be 0x01000000 ?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 16 years ago

> Looks like GetGradientWithAttr() returns null in some loop case,
> so we need to null-check 'element' here.
> (or did you intend GetGradientWithAttr to return 'aDefault' in that case?)

It should return 'aDefault' in that case, so I'll do that.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 10 • 16 years ago

This crashtest is interesting. First I found a different crash, which is due to the fact that we aren't wiping out nsSVGRenderingObservers on a frame which is being removed from the document as part of an entire subtree being removed. That was relatively easy to fix, by calling InvalidateDirectRenderingObservers from nsFrame::Destroy.

The next crash is a simple case of a pattern that contains itself. It looks to me like we've never checked for this; the crashtest didn't fail probably because we weren't tracking ID references correctly as the nodes enter and leave the document.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 11 • 16 years ago

Updated for Mats' comments plus the crash fix as indicated.

	Mats Palmgren (inactive)
	Comment 12 • 16 years ago

Comment on attachment 340043 [details] [diff] [review]
fix v3

Looks good.  sr=mats

I few nits:

> nsFrame.cpp

+  nsSVGEffects::InvalidateDirectRenderingObservers(this);

Would this work:

   if (!shell->IsDestroying())
     nsSVGEffects::InvalidateDirectRenderingObservers(this);


> nsSVGContainerFrame.cpp

+#include "nsSVGEffects.h"

Not necessary anymore.


> nsSVGGradientFrame.cpp

In nsSVGGradientFrame::GetGradientWithAttr (2nd one):
+    return static_cast<nsSVGGradientElement *>(aDefault);

return grad;


==========

FYI, I get the assertion in bug 443538 when reloading a page with mutually
recursive <pattern>s.  It seems to only occur with the patch.

	Mats Palmgren (inactive)
	Comment 13 • 16 years ago

STEPS TO REPRODUCE
1. click the Reload button repeatedly

ACTUAL RESULT
###!!! ASSERTION: No callbacks should be registered while we set up this entry: '!entry->HasContentChangeCallback()', file nsDocument.cpp, line 3292

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 16 years ago

(In reply to comment #12)
> Would this work:
> 
>    if (!shell->IsDestroying())
>      nsSVGEffects::InvalidateDirectRenderingObservers(this);

It would work for now, while the rendering observers have to be in the same presshell as the observed frames, but soon Boris will land his external references patch and that won't be true anymore. Anyway, the extra check doesn't buy you much --- InvalidateDirectRenderingObservers doesn't make any virtual calls or property lookups until it's checked the NS_FRAME_MAY_BE_TRANSFORMED_OR_HAVE_RENDERING_OBSERVERS state bit.

	Mats Palmgren (inactive)
	Comment 15 • 16 years ago

Fair enough, I wasn't so much concerned with the property lookup,
I just had a feeling that delaying it until the shell deletes its
property table would reduce the work.

So, in an effort to understand why it wouldn't work I had a fresh
look at frame/shell/property destruction and I'm afraid I don't see
how the current patch can work for cross-shell references either.

Consider shell A with child frames x and y, shell B (in y) with
frame z, and let z observe x:

A
  x
  y
    B
      z

Now destroy A. In x->Destroy() we will InvalidateViaReferencedFrame()
on z's rendering observer, nulling its mReferencedFrame, then DoUpdate()
where mFramePresShell (B) is still alive but mReferencedFrame is now
null so we don't RemoveRenderingObserver().
Then A continues to destroy y, which destroys B, which destroys
its frames and after that z's observer property: so we run
~nsSVGRenderingObserver() where mReferencedFrame is null and again
we avoid the RemoveRenderingObserver() call.

With the frame tree destroyed, A now destroys its property table which
contains x's observer list property: in ~nsSVGRenderingObserverList()
we call InvalidateAll() which will access z's deleted observer object.

Did I miss something?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 16 years ago

nsSVGRenderingObserverList::InvalidateAll actually removes all observers from mObservers (by returning PL_DHASH_REMOVE from GatherEnumerator). So x->Destroy() will actually wipe out all observers from the list, and when we get around to ~nsSVGRenderingObserverList on the property, InvalidateAll won't do anything.

Another thing, which doesn't really matter here but is worth mentioning, is that SVG external references don't refer to contained subdocuments but to a different sort of document, "external documents" which are (or rather, will be) attached to the "display document" and indexed by URI rather than having an element associated with them. So your scenario can't happen the way you described. (But at some point we might introduce features that would lead to your scenario, so I want to be able to handle it.)

	Mats Palmgren (inactive)
	Comment 17 • 16 years ago

Ah, that makes it very robust then.  Thanks for your patience explaining it.
BTW, the comment on nsSVGRenderingObserverList still mentions
nsSVGContainerFrame::RemoveFrame.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 18 • 16 years ago

Yeah, I'll fix that comment. Just need a review from tor (nudge :-) ).

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 19 • 16 years ago

Comment on attachment 340043 [details] [diff] [review]
fix v3

tor sounds busy --- Robert, if you get to this first, go for it

(Also, reading this patch would give you a head start on fixing up your markers patch)

	Robert Longson [:longsonr]
	Comment 20 • 16 years ago

Comment on attachment 340043 [details] [diff] [review]
fix v3


>--- a/layout/svg/base/src/Makefile.in
>+++ b/layout/svg/base/src/Makefile.in
>@@ -99,20 +99,21 @@ CPPSRCS		= \
> 		$(NULL)
> 
> include $(topsrcdir)/config/config.mk
> 
> # we don't want the shared lib, but we want to force the creation of a static lib.
> FORCE_STATIC_LIB = 1
> 
> EXPORTS = \
>+  nsSVGEffects.h \
>+  nsSVGFilterInstance.h \
>+  nsSVGForeignObjectFrame.h \
> 	nsSVGIntegrationUtils.h \
> 	nsSVGUtils.h \
>-	nsSVGFilterInstance.h \
>-	nsSVGForeignObjectFrame.h \
> 	$(NULL)

Presumably the lines above should contain tabs rather than spaces.

>-void
>-nsSVGFilterProperty::DoUpdate()
>+static void
>+InvalidateAllContinuations(nsIFrame* aFrame)
> {
>-  nsSVGOuterSVGFrame *outerSVGFrame = nsSVGUtils::GetOuterSVGFrame(mFrame);
>-  if (outerSVGFrame) {
>-    outerSVGFrame->Invalidate(mFilterRect);
>-    UpdateRect();
>-    outerSVGFrame->Invalidate(mFilterRect);
>+  for (nsIFrame* f = aFrame; f; f = f->GetNextContinuation()) {
>+    f->Invalidate(f->GetOverflowRect());

f->InvalidateOverflowRect();

>-void
>-nsSVGClipPathProperty::DoUpdate()
>-{

>+    InvalidateAllContinuations(mFrame);
>+    // Reflow so that changes in the filter overflow area get picked up
>+    mFrame->PresContext()->PresShell()->FrameNeedsReflow(
>+         mFrame, nsIPresShell::eResize, NS_FRAME_IS_DIRTY);

Why not mFramePresShell->FrameNeedsReflow(... ?

>+  /**
>+   * @param aOK this is only for the convenience of callers. We set *aOK to false
>+   * if this function returns null.
>+   */
>+  nsIFrame* GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK);
>+

Doesn't aOK distinguish between the cases:
a) the frame does not exist because the effect has not been configured
b) a frame exists but is invalid e.g. mask attribute pointing to a clipPath frame. In this case we should not draw the referencing frame.

>+++ b/layout/svg/base/src/nsSVGPathGeometryFrame.cpp

>-  if (!isHit && (mask & HITTEST_MASK_STROKE)) {
>-    SetupCairoStrokeHitGeometry(&context);
>-    isHit = context.PointInStroke(userSpacePoint);
>+  if (!isHit && (mask & HITTEST_MASK_STROKE) &&
>+      SetupCairoStrokeHitGeometry(&context)) {
>+      isHit = context.PointInStroke(userSpacePoint);

Last line indented two spaces too much.

>+nsSVGPatternFrame *
>+nsSVGPatternFrame::GetReferencedPattern()
>+{
>+  if (mNoHRefURI)
>+    return nsnull;
>+
>+  nsSVGPaintingProperty *property =
>+    static_cast<nsSVGPaintingProperty*>(GetProperty(nsGkAtoms::href));
>+
>+  if (!property) {
>+    // Fetch our gradient element's xlink:href attribute

s/gradient/pattern/

> void
> nsSVGUtils::UpdateEffects(nsIFrame *aFrame)
> {
>   aFrame->DeleteProperty(nsGkAtoms::filter);
>   aFrame->DeleteProperty(nsGkAtoms::mask);
>   aFrame->DeleteProperty(nsGkAtoms::clipPath);
>+  aFrame->DeleteProperty(nsGkAtoms::fill);
>+  aFrame->DeleteProperty(nsGkAtoms::stroke);
> }

This method exists in nsSVGUtils and nsSVGEffects. Could you just pick one and ditch the other. As far as I can see the only caller to either of these is in nsCSSFrameConstructor.cpp.

r=longsonr assuming review comments addressed.

	Robert Longson [:longsonr]
	Comment 21 • 16 years ago

(In reply to comment #20)
> Doesn't aOK distinguish between the cases:
> a) the frame does not exist because the effect has not been configured
> b) a frame exists but is invalid e.g. mask attribute pointing to a clipPath
> frame. In this case we should not draw the referencing frame.

By this I meant that I think that the method comment should say something along these lines.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 22 • 16 years ago

(In reply to comment #20)
> >+  /**
> >+   * @param aOK this is only for the convenience of callers. We set *aOK to false
> >+   * if this function returns null.
> >+   */
> >+  nsIFrame* GetReferencedFrame(nsIAtom* aFrameType, PRBool* aOK);
> >+
> 
> Doesn't aOK distinguish between the cases:
> a) the frame does not exist because the effect has not been configured
> b) a frame exists but is invalid e.g. mask attribute pointing to a clipPath
> frame. In this case we should not draw the referencing frame.

It does, in nsSVGEffects::GetClipPathFrame/GetMaskFrame/GetFilterFrame. But here in nsSVGFilterProperty, it's just what I said in the comment --- the nsSVGFilterProperty wouldn't exist if a filter hadn't been requested.

So I just removed aOK from that method to avoid confusion.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 23 • 16 years ago

This is the patch I want to check in.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 24 • 16 years ago

Pushed 1c681814281d

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 25 • 16 years ago

This was backed out, on suspicion of causing regressions, but then relanded again today. Looks good this time.