[webkit-reviews] review denied: [Bug 90838] constructing TypedArray from another TypedArray is slow : [Attachment 151992] updated patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied arno. <arno at renevier.net>'s request
for review:
Bug 90838: constructing TypedArray from another TypedArray is slow
https://bugs.webkit.org/show_bug.cgi?id=90838

Attachment 151992: updated patch
https://bugs.webkit.org/attachment.cgi?id=151992&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=151992&action=review


Thanks for this patch and sorry for taking so long to review it. Originally I
was concerned about the naming convention of your newly introduced TypedArray
superclass, but upon giving it more thought I don't think it's necessary to
expose that concept at all -- therefore the r-. Please contact me if you have
any concerns about this review. Looking forward to a revised patch.

> Source/WTF/wtf/TypedArray.h:36
> +	   TYPE_Int8,

While there isn't really a naming convention in WebKit for enums, using both
capitalization and underscores doesn't match any other I've seen. Please use
for example TypeInt8, TypeUint8, etc.

> Source/WTF/wtf/TypedArray.h:44
> +	   TYPE_Float64

Please add one for DataView as well.

> Source/WTF/wtf/TypedArray.h:46
> +    virtual ViewType getType() = 0;

Just go ahead and add this virtual to ArrayBufferView. It already contains a
bunch of other virtuals for run-time type identification. This will simplify
your patch since you won't need to touch any IDL files or the code generators.
You will need however a couple more tests in the custom bindings in order to
watch for and avoid DataViews as incoming arguments.

Actually, it would be good to clean up these virtuals. You can feel free to
remove the others in favor of your new one when you introduce it.

> Source/WTF/wtf/Uint16Array.h:49
> +    ViewType getType() 

All of these overrides should be in the private section to minimize visibility.
See for example the current isUnsignedShortArray in this class.