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

[bugzilla-daemon at webkit.org]

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 186615: proposed patch
https://bugs.webkit.org/attachment.cgi?id=186615&action=review

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
I think we are getting closer!

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


> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:489
> +	   &clImageFormat, m_absolutePaintRect.width(),
m_absolutePaintRect.height(), 0, source, &errorCode);
> +
> +    if (errorCode) {

This newline is unnecessary.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:163
> +    if (inError() || m_transformColorSpaceCompileStatus !=
openclNotCompiledYet)
> +	   return;

This transformColorSpaceCompileStatus should be a boolean. Name:
transformColorSpaceWasCompiled. And the value should be set to true right after
this check. The inError() can be checked after after that (this is faster since
only one condition needs to be tested in most cases). Or at least swap the two
sides of the || operation.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:177
> +    m_transformColorSpaceCompileStatus = openclCompileSuccessful;

Setting this value case is too late here. See above.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:141
> +		   if (m_error)
> +		       return 0;
>		   return handle;

I think this could be shortened to return !m_error ? handle : 0;

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEColorMatrix.cpp:78
>      m_colorMatrixProgram = compileProgram(colorMatrixKernelProgram);
> -    if (!m_colorMatrixProgram)
> -	   return false;
> +    if (!m_colorMatrixProgram)  {
> +	   setInError();
> +	   return;
> +    }

There are many such tests in the code. They could be shortened as:
if (!(m_colorMatrixProgram = compileProgram(colorMatrixKernelProgram))) {
    setInError();
    return;
}

Or perhaps I have an even better idea. The setInError() calls could be replaced
by an inline function, called checkError

bool isFailed(bool value) {
    if (!value)
	setInError();
}

if (isFailed((m_colorMatrixProgram =
compileProgram(colorMatrixKernelProgram))))
    return;

We might add various isFailed methods if gcc expect them.