734079 - Update SVG frame bounds asynchronously, and get rid of the suspendRedraw code

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Right now, whenever something changes in SVG we typically synchronously call nsSVGUtils::UpdateGraphic, which calls UpdateCoveredRegion on the frame and all of its descendants. For example, when someone changes something in an SVGTransformList using the DOM, we end up in nsSVGElement::DidChangeValue, which ends up calling AttributeChanged on the frame, which calls nsSVGUtils::UpdateGraphic.

The problem is that UpdateCoveredRegion is expensive (since it typically involves drawing into a gfxContext for all leaf frames), and if someone is building up a SVGTransformList (for example) with script and causing lots of UpdateGraphic calls, we can end up with a huge performance hit due the resulting unnecessary UpdateCoveredRegion calls.

We should really have a reflow-like mechanism for SVG where we mark SVG frames dirty and then sweep them at some later time to update their bounds.

Comment 1

[Jonathan Watt [:jwatt]]

Note that the problem does not just go away with display list based invalidation (once SVG supports display lists), since it's the many unnecessary UpdateCoveredRegion calls that are hurting us, not the invalidation calls. Besides that, I actually need a reflow like mechanism in SVG to fix bug 734082, which in turn needs to be fixed to support display lists in SVG.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch to remove suspendRedraw logic

The NS_STATE_SVG_REDRAW_SUSPENDED logic was really making implementing a reflow-like mechanism for SVG a nightmare, so this patch removes it first and makes SVGSVGElement.suspendRedraw a no-op.

Making SVGSVGElement.suspendRedraw a no-op is a perf concern for some in-the-wild content, but that aspect will be addressed by the follow-up patch.

We also relied on the NS_STATE_SVG_REDRAW_SUSPENDED flag to avoid the perf overhead of invalidating and updating the covered regions of SVG frames before their nsSVGOuterSVGFrame had had its initial reflow. This optimization is maintained by using the NS_FRAME_FIRST_REFLOW instead. The UnsuspendRedraw call in nsSVGOuterSVGFrame::DidReflow used to be responsible for making sure nsSVGUtils::UpdateGraphic was called on each existing frame at the end of an outer-<svg>'s initial reflow, just after the frames had had their InitialUpdate calls. The UpdateGraphic calls would do three things: call InvalidateRenderingObservers, update the frame's covered region, and invalidate its "old" and new area. The invalidation part was redundant, since after the reflow the entire area of the outer-<svg> would be invalidated. The InvalidateRenderingObservers call and updating of the covered region post viewport setting is important though. That work is now done in the InitialUpdate implementations.

Comment 3

[Jonathan Watt [:jwatt]]

The patch passes Try, BTW.

Comment 4

[Jonathan Watt [:jwatt]]

I locally updated the patch to change the bug IDs in the touched reftests from id=xxx to id=734079.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 604070 [details] [diff] [review]
patch to remove suspendRedraw logic

>diff --git a/layout/reftests/svg/suspend-01.svg b/layout/reftests/svg/suspend-01.svg

This isn't really testing anything any more. Probably best just removed.

>diff --git a/layout/reftests/svg/suspend-02.svg b/layout/reftests/svg/suspend-02.svg

ditto.

>diff --git a/layout/reftests/svg/suspend-03.svg b/layout/reftests/svg/suspend-03.svg

etc.

>diff --git a/layout/reftests/svg/suspend-04.svg b/layout/reftests/svg/suspend-04.svg

etc.

> void nsSVGForeignObjectFrame::DestroyFrom(nsIFrame* aDestructRoot)
> {
>@@ -181,16 +180,22 @@ nsSVGForeignObjectFrame::InvalidateInter
>                                             nsIFrame* aForChild,
>                                             PRUint32 aFlags)
> {
>   // This is called by our descendants when they change.
> 
>   if (GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)
>     return;
> 
>+  if (GetStateBits() & NS_FRAME_FIRST_REFLOW) {
>+    // When our outer-<svg> gets its first reflow it's entire area

s/it's/its/

Could combine this with previous line to get 

if (GetStateBits() & (NS_STATE_SVG_NONDISPLAY_CHILD | NS_FRAME_FIRST_REFLOW)) {
}

> void
> nsSVGForeignObjectFrame::FlushDirtyRegion(PRUint32 aFlags)
> {
>   if ((mSameDocDirtyRegion.IsEmpty() && mSubDocDirtyRegion.IsEmpty()) ||
>       mInReflow ||
>-      (GetStateBits() & NS_STATE_SVG_REDRAW_SUSPENDED))
>+      (GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD))
>     return;

What about foreignObject inside a <defs> i.e. part of a pattern. That will be nondisplay-child. I guess we could fix this as part of bug 732930 since that doesn't currently work anyway. Adding the nondisplay child test seems wrong though.

>+
>+  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
>+    // We only invalidate if our outer-<svg> has already had its
>+    // initial reflow (since if it hasn't, its entire area will be
>+    // invalidated when it gets that initial reflow):
>+    nsSVGUtils::InvalidateCoveredRegion(this);
>+  }
>+

Without bug 732930 we'll never clear the first reflow bit if we are a nondisplay child will we. Do dynamic changes to rects in patterns work? Do we have a dynamic rect change in a pattern reftest? i.e. a pattern with a red rect that becomes lime.

>-  if (aFrame->GetStateBits() & NS_STATE_SVG_REDRAW_SUSPENDED) {
>-    aFrame->AddStateBits(NS_STATE_SVG_DIRTY);
>+  if (aFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW) {
>+    // The nsSVGOuterSVGFrame has not recieved its first reflow,

s/recieved/received/

> // SVG Frame state bits
> #define NS_STATE_IS_OUTER_SVG                    NS_FRAME_STATE_BIT(20)
> 
>-#define NS_STATE_SVG_DIRTY                       NS_FRAME_STATE_BIT(21)
>-
> /* are we the child of a non-display container? */
> #define NS_STATE_SVG_NONDISPLAY_CHILD            NS_FRAME_STATE_BIT(22)
> 
> // If this bit is set, we are a <clipPath> element or descendant.
> #define NS_STATE_SVG_CLIPPATH_CHILD              NS_FRAME_STATE_BIT(23)
> 
>-// If this bit is set, redraw is suspended.
>-#define NS_STATE_SVG_REDRAW_SUSPENDED            NS_FRAME_STATE_BIT(24)
>-

Please don't leave holes. Change 22 and 23 to 21 and 22.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #5)
> Comment on attachment 604070 [details] [diff] [review]
> patch to remove suspendRedraw logic
> 
> >diff --git a/layout/reftests/svg/suspend-01.svg b/layout/reftests/svg/suspend-01.svg
> 
> This isn't really testing anything any more. Probably best just removed.

Actually they are now testing that redraw is _not_ suspended after the script completes. I changed the test titles to reflect that.

> Could combine this with previous line to get 
> 
> if (GetStateBits() & (NS_STATE_SVG_NONDISPLAY_CHILD |
> NS_FRAME_FIRST_REFLOW)) {
> }

I wanted to be clear about which flag the comment related to, so I kept them separate.

> > void
> > nsSVGForeignObjectFrame::FlushDirtyRegion(PRUint32 aFlags)
> > {
> >   if ((mSameDocDirtyRegion.IsEmpty() && mSubDocDirtyRegion.IsEmpty()) ||
> >       mInReflow ||
> >-      (GetStateBits() & NS_STATE_SVG_REDRAW_SUSPENDED))
> >+      (GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD))
> >     return;
> 
> What about foreignObject inside a <defs> i.e. part of a pattern. That will
> be nondisplay-child. I guess we could fix this as part of bug 732930 since
> that doesn't currently work anyway. Adding the nondisplay child test seems
> wrong though.

In that case we want to invalidate any elements referencing the fO, not the fO. I've changed DoReflow to proceed even with NS_STATE_SVG_NONDISPLAY_CHILD set so that when we add support for fO in <defs> it can use DoReflow (but changed all its current callers to assert that the NS_STATE_SVG_NONDISPLAY_CHILD bit is not set, since those are not valid paths to reflow a NS_STATE_SVG_NONDISPLAY_CHILD frame). I've then added code to make sure that we don't even call FlushDirtyRegion if NS_STATE_SVG_NONDISPLAY_CHILD is set.

> >+
> >+  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
> >+    // We only invalidate if our outer-<svg> has already had its
> >+    // initial reflow (since if it hasn't, its entire area will be
> >+    // invalidated when it gets that initial reflow):
> >+    nsSVGUtils::InvalidateCoveredRegion(this);
> >+  }
> >+
> 
> Without bug 732930 we'll never clear the first reflow bit if we are a
> nondisplay child will we.

No, but I don't think this is the patch to change this behavior. The second patch would be a more appropriate place, assuming that I change that in this bug at all.

> Do dynamic changes to rects in patterns work? Do
> we have a dynamic rect change in a pattern reftest? i.e. a pattern with a
> red rect that becomes lime.

We have at least:

layout/reftests/svg/dynamic-pattern-contents-01.svg
layout/reftests/svg/dynamic-pattern-contents-02.svg
layout/reftests/svg/pattern-live-01a.svg
layout/reftests/svg/pattern-live-01b.svg
layout/reftests/svg/pattern-live-01c.svg

> Please don't leave holes. Change 22 and 23 to 21 and 22.

The "reflow" patch fills the hole.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch to remove suspendRedraw logic

Comment 8

[Robert Longson [:longsonr]]

Comment on attachment 604693 [details] [diff] [review]
patch to remove suspendRedraw logic

> The "reflow" patch fills the hole.

The reflow patch can always add at at the end.

Are you only planning to land this at the same time as the reflow patch? It's quite a performance regression to land it before isn't it? I hope you'll at least hold off landing in 13 if it is a perf regression.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #8)
> Comment on attachment 604693 [details] [diff] [review]
> patch to remove suspendRedraw logic
> 
> > The "reflow" patch fills the hole.
> 
> The reflow patch can always add at at the end.

Sure, but I didn't see much point in changing 'blame' since it was easy not to.

I plan to land both patches together, and after 13.

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: [backed out] Give nsSVGSwitchElement::MaybeInvalidate a better name

Comment 11

[Ed Morley [:emorley]]

Landed as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe139851fb34

Backed out for M1 orange:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9990671&tree=Mozilla-Inbound
{
123275 ERROR TEST-UNEXPECTED-FAIL | /tests/content/svg/content/test/test_switch.xhtml | 3 switch.getBBox().width - got 50, expected 70
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9612c1006f6

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

csets for landing & backout merged over to m-c.
Landing: https://hg.mozilla.org/mozilla-central/rev/fe139851fb34
backout: https://hg.mozilla.org/mozilla-central/rev/c9612c1006f6

Comment 13

[Robert Longson [:longsonr]]

Other issues...

 void
 nsSVGTextFrame::UpdateGlyphPositioning(bool aForceGlobalTransform)
 {
-  if ((GetStateBits() & NS_STATE_SVG_REDRAW_SUSPENDED) || !mPositioningDirty)
+  if ((GetStateBits() & NS_FRAME_FIRST_REFLOW) || !mPositioningDirty)


If we have text in a pattern or marker or clipPath we'll never update the glyph positioning now. Since this didn't seem to break anything we clearly need a reftest with a pattern with positioned text in it.

Comment 14

[Jonathan Watt [:jwatt]]

Robert, I'm not sure what you mean. The logic at that bit of code hasn't really changed - with the patch, as before, we do an initial update, and then update in future only if mPositioningDirty is true. The fact that the patch makes that bit of code detect the initial update using NS_FRAME_FIRST_REFLOW instead of NS_STATE_SVG_REDRAW_SUSPENDED doesn't change that.

Comment 15

[Jonathan Watt [:jwatt]]

You may be right about text in pattern not working correctly, but if that's the case I think it's a preexisting problem, not something introduced by my patch. Assuming that's the case, I'd very much rather fix that in a separate bug.

Comment 16

[Robert Longson [:longsonr]]

Ah but it's not a preexisting problem, you'd be introducing it with this change. Text in a pattern never gets its NS_FRAME_FIRST_REFLOW bit cleared because it doesn't get an InitialUpdate. We work around this currently by calling UpdateGlyphPositioning all over the place in nsSVGTextFrame.

There's a test in the test suite that we currently pass: http://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlObjectApproved/masking-path-04-b.html

Comment 17

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #14)
> Robert, I'm not sure what you mean. The logic at that bit of code hasn't
> really changed - with the patch, as before, we do an initial update, and
> then update in future only if mPositioningDirty is true. The fact that the
> patch makes that bit of code detect the initial update using
> NS_FRAME_FIRST_REFLOW instead of NS_STATE_SVG_REDRAW_SUSPENDED doesn't
> change that.

<defs> children are never suspended so the bit is never set. They do have the reflow bit set by default though and it's never cleared.

Comment 18

[Jonathan Watt [:jwatt]]

Okay. My next patch to update frame bounds asynchronously actually removes the bit check so that it just becomes |if (!mPositioningDirty)|. For checkin I'll remove the bit in the first patch instead to fix that issue.

Comment 19

[Jonathan Watt [:jwatt]]

Good catch, BTW.

Comment 20

[Jonathan Watt [:jwatt]]

Attachment: patch to update SVG frame bounds asynchronously

I'd suggest starting review with nsSVGUtils.h to understand the new helpers for marking things dirty and requesting an update. The update then comes through the UpdateBounds calls which are initiated by nsSVGOuterSVGFrame::DidReflow.

Comment 21

[Jonathan Watt [:jwatt]]

Attachment: patch to update SVG frame bounds asynchronously

Sorry, that was an old patch from the wrong tree. Here's the correct one.

Comment 22

[Robert Longson [:longsonr]]

Comment on attachment 606672 [details] [diff] [review]
patch to update SVG frame bounds asynchronously

>diff --git a/layout/svg/base/src/nsSVGContainerFrame.cpp b/layout/svg/base/src/nsSVGContainerFrame.cpp
>--- a/layout/svg/base/src/nsSVGContainerFrame.cpp
>+++ b/layout/svg/base/src/nsSVGContainerFrame.cpp
>@@ -116,36 +116,43 @@ nsSVGDisplayContainerFrame::InsertFrames
>+  if (!(GetStateBits() &
>+        (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN |
>+         NS_STATE_SVG_NONDISPLAY_CHILD))) {
>     for (nsIFrame* kid = firstNewFrame; kid != firstOldFrame;
>          kid = kid->GetNextSibling()) {
>       nsISVGChildFrame* SVGFrame = do_QueryFrame(kid);
>-      if (SVGFrame) {
>-        SVGFrame->InitialUpdate(); 
>+      if (SVGFrame && !(kid->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)) {

Can't happen. If we're not nondisplay child and our kid is an SVGFrame then it can't be a nondisplay child. Remove or change to an assert if you really must.


>diff --git a/layout/svg/base/src/nsSVGForeignObject.cpp b/layout/svg/base/src/nsSVGForeignObject.cpp

>+  }
>+
>+  if (needNewCanvasTM) {
>+    // Only do this after calling InvalidateAndScheduleBoundsUpdate

because? i.e. add because... and the reason as the next comment line

>+    mCanvasTM = nsnull;
>   }
> }
> 


>diff --git a/layout/svg/base/src/nsSVGOuterSVGFrame.cpp b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp

>+  if (!(mState & NS_STATE_SVG_NONDISPLAY_CHILD)) {
>     nsIFrame* kid = mFrames.FirstChild();
>     while (kid) {
>       nsISVGChildFrame* SVGFrame = do_QueryFrame(kid);
>-      if (SVGFrame) {
>-        SVGFrame->InitialUpdate(); 
>+      if (SVGFrame && !(kid->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)) {

Redundant, if the frame is an SVGFrame then it can't be NS_STATE_SVG_NONDISPLAY_CHILD. You shouldn't have the test. make it an assertion if you want, otherwise get rid of it.


>+#ifdef DEBUG
>+bool
>+nsSVGUtils::OuterSVGIsCallingUpdateBounds(nsIFrame *aFrame)
>+{
>+  return nsSVGUtils::GetOuterSVGFrame(aFrame)->IsCallingUpdateBounds();
>+}
>+#endif
>+

Is this worth having? It doesn't make the caller much shorter. Keep it, if you're really attached to it, .

> 
>-    // Make sure elements styled by :hover get updated if script/animation moves
>-    // them under or out from under the pointer:
>-    aFrame->PresContext()->PresShell()->SynthesizeMouseMove(false);
>-
>+    // Note that filters can paint even if the element being filtered
>+    // has empty bounds, so we don't return early for that.
>+    // XXX Is that really true? Explain why, since the SVG bbox that filters
>+    // use will then also presumably be empty.

Most shapes (rect, circle etc) would not be drawn at all and filters can't change that. A path with only a move (and a stroke and rounded or square end to make it visible) would be drawn. Filters can make bounds bigger. The covered region includes the stroke so it should not be zero for a stroked zero length path.


>+    if (svgkid &&
>+        !(kid->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD) &&
>+        !(kid->GetStateBits() & NS_FRAME_IS_DIRTY)) {

You can combine the bit test here 

id->GetStateBits() & (NS_STATE_SVG_NONDISPLAY_CHILD | NS_FRAME_IS_DIRTY)


>+  // We must not add dirty bits to the nsSVGOuterSVGFrame or else
>+  // PresShell::FrameNeedsReflow won't work when we pass it in below.
>+  if (aFrame->GetType() == nsGkAtoms::svgOuterSVGFrame) {

test the state bit NS_STATE_IS_OUTER_SVG instead to check for an outer SVG frame.

>+    outerSVGFrame = static_cast<nsSVGOuterSVGFrame*>(aFrame);
>+  } else {
>+    aFrame->AddStateBits(NS_FRAME_IS_DIRTY);
>+
>+    nsIFrame *f = aFrame->GetParent();
>+    while (f && f->GetType() != nsGkAtoms::svgOuterSVGFrame) {

test NS_STATE_IS_OUTER_SVG state bit instead

r=longsonr with comments addressed

Comment 23

[Jonathan Watt [:jwatt]]

Thanks, Robert!

(In reply to Robert Longson from comment #22)
> >+      if (SVGFrame && !(kid->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)) {
> 
> Can't happen. If we're not nondisplay child and our kid is an SVGFrame then
> it can't be a nondisplay child. Remove or change to an assert if you really
> must.

It seemed like we set that flag on nsSVGOuterSVGFrame if it fails PassesConditionalProcessingTests(), and as if it's a bug that we don't do that on other frames. (It also seems like it's a bug that we don't remove that bit if the conditional processing attributes change so that PassesConditionalProcessingTests() passes.) So although it can't currently happen, it seems like if we fix that bug that it then the check above would be needed.

> >+nsSVGUtils::OuterSVGIsCallingUpdateBounds(nsIFrame *aFrame)
> 
> Is this worth having? It doesn't make the caller much shorter. Keep it, if
> you're really attached to it, .

It has the minor advantage of avoiding including nsSVGOuterSVGFrame.h in files that don't already include it, and avoids line breaks, so I'll keep it for now.

> >+    // Note that filters can paint even if the element being filtered
> >+    // has empty bounds, so we don't return early for that.
> >+    // XXX Is that really true? Explain why, since the SVG bbox that filters
> >+    // use will then also presumably be empty.
> 
> Most shapes (rect, circle etc) would not be drawn at all and filters can't
> change that. A path with only a move (and a stroke and rounded or square end
> to make it visible) would be drawn. Filters can make bounds bigger. The
> covered region includes the stroke so it should not be zero for a stroked
> zero length path.

Yes, but you can still give a filter a fixed size, and then use feFlood to fill that area. I've changed the comment to mention that.

Comment 24

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #23)
> Thanks, Robert!
> 
> (In reply to Robert Longson from comment #22)
> > >+      if (SVGFrame && !(kid->GetStateBits() & NS_STATE_SVG_NONDISPLAY_CHILD)) {
> > 
> > Can't happen. If we're not nondisplay child and our kid is an SVGFrame then
> > it can't be a nondisplay child. Remove or change to an assert if you really
> > must.
> 
> It seemed like we set that flag on nsSVGOuterSVGFrame if it fails
> PassesConditionalProcessingTests(), and as if it's a bug that we don't do
> that on other frames. (It also seems like it's a bug that we don't remove
> that bit if the conditional processing attributes change so that
> PassesConditionalProcessingTests() passes.) So although it can't currently
> happen, it seems like if we fix that bug that it then the check above would
> be needed.
> 

outer svg frames are special. It's not a bug that other frames do that really. We don't create them if they fail but we do create an outer which is why it gets this flag (perhaps we should have had a separate DONT_PAINT bit for outer SVG frames rather than using nondisplay child as it's not really the same semantics.

We recreate the frames via a frame update hint if conditional processing changes.

Comment 25

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #23)

> It has the minor advantage of avoiding including nsSVGOuterSVGFrame.h in
> files that don't already include it, and avoids line breaks, so I'll keep it
> for now.
> 

There's another ifdef debug method in nsSVGUtils isn't there? Some kind of bitmap output function IIRC. If you put them next to each other you'd only need one ifdef DEBUG. Just a nit so if you're not bothered then I'll live with it where it is.

Comment 26

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #24)
> outer svg frames are special. It's not a bug that other frames do that
> really. We don't create them if they fail but we do create an outer which is
> why it gets this flag (perhaps we should have had a separate DONT_PAINT bit
> for outer SVG frames rather than using nondisplay child as it's not really
> the same semantics.
> 
> We recreate the frames via a frame update hint if conditional processing
> changes.

Thanks, I've added a comment to document this and changed the bit checks to NS_ABORT_IF_FALSE checks.

(In reply to Robert Longson from comment #25)
> There's another ifdef debug method in nsSVGUtils isn't there? Some kind of
> bitmap output function IIRC. If you put them next to each other you'd only
> need one ifdef DEBUG. Just a nit so if you're not bothered then I'll live
> with it where it is.

Done.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Could it make sense to actually use Reflow here? (and to update the overflow areas in Reflow as well?)

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I get the feeling that SVG frames not participating in reflow is being more trouble than it's worth. It means that for foreign objects (and text, in Cameron's work), we have to making content reflow roots. If SVG implemented Reflow to establish the same invariants that the rest of layout does (i.e. sets mRect and the frame's overflow area to the right values), I think that might be an improvement on the current situation.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To conform to the rest of layout, we would also have to implement UpdateOverflow for some SVG frames.

Comment 30

[Jonathan Watt [:jwatt]]

This already landed on m-i.

This essentially makes us do reflow, but using a method called UpdateBounds. That method is called in nsSVGOuterSVGFrame::DidReflow, and cascades down to all child frames and reflows foreignObjects.

Setting mRect and the visual overflow rect on SVG frames is bug 734082, a dependency of this bug. I'm working on that now.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If it's essentially doing reflow, then why not actually do it in Reflow?

Comment 32

[Jonathan Watt [:jwatt]]

Because there is no "flow" in SVG, only bounds.

Comment 33

[Jonathan Watt [:jwatt]]

And because there's no point in passing around four unused arguments.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

True, but I think the consistency would outweigh that.

Comment 35

[Jonathan Watt [:jwatt]]

Let's discuss name changing later. Right now I'm building display list patches on top of this stuff, and I could do without bit rotting it.

Comment 36

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/4805515c9a20
https://hg.mozilla.org/mozilla-central/rev/247b83139d6d

Comment 37

[Jonathan Watt [:jwatt]]

CC'ing bz and dbaron who have expressed a desire to kill suspendRedraw in the past - it's now dead.

Also, the fixes here gave us order of magnitude speed-ups in some of our other bugs. See bug 698996 comment 6 and bug 313998 comment 37 in particular.

Comment 38

[Boris Zbarsky [:bzbarsky]]

I <3 order-of-magnitude speedups.  ;)

Comment 39

[Helder "Lthere" Magalhães]

This seems to solve the symptoms reported in bug 614732 comment 38 and bug 616892 comment 11. It seems like rendering is now a little "jumpy" (as if frames are being skipped all the time), probably as (un)suspendRedraw calls are now being ignored. I'll report follow-up bugs if I'm able to figure out if what's happening is just a positive side-effect or a new issue.

Thumbs up for the highly noticeable speed-up! :-)

Comment 40

[Laura Mesa [:lmesa] [:lvmesa]]

(In reply to Helder "Lthere" Magalhães from comment #39)
> This seems to solve the symptoms reported in bug 614732 comment 38 and bug
> 616892 comment 11. It seems like rendering is now a little "jumpy" (as if
> frames are being skipped all the time), probably as (un)suspendRedraw calls
> are now being ignored. I'll report follow-up bugs if I'm able to figure out
> if what's happening is just a positive side-effect or a new issue.
> 
> Thumbs up for the highly noticeable speed-up! :-)

I'd love to be able to talk about this with the 14 Beta release--who can I follow up with to talk about these speedups and put together some visual assets t communicate this improvement?