389865 - Improve filter architecture

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

We pass in arguments to various functions to configure things just because one or two filters are the exception to a rule. Instead we should define some virtual functions with standard return values that odd filters can override to get the results they want.

Also implements roc's suggestion in bug 326143 comment 13

Comment 1

[Robert Longson [:longsonr]]

Attachment: updated patch

Removed the instance GetStyledContent code as that was getting the parent filter frame rather than the child filter frames.

Comment 2

[tor]

Comment on attachment 274445 [details] [diff] [review]
updated patch

> class nsSVGFE : public nsSVGFEBase
...
> public:
>+  nsSVGFilterInstance::ColorModel
>+  GetColorModel(nsIDOMSVGAnimatedString* aIn) {
>+    return nsSVGFilterInstance::ColorModel (
>+          (OperatesOnSRGB(aIn) ?
>+             nsSVGFilterInstance::ColorModel::SRGB :
>+             nsSVGFilterInstance::ColorModel::LINEAR_RGB),
>+          (OperatesOnPremultipledAlpha() ?
>+             nsSVGFilterInstance::ColorModel::PREMULTIPLIED :
>+             nsSVGFilterInstance::ColorModel::UNPREMULTIPLIED));
>+  }

>+  virtual PRBool OperatesOnPremultipledAlpha() { return PR_TRUE; }
>+
>+  virtual PRBool OperatesOnSRGB(nsIDOMSVGAnimatedString *) {
>+    nsIFrame* frame = GetPrimaryFrame();
>+    if (!frame) return PR_FALSE;
>+
>+    nsStyleContext* style = frame->GetStyleContext();
>+    return style->GetStyleSVG()->mColorInterpolationFilters ==
>+             NS_STYLE_COLOR_INTERPOLATION_SRGB;
>+  }

These only seem to be accessed by GetColorModel, so could be protected.

Why does OperatesOnSRGB() have an argument?  None of the current implementations seem to use it.


>+
>+  // See http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion
>+  virtual PRBool SubregionIsUnionOfRegions() { return PR_TRUE; }
>+
>   // interfaces:
>   NS_DECL_ISUPPORTS_INHERITED
>   NS_DECL_NSIDOMSVGFILTERPRIMITIVESTANDARDATTRIBUTES
> 
> protected:
>-
>   // nsSVGElement specializations:
>   virtual LengthAttributesInfo GetLengthInfo();
> 
>   // nsIDOMSVGFitlerPrimitiveStandardAttributes values
>   enum { X, Y, WIDTH, HEIGHT };
>   nsSVGLength2 mLengthAttributes[4];
>   static LengthInfo sLengthInfo[4];
>

Comment 3

[Robert Longson [:longsonr]]

(In reply to comment #2)
> Why does OperatesOnSRGB() have an argument?  None of the current
> implementations seem to use it.

I thought you could use it for feDisplacementMap but I realised subsequently you would need the address of the input rather than the input itself. You could then compare addresses and see whether you were dealing with in or in2. I could remove this if this lands before feDisplacementMap or try to implement it properly if it lands after. My first go at doing it properly didn't compile as you can't pass &mIn2 as a nsIDOMSVGAnimatedString* *aIn. Any hints appreciated :-)

Comment 4

[tor]

I'm probably overlooking something obvious, but why won't a nsIDOMSVGAnimatedString* suffice for comparison purposes?  They are allocated in each filter element and have a lifetime of the element.

Comment 5

[Robert Longson [:longsonr]]

Attachment: update patch

(In reply to comment #2)
I've done the protected changes.

(In reply to comment #4)
> I'm probably overlooking something obvious, but why won't a
> nsIDOMSVGAnimatedString* suffice for comparison purposes?  They are allocated
> in each filter element and have a lifetime of the element.
> 

Looks like you are right. I've left in the argument ready for feDisplacementMap.

Comment 6

[Robert Longson [:longsonr]]

Comment on attachment 274956 [details] [diff] [review]
update patch

Risks: Could screw up SVG filter processing.
Mitigation: Have run through all the S3C SVG testcases and checked the output is as expected.
Rewards: Makes it easier to add support for feDisplacement filter, makes filter code maintenance easier, reduces code size

Comment 7

[Robert Longson [:longsonr]]

checked in.