498275 - Stop using nsIDOMSVGMatrix in nsSVGForeignObjectFrame + fixes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Splitting this off from bug 435356, since the nsSVGForeignObjectFrame stuff involves some subtle fixes and needs careful review.

I just killed the GetMatrixPropagation check in nsSVGInnerSVGFrame.cpp since matrix propagation stuff is dead except for nsSVGGlyphFrame, and so there was no point rewriting it for gfxMatrix here.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Oh, and I had GetCanvasTM include the x/y values by defining a nsSVGForeignObjectElement::PrependLocalTransformTo, since that makes in consistent with nsSVGInnerSVGFrame and nsSVGMarkerFrame. It also makes GetCanvasTM convert from the userspace established *by* the elemnt to device space, and that seems better and more consistent with other transform functions we have, and enables less code duplication.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Also fixes bug in nsSVGForeignObjectFrame::PaintSVG
Also fixes bug in nsSVGForeignObjectFrame::GetFrameForPoint (the point is already in canvas space in app units)

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to comment #3)
> Also fixes bug in nsSVGForeignObjectFrame::GetFrameForPoint (the point is
> already in canvas space in app units)

Oops. It needs to be in local space in app units, not canvas space in app units.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Robert Longson [:longsonr]]

Can you attach an updated patch please now that my clip property fix is in?

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Ouch, that really conflicted. :-)

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch

Correction for slight mismerge.

Comment 9

[Robert Longson [:longsonr]]

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

> NS_IMETHODIMP_(nsIFrame*)
> nsSVGForeignObjectFrame::GetFrameForPoint(const nsPoint &aPoint)
> {
...
>+  float x, y, width, height;
>+  static_cast<nsSVGElement*>(mContent)->
>+    GetAnimatedLengthValues(&x, &y, &width, &height, nsnull);
>+
>+  gfxMatrix tm = GetCanvasTM().Invert();
>+  if (tm.IsSingular())
>     return nsnull;

Shouldn't you test whether the matrix is singular before you invert it rather than after?

>diff --git a/layout/svg/base/src/nsSVGOuterSVGFrame.cpp b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>--- a/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>+++ b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>@@ -577,17 +577,16 @@ nsSVGOuterSVGFrame::Paint(nsIRenderingCo
>     // It's not really clear what area to invalidate here. We might have
>     // stuffed up rendering for the entire window in this paint pass,
>     // so we can't just invalidate our own rect. Invalidate everything
>     // in sight.
>     // This won't work for printing, by the way, but failure to print the
>     // odd document is probably no worse than printing horribly for all
>     // documents. Better to fix things so we don't need fallback.
>     nsIFrame* frame = this;
>-    nsPresContext* presContext = PresContext();
>     PRUint32 flags = 0;
>     while (PR_TRUE) {
>       nsIFrame* next = nsLayoutUtils::GetCrossDocParentFrame(frame);
>       if (!next)
>         break;
>       if (frame->GetParent() != next) {
>         // We're crossing a document boundary. Logically, the invalidation is
>         // being triggered by a subdocument of the root document. This will

This is part of the patch for bug 494690 that seems to have sneaked in.

r=longsonr with comments addressed where necessary.

Comment 10

[Jonathan Watt [:jwatt]]

I'm not sure why it would be better to test for singularity before inverting. Inverting can't make a singular matrix non-singular.

For the presContext thing, I just happened to notice that when I was building. I didn't know it was in another bug, but in that case I'll remove it. :-)

Comment 11

[Robert Longson [:longsonr]]

You can't invert a singular matrix.

Comment 12

[Jonathan Watt [:jwatt]]

Sure. More specifically what I meant was that gfxMatrix.Invert() will not make a singular gfxMatrix non-singular, so I don't think anything bad can happen here.

Comment 13

[Jonathan Watt [:jwatt]]

I've gone ahead and pushed http://hg.mozilla.org/mozilla-central/rev/7a983cc4040f

We can change the matrix thing if I'm wrong and something bad could feasibly happen.