Closed Bug 326143 Opened 17 years ago Closed 16 years ago

Implement feFlood and feTurbulence filter elements

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tor, Assigned: tor)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 7 obsolete files)

 
Attached patch feFlood and feTurbulence (obsolete) — Splinter Review
Attachment #210927 - Flags: review?(scootermorris)
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.
Attached patch fix nits (obsolete) — Splinter Review
Attachment #210927 - Attachment is obsolete: true
Attachment #211069 - Flags: review?(scootermorris)
Attachment #210927 - Flags: review?(scootermorris)
Comment on attachment 211069 [details] [diff] [review]
fix nits

turbulence filters are quite cool!  Nice!
Attachment #211069 - Flags: review?(scootermorris) → review+
Attached patch Updated for current tree (obsolete) — Splinter Review
Attachment #211069 - Attachment is obsolete: true
Attached patch Variable name fix (obsolete) — Splinter Review
Corrected the use of stitch and type in nsSVGFETurbulenceElement::Init()
Attachment #240175 - Attachment is obsolete: true
Attachment #241100 - Flags: review?(tor)
Comment on attachment 241100 [details] [diff] [review]
Variable name fix

Clearing review flag until updated to reflect bug 355249.
Attachment #241100 - Flags: review?(tor)
Attached patch Use ParseAttribute (obsolete) — Splinter Review
Updated turbulence filter to use ParseAttribute as opposed to SetAttr
Attachment #241100 - Attachment is obsolete: true
Attachment #245011 - Flags: review?(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.
Attachment #245011 - Flags: review?(tor) → review-
Attached patch Fix attribute mapping (obsolete) — Splinter Review
Attachment #245011 - Attachment is obsolete: true
(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.
Assignee: tor → amenzie
Attachment #246046 - Flags: review?(tor)
Attached patch Simplified flood (obsolete) — Splinter Review
Attachment #246046 - Attachment is obsolete: true
Attachment #246715 - Flags: review?(tor)
Attachment #246046 - Flags: review?(tor)
Attachment #246715 - Flags: review?(tor) → review+
Attachment #246715 - Flags: superreview?(roc)
nsSVGFilterInstance should provide a style context for the filter. Someone want to file a bug on that?
+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".
(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
Assignee: malex → tor
Attachment #246715 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #250598 - Flags: superreview?(roc)
Attachment #246715 - Flags: superreview?(roc)
Attachment #250598 - Flags: superreview?(roc) → superreview+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.
Attachment #250598 - Flags: review-
Attached patch fix flood styleSplinter Review
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.
Attachment #250971 - Flags: review?
...and I'll be sure to ask for style review next time I touch stuff there.  Sorry about that.
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
Attachment #250971 - Flags: review? → review+
Checked in.
Duplicate of this bug: 366535
Blocks: 311029
You need to log in before you can comment on or make changes to this bug.