[webkit-reviews] review granted: [Bug 220372] [WebGL2] fbostatequery, negativebufferapi, negativevertexarrayapi, shaderstatequery conformance failures : [Attachment 417503] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kenneth Russell <kbr at google.com>'s
request for review:
Bug 220372: [WebGL2] fbostatequery, negativebufferapi, negativevertexarrayapi,
shaderstatequery conformance failures
https://bugs.webkit.org/show_bug.cgi?id=220372

Attachment 417503: Patch

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




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 417503
  --> https://bugs.webkit.org/attachment.cgi?id=417503
Patch

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

It’s great that this has so many fixes; they look good. Are we confident that
each of these was covered by a test? With somewhere in the neighborhood of 30
failures fixed here and somewhere near 30 different logic changes, it’s hard
for me to spot which improvements are tested and which are untested.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:1755
> +    GCGLsizei bytesPerElement = size * typeSize;

What prevents this from overflowing?

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2787
> +	   // We can look directly at m_attributes because in WebGL 2,
> +	   // they are required to be honored.

No need to break this into two lines. Reads better in one line.

> Source/WebCore/html/canvas/WebGL2RenderingContext.cpp:2790
> +	   bool missingImage = (attachment == GraphicsContextGL::DEPTH &&
!hasDepth) || (attachment == GraphicsContextGL::STENCIL && !hasStencil);

Not a fan of naming a boolean "missing image" since it’s not an image. Prefer a
name like hasMissingImage.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:1380
> +	   // This differs in behavior to ValidateWebGLObject; null objects are
allowed
> +	   // in these entry points.

Given the function’s name, it does not seem like we need this comment. What
else would we assume "nullable" means other than "null is allowed"? I also
think the logic is easier to understand when more compact and less vertical.

    return !object || validateWebGLObject(functionName, object);