[Webkit-unassigned] [Bug 6021] WebKit+SVG does not support filterRes attribute
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 18 16:06:35 PST 2009
https://bugs.webkit.org/show_bug.cgi?id=6021
Nikolas Zimmermann <zimmermann at kde.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #43448|review? |review-
Flag| |
--- Comment #3 from Nikolas Zimmermann <zimmermann at kde.org> 2009-11-18 16:06:33 PST ---
(From update of attachment 43448)
Jeeezz, what a large patch, thank god I'm on a nightshift with nothing else to
do ;-)
Several comments:
- You need a much more detailed ChangeLog, as this is a large patch, ideally
per file, this time.
> Index: WebCore/platform/graphics/filters/FEGaussianBlur.cpp
...
> + sdx = std::max(sdx, static_cast<unsigned>(1));
> + sdy = std::max(sdy, static_cast<unsigned>(1));
Why not just std::max(sdx/y, 1); ?
> Index: WebCore/platform/graphics/filters/Filter.h
> ===================================================================
> --- WebCore/platform/graphics/filters/Filter.h (revision 51070)
> +++ WebCore/platform/graphics/filters/Filter.h (working copy)
> @@ -22,6 +22,7 @@
>
> #if ENABLE(FILTERS)
> #include "FloatRect.h"
> +#include "FloatSize.h"
> #include "ImageBuffer.h"
> #include "StringHash.h"
>
> @@ -40,15 +41,21 @@ namespace WebCore {
> void setSourceImage(PassOwnPtr<ImageBuffer> sourceImage) { m_sourceImage = sourceImage; }
> ImageBuffer* sourceImage() { return m_sourceImage.get(); }
>
> + FloatSize filterRes() const { return m_filterRes; }
> + void setFilterRes(const FloatSize& filterRes) { m_filterRes = filterRes; }
Hrm, I'd prefer to completly avoid abbrevations, aka. change it to
"setFilterResolution" here.
Filter* has no direct connection to SVG, and it's not in the "// SVG specific"
section below, so why not rename it?
We don't need to reuse SVG naming conventions here.
> Index: WebCore/svg/SVGFilterElement.cpp
...
> + else if (attr->name() == SVGNames::filterResAttr) {
> + float x, y;
> + if (parseNumberOptionalNumber(value, x, y)) {
> + setFilterResXBaseValue(x);
> + setFilterResYBaseValue(y);
> + }
An else case reporting errors through SVGDocumentExtensions seems to be missing
here.
> + if (this->hasAttribute(SVGNames::filterResAttr)) {
s/this->//
> + m_filter->setHasFilterRes(true);
> + m_filter->setFilterResSize(FloatSize(filterResX(), filterResY()));
Same thoughts as in Filter.h:
s/setHasFilterRes/setHasFilterResolution/
s/setFilterResSize/setFilterResolution/ (Size prefix is superflous IMHO)
> Index: WebCore/svg/graphics/SVGResourceFilter.cpp
> ===================================================================
> --- WebCore/svg/graphics/SVGResourceFilter.cpp (revision 51070)
> +++ WebCore/svg/graphics/SVGResourceFilter.cpp (working copy)
> @@ -35,6 +35,10 @@
> #include "SVGRenderTreeAsText.h"
> #include "SVGFilterPrimitiveStandardAttributes.h"
>
> +#define MAX_FILTER_SIZE 5000.f
Evil defines, please use a static constant here. I am not sure about our naming
convention, it may have a g, or k prefix, please check for common usage of
static constants in other files.
> +bool SVGResourceFilter::matchesMaxImageSize(const FloatSize& size) {
{ neeeds to go on a new line
> + bool matchesFilterSize = true;
> + if (size.width() > MAX_FILTER_SIZE) {
> + m_scaleX *= MAX_FILTER_SIZE / size.width();
> + matchesFilterSize = false;
> + }
> + if (size.height() > MAX_FILTER_SIZE) {
> + m_scaleY *= MAX_FILTER_SIZE / size.height();
> + matchesFilterSize = false;
> + }
> +
> + return matchesFilterSize;
> +}
matchesMaxImageSize is not a good name. How about "fitsInMaximumImageSize" or
something similar.
Matches sounds too much like "is equal".
> void SVGResourceFilter::prepareFilter(GraphicsContext*& context, const RenderObject* object)
...
> + // scale to big sourceImage size to MAX_FILTER_SIZE
> + tempSourceRect.scale(m_scaleX, m_scaleY);
> + matchesMaxImageSize(tempSourceRect.size());
Hrm, you're ignoring the return value of matchesMaxImageSize here??
> +
> // prepare Filters
> m_filter = SVGFilter::create(targetRect, m_filterBBox, m_effectBBoxMode);
> + m_filter->setFilterRes(FloatSize(m_scaleX, m_scaleY));
>
> FilterEffect* lastEffect = m_filterBuilder->lastEffect();
> - if (lastEffect)
> + if (lastEffect) {
> lastEffect->calculateEffectRect(m_filter.get());
> + // at least one FilterEffect has a to big image size,
s/to/too/
> void SVGResourceFilter::applyFilter(GraphicsContext*& context, const RenderObject*)
> {
> + if (!m_scaleX || !m_scaleY || !m_filterBBox.width() || !m_filterBBox.height())
> + return;
The same check is used as above, might make sense to refactor in a simple
"static inline bool shouldProcessFilter()" helper function?
> Index: WebCore/svg/graphics/SVGResourceFilter.h
> ===================================================================
> --- WebCore/svg/graphics/SVGResourceFilter.h (revision 51070)
> +++ WebCore/svg/graphics/SVGResourceFilter.h (working copy)
> @@ -53,6 +53,9 @@ public:
>
> virtual SVGResourceType resourceType() const { return FilterResourceType; }
>
> + void setFilterResSize(const FloatSize& filterResSize) { m_filterResSize = filterResSize; }
> + void setHasFilterRes(bool filterRes) { m_filterRes = filterRes; }
Same rename suggestions apply as above.
> + bool matchesMaxImageSize(const FloatSize&);
Ditto.
> + FloatSize m_filterResSize;
I'd suggest just "m_filterResolution" here.
> Index: WebCore/svg/graphics/filters/SVGFilter.h
...
> - bool effectBoundingBoxMode() const { return m_effectBBoxMode; }
> + virtual bool effectBoundingBoxMode() const { return m_effectBBoxMode; }
>
> - FloatRect filterRegion() const { return m_filterRect; }
> - FloatRect sourceImageRect() const { return m_itemBox; }
> - void calculateEffectSubRegion(FilterEffect*) const;
> + virtual FloatRect filterRegion() const { return m_filterRect; }
> + virtual FloatRect sourceImageRect() const { return m_itemBox; }
> +
> + virtual FloatSize maxImageSize() const { return m_maxImageSize; }
> + virtual void calculateEffectSubRegion(FilterEffect*);
I guess these functions have already been marked virtual in the base class, and
this is just for clarification?
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog (revision 51126)
> +++ LayoutTests/ChangeLog (working copy)
> @@ -1,3 +1,25 @@
> +2009-11-18 Dirk Schulze <krit at webkit.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + The first two example have a filter with a size of 20000x20000. We limit
> + the quality to be able to render this filter.
> + On the third example the filter qulitiy is limited by different values
> + to test the bahavior of effects with kernels or other values that need
> + to be scaled too. It also tests the correctness of the subRegion calculation
> + for filters with more than one effect.
Better name the tests, instead of relying on the reviewers ability to count ;-)
Just realized the patch is not that big, it's mostly the pngs, that create a
frightning big patch size.
Please fix these issues, and upload a new patch. I think the general concept
looks sane.
Wouldn't hurt if Simon Fraser / Eric Seidel / Oliver Hunt may have another
look. I'll cc them soon.
r- because of the mentioned issues above.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list