767647 - Stop invalidating once for every SVG descendant of a changed SVG container, and stop invalidating the descendants' rendering observers

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

While trying to fix bug 738192 so that it's easier for me to fix bug 614732, I got test failures because we're not calling nsSVGUtils::InvalidateAndScheduleBoundsUpdate in several places where we should be.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Robert Longson [:longsonr]]

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

>   if (aNameSpaceID == kNameSpaceID_None &&
>       aAttribute == nsGkAtoms::transform) {
>-
>+    nsSVGUtils::InvalidateAndScheduleBoundsUpdate(this);
>     NotifySVGChanged(TRANSFORM_CHANGED);

Are we not invalidating twice here, NotifySVGChanged would invalidate
all the children. Could we do one of

a) just schedule a bounds update without the invalidation
b) call NotifySVGChanged with SUPPRESS_INVALIDATION
c) have NotifySVGChanged itself manage the bounds update to get one invalidation

>diff --git a/layout/svg/base/src/nsSVGUseFrame.cpp b/layout/svg/base/src/nsSVGUseFrame.cpp
>--- a/layout/svg/base/src/nsSVGUseFrame.cpp
>+++ b/layout/svg/base/src/nsSVGUseFrame.cpp
>@@ -116,32 +116,31 @@ nsSVGUseFrame::AttributeChanged(PRInt32 
>                                 nsIAtom*        aAttribute,
>                                 PRInt32         aModType)
> {
...
>     } else if (aAttribute == nsGkAtoms::width ||
>                aAttribute == nsGkAtoms::height) {
>+      nsSVGUtils::InvalidateAndScheduleBoundsUpdate(this);
>       static_cast<nsSVGUseElement*>(mContent)->SyncWidthOrHeight(aAttribute);
>-
>       if (mHasValidDimensions != 
>           static_cast<nsSVGUseElement*>(mContent)->HasValidDimensions()) {
>-
>         mHasValidDimensions = !mHasValidDimensions;
>-        nsSVGUtils::InvalidateAndScheduleBoundsUpdate(this);
>       }
>     }

If the use is pointing to anything except a <symbol> or <svg> then modifying
the use width/height has no effect unless they change from valid (>0) to invalid
(<=0) mHasValidDimensions tracks this and avoids invalidating in this case.
If we have the InvalidateAndScheduleBoundsUpdate here then we're not taking
advantage of this knowledge any more.

Possible solutions would be one of...
a) Have SyncWidthOrHeight call nsSVGUtils::InvalidateAndScheduleBoundsUpdate 
   for symbols and svg elements and then move the call above into the test below
b) Implement a method on nsSVGUseElement that returns whether width/height override
   the cloned element's values, something like

  if (mClone) {
    nsCOMPtr<nsIDOMSVGSymbolElement> symbol = do_QueryInterface(mClone);
    nsCOMPtr<nsIDOMSVGSVGElement>    svg    = do_QueryInterface(mClone);
    return (symbol || svg);
  }

Then only call InvalidateAndScheduleBoundsUpdate if this returns true or the
width/height validity has changed
c) If neither a nor b are feasible then we should rip out mHasValidDimensions as
   it is now pointless.

FWIW I would prefer b).

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #2)
> Are we not invalidating twice here,

Not exactly, or at least not when landed in combination with the patch for bug 738192. Anyway, the point is mute because...

> Could we do one of
> 
> a) just schedule a bounds update without the invalidation

I was planning on doing this later since there's extra risk with making that change. But heck, let's morph this bug and do in now. Patch coming up.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Jonathan Watt [:jwatt]]

I've s/WidthAndHightAreUsed/WidthAndHeightAreUsed/ locally.

Comment 6

[Robert Longson [:longsonr]]

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

I don't much like even the corrected name of WidthAndHeightAreUsed how about
WidthAndHeightOverrideClone or WidthAndHeightOverrideCloneValues.

>+  if (needNewBounds) {
>+    // Ancestor changes can't affect how we render from the perspective or
>+    // any rendering observers that we may have, so we don't need to
>+    // invalidate them. We also don't need to invalidate ourself, since our
>+    // changed ancestor will have invalidated its entire area, which includes
>+    // our area.

I'm not sure I follow what the first sentence is trying to say, particularly the
"from the perspective" bit. Perhaps those 3 words just need to be omitted.

This same comment is repeated so make sure they all get updated.

>-
>-      if (mHasValidDimensions != 
>+      if (static_cast<nsSVGUseElement*>(mContent)->WidthAndHightAreUsed()) {
>+        nsSVGUtils::InvalidateAndScheduleBoundsUpdate(this);
>+      }
>+      if (mHasValidDimensions !=
>           static_cast<nsSVGUseElement*>(mContent)->HasValidDimensions()) {

Should use an else here so as not to call InvalidateAndScheduleBoundsUpdate twice if
WidthAndHightAreUsed(sic) is true and HasValidDimensions changes state.

>-
>         mHasValidDimensions = !mHasValidDimensions;
>         nsSVGUtils::InvalidateAndScheduleBoundsUpdate(this);
>       }
>+      static_cast<nsSVGUseElement*>(mContent)->SyncWidthOrHeight(aAttribute);

Pointless calling SyncWidthOrHeight unless WidthAndHightAreUsed(sic) returns true. 
Move into appropriate test above. Add an assert in nsSVGUseElement::SyncWidthOrHeight that
WidthAndHightAreUsed is true.

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #6)
> I don't much like even the corrected name of WidthAndHeightAreUsed how about
> WidthAndHeightOverrideClone or WidthAndHeightOverrideCloneValues.

I don't really like "clone" in the name, since we may well end up changing <use> to not clone sometime soon to match other implementations. I changed it to OurWidthAndHeightAreUsed to get rid of the possibly confusing "Or" in the name.

> I'm not sure I follow what the first sentence is trying to say, particularly
> the
> "from the perspective" bit. Perhaps those 3 words just need to be omitted.

Oops, I meant "perspective of", not "perspective or". I think that's probably what you found confusing, so I fixed that.

> Should use an else here...

This ended up being a bit awkward after you telling me to put the SyncWidthOrHeight() call in the if, but okay, done.

Comment 8

[Jonathan Watt [:jwatt]]

I'm not sure what happened when testing my patch as reviewed, but it seemed to pass at the time. (Maybe something to do with the fact that I'm qpush/qpop'ing like crazy right now. Anyway, there were a few things I had to change after I did a last test run before pushing to mozilla-inbound:

1) I had to make UpdateBounds() on containers invalidate, since we no longer traverse to all its leafs and ask them to.

2) I removed the MarkDirtyBitsOnDescendants() code from nsSVGUtils.cpp since we no longer need to do that...except for text frames, since we have a "invalidate-one-part-of-the-text-then-invalidate-it-all" policy there. So I actually moved the MarkDirtyBitsOnDescendants code to nsSVGTextFrame.cpp and called it as appropriate there.

Comment 9

[Jonathan Watt [:jwatt]]

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ba8a51adaf

The for stupid undefined variable bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa75c0cd1e5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7ba8a51adaf
https://hg.mozilla.org/mozilla-central/rev/8aa75c0cd1e5