Bug 19835 - WebKit needs cross-platform filter system
Summary: WebKit needs cross-platform filter system
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Alex Mathews
URL:
Keywords: InRadar, SVGHitList
Depends on:
Blocks: 6022 5526 6033 19991
  Show dependency treegraph
 
Reported: 2008-06-30 21:15 PDT by Alex Mathews
Modified: 2009-06-13 13:15 PDT (History)
7 users (show)

See Also:


Attachments
Filter Light name changes (19.52 KB, patch)
2008-07-01 01:55 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
above with webcore changelog (22.86 KB, patch)
2008-07-01 03:44 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
First few new files (10.22 KB, patch)
2008-07-01 14:35 PDT, Alex Mathews
oliver: review-
Details | Formatted Diff | Diff
Few New Files with change (14.52 KB, patch)
2008-07-01 16:29 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
Rename SVGFEBlend class (18.28 KB, patch)
2008-07-01 23:10 PDT, Alex Mathews
oliver: review-
Details | Formatted Diff | Diff
SVGFEBlend Rename (17.83 KB, patch)
2008-07-01 23:40 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
SVGFEColorMatrix Rename (19.79 KB, patch)
2008-07-02 23:54 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
SVGFEComponentTransfer Rename (25.18 KB, patch)
2008-07-03 15:39 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
SVGFEComposite Rename (20.00 KB, patch)
2008-07-03 16:22 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
Combined Color, Component, and Composite (60.00 KB, patch)
2008-07-03 17:00 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
SVGConvolveMatrix Rename (10.96 KB, text/plain)
2008-07-03 23:32 PDT, Alex Mathews
no flags Details
SVGFEDiffuseLighting Rename (14.06 KB, text/plain)
2008-07-03 23:37 PDT, Alex Mathews
no flags Details
SVGFEDisplacementMap Rename (14.66 KB, text/plain)
2008-07-04 00:22 PDT, Alex Mathews
no flags Details
SVGFEFlood Rename (7.69 KB, text/plain)
2008-07-04 00:41 PDT, Alex Mathews
no flags Details
SVGFEGaussianBlur Rename (7.66 KB, text/plain)
2008-07-04 00:58 PDT, Alex Mathews
no flags Details
Combined Convolve through Gaussian (66.36 KB, patch)
2008-07-04 09:19 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
Trying again with the last 5 (66.37 KB, patch)
2008-07-04 22:30 PDT, Alex Mathews
oliver: review-
Details | Formatted Diff | Diff
Trying again, again (66.66 KB, patch)
2008-07-05 14:29 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
Last twelve (133.05 KB, patch)
2008-07-07 00:22 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff
Large Refactoring (139.48 KB, patch)
2008-07-08 00:45 PDT, Alex Mathews
oliver: review-
Details | Formatted Diff | Diff
Large Refactoring again (144.04 KB, patch)
2008-07-09 00:36 PDT, Alex Mathews
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Mathews 2008-06-30 21:15:21 PDT
This bug will handle all the patches necessary to get the new Cross-platform filters system up and running.
Comment 1 Alex Mathews 2008-07-01 01:55:15 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 2 Nikolas Zimmermann 2008-07-01 03:01:06 PDT
Comment on attachment 22019 [details]
Filter Light name changes

Welcome aboard :-) r=me.
Comment 3 Alex Mathews 2008-07-01 03:44:44 PDT
Created attachment 22022 [details]
above with webcore changelog

Doh! Added WebCore ChangeLog to patch.
Comment 4 Oliver Hunt 2008-07-01 03:47:06 PDT
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
Comment 5 Oliver Hunt 2008-07-01 04:12:06 PDT
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

Comment 6 Alex Mathews 2008-07-01 14:35:11 PDT
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 7 Oliver Hunt 2008-07-01 14:39:59 PDT
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!
Comment 8 Alex Mathews 2008-07-01 16:29:33 PDT
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 9 Eric Seidel (no email) 2008-07-01 17:47:39 PDT
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.
Comment 10 Oliver Hunt 2008-07-01 18:23:20 PDT
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 11 Oliver Hunt 2008-07-01 19:23:15 PDT
Comment on attachment 22035 [details]
Few New Files with change

r=me
Comment 12 Oliver Hunt 2008-07-01 20:28:17 PDT
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 13 Oliver Hunt 2008-07-01 20:28:17 PDT
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 14 Alex Mathews 2008-07-01 23:10:01 PDT
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 15 Oliver Hunt 2008-07-01 23:26:43 PDT
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?
Comment 16 Alex Mathews 2008-07-01 23:40:32 PDT
Created attachment 22039 [details]
SVGFEBlend Rename

Same as previous, except the two changes Oliver wanted.
Comment 17 Oliver Hunt 2008-07-02 22:56:21 PDT
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
Comment 18 Alex Mathews 2008-07-02 23:54:31 PDT
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.
Comment 19 Oliver Hunt 2008-07-03 00:09:23 PDT
ColorMatrix changes look good.
Comment 20 Alex Mathews 2008-07-03 15:39:13 PDT
Created attachment 22073 [details]
SVGFEComponentTransfer Rename

Rename for SVGFEComponentTransfer. No extras this time, just the rename and changes required to build.
Comment 21 Alex Mathews 2008-07-03 16:22:00 PDT
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."
Comment 22 Oliver Hunt 2008-07-03 16:27:46 PDT
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.
Comment 23 Alex Mathews 2008-07-03 17:00:31 PDT
Created attachment 22077 [details]
Combined Color, Component, and Composite

Combined patch for the last three.
Comment 24 Oliver Hunt 2008-07-03 17:21:06 PDT
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 25 Oliver Hunt 2008-07-03 18:10:52 PDT
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
Comment 26 Alex Mathews 2008-07-03 23:32:10 PDT
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.
Comment 27 Alex Mathews 2008-07-03 23:37:11 PDT
Created attachment 22080 [details]
SVGFEDiffuseLighting Rename

Rename for SVGFEDiffuseLighting to FEDiffuseLighting.
Comment 28 Alex Mathews 2008-07-04 00:22:23 PDT
Created attachment 22081 [details]
SVGFEDisplacementMap Rename

Rename  SVGFEDisplacementMap to FEDisplacementMap.
Comment 29 Alex Mathews 2008-07-04 00:41:48 PDT
Created attachment 22083 [details]
SVGFEFlood Rename

Rename SVGFEFlood to FEFlood.
Comment 30 Alex Mathews 2008-07-04 00:58:14 PDT
Created attachment 22084 [details]
SVGFEGaussianBlur Rename

Rename SVGFEGaussianBlur to FEGaussianBlur.
Comment 31 Oliver Hunt 2008-07-04 01:54:04 PDT
Those all look basically sane :D

You should mark them as patches in future so i can get the pretty formatted diffs :D
Comment 32 Alex Mathews 2008-07-04 09:19:57 PDT
Created attachment 22088 [details]
Combined Convolve through Gaussian

Combined last 5.
Comment 33 Alex Mathews 2008-07-04 22:14:51 PDT
Comment on attachment 22088 [details]
Combined Convolve through Gaussian

Incorrect directory paths after commit 34993. Will fix and resubmit.
Comment 34 Alex Mathews 2008-07-04 22:30:07 PDT
Created attachment 22092 [details]
Trying again with the last 5

Retrying Last 5 Renames
Comment 35 Oliver Hunt 2008-07-05 01:36:18 PDT
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)
Comment 36 Alex Mathews 2008-07-05 12:45:41 PDT
> * 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?
Comment 37 Oliver Hunt 2008-07-05 14:02:49 PDT
(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.
Comment 38 Alex Mathews 2008-07-05 14:29:21 PDT
Created attachment 22101 [details]
Trying again, again

Patch with suggested changes in SVGFEDiffuseLightingElement and SVGFEFloodElement.
Comment 39 Alex Mathews 2008-07-07 00:22:08 PDT
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 40 Oliver Hunt 2008-07-07 03:51:39 PDT
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.
Comment 41 Alex Mathews 2008-07-08 00:45:27 PDT
Created attachment 22155 [details]
Large Refactoring

Changes Oliver suggested.
Comment 42 Oliver Hunt 2008-07-08 13:10:05 PDT
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.
Comment 43 Alex Mathews 2008-07-09 00:36:09 PDT
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 44 Oliver Hunt 2008-07-10 00:17:52 PDT
Comment on attachment 22169 [details]
Large Refactoring again

Landed r35094
Comment 45 Oliver Hunt 2008-07-10 16:17:22 PDT
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.
Comment 46 Nikolas Zimmermann 2008-08-11 08:04:27 PDT
Any news on filters so far? There are still a couple of patches here, not marked with r? etc.
Comment 47 Simon Fraser (smfr) 2009-06-02 18:29:46 PDT
<rdar://problem/6934155>
Comment 48 Simon Fraser (smfr) 2009-06-02 18:48:03 PDT
How does this related to bug 19991?
Comment 49 Dirk Schulze 2009-06-13 13:15:57 PDT
The refactoring and clean-up of the new filter system is ready. I close this bug and open new buge reports if necessary.