411555 - Text inside filter causing invalidation loop

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Attachment: testcase

If text is inside filtered content, the NotifyCTM sent to it when setting up for drawing to the filter surface causes it to invalidate itself, causing the page to continuously repaint.

Possibly a regression from the changes of bug 387422?

Comment 1

[Robert Longson [:longsonr]]

Regression from bug 396771. The NotifyGlyphMetricsChange in nsSVGTextFrame::NotifyCanvasTMChanged is killing us. 

The best option I see is to 

a) introduce a NotifyViewportChanged method which does what NotifyCanvasTMChanged does now.
b) remove the NotifyGlyphMetricsChange call from nsSVGTextFrame::NotifyCanvasTMChanged
c) rename nsSVGUtils::NotifyChildrenCanvasTMChanged to be nsSVGUtils::NotifyChildrenViewportChanged and to call NotifyViewportChanged
d) adjust callers of method in c) to call the new name

So viewport changes will end up calling NotifyGlyphMetricsChange but filter application will not.

Comment 2

[Jonathan Watt [:jwatt]]

I was thinking a method called e.g. NotifySVGChange(PRUint bits) which could be passed various bits to indicate what's changed. As noted elsewhere, we will sometimes want to give notification of both a viewport and TM change, but would quite like to send that as one notification throughout the tree, not two.

I haven't gotten to thinking in any detail about the specific issue in this bug yet.

Comment 3

[Robert Longson [:longsonr]]

That does sound like it would generate less almost duplicate code.

Comment 4

[Robert Longson [:longsonr]]

Attachment: distinguish viewport changes

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 296343 [details] [diff] [review]
distinguish viewport changes

>-  NS_IMETHOD NotifyCanvasTMChanged(PRBool suppressInvalidation)=0;
>+  enum CanvasTMChangedFlags {
>+    SUPPRESS_INVALIDATION = 0x01,
>+    VIEWPORT_CHANGED      = 0x02
>+  };
>+  NS_IMETHOD NotifyCanvasTMChanged(PRUint32 flags = 0)=0;

If the SVG has no viewBox attribute then pure viewport changes will not cause a TM change, so the name NotifyCanvasTMChanged wouldn't make sense. Having a more generic notify method name would seem better, and also allow us to more easily add other notifications in future. I'd suggest adding a TRANSFORM_CHANGED item to the enum and renaming NotifyCanvasTMChanged to NotifySVGChange or something. It would also be nice to keep with convention and rename "flags" to "aFlags" (and remove the defaulting to 0 of course).

I'd also suggest renaming nsSVGUtils::NotifyChildrenCanvasTMChanged to NotifyChildrenOfSVGChange.

>+      SVGFrame->NotifyCanvasTMChanged(nsISVGChildFrame::SUPPRESS_INVALIDATION);

You don't need the "nsISVGChildFrame::" inside the methods of classes that inherit nsISVGChildFrame (here and elsewhere in the patch).

Comment 6

[Robert Longson [:longsonr]]

Attachment: address review comments

(In reply to comment #5)
> (From update of attachment 296343 [details] [diff] [review])
> >-  NS_IMETHOD NotifyCanvasTMChanged(PRBool suppressInvalidation)=0;
> >+  enum CanvasTMChangedFlags {
> >+    SUPPRESS_INVALIDATION = 0x01,
> >+    VIEWPORT_CHANGED      = 0x02
> >+  };
> >+  NS_IMETHOD NotifyCanvasTMChanged(PRUint32 flags = 0)=0;
> 
> If the SVG has no viewBox attribute then pure viewport changes will not cause a
> TM change, so the name NotifyCanvasTMChanged wouldn't make sense. 

I'm think there must be something wrong with that assertion. If you modify this patch such that nsSVGOuterSVGFrame::NotifyViewportChange and nsSVGInnerSVGFrame::NotifyViewportChangeonly only set TRANSFORM_CHANGED when they have a viewbox attribute then the attachment in bug 394463 does not work i.e. when you resize the browser window the mask circle does not resize as it should.

>                                                                  Having a more
> generic notify method name would seem better, and also allow us to more easily
> add other notifications in future. I'd suggest adding a TRANSFORM_CHANGED item
> to the enum and renaming NotifyCanvasTMChanged to NotifySVGChange or something.

Done. I made it NotifySVGChanged to fit in with NotifyRedrawSuspended etc.

> It would also be nice to keep with convention and rename "flags" to "aFlags"
> (and remove the defaulting to 0 of course).

Done.

> 
> I'd also suggest renaming nsSVGUtils::NotifyChildrenCanvasTMChanged to
> NotifyChildrenOfSVGChange.

Done.

> 
> >+      SVGFrame->NotifyCanvasTMChanged(nsISVGChildFrame::SUPPRESS_INVALIDATION);
> 
> You don't need the "nsISVGChildFrame::" inside the methods of classes that
> inherit nsISVGChildFrame (here and elsewhere in the patch).
> 

I can't find anywhere I have made such a mistake. Note that not all frames inherit from nsISVGChildFrame.

Comment 7

[Robert Longson [:longsonr]]

Attachment: best if content is converted too

added 2 missing content files.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #5)
> If the SVG has no viewBox attribute then pure viewport changes will not cause a
> TM change, so the name NotifyCanvasTMChanged wouldn't make sense. 

It looks to me that if any of the inner content e.g. <symbol> contains a viewBox then its content needs a TM change.

Comment 9

[Robert Longson [:longsonr]]

Attachment: check for viewBox

This one optimises the viewport change without breaking bug 394463.

Comment 11

[Jonathan Watt [:jwatt]]

> I can't find anywhere I have made such a mistake. Note that not all frames
> inherit from nsISVGChildFrame.

Ah, sorry, my bad.

> It looks to me that if any of the inner content e.g. <symbol> contains a
> viewBox then its content needs a TM change.

Indeed, any frames for which viewBox is valid and set will have to add TRANSFORM_CHANGED to the flags when they are passed VIEWPORT_CHANGED.

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 296560 [details] [diff] [review]
check for viewBox

Hmm, maybe we would be better off naming the methods just NotifySVG and nsSVGUtils::NotifySVGChildren so we can role in NotifyRedrawSuspended and other things more easily later. Up to you whether you want to do the rename here or not.


nsSVGTextFrame::AttributeChanged(PRInt32
>     // make sure our cached transform matrix gets (lazily) updated
>     mCanvasTM = nsnull;
>     
>     nsIFrame* kid = mFrames.FirstChild();
>     while (kid) {
>       nsISVGChildFrame* SVGFrame = nsnull;
>       CallQueryInterface(kid, &SVGFrame);
>       if (SVGFrame)
>-        SVGFrame->NotifyCanvasTMChanged(PR_FALSE);
>+        SVGFrame->NotifySVGChanged(SUPPRESS_INVALIDATION);
>       kid = kid->GetNextSibling();
>     }

I think you mean TRANSFORM_CHANGED, not SUPPRESS_INVALIDATION. ;-) Also, can you use nsSVGUtils::NotifyChildrenOfSVGChange here and in:

nsSVGAFrame::AttributeChanged
nsSVGGFrame::AttributeChanged
nsSVGUseFrame::AttributeChanged


>+nsSVGUtils::NotifyChildrenOfSVGChange(nsIFrame *aFrame, PRUint32 aFlags)

I don't really like having the viewBox logic hidden in here. I'd much rather see it out in the open where it applies in nsSVGOuterSVGFrame etc. Besides viewBox not applying to many of the frames on which these checks would be made, it seems wasteful and ugly. How about in frames that use viewBox using something along the lines of:

  PRUint32 flags = VIEWPORT_CHANGE;
  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
    flags |= TRANSFORM_CHANGE;
  }


I have more comments to come on foreignObject.

Comment 13

[Jonathan Watt [:jwatt]]

There are a few things wrong with nsSVGForeignObjectFrame::NotifySVGChanged:

a) My comment about not not requiring the IsReflowLocked if we have a separate notification for viewport changes is wrong. If the viewport change originated from a change to the width/height of an _inner_ <svg>, we still want to reflow for example.

b) The UpdateGraphic and RequestReflow calls should go together (so that's a bug in the existing code too).

c) If we add notification types other than for CTM and viewport changes, it will call reflow.

d) The width/height comments don't really make sense under the TRANSFORM_CHANGED check.

I've thought about this for a while and I think what we want is the following (it's easier to provide the code than try to describe it):

nsSVGForeignObjectFrame::NotifySVGChanged(PRUint32 aFlags)
{
  PRBool reflow = PR_FALSE;

  if (aFlags & TRANSFORM_CHANGED) {
    // Perhaps unexpectedly, we reflow if our CTM changes. This is because
    // glyph metrics do not necessarily scale uniformly with change in scale
    // and, as a result, CTM changes may require text to break at different
    // points.
    // XXX roc says we shouldn't do this. See bug 381285 comment 20.
    reflow = PR_TRUE;
    mCanvasTM = nsnull;
  }
  else if (aFlags & VIEWPORT_CHANGED) {
    // Our coordinate context's width/height has changed. If we have a
    // percentage width/height our dimensions will change so we must reflow.
    nsSVGForeignObjectElement *fO =
      static_cast<nsSVGForeignObjectElement*>(mContent);
    if (fO->mLengthAttributes[nsSVGForeignObjectElement::WIDTH]
        .GetSpecifiedUnitType() == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE ||
        fO->mLengthAttributes[nsSVGForeignObjectElement::HEIGHT]
        .GetSpecifiedUnitType() == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE) {
      reflow = PR_TRUE;
    }
  }

  if (reflow) {
    // If we're called while the PresShell is handling reflow events then we
    // must have been called as a result of the NotifyViewportChange() call in
    // our nsSVGOuterSVGFrame's Reflow() method. We must not call RequestReflow
    // at this point (i.e. during reflow) because it could confuse the
    // PresShell and prevent it from reflowing us properly in future. Besides
    // that, nsSVGOuterSVGFrame::DidReflow will take care of reflowing us
    // synchronously, so there's no need.
    PRBool reflowing;
    PresContext()->PresShell()->IsReflowLocked(&reflowing);
    if (!reflowing) {
      UpdateGraphic(); // update mRect before requesting reflow
      RequestReflow(nsIPresShell::eResize);
    }
  }
}


One other thing, you might as well make NotifySVGChanged return void, since we never use/check the return value, and I don't think we'll ever want/need to.

Comment 14

[Robert Longson [:longsonr]]

Attachment: address review comments

I didn't do the optional name change, combining redraw suspended is probably a post 1.9 thing at this point.

I did everything else including foreignObject changes though.

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 296938 [details] [diff] [review]
address review comments

>+  enum SVGChangedFlags {
>+    SUPPRESS_INVALIDATION = 0x01,
>+    TRANSFORM_CHANGED     = 0x02,
>+    VIEWPORT_CHANGED      = 0x04
>+  };

Just above the enum can you put a comment like this:

  // Flags to pass to NotifySVGChange:
  //
  // SUPPRESS_INVALIDATION - do not invalidate stored transforms (only to be used in conjunction with TRANSFORM_CHANGED)
  // TRANSFORM_CHANGED - the current transform matrix for this frame has changed
  // VIEWPORT_CHANGED - the dimensions of this frame's coordinate context has changed (percentage lengths must be reevaluated)


>+nsSVGAFrame::NotifySVGChanged(PRUint32 aFlags)
> {
>+  if (aFlags & TRANSFORM_CHANGED) {
>+    // make sure our cached transform matrix gets (lazily) updated
>+    mCanvasTM = nsnull;
>+  }

Shouldn't we check for SUPPRESS_INVALIDATION? Can you put in an XXX comment about that? Also see nsSVGGFrame::NotifySVGChanged.


nsSVGClipPathFrame::ClipPaint(nsSVGRende
>                            isTrivial ? nsSVGRenderState::CLIP
>                                      : nsSVGRenderState::CLIP_MASK);
> 
>   for (nsIFrame* kid = mFrames.FirstChild(); kid;
>        kid = kid->GetNextSibling()) {
>     nsISVGChildFrame* SVGFrame = nsnull;
>     CallQueryInterface(kid, &SVGFrame);
>     if (SVGFrame) {
>-      SVGFrame->NotifyCanvasTMChanged(PR_TRUE);
>+      SVGFrame->NotifySVGChanged(nsISVGChildFrame::SUPPRESS_INVALIDATION | 
>+                                 nsISVGChildFrame::TRANSFORM_CHANGED);
>       SVGFrame->PaintSVG(aContext, nsnull);
>     }
>   }

Not sure why we're doing the notify here? Perhaps you can add an XXX comment about that, or else a comment saying why.


>+nsSVGDisplayContainerFrame::NotifySVGChanged(PRUint32 aFlags)

Can you put an assertion at the beginning of this method asserting that aFlags contains TRANSFORM_CHANGED or VIEWPORT_CHANGED (so we'll notice to update that method if we add more flags for which we don't want to call UpdateFilterRegion).

Also please add an XXX comment about only wanting to invalidate for VIEWPORT_CHANGED if we have a percentage dimension (perhaps we need a HasPercentageDimension on nsISVGChildFrame or something, but that's for another bug).


>+nsSVGGlyphFrame::NotifySVGChanged(PRUint32 aFlags)
> {
>+  if (aFlags & TRANSFORM_CHANGED) {
>+    UpdateGeometry(PR_TRUE, (aFlags & SUPPRESS_INVALIDATION) != 0);
>+  }
> }

Do we want to call UpdateGeometry for VIEWPORT_CHANGED too since text can have percentage based positioning? Or is that only TextFrame, not GlyphFrame? Hmm. At any rate, this is certainly the case for nsSVGPathGeometryFrame::NotifySVGChanged where we should call UpdateGraphic for TRANSFORM_CHANGED to get the right invalidation of the canvas.


>+nsSVGTextFrame::NotifySVGChanged(PRUint32 aFlags)
> {
>+  if (aFlags & TRANSFORM_CHANGED) {
>+    // make sure our cached transform matrix gets (lazily) updated
>+    mCanvasTM = nsnull;
>+  }
> 
>+  if (aFlags & VIEWPORT_CHANGED) {
>+    // If we are positioned using percentage values we need to update our
>+    // position whenever our viewport's dimensions change.
>+    NotifyGlyphMetricsChange();

We need to call NotifyGlyphMetricsChange for TRANSFORM_CHANGED too to invalidate the canvas area. Can you also make this an XXX comment again (to draw attention to the fact that we're not actually checking for percentage values right now).


I'm still thinking about nsSVG[Outer|Inner]SVGFrame.

Comment 16

[Jonathan Watt [:jwatt]]

Please rename VIEWPORT_CHANGED to COORD_CONTEXT_CHANGED. We'll want to issue this notification for changes to the viewBox attribute as well as viewport changes, so the word "viewport" in there could be misleading.

Comment 17

[Robert Longson [:longsonr]]

(In reply to comment #15)
> (From update of attachment 296938 [details] [diff] [review])
> Just above the enum can you put a comment like this:
> 
>   // Flags to pass to NotifySVGChange:
>   //
>   // SUPPRESS_INVALIDATION - do not invalidate stored transforms (only to be
> used in conjunction with TRANSFORM_CHANGED)
>   // TRANSFORM_CHANGED - the current transform matrix for this frame has
> changed
>   // VIEWPORT_CHANGED - the dimensions of this frame's coordinate context has
> changed (percentage lengths must be reevaluated)

OK. Although I'll make SUPPRESS_INVALIDATION say 

  // SUPPRESS_INVALIDATION - do not invalidate rendered areas (only to be
  //                           used in conjunction with TRANSFORM_CHANGED)

> 
> 
> >+nsSVGAFrame::NotifySVGChanged(PRUint32 aFlags)
> > {
> >+  if (aFlags & TRANSFORM_CHANGED) {
> >+    // make sure our cached transform matrix gets (lazily) updated
> >+    mCanvasTM = nsnull;
> >+  }
> 
> Shouldn't we check for SUPPRESS_INVALIDATION? Can you put in an XXX comment
> about that? Also see nsSVGGFrame::NotifySVGChanged.

No, SUPPRESS_INVALIDATION is about not updating or processing the invalidation region, neither of which occur here. I don't think a comment is warranted, if you still think it is then I'd appreciate suggested wording.

> 
> 
> nsSVGClipPathFrame::ClipPaint(nsSVGRende
> >                            isTrivial ? nsSVGRenderState::CLIP
> >                                      : nsSVGRenderState::CLIP_MASK);
> > 
> >   for (nsIFrame* kid = mFrames.FirstChild(); kid;
> >        kid = kid->GetNextSibling()) {
> >     nsISVGChildFrame* SVGFrame = nsnull;
> >     CallQueryInterface(kid, &SVGFrame);
> >     if (SVGFrame) {
> >-      SVGFrame->NotifyCanvasTMChanged(PR_TRUE);
> >+      SVGFrame->NotifySVGChanged(nsISVGChildFrame::SUPPRESS_INVALIDATION | 
> >+                                 nsISVGChildFrame::TRANSFORM_CHANGED);
> >       SVGFrame->PaintSVG(aContext, nsnull);
> >     }
> >   }
> 
> Not sure why we're doing the notify here? Perhaps you can add an XXX comment
> about that, or else a comment saying why.

That's bug 341257. I'll added the following comment.

      // Notify children that their CTM may have changed

> 
> 
> >+nsSVGDisplayContainerFrame::NotifySVGChanged(PRUint32 aFlags)
> 
> Can you put an assertion at the beginning of this method asserting that aFlags
> contains TRANSFORM_CHANGED or VIEWPORT_CHANGED (so we'll notice to update that
> method if we add more flags for which we don't want to call
> UpdateFilterRegion).

The filter region is an invalidation region, connected with SUPPRESS_INVALIDATION, I can't see it ever being connected with anything else. If some new notify wanted to avoid it it should set SUPPRESS_INVALIDATION too.

> 
> Also please add an XXX comment about only wanting to invalidate for
> VIEWPORT_CHANGED if we have a percentage dimension (perhaps we need a
> HasPercentageDimension on nsISVGChildFrame or something, but that's for another
> bug).

Not sure where you want this. nsSVGDisplayContainerFrame is too generic we wouldn't change it there. I guess we might add this comment to nsSVG[Outer/Inner]SVGFrame

> 
> 
> >+nsSVGGlyphFrame::NotifySVGChanged(PRUint32 aFlags)
> > {
> >+  if (aFlags & TRANSFORM_CHANGED) {
> >+    UpdateGeometry(PR_TRUE, (aFlags & SUPPRESS_INVALIDATION) != 0);
> >+  }
> > }
> 
> Do we want to call UpdateGeometry for VIEWPORT_CHANGED too since text can have
> percentage based positioning? Or is that only TextFrame, not GlyphFrame? Hmm.
> At any rate, this is certainly the case for
> nsSVGPathGeometryFrame::NotifySVGChanged where we should call UpdateGraphic for
> TRANSFORM_CHANGED to get the right invalidation of the canvas.

I think you are right here.

> 
> 
> >+nsSVGTextFrame::NotifySVGChanged(PRUint32 aFlags)
> > {
> >+  if (aFlags & TRANSFORM_CHANGED) {
> >+    // make sure our cached transform matrix gets (lazily) updated
> >+    mCanvasTM = nsnull;
> >+  }
> > 
> >+  if (aFlags & VIEWPORT_CHANGED) {
> >+    // If we are positioned using percentage values we need to update our
> >+    // position whenever our viewport's dimensions change.
> >+    NotifyGlyphMetricsChange();
> 
> We need to call NotifyGlyphMetricsChange for TRANSFORM_CHANGED too to
> invalidate the canvas area. Can you also make this an XXX comment again (to
> draw attention to the fact that we're not actually checking for percentage
> values right now).

The whole point of this bug is NOT to do that (even with percentage values) or you get into an infinite loop with the testcase. Checking for percentages might not ever be worthwhile as you will eventually be able to have coordinates for each glyph.

> 
> 
> I'm still thinking about nsSVG[Outer|Inner]SVGFrame.
> 

Comment 18

[Robert Longson [:longsonr]]

Attachment: address some review comments

(In reply to comment #16)
> Please rename VIEWPORT_CHANGED to COORD_CONTEXT_CHANGED. We'll want to issue
> this notification for changes to the viewBox attribute as well as viewport
> changes, so the word "viewport" in there could be misleading.
> 

Done, including the things I said I would do in comment 17.

Comment 19

[Jonathan Watt [:jwatt]]

> OK. Although I'll make SUPPRESS_INVALIDATION say 
> 
>   // SUPPRESS_INVALIDATION - do not invalidate rendered areas (only to be
>   //                           used in conjunction with TRANSFORM_CHANGED)

A good demonstration of why a clarifying comment is helpful. ;-)


> That's bug 341257. I'll added the following comment.
> 
>       // Notify children that their CTM may have changed

Well we're sending the CTM changed notification, so that's self evident. What I was more looking for was a "why". Can you make it:

  // The CTM of each frame referencing us can be different.


> The filter region is an invalidation region, connected with
> SUPPRESS_INVALIDATION, I can't see it ever being connected with anything else.
> If some new notify wanted to avoid it it should set SUPPRESS_INVALIDATION too.

If we add a REDRAW_SUSPENDED notification, it doesn't really make sense to require callers to also pass SUPPRESS_INVALIDATION. That said, anyone adding new notifications should really examine all implementations and notice this problem in future. The assertion would just catch things if they err.


> Not sure where you want this. nsSVGDisplayContainerFrame is too generic we
> wouldn't change it there. I guess we might add this comment to
> nsSVG[Outer/Inner]SVGFrame

Okay, leave this for the moment.


> The whole point of this bug is NOT to do that (even with percentage values) or
> you get into an infinite loop with the testcase.

Ah, hmm.

> Checking for percentages might
> not ever be worthwhile as you will eventually be able to have coordinates for
> each glyph.

Okay, can you just add a comment saying we could, but it's [probably] not worth it. It just helps capture a little more knowledge for new people that isn't conveyed by the code.

Comment 20

[Robert Longson [:longsonr]]

Attachment: address more review comments

(In reply to comment #19)
> Well we're sending the CTM changed notification, so that's self evident. What I
> was more looking for was a "why". Can you make it:
> 
>   // The CTM of each frame referencing us can be different.

Done.

> 
> 
> > The filter region is an invalidation region, connected with
> > SUPPRESS_INVALIDATION, I can't see it ever being connected with anything else.
> > If some new notify wanted to avoid it it should set SUPPRESS_INVALIDATION too.
> 
> If we add a REDRAW_SUSPENDED notification, it doesn't really make sense to
> require callers to also pass SUPPRESS_INVALIDATION. That said, anyone adding
> new notifications should really examine all implementations and notice this
> problem in future. The assertion would just catch things if they err.

Requested assertion added, although I would have done it as REDRAW_SUSPENDED = 0x08 | SUPPRESS_INVALIDATION.

> Okay, can you just add a comment saying we could, but it's [probably] not worth
> it. It just helps capture a little more knowledge for new people that isn't
> conveyed by the code.
> 

Done.

Comment 21

[Jonathan Watt [:jwatt]]

Comment on attachment 297750 [details] [diff] [review]
address more review comments

>+nsSVGInnerSVGFrame::NotifySVGChanged(PRUint32 aFlags)
> {
>+  // if we have a viewBox attribute, viewport changes affect our transform
>+  if (((aFlags & (COORD_CONTEXT_CHANGED | TRANSFORM_CHANGED)) == COORD_CONTEXT_CHANGED) &&
>+      mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
>+    aFlags |= TRANSFORM_CHANGED;
>+  }

Actually if we have percentage 'x' or 'y', we also need to add TRANSFORM_CHANGED when we get a COORD_CONTEXT_CHANGED notification. We also want to remove COORD_CONTEXT_CHANGED under certain conditions. For now though I'd suggest just adding TRANSFORM_CHANGED to aFlags unconditionally, preceeded by the following XXX comment:

  // XXX Coordinate context changes only affect our transform if we have a
  // percentage 'x' or 'y', or if we have a percentage 'width' or 'height' AND
  // a 'viewBox'. Also note that we can drop COORD_CONTEXT_CHANGED
  // notifications if we have a non-percentage 'width' AND 'height, or if we
  // have a 'viewBox'. For now though, just do the sample thing.

Unless you feel like implementing that logic of course. Besides that, I'd have found this more readable:

  if (!(aFlags & TRANSFORM_CHANGED) && (aFlags & COORD_CONTEXT_CHANGED) &&


> nsSVGInnerSVGFrame::NotifyViewportChange()
> {
>+  PRUint32 flags = COORD_CONTEXT_CHANGED;
>+
>+  // viewport changes only affect our transform if we have a viewBox attribute
>+  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
>+    // make sure canvas transform matrix gets (lazily) recalculated:
>+    mCanvasTM = nsnull;
>+
>+    flags |= TRANSFORM_CHANGED;
>+  }

Sadly we can't do this right now. nsSVGSVGElement::InvalidateTransformNotifyFrame calls result in NotifyViewportChange being called for changes to 'x' and 'y' which require stored transforms to be updated. I'd suggest that put in something like:

#if 1
  // XXX nsSVGSVGElement::InvalidateTransformNotifyFrame calls us for changes
  // to 'x' and 'y'. Until this is fixed, add TRANSFORM_CHANGED to flags
  // unconditionally.
  flags |= TRANSFORM_CHANGED;
else
  <your correct code here>
endif


nsSVGMarkerFrame::PaintMark(nsSVGRenderS
>     nsSVGUtils::SetClipRect(gfx, matrix, x, y, width, height);
>   }
> 
>   for (nsIFrame* kid = mFrames.FirstChild(); kid;
>        kid = kid->GetNextSibling()) {
>     nsISVGChildFrame* SVGFrame = nsnull;
>     CallQueryInterface(kid, &SVGFrame);
>     if (SVGFrame) {
>-      SVGFrame->NotifyCanvasTMChanged(PR_TRUE);
>+      SVGFrame->NotifySVGChanged(nsISVGChildFrame::SUPPRESS_INVALIDATION |
>+                                 nsISVGChildFrame::TRANSFORM_CHANGED);

Can you also add the comment here:

  // The CTM of each frame referencing us may be different.

Comment 22

[Robert Longson [:longsonr]]

(In reply to comment #21)
> 
>   // XXX Coordinate context changes only affect our transform if we have a
>   // percentage 'x' or 'y', or if we have a percentage 'width' or 'height' AND
>   // a 'viewBox'. Also note that we can drop COORD_CONTEXT_CHANGED
>   // notifications if we have a non-percentage 'width' AND 'height, or if we
>   // have a 'viewBox'. For now though, just do the sample thing.


Not sure I understand the penultimate sentence about dropping COORD_CONTEXT_CHANGED and having a viewBox. Did you mean non-percentage width and height and a viewBox. Otherwise it is not consistent with the logic in the first sentence where we are adding COORD_CONTEXT_CHANGED in some circumstances if we have a viewBox.

Comment 23

[Robert Longson [:longsonr]]

Attachment: address additional review comments

(In reply to comment #21)
>   // XXX Coordinate context changes only affect our transform if we have a
>   // percentage 'x' or 'y', or if we have a percentage 'width' or 'height' AND
>   // a 'viewBox'. Also note that we can drop COORD_CONTEXT_CHANGED
>   // notifications if we have a non-percentage 'width' AND 'height, or if we
>   // have a 'viewBox'. For now though, just do the sample thing.
> 
> Unless you feel like implementing that logic of course. Besides that, I'd have
> found this more readable:

I implemented the add logic but not the removal logic as I'm less sure of it. I left an XXX comment for the removal logic.

> #if 1
>   // XXX nsSVGSVGElement::InvalidateTransformNotifyFrame calls us for changes
>   // to 'x' and 'y'. Until this is fixed, add TRANSFORM_CHANGED to flags
>   // unconditionally.
>   flags |= TRANSFORM_CHANGED;
> else
>   <your correct code here>
> endif

Done.

> Can you also add the comment here:
> 
>   // The CTM of each frame referencing us may be different.
> 

Done.

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 297803 [details] [diff] [review]
address additional review comments

>+nsSVGInnerSVGFrame::NotifySVGChanged(PRUint32 aFlags)
> {
>+  if (aFlags & COORD_CONTEXT_CHANGED) {
>+
>+    nsSVGSVGElement *svg = static_cast<nsSVGSVGElement*>(mContent);
>+
>+    // Coordinate context changes only affect our transform if we have a

Actually can you drop the "only"? Also maybe s/our transform/mCanvasTM/.


>+    // percentage 'x' or 'y', or if we have a percentage 'width' or 'height' AND
>+    // a 'viewBox'.
>+
>+    if (!(aFlags & TRANSFORM_CHANGED) &&
>+        svg->mLengthAttributes[nsSVGSVGElement::X]
>+          .GetSpecifiedUnitType() == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE ||
>+        svg->mLengthAttributes[nsSVGSVGElement::Y]
>+          .GetSpecifiedUnitType() == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE ||
>+        (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox) &&
>+         (svg->mLengthAttributes[nsSVGSVGElement::WIDTH]
>+            .GetSpecifiedUnitType() == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE ||
>+          svg->mLengthAttributes[nsSVGSVGElement::HEIGHT]
>+            .GetSpecifiedUnitType() == nsIDOMSVGLength::SVG_LENGTHTYPE_PERCENTAGE))) {
>+    
>+      aFlags |= TRANSFORM_CHANGED;
>+    }

It's almost worth adding an inline method to nsSVGElement so we can do svg->IsPercentageLength(nsSVGSVGElement::X) --  it would certainly be more readable. :-)


(In reply to comment #22)
> (In reply to comment #21)
> >   // XXX Coordinate context changes only affect our transform if we have a
> >   // percentage 'x' or 'y', or if we have a percentage 'width' or 'height' AND
> >   // a 'viewBox'. Also note that we can drop COORD_CONTEXT_CHANGED
> >   // notifications if we have a non-percentage 'width' AND 'height, or if we
> >   // have a 'viewBox'.
> 
> Not sure I understand the penultimate sentence about dropping
> COORD_CONTEXT_CHANGED and having a viewBox. Did you mean non-percentage width
> and height and a viewBox. Otherwise it is not consistent with the logic in the
> first sentence where we are adding COORD_CONTEXT_CHANGED in some circumstances
> if we have a viewBox.

The first sentence is about adding TRANSFORM_CHANGED, not COORD_CONTEXT_CHANGED, so there's no inconsistency.

Essentially we'd be changing the COORD_CONTEXT_CHANGED notification into a TRANSFORM_CHANGED notification. This is because, when we have a viewBox rect, the viewBox rect is the coordinate context for our children, and it isn't changing. Percentage lengths on our children will continue to resolve to the same number of user units because they're relative to our viewBox rect. The same is true if we have a non-percentage width and height and don't have a viewBox. We (the <svg>) establish the coordinate context for our children. Our children don't care about changes to our parent coordinate context unless that change results in a change to the coordinate context that _we_ establish. Hence we can (should, really) stop propagating COORD_CONTEXT_CHANGED in these cases.

Anyway, we'd actually need to check that we have a viewBox rect and not just that viewBox is set, since it could be set to none of course. That'll need to wait until I've finished my viewBox rewrite. For now the comment is good (maybe add some text pointing to this comment to make sure anyone not understanding it in future can find this explanation).

Comment 25

[Robert Longson [:longsonr]]

Attachment: address final review comments

(In reply to comment #24)
> Actually can you drop the "only"? Also maybe s/our transform/mCanvasTM/.

Done.


> It's almost worth adding an inline method to nsSVGElement so we can do
> svg->IsPercentageLength(nsSVGSVGElement::X) --  it would certainly be more
> readable. :-)

Added an IsPercentage to nsSVGLength2 instead which has a similar effect.

> The first sentence is about adding TRANSFORM_CHANGED, not
> COORD_CONTEXT_CHANGED, so there's no inconsistency.
> 
> Essentially we'd be changing the COORD_CONTEXT_CHANGED notification into a
> TRANSFORM_CHANGED notification. This is because, when we have a viewBox rect,
> the viewBox rect is the coordinate context for our children, and it isn't
> changing. Percentage lengths on our children will continue to resolve to the
> same number of user units because they're relative to our viewBox rect. The
> same is true if we have a non-percentage width and height and don't have a
> viewBox. We (the <svg>) establish the coordinate context for our children. Our
> children don't care about changes to our parent coordinate context unless that
> change results in a change to the coordinate context that _we_ establish. Hence
> we can (should, really) stop propagating COORD_CONTEXT_CHANGED in these cases.
> 
> Anyway, we'd actually need to check that we have a viewBox rect and not just
> that viewBox is set, since it could be set to none of course. That'll need to
> wait until I've finished my viewBox rewrite. For now the comment is good (maybe
> add some text pointing to this comment to make sure anyone not understanding it
> in future can find this explanation).
> 

Added this text to the comment.

Comment 26

[Robert Longson [:longsonr]]

Attachment: fix reftest failures

I was on the point of checking this in when I noticed that it causes reftest failures in

/mozilla/layoutreftests/svg/sizing/dynamic--inline-css-height.xhtml
/mozilla/layout/reftests/svg/sizing/dynamic--inline-css-width.xhtml

The only change in this patch is in the method nsSVGOuterSVGFrame::NotifyViewportChange 

I've gone back to the old code which clears the mCanvasTM and sets the TRANSFORM_CHANGED flag unconditionally. i.e.

+  PRUint32 flags = COORD_CONTEXT_CHANGED;
 
-/* XXX this caused reftest failures
   // viewport changes only affect our transform if we have a viewBox attribute
-  nsSVGSVGElement *svgElem = static_cast<nsSVGSVGElement*>(mContent);
-  if (!svgElem->HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
-    return NS_OK;
+#if 1
+  {
+#else
+  // XXX this caused reftest failures
+  if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::viewBox)) {
+#endif
+    // make sure canvas transform matrix gets (lazily) recalculated:
+    mCanvasTM = nsnull;
+
+    flags |= TRANSFORM_CHANGED;
   }
-*/

Comment 27

[Jonathan Watt [:jwatt]]

Comment on attachment 298923 [details] [diff] [review]
fix reftest failures

r=jwatt

It seems like you've uncovered a weird bug with early reflow. Anyway, this change puts things back to the full notification as the code is now, so that's fine. We can look into this new bug and try to put the notification optimization back later.

Comment 28

[Jonathan Watt [:jwatt]]

Can you file another bug on this and add the bug number to the comment in the source?

Comment 29

[Robert Longson [:longsonr]]

bug 413960 created

Comment 30

[Robert Longson [:longsonr]]

checked in with comment change to mention bug 413960.