Bug 19835

Created attachment 22019 [details] Filter Light name changes Class name changes for: SVGLightSource -> LightSource SVGDistantLightSource -> DistantLightSource SVGPointLightSource -> PointLightSource SVGSpotLightSource -> SpotLightSource There are a few formatting changes as well, so as to match WebKit coding style. Otherwise, no changes were made except for naming.

Comment on attachment 22019 [details] Filter Light name changes Welcome aboard :-) r=me.

Created attachment 22022 [details] above with webcore changelog Doh! Added WebCore ChangeLog to patch.

Comment on attachment 22022 [details] above with webcore changelog This is the same patch as reviewed by niko, but with a changelog for landing r=niko

rename patch #1 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/svg/SVGFEDiffuseLightingElement.cpp M WebCore/svg/SVGFEDistantLightElement.cpp M WebCore/svg/SVGFEDistantLightElement.h M WebCore/svg/SVGFELightElement.h M WebCore/svg/SVGFEPointLightElement.cpp M WebCore/svg/SVGFEPointLightElement.h M WebCore/svg/SVGFESpecularLightingElement.cpp M WebCore/svg/SVGFESpotLightElement.cpp M WebCore/svg/SVGFESpotLightElement.h M WebCore/svg/graphics/filters/SVGDistantLightSource.h M WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp M WebCore/svg/graphics/filters/SVGFEDiffuseLighting.h M WebCore/svg/graphics/filters/SVGFESpecularLighting.cpp M WebCore/svg/graphics/filters/SVGFESpecularLighting.h M WebCore/svg/graphics/filters/SVGLightSource.cpp M WebCore/svg/graphics/filters/SVGLightSource.h M WebCore/svg/graphics/filters/SVGPointLightSource.h M WebCore/svg/graphics/filters/SVGSpotLightSource.h M WebCore/svg/graphics/filters/cg/SVGFEDiffuseLightingCg.mm M WebCore/svg/graphics/filters/cg/SVGFEHelpersCg.h M WebCore/svg/graphics/filters/cg/SVGFEHelpersCg.mm M WebCore/svg/graphics/filters/cg/SVGFESpecularLightingCg.mm Committed r34912

Created attachment 22032 [details] First few new files Just adding three new header files that will be necessary to have going forward with the class renaming as well as the building of the new Filter stack.

Comment on attachment 22032 [details] First few new files After some thought i think you should put the Filter and FilterEffect constructors and create methods into appropriate C++ files, as this matches our style in other similar cases, and may be beneficial as the classes get more complete. Otherwise this looks great!

Created attachment 22035 [details] Few New Files with change Created the files oliver suggested and cleaned up #includes a little because there were a few that were unnecessary. Otherwise, unchanged from above.

Comment on attachment 22035 [details] Few New Files with change The patch itself is fine, but doesn't actually move us much closer to having a new filter system. This is one of those refactoring patches which might go better on a branch. I spoke with AMatthews (penguin bob) over the phone this afternoon and encouraged him to get a minimal filter compositor up and running ASAP (as that's the hard part compared to these renames and changes to create methods). A minimal filter compositor is also on the critical path towards completion.

Eric, I would prefer we get a decent and clean groundwork up and running first -- AMatthews is working on a path i suggested :p I would hope to have the new dag's being built with correct properties for all of these done by tonight, at which point we should get feImage going. Once we've done that we can actually start working on composition.

Comment on attachment 22035 [details] Few New Files with change r=me

Comment on attachment 22035 [details] Few New Files with change Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/WebCore.xcodeproj/project.pbxproj A WebCore/svg/Filter.cpp A WebCore/svg/Filter.h A WebCore/svg/FilterBuilder.h A WebCore/svg/FilterEffect.cpp A WebCore/svg/FilterEffect.h Committed r34942

Comment on attachment 22035 [details] Few New Files with change Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/WebCore.xcodeproj/project.pbxproj A WebCore/svg/Filter.cpp A WebCore/svg/Filter.h A WebCore/svg/FilterBuilder.h A WebCore/svg/FilterEffect.cpp A WebCore/svg/FilterEffect.h Committed r34942

Created attachment 22038 [details] Rename SVGFEBlend class This is the rename of just SVGFEBlend to FEBlend and anything required to get it building again after the name change. After this I hope it should be easier to do name changes. I hope. :-)

Comment on attachment 22038 [details] Rename SVGFEBlend class You should probably make "SVGFEBlendElement::filterEffect" etc do ASSERT_NOT_REACHED(); return 0; In general comments about future work shouldn't be present unless they're FIXME's (ideally with bug#'s). In cases like this where you expect to remove the code in the near future an ASSERT_NOT_REACHED is sensible. Is the change to WebCore/svg/SVGFilterPrimitiveStandardAttributes.h really necessary?

Created attachment 22039 [details] SVGFEBlend Rename Same as previous, except the two changes Oliver wanted.

Comment on attachment 22039 [details] SVGFEBlend Rename Committing to http://svn.webkit.org/repository/webkit/trunk ... D WebCore/svg/graphics/filters/cg/SVGFEBlendCg.mm M WebCore/ChangeLog M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/svg/FilterBuilder.h M WebCore/svg/SVGFEBlendElement.cpp M WebCore/svg/SVGFEBlendElement.h M WebCore/svg/graphics/filters/SVGFEBlend.cpp M WebCore/svg/graphics/filters/SVGFEBlend.h Committed r34971

Created attachment 22061 [details] SVGFEColorMatrix Rename Rename for SVGFEColorMatrix to FEColorMatrix as well as a few other small changes dealing with #includes noted in the ChangeLog.

ColorMatrix changes look good.

Created attachment 22073 [details] SVGFEComponentTransfer Rename Rename for SVGFEComponentTransfer. No extras this time, just the rename and changes required to build.

Created attachment 22076 [details] SVGFEComposite Rename Rename for SVGFEComposite to FEComposite. This also includes the webcore xcodeproj changes from the last three. Though be aware that it duplicates the webcore xcodeproj change in "SVGFEColorMatrix Rename."

Okay, that series of patches looks good, can you just make one combined patch with a changelog entry? You should really prepend the changelog with the bug description and link (look at other changelog entries) Then have something along the lines of "Continue refactoring FilterEffect classes in preparation for cross-platform implementation" or something else to that effect.

Created attachment 22077 [details] Combined Color, Component, and Composite Combined patch for the last three.

Comment on attachment 22077 [details] Combined Color, Component, and Composite r=me, will land shortly. Need to do update and build on this machine.

Comment on attachment 22077 [details] Combined Color, Component, and Composite Committing to http://svn.webkit.org/repository/webkit/trunk ... D WebCore/svg/graphics/filters/cg/SVGFEColorMatrixCg.mm D WebCore/svg/graphics/filters/cg/SVGFEComponentTransferCg.mm D WebCore/svg/graphics/filters/cg/SVGFECompositeCg.mm M WebCore/ChangeLog M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/svg/FilterEffect.h M WebCore/svg/SVGComponentTransferFunctionElement.cpp M WebCore/svg/SVGComponentTransferFunctionElement.h M WebCore/svg/SVGFEColorMatrixElement.cpp M WebCore/svg/SVGFEColorMatrixElement.h M WebCore/svg/SVGFEComponentTransferElement.cpp M WebCore/svg/SVGFEComponentTransferElement.h M WebCore/svg/SVGFECompositeElement.cpp M WebCore/svg/SVGFECompositeElement.h M WebCore/svg/graphics/filters/SVGFEBlend.cpp M WebCore/svg/graphics/filters/SVGFEBlend.h M WebCore/svg/graphics/filters/SVGFEColorMatrix.cpp M WebCore/svg/graphics/filters/SVGFEColorMatrix.h M WebCore/svg/graphics/filters/SVGFEComponentTransfer.cpp M WebCore/svg/graphics/filters/SVGFEComponentTransfer.h M WebCore/svg/graphics/filters/SVGFEComposite.cpp M WebCore/svg/graphics/filters/SVGFEComposite.h Committed r34992

Created attachment 22079 [details] SVGConvolveMatrix Rename Rename SVGFEConvolveMatrix to FEConvolveMatrix. Also, a little bit of #include movement to prevent needing #include "FilterBuilder.h" in every SVGFE*Element class.

Created attachment 22080 [details] SVGFEDiffuseLighting Rename Rename for SVGFEDiffuseLighting to FEDiffuseLighting.

Created attachment 22081 [details] SVGFEDisplacementMap Rename Rename SVGFEDisplacementMap to FEDisplacementMap.

Created attachment 22083 [details] SVGFEFlood Rename Rename SVGFEFlood to FEFlood.

Created attachment 22084 [details] SVGFEGaussianBlur Rename Rename SVGFEGaussianBlur to FEGaussianBlur.

Those all look basically sane :D You should mark them as patches in future so i can get the pretty formatted diffs :D

Created attachment 22088 [details] Combined Convolve through Gaussian Combined last 5.

Comment on attachment 22088 [details] Combined Convolve through Gaussian Incorrect directory paths after commit 34993. Will fix and resubmit.

Created attachment 22092 [details] Trying again with the last 5 Retrying Last 5 Renames

Comment on attachment 22092 [details] Trying again with the last 5 A few issues i've noticed in this run through: SVGFEFloodElement::build * nonConstThis is not necessary as build() is not a const method. * You removed the deref of the style objects, why? This will result in a leak afaik Alex, I'm r-'ing pending your response; if i feel the needed changes are minimal after that i'll do them and land myself (unless of course you have a new patch up already at that point)

> * nonConstThis is not necessary as build() is not a const method. Ok. I removed the nonConstThis line and changed the lines using it to "this" > * You removed the deref of the style objects, why? This will result in a leak > afaik I'm not actually sure why I didn't include the deref lines. It may have been because I know that you're not supposed to use explicit ref/deref anymore but I forgot to check if the fields were smart ptrs. They are not. I looked into moving them to RefPtrs but didn't feel comfortable enough to make those changes without breaking something. So, for the moment, I'll add a "FIXME" line above the explicit derefs saying that they need to be transitioned to RefPtrs in CSSStyleSelector. Sound good?

(In reply to comment #36) > > * nonConstThis is not necessary as build() is not a const method. > > Ok. I removed the nonConstThis line and changed the lines using it to "this" > > > * You removed the deref of the style objects, why? This will result in a leak > > afaik > > I'm not actually sure why I didn't include the deref lines. It may have been > because I know that you're not supposed to use explicit ref/deref anymore but I > forgot to check if the fields were smart ptrs. Alas they are not actually smart pointers -- if you look you can see that deref takes an argument. That's because the refcounting rules for the dom are more complicated than those elsewhere, so the manual deref's are necessary.

Created attachment 22101 [details] Trying again, again Patch with suggested changes in SVGFEDiffuseLightingElement and SVGFEFloodElement.

Created attachment 22122 [details] Last twelve This is the last 12 renames. After this everything in the "WebCore/svg/graphics/filters/" directory, other than SVGFilterEffect.h/cpp, is ready for file rename/move. I hope this is all correct :-). PS. Oliver I am very sorry for the monstrous size of this :-(.

Comment on attachment 22122 [details] Last twelve SVGFEMergeElement::build needs to handle requested subcomponents of the filter not being present. SVGFEOffsetElement::build doesn't need the addedEffect temporary, ditto for FETileElement, SVGFETurbulenceElement I don't like the name updateLights name, it's not updating anything -- it's discovering the lightsource, not updating any cache. I was also looking at the dump methods, and I think it would be best to keep them for now, and just make them dump the address of any filters they depend on (in the short term), so the externalRepresentation methods should have any significant changes. I can fix up the existing filters later, provided you make sure they continue building.

Created attachment 22155 [details] Large Refactoring Changes Oliver suggested.

Comment on attachment 22155 [details] Large Refactoring r- -- the externalRepresentation method should be identical t what they were originally, the only difference being that the references to any sub-filters should be replaced with the address of the subfilter. Other than that there should be no difference.

Created attachment 22169 [details] Large Refactoring again Changes Oliver wanted as well as adding an "operator<<" overload in SVGRenderTreeAsText.h to make printing the pointer addresses of the "in" fields easier.

Comment on attachment 22169 [details] Large Refactoring again Landed r35094

There's still some tidy up for this bug, but nothing too dramatic Basically remove the SVGFEFilter class, rename and move the remaining SVGFE* files.

Any news on filters so far? There are still a couple of patches here, not marked with r? etc.

<rdar://problem/6934155>

How does this related to bug 19991?

The refactoring and clean-up of the new filter system is ready. I close this bug and open new buge reports if necessary.