[webkit-reviews] review denied: [Bug 47922] Merge ColorSpace and ImageColorSpace enums : [Attachment 71198] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 47922: Merge ColorSpace and ImageColorSpace enums
https://bugs.webkit.org/show_bug.cgi?id=47922

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71198&action=review

Looks good overall, except for some minor things. But I'd really like to see
whether we can avoid code duplication, by not using RetainPtr for the
colorspace in ImageBuffer.

> WebCore/css/CSSPrimitiveValueMappings.h:2198
> +	   default:

default is not added here on purpose, so the compiler warns if anything is
missing. If you need to add sth. add the real case value.

> WebCore/platform/graphics/ColorSpace.h:34
> +    sRGBColorSpace,
> +    linearRGBColorSpace

I don't like these names, style guide says "Enum members should user InterCaps
with an initial capital letter.".
So rename to SRGB and LinearRGB..

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:83
> +    case DeviceColorSpace:
> +	   m_data.m_colorSpace.adoptCF(CGColorSpaceCreateDeviceRGB());
> +	   break;

Can you investigate, if it's possible to not use a RetainPtr for m_colorSpace.
Then we could use the deviceRGBRef() static methods, instead of duplication
code.