Bug 237871

Created attachment 454658 [details]
Patch

Comment on attachment 454658 [details]
Patch

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

> Source/WebGPU/WebGPU/CommandEncoder.mm:144
> +    if (validateCopyBufferToBuffer(source, sourceOffset, destination, destinationOffset, size)) {

if (!validateCopyBufferToBuffer(source, sourceOffset, destination, destinationOffset, size))

Created attachment 454664 [details]
Patch

Comment on attachment 454664 [details]
Patch

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

> Source/WebGPU/WebGPU/CommandEncoder.mm:52
> +    commandBuffer.label = [NSString stringWithCString:descriptor.label encoding:NSUTF8StringEncoding];

nullptr label remark (in case it's not handled in the fromAPI patch)

> Source/WebGPU/WebGPU/CommandsMixin.h:32
> +class CommandsMixin : public RefCounted<CommandsMixin> {

Based on this commit alone, it's not clear why would the refcounted be in the mixin. it would indicate objects hold references to the mixin, which is counter (my) expectation of mixins being just "way to import a bag of common functionality"

Also having the WTF_MAKE_FAST_ALLOCATED here would imply you allocate the mixins

Comment on attachment 454664 [details]
Patch

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

>> Source/WebGPU/WebGPU/CommandEncoder.mm:52
>> +    commandBuffer.label = [NSString stringWithCString:descriptor.label encoding:NSUTF8StringEncoding];
> 
> nullptr label remark (in case it's not handled in the fromAPI patch)

Checked, I think the fromAPI patch was not written with this patch in, so there's that.
so probably you could have some sort of fromAPI<NSString*>(descriptor.label) for these cases (or something else you come up with)

Comment on attachment 454664 [details]
Patch

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

>>> Source/WebGPU/WebGPU/CommandEncoder.mm:52
>>> +    commandBuffer.label = [NSString stringWithCString:descriptor.label encoding:NSUTF8StringEncoding];
>> 
>> nullptr label remark (in case it's not handled in the fromAPI patch)
> 
> Checked, I think the fromAPI patch was not written with this patch in, so there's that.
> so probably you could have some sort of fromAPI<NSString*>(descriptor.label) for these cases (or something else you come up with)

Right. I'll have to update the fromAPI patch after this lands.

>> Source/WebGPU/WebGPU/CommandsMixin.h:32
>> +class CommandsMixin : public RefCounted<CommandsMixin> {
> 
> Based on this commit alone, it's not clear why would the refcounted be in the mixin. it would indicate objects hold references to the mixin, which is counter (my) expectation of mixins being just "way to import a bag of common functionality"
> 
> Also having the WTF_MAKE_FAST_ALLOCATED here would imply you allocate the mixins

Ah, I see what you're saying. Yeah, I guess multiple inheritance is better than making a mixin class that appears to be refcounted.

Committed r291372 (248504@trunk): <https://commits.webkit.org/248504@trunk>

<rdar://problem/90394764>