[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