Last Comment Bug 767823 - Make the SVG filter code use the visual overflow rect of SVG frames
: Make the SVG filter code use the visual overflow rect of SVG frames
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jonathan Watt [:jwatt]
:
Mentors:
Depends on: 734082
Blocks: 614732 707960 767734
  Show dependency treegraph
 
Reported: 2012-06-24 12:12 PDT by Jonathan Watt [:jwatt]
Modified: 2012-06-30 17:44 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (38.91 KB, patch)
2012-06-25 18:04 PDT, Jonathan Watt [:jwatt]
no flags Details | Diff | Review
patch (39.10 KB, patch)
2012-06-25 18:21 PDT, Jonathan Watt [:jwatt]
longsonr: review+
Details | Diff | Review

Description Jonathan Watt [:jwatt] 2012-06-24 12:12:44 PDT
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] 2012-06-25 18:04:03 PDT
Created attachment 636562 [details] [diff] [review]
patch
Comment 2 Jonathan Watt [:jwatt] 2012-06-25 18:21:15 PDT
Created attachment 636569 [details] [diff] [review]
patch
Comment 3 Robert Longson 2012-06-26 02:34:11 PDT
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] 2012-06-26 03:51:25 PDT
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] 2012-06-26 09:24:33 PDT
https://hg.mozilla.org/mozilla-central/rev/224e353a3e40

Note You need to log in before you can comment on or make changes to this bug.