Closed
Bug 326143
Opened 19 years ago
Closed 18 years ago
Implement feFlood and feTurbulence filter elements
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: tor, Assigned: tor)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 7 obsolete files)
70.85 KB,
patch
|
dbaron
:
review-
roc
:
superreview+
|
Details | Diff | Splinter Review |
17.69 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
17.71 KB,
patch
|
Details | Diff | Splinter Review |
Attachment #210927 -
Flags: review?(scootermorris)
Comment 2•19 years ago
|
||
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.
Attachment #210927 -
Attachment is obsolete: true
Attachment #211069 -
Flags: review?(scootermorris)
Attachment #210927 -
Flags: review?(scootermorris)
Comment 4•19 years ago
|
||
Comment on attachment 211069 [details] [diff] [review]
fix nits
turbulence filters are quite cool! Nice!
Attachment #211069 -
Flags: review?(scootermorris) → review+
Comment 5•18 years ago
|
||
Attachment #211069 -
Attachment is obsolete: true
Comment 6•18 years ago
|
||
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)
Comment 8•18 years ago
|
||
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-
Comment 10•18 years ago
|
||
Attachment #245011 -
Attachment is obsolete: true
Comment 11•18 years ago
|
||
(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.
Updated•18 years ago
|
Attachment #246046 -
Flags: review?(tor)
Comment 12•18 years ago
|
||
Attachment #246046 -
Attachment is obsolete: true
Attachment #246715 -
Flags: review?(tor)
Attachment #246046 -
Flags: review?(tor)
Attachment #246715 -
Flags: review?(tor) → review+
Updated•18 years ago
|
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".
Assignee | ||
Comment 15•18 years ago
|
||
(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 | ||
Comment 16•18 years ago
|
||
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+
Assignee | ||
Comment 17•18 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 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-
Assignee | ||
Comment 19•18 years ago
|
||
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?
Assignee | ||
Comment 20•18 years ago
|
||
...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+
Assignee | ||
Comment 22•18 years ago
|
||
Assignee | ||
Comment 23•18 years ago
|
||
Checked in.
Comment 25•18 years ago
|
||
update of http://www.mozilla.org/projects/svg/status.html needed
You need to log in
before you can comment on or make changes to this bug.
Description
•