[webkit-reviews] review denied: [Bug 38492] CG implementation needed for compression quality in canvas.toDataURL : [Attachment 57583] Patch implements CG support for quality param in toDataURL canvas feature.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Matthew Delaney <mdelaney at apple.com>'s
request for review:
Bug 38492: CG implementation needed for compression quality in canvas.toDataURL
https://bugs.webkit.org/show_bug.cgi?id=38492

Attachment 57583: Patch implements CG support for quality param in toDataURL
canvas feature.
https://bugs.webkit.org/attachment.cgi?id=57583&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good.

> +	Fix for 38492. Added CG implementation to support quality parameter in
canvas toDataURL().

This has a tab character.

The reference to the bug should be a bug URL not just the bug number.

> +
> +	   * bindings/js/JSHTMLCanvasElementCustom.cpp:
> +	   (WebCore::JSHTMLCanvasElement::toDataURL):
> +	   * dom/CanvasSurface.h:
> +	   (WebCore::CanvasSurface::toDataURL):
> +	   * platform/graphics/ImageBuffer.h:
> +	   * platform/graphics/cg/ImageBufferCG.cpp:
> +	   (WebCore::jpegUTI):
> +	   (WebCore::utiFromMIMEType):
> +	   (WebCore::ImageBuffer::toDataURL):

Each file and function should have a short comment explaining the change.

> +#define DEFAULT_QUALITY -1.0

This should be a C++ constant, not a macro. Also, if the name is at the WebCore
namespace level (top level), then it needs to be a specific-enough name to
stand on its own.

    const double defaultImageCompressionQuality = -1;

I also am surprised that the default quality is -1. The old default was 1. Why
the change? The change log should explain this. If the idea is that an explicit
quality is optional, I think we should find an explicit way of doing that
instead of defining a magic constant. We call that "in-band signaling" and
usually try to avoid it. One way to do it would be to make the image quality
argument be a "const double*" instead of just double and then we could use a 0
to mean "not specified".

If you do want to use in-band signaling, I think that NAN is probably a better
value to mean "no quality specified". One advantage of that is that JavaScript
code will automatically supply a NAN for any value that is not a number,
without any custom bindings.

Did you add code to handle this default quality concept to the other platforms?
Or do they already work as-is?

>	   void putUnmultipliedImageData(ImageData*, const IntRect& sourceRect,
const IntPoint& destPoint);
>	   void putPremultipliedImageData(ImageData*, const IntRect&
sourceRect, const IntPoint& destPoint);
>  
> -	   String toDataURL(const String& mimeType, double quality = 1.0)
const;
> +
> +	   String toDataURL(const String& mimeType, double quality =
DEFAULT_QUALITY) const;

Why are you adding an extra blank line? I suggest omitting that.

> -    CGImageDestinationAddImage(destination.get(), image.get(), 0);
> +    RetainPtr<CFDictionaryRef> dict = 0;

I suggest naming this either "properties" or "imageProperties". The name "dict"
isn't as good because it's an abbreviation, not a word, and also naming the
data structure is not as useful as naming its purpose.

> +    if (CFEqual(uti.get(), jpegUTI()) && quality >= 0.0 && quality <= 1.0) {

> +
> +	   // Apply the compression quality to the image destination.
> +	   RetainPtr<CFNumberRef> compressionQuality(AdoptCF,
CFNumberCreate(kCFAllocatorDefault, kCFNumberDoubleType, &quality));

You should get rid of that extra newline above.

> +	   const void* keys[] = { kCGImageDestinationLossyCompressionQuality };

> +	   const void* values[] = { compressionQuality.get() };
> +	   dict.adoptCF(CFDictionaryCreate(0, keys, values, 1,
&kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

I suggest you explicitly use the size "1" for these arrays instead of omitting
the size. But also, if there's only a single key and value you don't actually
need an array. This works:

    const void* key = kCGImageDestinationLossyCompressionQuality;
    const void* value = compressionQuality.get();
    imageProperties.adoptCF(CFDictionaryCreate(0, &key, &value, 1,
&kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

I think those "&" before kCFTypeDictionaryKeyCallBacks and
kCFTypeDictionaryValueCallBacks are optional and you should omit them.

I'm going to say review- because I think that you should deal with at least one
of the bugs above.

Also, is there any test for this? A bug fix should add a test. A new test, or
an existing test that was disabled on Mac before, or an existing test with new
success results that before had failure results.