Created attachment 38470 [details] feMorphologyElement Added feMorphologyElement as preparation for FEMorphology filter effect.
Created attachment 38473 [details] feMorphology filter effect This is the filter effect feMorphology. I hope that I can make the algorithm more efficient. So this is just for testing atm.
Comment on attachment 38470 [details] feMorphologyElement m__operator seems like a strange name.
Comment on attachment 38470 [details] feMorphologyElement I don't understand why these exist: char SVGRadiusXAttrIdentifier[] = "SVGRadiusXAttr"; 33 char SVGRadiusYAttrIdentifier[] = "SVGRadiusYAttr"; Why the _? set_operatorBaseValue is this all to avoid "operator" in C++? This seems wrong, no? 36 readonly attribute SVGAnimatedEnumeration _operator;
(In reply to comment #5) > (From update of attachment 38470 [details]) > I don't understand why these exist: > char SVGRadiusXAttrIdentifier[] = "SVGRadiusXAttr"; > 33 char SVGRadiusYAttrIdentifier[] = "SVGRadiusYAttr"; Radius is one attribute, that may have one or two arguments. These lines create the "virtual" attributes radiusX and radiusY to differ between the both values. See also SVGGaussianBlurElement. radiusX and radiusY are not part of svgattr.in, because they don't realy exist in the SVG Spec. > Why the _? > set_operatorBaseValue > is this all to avoid "operator" in C++? Yes, the set...BaseValue is gernerated by the attribute name and you can't name the attribute operator. The way with the underscore is also use in SVGFECompositeElement. > > This seems wrong, no? > 36 readonly attribute SVGAnimatedEnumeration _operator; No, why should it be wrong? the attribute operator is an enumeration and animateable. Because of the underscore see comments above. Can you please review the patch again eric? I would like to set the r? again.
Why are the external strings needed? Why not just "SVGRadiusXAttr" as a literal c string? readonly attribute SVGAnimatedEnumeration _operator; will expose "_operator" to javascript instead of "operator". That seems bad. Seems we would need to hack the js generator to work around this c++ keyword. Or have some custom attribute for the attribute in the idl
(In reply to comment #7) > Why are the external strings needed? Why not just "SVGRadiusXAttr" as a > literal c string? The external string are perfectly fine. There are no "radiusX" / "radiusY" Attribute Strings that would be exported in SVGNames.h, there's only "radius", so Dirk needs to define these values. Exactly like it's done for ie. SVGKernelUnitLengthXIdentifier / SVGKernelUnitLengthYIdentifier and numerous other classes in svg/. If you remember, this stuff is needed for SVG<->DOM attribute synchronization. > > readonly attribute SVGAnimatedEnumeration _operator; > will expose "_operator" to javascript instead of "operator". That seems bad. Yes, that's a real problem. Already persistent in other classes like SVGFECompositeElement, in trunk. I'd rather suggest to fix all these problems in one-shot in a follow-up patch. Though if Dirk wants to fix it here, no problem as well...
Created attachment 40904 [details] feMorphologyElemnt A preperation for the Effect. We need to add the Element first.
Created attachment 40913 [details] feMorphologyElemnt Moved out XCode clean up. Commited by Nikolas.
Comment on attachment 40913 [details] feMorphologyElemnt r-, the test is buggy, already discussed with krit on IRC.
Created attachment 40956 [details] feMorphologyElement Same patch, better test.
Comment on attachment 40956 [details] feMorphologyElement Wonderful patch, r=me. Some small style issues: > +SVGFEMorphologyElement::SVGFEMorphologyElement(const QualifiedName& tagName, Document* doc) > + : SVGFilterPrimitiveStandardAttributes(tagName, doc) Use "document" instead of "doc", old legacy code uses abbrevations here. > + interface [Conditional=SVG&FILTERS, GenerateConstructor] SVGFEMorphologyElement : SVGElement, > + SVGFilterPrimitiveStandardAttributes { Please line up SVGElement & SVGFilterPrimitiveStandardAttributes. > - PassRefPtr<FEMorphology> create(FilterEffect*, MorphologyOperatorType, const float&, const float&); > + static PassRefPtr<FEMorphology> create(FilterEffect*, MorphologyOperatorType, float, float); Please specify parameter names for the floats. > - FEMorphology(FilterEffect*, MorphologyOperatorType, const float&, const float&); > + FEMorphology(FilterEffect*, MorphologyOperatorType, float, float); Ditto. Please fix before landing.
Comment on attachment 40956 [details] feMorphologyElement clearing review flag. Landed in r49400.
Created attachment 42647 [details] feMorphology filter Implementation of the graphic filter.
Comment on attachment 42647 [details] feMorphology filter Nice job Dirk, some style nitpicks: > + if (m_radiusX == 0 || m_radiusY == 0) > + return; if (!m_radiusX || !m_radiusY) > + radiusX = std::min(effectDrawingRect.width() - 1, radiusX); I suggest to use "using std::min" / "using std::max" in the beginning. > + if ((m_type == 1 && pixel <= columnExtrema) || (m_type == 2 && pixel >= columnExtrema)) ... > + if ((m_type == 1 && extrema[kernelIndex] <= entireExtrema) || > + (m_type == 2 && extrema[kernelIndex] >= entireExtrema)) ... Don't we have any named constants for this? Didn't check the source... Please clarify these things, almost ready to give r+.
Created attachment 42653 [details] feMorphology filter corrected the mentioned style issues.
Can you also update WebKitSite/projects/svg/status.xml as part of your patch? :)
Comment on attachment 42653 [details] feMorphology filter Looks good to me. r+. Please change from "using namespace std" to an explicit "using std::min; using std::max;", before landing. I think that's rather common style in WebKit.
Comment on attachment 42653 [details] feMorphology filter Marking cq- since there were changes requested.
landed in r50604. Closing bug.