[webkit-reviews] review denied: [Bug 123716] SVG rectangle with filter will not render : [Attachment 225411] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Adenilson Cavalcanti Silva
<savagobr at yahoo.com>'s request for review:
Bug 123716: SVG rectangle with filter will not render
https://bugs.webkit.org/show_bug.cgi?id=123716

Attachment 225411: Patch
https://bugs.webkit.org/attachment.cgi?id=225411&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=225411&action=review


The patched files look good. See comments to change log and testing.

> Source/WebCore/ChangeLog:10
> +	   A filtered SVG element with width or length bigger than 5000
> +	   (counting the margin/border) will fail to render. This patch will
> +	   instead test against the total element area.

This might not be an actual fix. See later comment. If a filter > 4096x4096
still fails, this change log should change wording. Right now it implies that
taking the are will fix the overall problem.

> Source/WebCore/ChangeLog:20
> +	   * platform/graphics/filters/FilterEffect.cpp:

Could you do some inline comments here please? Just say what you changed in
some of the named methods. This makes it easier to review the change log later.


> LayoutTests/svg/filters/big-height-filter-expected.svg:1
> +<svg xmlns="http://www.w3.org/2000/svg">

You need to create a change log entry for tests as well. The change log is
located at LayoutTests/ChangeLog

I wonder how the tests test the changed behavior. We do use buffers differently
now. In general all filters should run. Even if the buffer size extends beyond
4096x4096. If the filters don't run anymore, we need a test for a filter that
is bigger than this area. I support the change in general. If a filter still
fails if it is bigger than 4096x4096, then we should open a new bug report and
fix it there. This would not be the proper fix for that and I suggest renaming
the bug report to reflect the change you made.