1066270 - Prepare nsFilterInstance for nsIFrame-less filtering

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Markus Stange [:mstange]]

Attachment: part 1: Add the ability to specify the filter chain in the nsFilterInstance constructor instead of always getting it from the frame.

Comment 2

[Markus Stange [:mstange]]

Attachment: part 2: Generalize SVG length resolution for nsIFrame-less consumers.

In nsSVGFilterInstance I need to call nsSVGUtils::GetRelativeRect and nsSVGUtils::UserSpace with something that resolves the lengths relative to the canvas space, and in cases where there's no nsIFrame for the canvas. At the moment you need to supply an nsIFrame to the length resolution functions, and many of them have two special cases: one for SVG frames (which end up calling methods of the frame's SVG element), and one for non-SVG frames. I didn't want to add a third overload for canvas all over the place, so I decided to introduce an interface called UserSpaceMetrics which has two implementations for the existing users but can be subclassed for the canvas case.
I'm not too happy with the name "UserSpaceMetrics"... maybe "SVGLengthContext" would be better?

I'm pretty sure this patch keeps the behavior of all existing methods unchanged.

Comment 3

[Markus Stange [:mstange]]

Attachment: part 3: Resolve SVG lengths in nsSVGFilterInstance using the UserSpaceMetrics supplied in the nsFilterInstance constructor.

Comment 4

[Markus Stange [:mstange]]

Attachment: part 4: Add an nsIContent* parameter to the nsFilterInstance constructor.

Comment 5

[Markus Stange [:mstange]]

Attachment: part 5: Replace the nsIFrame* argument to nsCSSFilterInstance with a shadowFallbackColor argument.

Comment 6

[Markus Stange [:mstange]]

Attachment: part 6: Allow aTargetFrame to be null in nsFilterInstance.

Comment 7

[Markus Stange [:mstange]]

Attachment: part 7: Add nsFilterInstance::GetFilterDescription.

Comment 8

[Markus Stange [:mstange]]

I'm going to clean up the nsFilterInstance constructor in a new bug, after this one has landed.

Comment 9

[Markus Stange [:mstange]]

Attachment: part 8: Don't crash when attempting to resolve a filter from an external resource document that hasn't undergone frame construction yet.

Comment 10

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

Comment on attachment 8488184 [details] [diff] [review]
part 6: Allow aTargetFrame to be null in nsFilterInstance.

Review of attachment 8488184 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsFilterInstance.cpp
@@ +203,2 @@
>      nsRect preFilterVOR = mTargetFrame->GetPreEffectsVisualOverflowRect();
>      mTargetBounds = FrameSpaceToFilterSpace(&preFilterVOR);

Shouldn't something set mTargetBounds when mTargetFrame is null? What is going to do that?

Comment 11

[Markus Stange [:mstange]]

Attachment: Remove useless mTargetBounds field

mTargetBounds looks like a leftover. Every time its value is used, it's union'ed with mTargetBBoxInFilterSpace, but as far as I can tell mTargetBBoxInFilterSpace always contains mTargetBounds. I've tested with SVG frames, non-SVG frames, and non-SVG frames with continuations. In the continuation case mTargetBBoxInFilterSpace is the bounding box over all continuations and mTargetBounds is just the first frame.

Comment 12

[Markus Stange [:mstange]]

https://tbpl.mozilla.org/?tree=Try&rev=66a394772da4

Comment 13

[Markus Stange [:mstange]]

Attachment: part 9: Simplify mTargetBounds usage

We do need mTargetBounds after all because for SVG elements with a stroke, the BBox doesn't include the stroke.
This patch at least puts the unioning into one place and makes sure that mTargetBounds has a useful value even if mTargetFrame is null.

Comment 14

[Markus Stange [:mstange]]

https://tbpl.mozilla.org/?tree=Try&rev=e113926712c0

Comment 15

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1c65e7c68c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6154a1235ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/769fcdda208d
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a4011365692
https://hg.mozilla.org/integration/mozilla-inbound/rev/53e6a6204893
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7bed5a0ee60
https://hg.mozilla.org/integration/mozilla-inbound/rev/89bc7483714f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bda71abf648
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ab7e76389e

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fd1c65e7c68c
https://hg.mozilla.org/mozilla-central/rev/b6154a1235ed
https://hg.mozilla.org/mozilla-central/rev/769fcdda208d
https://hg.mozilla.org/mozilla-central/rev/5a4011365692
https://hg.mozilla.org/mozilla-central/rev/53e6a6204893
https://hg.mozilla.org/mozilla-central/rev/b7bed5a0ee60
https://hg.mozilla.org/mozilla-central/rev/89bc7483714f
https://hg.mozilla.org/mozilla-central/rev/3bda71abf648
https://hg.mozilla.org/mozilla-central/rev/f5ab7e76389e