[webkit-reviews] review denied: [Bug 31321] Make -webkit-color-correction work with untagged images : [Attachment 43218] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 43218: Patch
https://bugs.webkit.org/attachment.cgi?id=43218&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Generally speaking, having a colorSpace argument that is used only when an
image is untagged seems subtle. I think we need a name for this argument other
than just "colorSpace", since it's only used for an image that has no intrinsic
color space. This applies to the GraphicsContext member functions as well as
the Image member functions.

>      if (mayFillWithSolidColor()) {
> -	   fillWithSolidColor(ctxt, dstRect, solidColor(), op);
> +	   fillWithSolidColor(ctxt, dstRect, solidColor(), colorSpace, op);
>	   return;
>      }

I suspect that mayFillWithSolidColor returns true even in cases where the image
is tagged with a color space. We need to test these cases. I suspect that we
will do the wrong thing with a single pixel image that has a color space in it.
I don't think the bug is necessarily new to this patch, although there is a
chance this has been made worse by introducing the sRGB feature.

> -    static void fillWithSolidColor(GraphicsContext* ctxt, const FloatRect&
dstRect, const Color& color, CompositeOperator op);
> +    static void fillWithSolidColor(GraphicsContext* ctxt, const FloatRect&
dstRect, const Color& color, ColorSpace, CompositeOperator op);

As long as you are modifying this line, you should remove the unhelpful
argument names, "ctxt", "color", and "op".

>  #if PLATFORM(WIN)
>      virtual void drawFrameMatchingSourceSize(GraphicsContext*, const
FloatRect& dstRect, const IntSize& srcSize, CompositeOperator) { }
>  #endif

Seems this needs a ColorSpace argument too.

> -    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const
FloatRect& srcRect, CompositeOperator) = 0;
> -    void drawTiled(GraphicsContext*, const FloatRect& dstRect, const
FloatPoint& srcPoint, const FloatSize& tileSize, CompositeOperator);
> -    void drawTiled(GraphicsContext*, const FloatRect& dstRect, const
FloatRect& srcRect, TileRule hRule, TileRule vRule, CompositeOperator);
> +    virtual void draw(GraphicsContext*, const FloatRect& dstRect, const
FloatRect& srcRect, ColorSpace, CompositeOperator) = 0;
> +    void drawTiled(GraphicsContext*, const FloatRect& dstRect, const
FloatPoint& srcPoint, const FloatSize& tileSize, ColorSpace,
CompositeOperator);
> +    void drawTiled(GraphicsContext*, const FloatRect& dstRect, const
FloatRect& srcRect, TileRule hRule, TileRule vRule, ColorSpace,
CompositeOperator);

To me, the purpose of the ColorSpace argument is not at all clear. The fact
that it's a color space to use with untagged images is a tricky concept, I
think, and needs a comment and perhaps an argument name as well.

>      virtual void drawPattern(GraphicsContext*, const FloatRect& srcRect,
const TransformationMatrix& patternTransform,
> -				const FloatPoint& phase, CompositeOperator,
const FloatRect& destRect);
> +				const FloatPoint& phase, ColorSpace,
CompositeOperator, const FloatRect& destRect);

Same here.

> +    static CGColorSpaceRef deviceSpace = CGColorSpaceCreateDeviceRGB();

Isn't there already a helper function that holds a global copy of the device
color space?

> +    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 != deviceSpace)
> +	   return 0;

Can you really detect an untagged image this way? What about an image with a
color space of null (a "mask image")? Do untagged images really return
something that is == the result of CGColorSpaceCreateDeviceRGB? Is this a
guaranteed part of the CoreGraphics or ImageIO API? Do you need to compare with
CFEqual instead?

> +    switch (colorSpace) {
> +    case DeviceColorSpace:
> +	   return 0;
> +    case sRGBColorSpace: {
> +#if PLATFORM(WIN) || defined(BUILDING_ON_TIGER)
> +	   return 0;
> +#else
> +	   static CGColorSpaceRef sRGBSpace =
CGColorSpaceCreateWithName(kCGColorSpaceSRGB);
> +	   return CGImageCreateCopyWithColorSpace(originalImage, sRGBSpace);
> +#endif

This will leak a CGImageRef each time. The function needs to return a
RetainPtr, or be renamed to have the word "create" in its name, and callers
need to adopt or release the returned pointer.

It also seems likely to be inefficient to create a new image every time. We
probably need to cache the color-spaced image in a data member rather than
generating it every time we draw. But I could be wrong about that.

> +    default:
> +	   ASSERT_NOT_REACHED();
> +	   return 0;
> +    }

It's better to put this code outside the switch statement instead of in a
default. That way we'll get a warning if we leave anything out.

Is there any way to make the regression tests so they would fail even without
doing pixel tests? I'm not sure these tests are helpful enough if only the
pixel test results indicate the failure.

review- because of the storage leak