[webkit-reviews] review granted: [Bug 225853] HTMLCanvasElement toDataURL and toBlob do unnecessary data copies through a CFDataRef : [Attachment 428835] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 225853: HTMLCanvasElement toDataURL and toBlob do unnecessary data copies
through a CFDataRef
https://bugs.webkit.org/show_bug.cgi?id=225853

Attachment 428835: Patch

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




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

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

> Source/WTF/wtf/text/Base64.cpp:94
> +template<typename CharacterType> static inline void
base64EncodeInternal(const unsigned char* inputDataBuffer, unsigned
inputLength, CharacterType* destinationDataBuffer, unsigned destinationLength,
Base64EncodePolicy policy)

Maybe we should be using uint8_t instead of unsigned char?

> Source/WTF/wtf/text/Base64.cpp:143
> +static inline void base64EncodeInternal(const unsigned char*
inputDataBuffer, unsigned inputLength, Vector<char>& destinationVector,
Base64EncodePolicy policy)

Can this use a return value instead of an out vector?

> Source/WTF/wtf/text/Base64.h:52
> +static constexpr unsigned maximumBase64EncoderInputBufferSize = UINT_MAX /
77 * 76 / 4 * 3 - 2;

std::numeric_limits instead of UINT_MAX? Comment should explain the nature of
the conservative estimate.

> Source/WTF/wtf/text/Base64.h:153
> +WTF_EXPORT_PRIVATE void base64Encode(const void*, unsigned, Vector<char>&,
Base64EncodePolicy = Base64DoNotInsertLFs);

Can we change this to product a return value instead of taking an out argument?

> Source/WTF/wtf/text/Base64.h:159
> +void base64Encode(ConstSignedOrUnsignedCharVectorAdapter, Vector<char>&,
Base64EncodePolicy = Base64DoNotInsertLFs);
> +void base64Encode(const CString&, Vector<char>&, Base64EncodePolicy =
Base64DoNotInsertLFs);

Can we change this to product a return value instead of taking an out argument?

> Source/WTF/wtf/text/Base64.h:252
> +	   if (result > 76)
> +	       return result + ((result - 1) / 76);

Can we name the 76 constant?

Why the if statement? Seems like values in the 1-76 range would still yield the
correct result.

I would drop one set of parentheses here.

> Source/WebCore/ChangeLog:49
> +	   Replace encodeImage(), which use a filled in a CFMutableDataRef with
overloads
> +	   of data and dataURL that now use a callback based CGDataConsumer and
some callback
> +	   functors to allow the encoded data to be consumed as it is being
created.

Some grammar problems in this sentence. "which use a", and not enough commas.

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.cpp:203
> +	   auto image = nativeImage->platformImage();
> +	   return createCroppedImageIfNecessary(image.get(), backendSize());

Better without the local?

> Source/WebCore/platform/graphics/cg/ImageBufferCGBackend.h:58
> -    virtual RetainPtr<CFDataRef> toCFData(const String& mimeType,
Optional<double> quality, PreserveResolution) const;
> +
> +    virtual PlatformImagePtr copyPlatformImageForEncoding(CFStringRef
destinationUTI, PreserveResolution) const;

Is this really an improvement? I’m not sure we need to use the typedef name
when this is not platform-independent code.