767823 - Make the SVG filter code use the visual overflow rect of SVG frames

Status: RESOLVED FIXED

Description

[Jonathan Watt [:jwatt]]

Here's another set of patches split out from 614732. This time it's to make the SVG filter code use the visual overflow rect of SVG frames (instead of their soon-to-die covered region) to make it consistent with how the filter code works for non-SVG frames. This allows nsDisplaySVGEffects to be used to do the filtering when painting SVG via display lists.

As a bonus, these patches reduce the internal use of GetCoveredRegion(), allowing bug 707960 to be fixed, and limit one of the filter code's GetCanvasTM() calls to occurring only during painting, helping with bug 767734.

Comment 1

[Jonathan Watt [:jwatt]]

Created attachment 636562 [details] [diff] [review]
patch

Comment 2

[Jonathan Watt [:jwatt]]

Created attachment 636569 [details] [diff] [review]
patch

Comment 3

[Robert Longson [:longsonr]]

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

>+  // "Frame space" is the origin of a frame, aka the top-left corner of its
>+  // border box, aka the top left corner of its mRect.
>+  gfxMatrix filterToFrameSpaceInCSSPx;

I think this calculation should go in a separate method. The constructor is getting very big.

>+  {
>+    gfxMatrix userToFrameSpaceInCSSPx;
>+
>+    if ((aTarget->GetStateBits() & NS_FRAME_SVG_LAYOUT)) {
>+      // As currently implemented by Mozilla for the purposes of filters, user
>+      // space is the coordinate system established by GetCanvasTM(), since
>+      // that's what we use to set filterToDeviceSpace above. In other words,
>+      // for SVG, user space is actually the coordinate system aTarget
>+      // establishes for _its_ children (i.e. after taking account of any x/y
>+      // and viewBox attributes), not the coordinate system that is established
>+      // for it by its 'transform' attribute (or by its _parent_) as it's
>+      // normally defined. (XXX We should think about fixing this.) The only
>+      // frame type for which these extra transforms are not simply an x/y
>+      // translation is nsSVGInnerSVGFrame, hence we treat it specially here.
>+      if (aTarget->GetType() == nsGkAtoms::svgInnerSVGFrame) {
>+        userToFrameSpaceInCSSPx =
>+          static_cast<nsSVGElement*>(aTarget->GetContent())->
>+            PrependLocalTransformsTo(gfxMatrix());

Do we have a reftest that would exercise this code path with a transform that isn't simply a translation? If not could you create one please.

>+gfxMatrix
>+nsSVGUtils::GetUserToCanvasTM(nsIFrame *aFrame)

Have you plans for this outside this bug? If not, make it an internal static helper rather than an externally accessible static member.

>   /**
>+   * Returns the transform from aFrame's user space to canvas space. Only call
>+   * with SVG frames. This is like GetCanvasTM, except that it only includes
>+   * the transforms from aFrame's user space (i.e. the coordinate context
>+   * established by its 'transform' attribute, or by it's parent for its

s/it's/its/ You get it right 3 words later in the same sentence!

>+   * children) to outer-<svg> device space. It does not include any other
>+   * transforms such as x/y offsets and viewBox attributes.
>+   */

r=longsonr with review comments addressed.

Comment 4

[Jonathan Watt [:jwatt]]

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

(In reply to Robert Longson from comment #3)
> I think this calculation should go in a separate method. The constructor is
> getting very big.

Done.

> Do we have a reftest that would exercise this code path with a transform
> that isn't simply a translation?

Yes, which is how I realized that we're applying filters after those transforms. :/

> Have you plans for this outside this bug?

Yes.

> s/it's/its/ You get it right 3 words later in the same sentence!

Gah, I'm trying, but my spelin int to good sometimes! :/

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/224e353a3e40