[webkit-reviews] review denied: [Bug 61603] FEConcolveMatrix::getPixelValue() is range checking x against height, instead of y, when determining if a pixel is in bounds : [Attachment 95144] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Ryan Sleevi
<rsleevi at chromium.org>'s request for review:
Bug 61603: FEConcolveMatrix::getPixelValue() is range checking x against
height, instead of y, when determining if a pixel is in bounds
https://bugs.webkit.org/show_bug.cgi?id=61603

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
Layout test results as well as pixel test results are missing.

I guess this is a crash testcase, so we won't need a pixel test result nor a
render tree dump. So I suggest you to include this before the <defs> section:
<script>
if (window.layoutTestController)
    layoutTestController.dumpAsText();
</script>

And create a text node somewhere in the document saying "Passes, if it doesn't
crash", so the "feConvolveFilter-bounds-expected.txt" file includes just this
text.

I guess this is a crash test, you haven't explained the test in the ChangeLog,
you should do that, and also rename to test to sth like
"feConvoleMatrix-y-bounds-crash.svg".
I am not sure though how this testcase triggers the bug? SHouldn't y be
negative??

Last note: The ChangeLog is wrong, you have to create individual change logs
once for LayoutTests/ChangeLog, once for Source/WebCore/ChangeLog.
Easiest is to use "prepare-ChangeLog --bug=<bugNumber>" in the root directory
of your checkout, it will take care of using the templates for you.

Looking forward to the next patch :-) The first webkit contribution is always
tricky, so no worries...