[webkit-reviews] review denied: [Bug 107444] Add error checking into OpenCL version of SVG filters. : [Attachment 187595] proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 12 00:23:56 PST 2013


Zoltan Herczeg <zherczeg at webkit.org> has denied Tamas Czene
<tczene at inf.u-szeged.hu>'s request for review:
Bug 107444: Add error checking into OpenCL version of SVG filters.
https://bugs.webkit.org/show_bug.cgi?id=107444

Attachment 187595: proposed patch
https://bugs.webkit.org/attachment.cgi?id=187595&action=review

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
Now we are quite close! Only a few more comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=187595&action=review


> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:284
> +    int errorCode = 0;

Do we really need this variable?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:293
> +    errorCode = clFinish(context->commandQueue());
> +    if (context->isFailed(errorCode))
> +	   return 0;

This could be combined into one check, without the errorCode.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:163
> +    if (m_transformColorSpaceWasCompiled)
> +	   return true;

This is incorrect. m_transformColorSpaceWasCompiled can be true, even if there
is an error.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:166
> +    if (inError())
> +	   return false;

I think this is the correct form:

if (m_transformColorSpaceWasCompiled || inError())
    return !inError();

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:240
>      errorNumber = clBuildProgram(program, 0, 0, 0, 0, 0);

Same as above. This variable seems unnecessary.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:75
> +    inline bool resourceAllocationIsFailed(bool);

I am thinking about this name for some time. Perhaps isResourceAllocationFailed
a better name...


More information about the webkit-reviews mailing list