[webkit-reviews] review granted: [Bug 206782] [WebGL2] Implement sub-source texImage2D and texSubImage2D : [Attachment 388746] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 206782: [WebGL2] Implement sub-source texImage2D and texSubImage2D
https://bugs.webkit.org/show_bug.cgi?id=206782

Attachment 388746: Patch

https://bugs.webkit.org/attachment.cgi?id=388746&action=review




--- Comment #4 from Dean Jackson <dino at apple.com> ---
Comment on attachment 388746
  --> https://bugs.webkit.org/attachment.cgi?id=388746
Patch

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

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:158
> +	   FOR_EACH_TYPED_ARRAY_TYPE_EXCLUDING_DATA_VIEW(FACTORY_CASE);

Too much indentation here.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:187
> -    Checked<GCGLuint, RecordOverflow> checkedByteLength = checkedlength *
checkedElementSize;
> +    Checked<GCGLuint, RecordOverflow> checkedByteLength = length *
checkedElementSize;

Does this still preserve the checked arithmetic? If so, then we can remove the
checkedLength variable, and do the same for checkedSrcOffset above.

Or maybe use WTF::checkedProduct<GCGLuint> for both?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:711
> +	   synthesizeGLError(GraphicsContextGL::INVALID_OPERATION,
functionName, "Invalid element offset!");

Remove the !

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:769
> +    auto slicedData = sliceTypedArrayBufferView("texSubImage2D", srcData,
srcOffset);
> +
> +    WebGLRenderingContextBase::texSubImage2D(target, level, xoffset,
yoffset, width, height, format, type, WTFMove(slicedData));

I was wondering if we should skip the call if slicedData is null, but I guess
we do output a message from the slice function.

>
LayoutTests/webgl/2.0.0/resources/webgl_test_files/conformance2/buffers/buffer-
data-and-buffer-sub-data-sub-source.html:55
> -	       testFailed("expected data at " + ii + ": " + data[iit] + ", got
" + readbackView[ii]);
> +	       testFailed("expected data at " + ii + ": " + data[ii] + ", got "
+ readbackView[ii]);

Has this been fixed in the WebGL2 test suite?