689546 - Simplify attribute updates to svg elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

Currently we override DidSetXXX rather than just picking up the change in AttributeChanged.

Comment 1

[Jonathan Watt [:jwatt]]

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

It seems to me that the AttributeChanged implementations should be sending the COORD_CONTEXT_CHANGED flag down to their children if the viewBox changed, or if the width/height changed and there is no viewBox.

> nsSVGInnerSVGFrame::AttributeChanged(PRInt32  aNameSpaceID,
>                                      nsIAtom* aAttribute,
>                                      PRInt32  aModType)
> {
>   if (aNameSpaceID == kNameSpaceID_None) {
>     if (aAttribute == nsGkAtoms::width ||
>-        aAttribute == nsGkAtoms::height ||
>-        aAttribute == nsGkAtoms::preserveAspectRatio ||
>-        aAttribute == nsGkAtoms::viewBox) {
>+        aAttribute == nsGkAtoms::height) {
>       nsSVGUtils::UpdateGraphic(this);
>     } else if (aAttribute == nsGkAtoms::transform ||
>+               aAttribute == nsGkAtoms::preserveAspectRatio ||
>+               aAttribute == nsGkAtoms::viewBox ||
>                aAttribute == nsGkAtoms::x ||
>                aAttribute == nsGkAtoms::y) {
>       // make sure our cached transform matrix gets (lazily) updated
>       mCanvasTM = nsnull;
> 
>       nsSVGUtils::NotifyChildrenOfSVGChange(this, TRANSFORM_CHANGED);
>     }
>   }

It also seems like we still want the InvalidateRenderingObservers, UpdateAndInvalidateCoveredRegion and NotifyAncestorsOfFilterRegionChange calls in UpdateGraphic when viewBox/preserveAspectRatio change (and x and y actually), or does that happen elsewhere?

Comment 2

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #1)
> 
> It seems to me that the AttributeChanged implementations should be sending
> the COORD_CONTEXT_CHANGED flag down to their children if the viewBox
> changed, or if the width/height changed and there is no viewBox.

OK, I'll update the patch for that.

> It also seems like we still want the InvalidateRenderingObservers,
> UpdateAndInvalidateCoveredRegion and NotifyAncestorsOfFilterRegionChange
> calls in UpdateGraphic when viewBox/preserveAspectRatio change (and x and y
> actually), or does that happen elsewhere?

when NotifySVGChanged propagates to leaf frames (e.g. nsSVGGlyphFrame) it calls nsSVGUtils:UpdateGraphic which calls InvalidateRenderingObservers,  UpdateAndInvalidateCoveredRegion and NotifyAncestorsOfFilterRegionChange so yes, that happens elsewhere.

Comment 3

[Jonathan Watt [:jwatt]]

Ah, I see.

Did you consider removing NotifyViewportChange from nsISVGSVGFrame and nsSVGInnerSVGFrame? In nsSVGSVGElement::InvalidateTransformNotifyFrame() you could check !IsInner() and cast to nsSVGOuterSVGFrame...or I think you can QI to nsSVGOuterSVGFrame.

Hmm...I'm thinking probably we should be returning a "not implemented" exception from nsSVGSVGElement::SetCurrentScaleTranslate if !IsRoot() though.

I'll review your next patch.

Comment 4

[Robert Longson [:longsonr]]

Attachment: updated patch

(In reply to Jonathan Watt [:jwatt] (away 1-9 Oct, inc.) from comment #3)
> 
> Did you consider removing NotifyViewportChange from nsISVGSVGFrame and
> nsSVGInnerSVGFrame? In nsSVGSVGElement::InvalidateTransformNotifyFrame() you
> could check !IsInner() and cast to nsSVGOuterSVGFrame...or I think you can
> QI to nsSVGOuterSVGFrame.
> 

No, that seems overly complicated now that most callers of NotifyViewportChange have gone. You'd need to either resize the browser window or zoom. Before it would be called whenver you'd animate an <svg> attribute so it's much less animation performance sensitive now.

I've better performance fish to fry next as I intend to create a state bit for redraw suspended in my next bug and propagate that so that update graphic doesn't need to find the outer frame if redraw is suspended. I might even be able to get rid of the traversal to the outer svg frame on redraws altogether.

Comment 5

[Jonathan Watt [:jwatt]]

I was suggesting removing NotifyViewportChange from nsISVGSVGFrame to simplify the code, and as a step to getting rid of that interface. Anyway, it's not a big deal.

Comment 6

[Robert Longson [:longsonr]]

I'll look into it as part of bug 413960

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 563340 [details] [diff] [review]
updated patch

>+  // we'll get notified by the outerSVGFrame anyway

This comment seems wrong. nsSVGInnerSVGFrame::NotifyViewportChange is called by the nsSVGSVGElement mContent. It's not that the outerSVGFrame will notify us, but rather that our AttributeChanged method will be called by our own nsSVGSVGElement, and we'll handle the changes there.

>+    } else if (aAttribute == nsGkAtoms::width ||
>+               aAttribute == nsGkAtoms::height) {
>+
>+        PRBool hasValidViewBox =
>+                 static_cast<nsSVGSVGElement*>(mContent)->mViewBox.IsValid();
>+
>+        if (hasValidViewBox) {
>+          // make sure our cached transform matrix gets (lazily) updated
>+          mCanvasTM = nsnull;
>+        }
>+        nsSVGUtils::NotifyChildrenOfSVGChange(
>+           this, hasValidViewBox ? TRANSFORM_CHANGED : COORD_CONTEXT_CHANGED);
>+
>+        nsIFrame* embeddingFrame;
>+        if (IsRootOfReplacedElementSubDoc(&embeddingFrame) && embeddingFrame) {
>+          if (DependsOnIntrinsicSize(embeddingFrame)) {
>+            // Tell embeddingFrame's presShell it needs to be reflowed (which takes
>+            // care of reflowing us too).
>+            embeddingFrame->PresContext()->PresShell()->
>+              FrameNeedsReflow(embeddingFrame, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);
>+          }
>+          // else our width and height is overridden - don't reflow anything
>+        } else {
>+          // We are not embedded by reference, so our 'width' and 'height'
>+          // attributes are not overridden - we need to reflow.
>+          PresContext()->PresShell()->
>+            FrameNeedsReflow(this, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);
>+        }

When we get our reflow, our nsSVGOuterSVGFrame::Reflow will call nsSVGOuterSVGFrame::NotifyViewportChange, which will do the nulling out of mCanvasTM and calling of NotifyChildrenOfSVGChange, so doing that here is just extra work.

Comment 8

[Robert Longson [:longsonr]]

Attachment: updated again

(In reply to Jonathan Watt [:jwatt] (away 1-9 Oct, inc.) from comment #7)
> 
> >+  // we'll get notified by the outerSVGFrame anyway
> 
> This comment seems wrong. nsSVGInnerSVGFrame::NotifyViewportChange is called
> by the nsSVGSVGElement mContent. It's not that the outerSVGFrame will notify
> us, but rather that our AttributeChanged method will be called by our own
> nsSVGSVGElement, and we'll handle the changes there.

I meant that the outer SVG element will pick up the viewport change and call nsSVGOuterSVGFrame::NotifyViewportChange which will call nsSVGUtils::NotifyChildrenOfSVGChange which will call nsSVGInnerSVGFrame::NotifySVGChanged and that's where the inner svg frame processes the viewport change. So it doesn't need to do so in nsSVGInnerSVGFrame::NotifyViewportChange as well.

> 
> When we get our reflow, our nsSVGOuterSVGFrame::Reflow will call
> nsSVGOuterSVGFrame::NotifyViewportChange, which will do the nulling out of
> mCanvasTM and calling of NotifyChildrenOfSVGChange, so doing that here is
> just extra work.

Fixed.

Comment 9

[Robert Longson [:longsonr]]

Attachment: without the reftest changes this time

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #8)
> I meant that the outer SVG element will pick up the viewport change and call
> nsSVGOuterSVGFrame::NotifyViewportChange which will call
> nsSVGUtils::NotifyChildrenOfSVGChange which will call
> nsSVGInnerSVGFrame::NotifySVGChanged and that's where the inner svg frame
> processes the viewport change. So it doesn't need to do so in
> nsSVGInnerSVGFrame::NotifyViewportChange as well.

I think there's some confusion here. The viewport change in question is the change in the SVG viewport defined by an inner-<svg> for its children, not a change in the window dimensions or something. When the width/height of that inner-<svg> changes, probably the outer-<svg> isn't notified about that change (other than to invalidate an area of the canvas), so I'm not sure what you mean by "outer SVG element will pick up the viewport change and...". I think my comment about that comment still stands - the reason we don't need to do anything in nsSVGInnerSVGFrame::NotifyViewportChange is because we handle the "viewport" change in our AttributeChanged method.

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 563354 [details] [diff] [review]
without the reftest changes this time

> nsSVGInnerSVGFrame::AttributeChanged(PRInt32  aNameSpaceID,
>                                      nsIAtom* aAttribute,
>                                      PRInt32  aModType)
> {
>   if (aNameSpaceID == kNameSpaceID_None) {
>     if (aAttribute == nsGkAtoms::width ||
>-        aAttribute == nsGkAtoms::height ||
>-        aAttribute == nsGkAtoms::preserveAspectRatio ||
>-        aAttribute == nsGkAtoms::viewBox) {
>-      nsSVGUtils::UpdateGraphic(this);
>+        aAttribute == nsGkAtoms::height) {
>+
>+        PRBool hasValidViewBox =
>+                 static_cast<nsSVGSVGElement*>(mContent)->mViewBox.IsValid();
>+
>+        if (hasValidViewBox) {
>+          // make sure our cached transform matrix gets (lazily) updated
>+          mCanvasTM = nsnull;
>+        }
>+        nsSVGUtils::NotifyChildrenOfSVGChange(
>+           this, hasValidViewBox ? TRANSFORM_CHANGED : COORD_CONTEXT_CHANGED);
>+

Rather than having two conditionals on whether there is a viewBox rect, I think this would be clearer to have just one:

      if (static_cast<nsSVGSVGElement*>(mContent)->mViewBox.IsValid()) {
        mCanvasTM = nsnull;
        nsSVGUtils::NotifyChildrenOfSVGChange(this, TRANSFORM_CHANGED);
      } else {
        nsSVGUtils::NotifyChildrenOfSVGChange(this, COORD_CONTEXT_CHANGED);
      }

Other than that, can you please rename hasValidViewBox to hasViewBoxRect in the other part of your patch? (To me "none" is a valid attribute value, but what's of interest here is whether the viewBox defines a *rect*. I'd like us to change the name of IsValid to HasValidRect or something, but that's for another patch I guess.)

Comment 12

[Jonathan Watt [:jwatt]]

Other than that, the patch looks great.

Comment 13

[Robert Longson [:longsonr]]

Attachment: updated

(In reply to Jonathan Watt [:jwatt] (away 1-9 Oct, inc.) from comment #11)
> Comment on attachment 563354 [details] [diff] [review] [diff] [details] [review]
> without the reftest changes this time
> 
> Rather than having two conditionals on whether there is a viewBox rect, I
> think this would be clearer to have just one:

OK.

> Other than that, can you please rename hasValidViewBox to hasViewBoxRect in
> the other part of your patch? (To me "none" is a valid attribute value, but
> what's of interest here is whether the viewBox defines a *rect*. I'd like us
> to change the name of IsValid to HasValidRect or something, but that's for
> another patch I guess.)

That's the ony place it occurred in this patch and it's gone now with the above change.


(In reply to Jonathan Watt [:jwatt] (away 1-9 Oct, inc.) from comment #10)
> 
> I think there's some confusion here. The viewport change in question is the
> change in the SVG viewport defined by an inner-<svg> for its children, not a
> change in the window dimensions or something. When the width/height of that
> inner-<svg> changes, probably the outer-<svg> isn't notified about that
> change (other than to invalidate an area of the canvas), so I'm not sure
> what you mean by "outer SVG element will pick up the viewport change
> and...". I think my comment about that comment still stands - the reason we
> don't need to do anything in nsSVGInnerSVGFrame::NotifyViewportChange is
> because we handle the "viewport" change in our AttributeChanged method.

It's never called any more so I added an NS_ERROR to that effect. I might try to remove entirely it in bug 413960

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 563381 [details] [diff] [review]
updated

Thanks! r=jwatt

Comment 15

[Robert Longson [:longsonr]]

pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/831df43787ef

Comment 16

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/831df43787ef