488314 - Kill SetMatrixPropagation

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Jonathan Watt [:jwatt]]

Right now the code that implements GetBBox requires us to use SetMatrixPropagation, which is a horrible function that complicates the code. We can simplify things by building up the transform as we recurse into child frames, and that will actually same some matrix multiplication since we won't need to multiply the transform chain all the way up to the element on which GetBBox was called for every descendant element.

The remaining SetMatrixPropagation in nsSVGGlyphFrame can just be removed I think, if we instead pass PR_FALSE into the CharacterIterator ctor.

When changing the GetBBox code, the new function should probably be called something like GetUserSpaceBounds. It would also be good if it would take a flag to indicate whether stroke should be included. It seems wrong to me that nsSVGFilterInstance::BuildSources uses mTargetBBox to calculate mResultBoundingBox, since that comes from GetBBox which doesn't include stroke. If the new function is allowed to return stroke bounds, then it will be especially important that in not have "BBox" in its name, since that term has special meaning in SVG, and excludes stroke. (Shame a lot of our existing code is confused about that.)

Comment 1

[Jonathan Watt [:jwatt]]

Hmm, actually nsSVGFilterInstance::BuildSources should probably be using the covered region, so maybe forget about that.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch, part 1

In this patch I've renamed to GetBBox to GetBBoxContribution where appropriate and added a gfxMatrix argument to have GetBBoxContribution incrementally build up the transform to the element on which GetBBox was called as in iterates through its children. This means we only do one multiplication (kinda) as we go to a child, instead of requiring the child to call GetCanvasTM and multiply all the matrices all the way up to the element on which GetBBox was called.

To support GetBBoxContribution I've added a member PrependLocalTransformTo() to nsSVGElement. The reason its PrependLocalTransformTo and not GetLocalTransform is because this way we can save unnecessary multiplications when there is no local tm (the elemet has that knowledge, but calles, less so). The reason it's on an element class and not a frame class is because we want the logic there when we come to supporting getBBox, getCTM, getTransformToElement, and a bunch of other DOM interface stuff being called on elements not yet in the document. The spec requires we can do that, we need it for parity with other implementations, and we've had multiple complains about not supporting it.

Most of the logic for the PrependLocalTransformTo methods came from GetCanvasTM methods on the corresponding frames, so I removed most of the duplicate logic in the GetCanvasTM methods and made them use PrependLocalTransformTo. Since PrependLocalTransformTo returns a gfxMatrix it was just easier to change GetCanvasTM to return a gfxMatrix too, so I did. It's a very slippery slope to turning this into a "deCOM matrices in the SVG code" patch though, so I decided to stop there before things got too messy. I know it leaves the matrix code looking a bit dumb and messy in places, but I really don't want to go any further down that route in this patch. I'm scheduled to get rid of internal use of nsIDOMSVGMatrix this quarter, so it'll get cleaned up soon enough.

The reason I've removed some GetMatrixPropagation checks is because this patch makes nsSVGGlyphFrame the only location still calling SetMatrixPropagation, so we know that the checks on other frames are redundant. I'll clean up the MatrixPropagation stuff properly in the followup patch to this one to remove the nsSVGGlyphFrame call. I'm not familiar with the SVG text code, so I'd like to get this patch in for now, and consider the nsSVGGlyphFrame issues in more detail in a followup patch.

Comment 3

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

+   * XXX should we pass down a temp gfxContext instead?

No, why would we?

   // XXX move this function into interface nsISVGLocatableMetrics

Get rid of this comment?

+   * Get this frame's contribution to the rect returned by a GetBBox() call.

I'm not sure what this means. How is the result different from GetBBox on the frame's element?

+    mCanvasTM = NS_NewSVGMatrix(tm);

We should change mCanvasTM to gfxMatrix here, but I suppose gfxMatrix uses doubles and we might want to stick to floats for long-lived data, so we need a new gfxFloatMatrix for that and I suppose we should do it later.

+   * @param aParentTransform If GetBBox was called on an ancestor, this is
+   *   the transform from the user space established by our parent frame to the
+   *   user space established by the frame on which GetBBox was called (the
+   *   identity matrix if they are the same frame). If GetBBox was called on
+   *   this frame, then nsnull.

This seems a bit wrong, the behaviour of the function changes quite a bit based on whether this is null. I think we should make aParentTransform include the parent-frame-to-this-frame transform, so aParentTransform is never null and can be a reference. If we need to, we can make a helper function that takes the current aParentTransform matrix and prepends the local transform.

+gfxRect
+nsSVGUtils::GetBBoxContributions(nsFrameList *aFrames,
+                                 const gfxMatrix *aParentTransform)

The second parameter should be a reference since aParentTransform isn't null.

+  gfxRect GetBBoxContribution(const gfxMatrix *aParentTransform);

Methods that override virtual methods should also be marked virtual.

Comment 4

[Jonathan Watt [:jwatt]]

> +   * Get this frame's contribution to the rect returned by a GetBBox() call.
> 
> I'm not sure what this means. How is the result different from GetBBox on the
> frame's element?

When GetBBoxContribution is called on a frame as a result of a GetBBox call on an ancestor, then the rect it returns is its fill bounds in the userspace established by the ancestor, not the fill bounds in the userspace the frame establishes. Since in that case the rect is not in the frame's own userspace, it is not the frame's bbox. Rather, it's a contribution to the bbox of the ancestor. I'll try and come up with a clearer comment.

> We should change mCanvasTM to gfxMatrix here, but I suppose gfxMatrix uses
> doubles and we might want to stick to floats for long-lived data, so we need a
> new gfxFloatMatrix for that and I suppose we should do it later.

Right. I particularly avoided addressing member variables since that's another non-trivial task.

> +   * @param aParentTransform If GetBBox was called on an ancestor, this is
> +   *   the transform from the user space established by our parent frame to
> the
> +   *   user space established by the frame on which GetBBox was called (the
> +   *   identity matrix if they are the same frame). If GetBBox was called on
> +   *   this frame, then nsnull.
> 
> This seems a bit wrong, the behaviour of the function changes quite a bit based
> on whether this is null. I think we should make aParentTransform include the
> parent-frame-to-this-frame transform, so aParentTransform is never null and can
> be a reference. If we need to, we can make a helper function that takes the
> current aParentTransform matrix and prepends the local transform.

If by "changes quite a bit" you mean that it does or does not include its own local transform based on whether aParentTransform is nsnull or not, right, but that's what we need. The thing is that when nsSVGUtils::GetBBox is called with the frame itself (as opposed to with an ancestor), the frame should *not* include its own local transform, because the bbox it returns should be in the userspace established *by* the frame (after it's local transform), not before it. It's not good enough to pass in an identity matrix because it needs to know not to include its local transform.

> +gfxRect
> +nsSVGUtils::GetBBoxContributions(nsFrameList *aFrames,
> +                                 const gfxMatrix *aParentTransform)
> 
> The second parameter should be a reference since aParentTransform isn't null.

Okay.

> +  gfxRect GetBBoxContribution(const gfxMatrix *aParentTransform);
> 
> Methods that override virtual methods should also be marked virtual.

Indeed. Thanks.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: [checked in] patch, part 1, v2

> This seems a bit wrong, the behaviour of the function changes quite a bit based
> on whether this is null. I think we should make aParentTransform include the
> parent-frame-to-this-frame transform, so aParentTransform is never null and can
> be a reference. If we need to, we can make a helper function that takes the
> current aParentTransform matrix and prepends the local transform.

Okay, I've done this, and it is quite a bit better that way. :-) No helper was needed.

Comment 6

[Jonathan Watt [:jwatt]]

I've pushed part 1 in http://hg.mozilla.org/mozilla-central/rev/fc3def2eef99

Comment 7

[jonathan chetwynd]

does this bug cover the issue:

Mozilla does not allow you to call getBBox in the load handler of the document?

http://groups.google.com/group/mozilla.dev.tech.svg/browse_thread/thread/7cdbd82f7671daf3/d6e31532ec6872b6

Comment 8

[Robert Longson [:longsonr]]

No, that was fixed in some other bug some time ago.

Comment 9

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

Not making it. Can we finish this off? It should be easy now!

Comment 10

[Jonathan Watt [:jwatt]]

Attachment: patch to remove final vestiges

This should do for the purposes of closing this bug I think.

Comment 11

[Jonathan Watt [:jwatt]]

http://hg.mozilla.org/mozilla-central/rev/1e025cfeb9c2