Summary: | WebKit needs cross-platform filter system | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Mathews <possessedpenguinbob> | ||||||||||||||||||||||||||||||||||||||||||||
Component: | SVG | Assignee: | Alex Mathews <possessedpenguinbob> | ||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | dino, eric, jeffschiller, krit, lars.sonchocky-helldorf, oliver, zimmermann | ||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar, SVGHitList | ||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 6022, 5526, 6033, 19991 | ||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Alex Mathews
2008-06-30 21:15:21 PDT
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. 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. |