326143 - Implement feFlood and feTurbulence filter elements

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: feFlood and feTurbulence

Comment 2

[Scooter Morris]

Comment on attachment 210927 [details] [diff] [review]
feFlood and feTurbulence



>+//---------------------Flood------------------------
>+
>+typedef nsSVGFE nsSVGFEFloodElementBase;
>+
>+class nsSVGFEFloodElement : public nsSVGFEFloodElementBase,
>+                            public nsIDOMSVGFEFloodElement,
>+                            public nsISVGFilter
>+{
>+protected:
>+  friend nsresult NS_NewSVGFEFloodElement(nsIContent **aResult,
>+                                           nsINodeInfo *aNodeInfo);
NIT: fix spacing, please?


>+//---------------------Turbulence------------------------
>+
>+typedef nsSVGFE nsSVGFETurbulenceElementBase;
>+
>+class nsSVGFETurbulenceElement : public nsSVGFETurbulenceElementBase,
>+                                 public nsIDOMSVGFETurbulenceElement,
>+                                 public nsISVGFilter
>+{
>+protected:
>+  friend nsresult NS_NewSVGFETurbulenceElement(nsIContent **aResult,
>+                                           nsINodeInfo *aNodeInfo);
NIT: fix spacing, please?


>+
>+nsresult
>+nsSVGFETurbulenceElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                                    nsIAtom* aPrefix, const nsAString& aValue,
>+                                    PRBool aNotify)
>+{
>+  nsresult rv = nsSVGFEGaussianBlurElementBase::SetAttr(aNameSpaceID, aName, aPrefix,
>+                                                        aValue, aNotify);

Shouldn't this be nsSVGFETurbulenceElementBase?


Looks fine, except for the nits above.  I haven't walked through the algorithm in detail, though.

Comment 3

[tor]

Attachment: fix nits

Comment 4

[Scooter Morris]

Comment on attachment 211069 [details] [diff] [review]
fix nits

turbulence filters are quite cool!  Nice!

Comment 5

[Alex Menzies]

Attachment: Updated for current tree

Comment 6

[Alex Menzies]

Attachment: Variable name fix

Corrected the use of stitch and type in nsSVGFETurbulenceElement::Init()

Comment 7

[tor]

Comment on attachment 241100 [details] [diff] [review]
Variable name fix

Clearing review flag until updated to reflect bug 355249.

Comment 8

[Alex Menzies]

Attachment: Use ParseAttribute

Updated turbulence filter to use ParseAttribute as opposed to SetAttr

Comment 9

[tor]

Comment on attachment 245011 [details] [diff] [review]
Use ParseAttribute

>+// PresentationAttributes-feFlood
>+/* static */ const nsGenericElement::MappedAttributeEntry
>+nsSVGElement::sFEFloodMap[] = {
>+  { &nsSVGAtoms::flood_color },
>+  { &nsSVGAtoms::flood_opacity },
>+  { nsnull }
>+};

This needs to be allowed for elements other than feFlood:

  http://www.w3.org/TR/SVG11/attindex.html

> //--------------------Filter Resource-----------------------

Did you mean to make the comment changes to nsSVGFilterResource?

>+NS_IMETHODIMP
>+nsSVGFEFloodElement::Filter(nsSVGFilterInstance *instance)
>+{
...
>+}

Once your cairo feMerge changes goes in, this could be done with cairo too.

>+  DOM_CLASSINFO_MAP_BEGIN(SVGFETurbulenceElement, nsIDOMSVGFETurbulenceElement)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGFETurbulenceElement)
>+    DOM_CLASSINFO_MAP_ENTRY(nsIDOMSVGStylable)
>+    DOM_CLASSINFO_SVG_ELEMENT_MAP_ENTRIES
>+  DOM_CLASSINFO_MAP_END

This list should contain nsIDOMSVGFilterPrimitiveStandardAttributes.

Comment 10

[Alex Menzies]

Attachment: Fix attribute mapping

Comment 11

[Alex Menzies]

(In reply to comment #9)

> Did you mean to make the comment changes to nsSVGFilterResource?

Yes, the old comments refer to locking and unlocking images which is no longer accurate since we now use Cairo to acquire images.

Comment 12

[Alex Menzies]

Attachment: Simplified flood

Comment 13

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

nsSVGFilterInstance should provide a style context for the filter. Someone want to file a bug on that?

Comment 14

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

+nsSVGFETurbulenceElement::init(long lSeed)

Would you mind using standard naming conventions --- Init and aSeed? Ditto for the other places parameters don't start with "a".

Comment 15

[tor]

(In reply to comment #14)
> +nsSVGFETurbulenceElement::init(long lSeed)
> 
> Would you mind using standard naming conventions --- Init and aSeed? Ditto for
> the other places parameters don't start with "a".

The core noise functions come verbatim from the SVG specification:

  http://www.w3.org/TR/SVG11/filters.html#feTurbulence

Comment 16

[tor]

Attachment: update to tip, adjust naming of turbulence code to match moz code style

Comment 17

[tor]

Checked in.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 250598 [details] [diff] [review]
update to tip, adjust naming of turbulence code to match moz code style

You should really be asking style system peers for review when adding new CSS properties.  This patch adds two non-inherited properties as inherited properties.  They need to be in nsStyleSVGReset, not nsStyleSVG.

Comment 19

[tor]

Attachment: fix flood style

My stupid mistake - I saw "inherit" in the allowed values for the attributes and assumed that meant they were inheritable properties.  Reading down a couple lines of course reveals they aren't.

I'd back out the patch to fix this, but Robert Longson has landed stuff since that overlaps the patch, so it seems simpler to patch over.

Comment 20

[tor]

...and I'll be sure to ask for style review next time I touch stuff there.  Sorry about that.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 250971 [details] [diff] [review]
fix flood style

>Index: layout/style/nsRuleNode.cpp
>-  // stop-color: 
>-  SetColor(SVGData.mStopColor, parentSVGReset->mStopColor, mPresContext, aContext, svgReset->mStopColor, aInherited);
>+  // stop-color:
>+  SetColor(SVGData.mStopColor, parentSVGReset->mStopColor,
>+           mPresContext, aContext, svgReset->mStopColor, aInherited);
>+
>+  // flood-color:
>+  SetColor(SVGData.mFloodColor, parentSVGReset->mFloodColor,
>+           mPresContext, aContext, svgReset->mFloodColor, aInherited);

These two occurrences of |aInherited| should both be |inherited|.

>   // stop-opacity:
>   SetSVGOpacity(SVGData.mStopOpacity, parentSVGReset->mStopOpacity,
>-                svgReset->mStopOpacity, inherited);
>+                svgReset->mStopOpacity, aInherited);
>+
>+  // flood-opacity:
>+  SetSVGOpacity(SVGData.mFloodOpacity, parentSVGReset->mFloodOpacity,
>+                svgReset->mFloodOpacity, aInherited);

same here

r=dbaron with that fixed

Comment 22

[tor]

Attachment: checkin version of flood style fix - aInherited -> inherited

Comment 23

[tor]

Checked in.

Comment 25

[Mathieu Parent]

update of http://www.mozilla.org/projects/svg/status.html needed