https://bugs.webkit.org/show_bug.cgi?id=73643
--- Comment #13 from Branimir Lambov <blambov at google.com> 2011-12-08 08:55:01 PST ---
Thank you for your review. The patch is updated, please see comments below.
On Thu, Dec 8, 2011 at 7:48 AM, <bugzilla-daemon at webkit.org> wrote:
>
> https://bugs.webkit.org/show_bug.cgi?id=73643
>
>
> Dirk Schulze <krit at webkit.org> changed:
>
> What |Removed |Added
> ----------------------------------------------------------------------------
> Attachment #118088|review? |review-
> Flag| |
>
>
>
>
> --- Comment #10 from Dirk Schulze <krit at webkit.org> 2011-12-07 23:48:05 PST ---
> (From update of attachment 118088)
> View in context: https://bugs.webkit.org/attachment.cgi?id=118088&action=review
>
> This is just a code review. I have to check the functionality again in a second step.
>
> > LayoutTests/ChangeLog:11
> > + * svg/filters/filter-placement-issue.svg: Added.
>
> Is this a manual test? You miss the results for this test otherwise (I'd like to have an automated test with pixel result). Also, do you have examples for clipping and masking at well? Won't this code influence existing results? Don't you need to update pixel results?
I was having a bit of trouble the patch upload system and sent the expectations update as a separate upload. It is now included in the patch.
A clipping and masking test is added. I will rebaseline the Chrome versions of these tests after the patch is landed.
>
>
> > Source/WebCore/platform/graphics/filters/FETile.cpp:58
> > + FloatRect tileRect = enclosingIntRect(in->maxEffectRect());
>
> Why don't you make it an IntRect?
Changed.
>
>
> > Source/WebCore/platform/graphics/filters/FETile.cpp:67
> > + // TODO: Clamp image size?
>
> This should be a statement and use FIXME. Also, is it correct to remove the SVGImageBufferTools::createImageBuffer call? Is the following code a complete replacement of the functionality?
Yes, it is a complete replacement as used by FETile (i.e. without clamping). Removing this reference is required to make sure non-SVG targets work properly.
Comment changed.
>
>
> > Source/WebCore/platform/graphics/filters/FETile.cpp:68
> > + OwnPtr<ImageBuffer> tileImage = ImageBuffer::create(IntSize((int)tileRect.width(), (int)tileRect.height()), ColorSpaceDeviceRGB, filter()->renderingMode());
>
> the (int)'s would be unnecessary if you use IntRect, same for IntSize.
Changed.
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:-189
> > - // The save/restore pair is needed for clipToImageBuffer - it doesn't work without it on non-Cg platforms.
> > - GraphicsContextStateSaver stateSaver(*maskContext);
>
> You seem to ignore this comment. This is not addressed somewhere later. This code was added for Cairo and Skia ports.
The saving of the context here does not affect the placement of the clipping/masking rectangle (it applies to the mask context, while it should be done on the parent one).
Clipping and masking do not currently work on Skia, due to incorrect treatment of transformations (see https://bugs.webkit.org/show_bug.cgi?id=53378). That problem is properly solved by the patch attached to that bug.
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:231
> > absoluteDrawingRegion.scale(scale.width(), scale.height());
>
> Do we still need this rect?
Removed.
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:251
> > + // Context is now set up to draw in local coordinates. This shouldn't work.
>
> You should check this first. I don't think that such a comment should be in trunk :)
Yes, I have tested that it does work correctly (on Mac and Chrome Linux) without the call. As it was set up, the clearRect call had no effect.
>
>
> > Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:252
> > + // sourceGraphicContext->clearRect(FloatRect(FloatPoint(), absoluteDrawingRegion.size()));
>
> Don't use commented out code. Please be sure that this works correctly and why it works.
Removed.
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:72
> > + // This is done in absolute coordinates
>
> Please use sentences and a dot at the end of a phrase.
Done.
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:77
> > + // This happens in local coordinates
>
> Ditto.
Done.
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:78
> > + imageContext->scale(FloatSize(((float)clampedSize.width()) / paintRect.width(), ((float)clampedSize.height()) / paintRect.height()));
>
> you don't check if paintRect can be empty (width or height is zero).
This was done by the clampedSize.isEmpty() call above, but I agree checking for the original paintRect being empty makes more sense. Changed.
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.cpp:145
> > +IntSize SVGImageBufferTools::clampedAbsoluteSize(const IntSize& absoluteSize)
> > +{
> > + IntSize clampedAbsoluteSize = absoluteSize;
> > +
> > + if (clampedAbsoluteSize.width() > kMaxImageBufferSize)
> > + clampedAbsoluteSize.setWidth(kMaxImageBufferSize);
> > +
> > + if (clampedAbsoluteSize.height() > kMaxImageBufferSize)
> > + clampedAbsoluteSize.setHeight(kMaxImageBufferSize);
> > +
> > + return clampedAbsoluteSize;
> > +}
>
> I'm sure that we have this code somewhere else. Did you copy this? We shouldn't have duplicated code.
This is an integer version of clampedAbsoluteTargetRect above, applied to IntSize. It wouldn't be efficient or entirely correct to force one to call the other (unless you prefer to have clampedAbsoluteSize as a template function in the header; please let me know if that's the case and I'll be happy to change it).
>
>
> > Source/WebCore/rendering/svg/SVGImageBufferTools.h:48
> > + static IntRect calcImageBufferRect(const FloatRect& targetRect, const AffineTransform& absoluteTransform)
> > + { return enclosingIntRect(absoluteTransform.mapRect(targetRect)); };
>
> This is the wrong style, please use multiple lines for a function, even on headers.
Done.
>
>
> --
> Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
--
Regards,
Branimir
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.