Implement feFlood and feTurbulence filter elements

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
5 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

13 years ago
 
(Assignee)

Comment 1

13 years ago
Created attachment 210927 [details] [diff] [review]
feFlood and feTurbulence
Attachment #210927 - Flags: review?(scootermorris)

Comment 2

13 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.
(Assignee)

Comment 3

13 years ago
Created attachment 211069 [details] [diff] [review]
fix nits
Attachment #210927 - Attachment is obsolete: true
Attachment #211069 - Flags: review?(scootermorris)
Attachment #210927 - Flags: review?(scootermorris)

Comment 4

13 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

12 years ago
Created attachment 240175 [details] [diff] [review]
Updated for current tree
Attachment #211069 - Attachment is obsolete: true

Comment 6

12 years ago
Created attachment 241100 [details] [diff] [review]
Variable name fix

Corrected the use of stitch and type in nsSVGFETurbulenceElement::Init()
Attachment #240175 - Attachment is obsolete: true
Attachment #241100 - Flags: review?(tor)
(Assignee)

Comment 7

12 years ago
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

12 years ago
Created attachment 245011 [details] [diff] [review]
Use ParseAttribute

Updated turbulence filter to use ParseAttribute as opposed to SetAttr
Attachment #241100 - Attachment is obsolete: true
Attachment #245011 - Flags: review?(tor)
(Assignee)

Comment 9

12 years ago
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.
(Assignee)

Updated

12 years ago
Attachment #245011 - Flags: review?(tor) → review-

Comment 10

12 years ago
Created attachment 246046 [details] [diff] [review]
Fix attribute mapping
Attachment #245011 - Attachment is obsolete: true

Comment 11

12 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.
(Assignee)

Updated

12 years ago
Assignee: tor → amenzie

Updated

12 years ago
Attachment #246046 - Flags: review?(tor)

Comment 12

12 years ago
Created attachment 246715 [details] [diff] [review]
Simplified flood
Attachment #246046 - Attachment is obsolete: true
Attachment #246715 - Flags: review?(tor)
Attachment #246046 - Flags: review?(tor)
(Assignee)

Updated

12 years ago
Attachment #246715 - Flags: review?(tor) → review+

Updated

12 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

12 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

12 years ago
Created attachment 250598 [details] [diff] [review]
update to tip, adjust naming of turbulence code to match moz code style
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

12 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 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

12 years ago
Created attachment 250971 [details] [diff] [review]
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.
Attachment #250971 - Flags: review?
(Assignee)

Comment 20

12 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

12 years ago
Created attachment 250972 [details] [diff] [review]
checkin version of flood style fix - aInherited -> inherited
(Assignee)

Comment 23

12 years ago
Checked in.
Duplicate of this bug: 366535

Updated

11 years ago
Blocks: 311029
Blocks: 884020
You need to log in before you can comment on or make changes to this bug.