Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Get rid of the horrendous "invalidate everything" hack in nsSVGUtils::FindFilterInvalidation

RESOLVED FIXED in mozilla16

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 1 bug, {perf})

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

We should get rid of the horrendous "invalidate everything" hack in nsSVGUtils::FindFilterInvalidation that was introduced in bug 463939.
(Assignee)

Updated

5 years ago
No longer blocks: 734082
Depends on: 734082
(Assignee)

Updated

5 years ago
Assignee: nobody → jwatt
(Assignee)

Updated

5 years ago
Depends on: 767647
Created attachment 635998 [details] [diff] [review]
patch
Attachment #635998 - Flags: review?(longsonr)

Comment 2

5 years ago
Comment on attachment 635998 [details] [diff] [review]
patch


>diff --git a/layout/svg/base/src/nsSVGFilterFrame.cpp b/layout/svg/base/src/nsSVGFilterFrame.cpp
>--- a/layout/svg/base/src/nsSVGFilterFrame.cpp
>+++ b/layout/svg/base/src/nsSVGFilterFrame.cpp
>@@ -10,26 +10,31 @@
> #include "gfxASurface.h"
> #include "gfxUtils.h"
> #include "nsGkAtoms.h"
> #include "nsRenderingContext.h"
> #include "nsSVGEffects.h"
> #include "nsSVGFilterElement.h"
> #include "nsSVGFilterInstance.h"
> #include "nsSVGFilterPaintCallback.h"
>+#include "nsSVGIntegrationUtils.h"

This is supposed to be for SVG effects in html only. Move GetCSSPxToDevPxMatrix to nsSVGUtils instead.

> void
>-nsSVGUtils::InvalidateBounds(nsIFrame *aFrame, bool aDuringUpdate)
>+nsSVGUtils::InvalidateBounds(nsIFrame *aFrame, bool aDuringUpdate,
>+                             const nsRect *aBoundsSubArea, PRUint32 aFlags)
> {
...
>-  nsSVGOuterSVGFrame* outerSVGFrame = GetOuterSVGFrame(aFrame);
>-  NS_ASSERTION(outerSVGFrame, "no outer svg frame");
>-  if (outerSVGFrame) {
>-    nsISVGChildFrame *svgFrame = do_QueryFrame(aFrame);
>-    if (!svgFrame)
>+  nsRect rect;

Could we have a more informative variable name than rect please?

>+  if (aBoundsSubArea) {
>+    rect = *aBoundsSubArea;
>+  } else {
>+    rect = aFrame->GetVisualOverflowRect();
>+    // GetVisualOverflowRect() already includes filter effects and transforms,
>+    // so advance to our parent before the loop below:
>+    rect += aFrame->GetPosition();
>+    aFrame = aFrame->GetParent();
>+  }
>+
>+    if (aFrame->GetType() == nsGkAtoms::svgInnerSVGFrame &&
>+        aFrame->GetStyleDisplay()->IsScrollableOverflow()) {
>+      // Clip rect to the viewport established by this inner-<svg>:
>+      float x, y, width, height;
>+      static_cast<nsSVGSVGElement*>(aFrame->GetContent())->
>+        GetAnimatedLengthValues(&x, &y, &width, &height, nsnull);

Are we sure that width, height are > 0 here. I think we are so we should assert. If you
think we aren't then you should test and return as // nothing to invalidate so as not to
call RoundGfxRectToAppRect with either width or height <= 0

>+      nsRect viewportRect =
>+        nsLayoutUtils::RoundGfxRectToAppRect(gfxRect(0.0, 0.0, width, height),
>+                                             appUnitsPerCSSPx);

r=longsonr with comments addressed.
Attachment #635998 - Flags: review?(longsonr) → review+
(Assignee)

Updated

5 years ago
Blocks: 767734
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5c26b1320e1a
Target Milestone: --- → mozilla16
I had a bit of trouble with layout/reftests/svg/dynamic-text-04.svg failing intermittently after that push, although it passed on Try on all machines. I changed it to use MozReftestInvalidate in fe96d0162dee, but that just turned it perma-orange. After temporarily disabling it in 2e40e269180e to give me a bit of time to investigate I discovered the problem.

As mentioned in bug 767647 comment 8, I killed off the MarkDirtyBitsOnDescendants function in nsSVGUtils.cpp, and reinstated it as a text-only thing in nsSVGTextFrame.cpp. When I did so I added some optimizations to it to make it return early if aFrame was already dirty, or to skip already dirty descendants (and their descendants). Turns out that was a bad idea, since for whatever reason it seems we can end up dirtying a text frame without dirtying its descendants, which then means we get a bad visual overflow rect when we update. This was causing the dynamic-text-04.svg failure.
So in my final follow-up checkin, 12e4d32ecd07, I put MarkDirtyBitsOnDescendants back to the way it used to be when it was in nsSVGUtils.cpp, and re-enabled dynamic-text-04.svg

Comment 6

5 years ago
What happened to my review comments? They don't seem to have been actioned.
The only review comment I didn't change the patch for was the request to move GetCSSPxToDevPxMatrix. I meant to comment on that part, but got distracted by the issues described in comment 4, sorry.

Regarding moving GetCSSPxToDevPxMatrix, nsSVGIntegrationUtils becomes "integration of SVG effects into display lists", rather than "integration of SVG effects into non-SVG". So in fact I think it makes more sense to keep GetCSSPxToDevPxMatrix there (or as much sense as anywhere). Besides that it would have meant working the changes through my patch queue, which is a pain I could do without right now for something that doesn't clearly need changed.

If you want to change it after my other bug 614738 work has landed, then we can do that.
Err, bug 614732, I mean.

Comment 9

5 years ago
> Regarding moving GetCSSPxToDevPxMatrix, nsSVGIntegrationUtils becomes
> "integration of SVG effects into display lists", rather than "integration of
> SVG effects into non-SVG". So in fact I think it makes more sense to keep
> GetCSSPxToDevPxMatrix there (or as much sense as anywhere). Besides that it
> would have meant working the changes through my patch queue, which is a pain
> I could do without right now for something that doesn't clearly need changed.

OK, I guess I buy that.

> 
> If you want to change it after my other bug 614738 work has landed, then we
> can do that.

No.
https://hg.mozilla.org/mozilla-central/rev/5c26b1320e1a
https://hg.mozilla.org/mozilla-central/rev/fe96d0162dee
https://hg.mozilla.org/mozilla-central/rev/2e40e269180e
https://hg.mozilla.org/mozilla-central/rev/12e4d32ecd07
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 767996
Depends on: 768237
(Assignee)

Updated

5 years ago
Blocks: 772152
(Assignee)

Updated

5 years ago
Depends on: 779403
(Assignee)

Updated

5 years ago
Depends on: 787837

Updated

5 years ago
Depends on: 807002
You need to log in before you can comment on or make changes to this bug.