[webkit-reviews] review denied: [Bug 31196] Implement -webkit-color-correction for CSS colors : [Attachment 42622] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Beth Dakin
<bdakin at apple.com>'s request for review:
Bug 31196: Implement -webkit-color-correction for CSS colors
https://bugs.webkit.org/show_bug.cgi?id=31196

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> +CSSColorSpace GraphicsContext::cgToCSSColorSpace(ColorSpace colorSpace)
> +{
> +    if (colorSpace == sRGBColorSpace)
> +	   return sRGB;
> +    return Default;
> +}
> +
> +ColorSpace GraphicsContext::cssToCGColorSpace(CSSColorSpace colorSpace)
> +{
> +    if (colorSpace == sRGB)
> +	   return sRGBColorSpace;
> +    return DeviceRGBColorSpace;
> +}

Using switch() statements here would be good because then if someone added new
color space enum values, it would give a warning.

> Index: WebCore/platform/graphics/GraphicsContext.h
> ===================================================================

>      // a fill or stroke is using a gradient or pattern instead of a solid
color.
>      // https://bugs.webkit.org/show_bug.cgi?id=20558
>      enum ColorSpace {
> -	   SolidColorSpace,
> +	   DeviceRGBColorSpace,
> +	   sRGBColorSpace,
>	   PatternColorSpace,
>	   GradientColorSpace

This is a bit confusing; why is sRGBColorSpace a peer of PatternColorSpace and
GradientColorSpace? Wouldn't you want to be able to say that a pattern or
gradient is in sRGB or deviceRGB?

I think the existing ColorSpace enum is misnamed; it's more like a "brush". You
may need Device/sRGB in a new enum.

> -	   void setStrokeColor(const Color&);
> +	   void setStrokeColor(const Color&, CSSColorSpace colorSpace =
Default);

Rather than all these changes to GraphicsContext methods, maybe it make more
sense to push a color space onto the graphics context and have it be part of
the state?

> Index: WebCore/platform/graphics/cg/GraphicsContextCG.cpp
> ===================================================================

> +static void setCGFillColor(CGContextRef context, const Color& color,
ColorSpace colorSpace)
> +{	
> +    CGFloat components[4];
> +    color.getRGBA(components[0], components[1], components[2],
components[3]);
> +
> +    CGColorRef cgColor;
> +    if (colorSpace == sRGBColorSpace)
> +	   cgColor = CGColorCreate(sRGBColorSpaceRef(), components);
> +    else
> +	   cgColor = CGColorCreate(deviceRGBColorSpaceRef(), components);
> +
> +    CGContextSetFillColorWithColor(context, cgColor);

This is leaking a CGColorRef.

> +static void setCGStrokeColor(CGContextRef context, const Color& color,
ColorSpace colorSpace)
> +{
> +    CGFloat components[4];
> +    color.getRGBA(components[0], components[1], components[2],
components[3]);
> +
> +    CGColorRef cgColor;
> +    if (colorSpace == sRGBColorSpace)
> +	   cgColor = CGColorCreate(sRGBColorSpaceRef(), components);
> +    else
> +	   cgColor = CGColorCreate(deviceRGBColorSpaceRef(), components);
> +
> +    CGContextSetStrokeColorWithColor(context, cgColor);

Ditto.

I may have missed them, but:
* Where are untagged images tagged with sRGB?
* What about <canvas>, WebGL, gradients, etc?

r- for the leaks, for now, but this is a good start. Maybe talked to dhyatt
about the graphics context changes.