[webkit-reviews] review granted: [Bug 227452] ANGLE Metal primitive restart range computation should not be done unless primitive restart is enabled : [Attachment 432397] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 227452: ANGLE Metal primitive restart range computation should not be done
unless primitive restart is enabled
https://bugs.webkit.org/show_bug.cgi?id=227452

Attachment 432397: Patch

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




--- Comment #3 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 432397
  --> https://bugs.webkit.org/attachment.cgi?id=432397
Patch

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

This is easier to read and understand. r+ with a couple of small questions.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:753
> +    if (!isSimpleType || !glContext->getState().isPrimitiveRestartEnabled())

I think it would be worth keeping the comment above, perhaps revised.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:781
> +    size_t currentIndexOffset = offset / indexTypeBytes;

Is offset verified by this point to be divisible by indexTypeBytes?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:787
> +	       int64_t nIndicesInSlice = ((int64_t)range.restartBegin -
currentIndexOffset) - ((int64_t) range.restartBegin - currentIndexOffset) %
nIndicesPerPrimitive;

Here and below: do you want to upgrade these C-style casts to C++ style to make
their intent clearer?

Also: I never remember operator precedence; wonder whether a temporary variable
or more parentheses would make this clearer.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/VertexArrayMtl.mm:819
> +	   drawCommands.push_back({indicesLeft, currentIndexOffset *
indexTypeBytes});

Just checking - it's guaranteed that this last if-test will always pick up any
trailing draw commands from the loop above?