[webkit-reviews] review granted: [Bug 31321] Make -webkit-color-correction work with untagged images : [Attachment 43521] Newer Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Beth Dakin <bdakin at apple.com>'s
request for review:
Bug 31321: Make -webkit-color-correction work with untagged images
https://bugs.webkit.org/show_bug.cgi?id=31321

Attachment 43521: Newer Patch
https://bugs.webkit.org/attachment.cgi?id=43521&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +inline static CGColorSpaceRef deviceRGBColorSpaceRef()
> +{
> +    static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB();
> +    return deviceSpace;
> +}
> +
> +inline static CGColorSpaceRef sRGBColorSpaceRef()
> +{
> +    // FIXME: Windows should be able to use kCGColorSpaceSRGB, this is
tracked by http://webkit.org/b/31363.
> +#if PLATFORM(WIN) || defined(BUILDING_ON_TIGER)
> +    return deviceRGBColorSpaceRef();
> +#else
> +    static CGColorSpaceRef sRGBSpace =
CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> +    return sRGBSpace;
> +#endif
> +}

These should not be marked static, since they are in header files. If marked
static, there will be multiple copies of the global variables. Since not marked
static, there will not be.

Probably should mark these as FIXME, since ideally they would go in
GraphicsContextCG.h if it existed.

> +static RetainPtr<CGImageRef> imageWithColorSpace(CGImageRef originalImage,
ColorSpace colorSpace)
> +{
> +    CGColorSpaceRef originalColorSpace =
CGImageGetColorSpace(originalImage);
> +
> +    // If the image already has a (non-device) color space, we don't want to

> +    // override it, so return.
> +    if (!originalColorSpace || !CFEqual(originalColorSpace,
deviceRGBColorSpaceRef()))
> +	   return 0;

If you changed this to return originalImage instead of 0 whenever color change
is not needed, then the callers would all be simplified.

> +    // Adjust the color space.
> +    if (RetainPtr<CGImageRef> colorSpaceImage =
imageWithColorSpace(image.get(), styleColorSpace))
> +	   image = colorSpaceImage;

Here's an example. You could eliminate the local variable and just say:

    image = imageWithColorSpace(image.get(), styleColorSpace);

> -    if (shouldUseSubimage)

I suspect we no longer need shouldUseSubimage to be in a local variable, but it
might still help clarity to keep it so I'm not sure it should be removed.

r=me

I really want better regression tests that don't rely on pixel results.